New issue
Advanced search Search tips
Starred by 11 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocking:
issue 426309



Sign in to add a comment
link

Issue 520784: Support ignoreSearch option

Reported by nhiroki@chromium.org, Aug 14 2015 Project Member

Issue description

In the current implementation, Cache.match and Cache.matchAll don't ignore search params even if 'ignoreSearch' is true.

Spec: https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#cache-query-options-dictionary
 

Comment 1 by nhiroki@chromium.org, Aug 14 2015

Blocking: chromium:426309

Comment 2 by jakearchibald@chromium.org, Aug 26 2015

I'd say this is the most important of the cache query options.

Comment 3 by bugdroid1@chromium.org, Aug 27 2015

Project Member
The following revision refers to this bug:
  http://src.chromium.org/viewvc/blink?view=rev&rev=201316

------------------------------------------------------------------
r201316 | nhiroki@chromium.org | 2015-08-27T14:46:55.246438Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/http/tests/cachestorage/window/cache-matchAll-expected.txt?r1=201316&r2=201315&pathrev=201316
   M http://src.chromium.org/viewvc/blink/trunk/Source/modules/cachestorage/Cache.cpp?r1=201316&r2=201315&pathrev=201316
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/http/tests/cachestorage/worker/cache-matchAll-expected.txt?r1=201316&r2=201315&pathrev=201316
   M http://src.chromium.org/viewvc/blink/trunk/Source/modules/cachestorage/CacheStorage.cpp?r1=201316&r2=201315&pathrev=201316
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/http/tests/cachestorage/window/cache-match-expected.txt?r1=201316&r2=201315&pathrev=201316
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/http/tests/cachestorage/worker/cache-match-expected.txt?r1=201316&r2=201315&pathrev=201316

CacheStorage: Show a warning message when unsupported CacheQueryOptions are specified

Currently CacheQueryOptions(ignoreSearch, ignoreMethod, ignoreVary) are not
supported on Cache.match/matchAll/delete/keys and CacheStorage.match, but
these APIs can accept the options and just drop them internally.

This CL makes these APIs show a warning message when unsupported options
are specified.

BUG= 426309 ,  499216 ,  482256 ,  520784 

Review URL: https://codereview.chromium.org/1320823003
-----------------------------------------------------------------

Comment 4 by jsb...@chromium.org, Sep 8 2015

Labels: Hotlist-Interop

Comment 5 by falken@chromium.org, Dec 8 2015

Cc: nhiroki@chromium.org
Given "this is the most important of the cache query options" and that Firefox supports this, this is a high priority P2.

Comment 6 by zino@chromium.org, Dec 17 2015

Hi all,
Can I take this issue? (If anyone isn't working on this yet..)

Comment 7 by nhiroki@chromium.org, Dec 17 2015

Labels: M-49
Owner: zino@chromium.org
Status: Assigned
Yep! This issue waited for you :)

Comment 8 by kenjibaheux@chromium.org, Jan 13 2016

Should we add a chromestatus entry for this?

Comment 9 by nhiroki@chromium.org, Jan 13 2016

Yes, we should.

zino@, could you make an entry?

Comment 11 by jinho.b...@samsung.com, Jan 13 2016

Owner: jinho.b...@samsung.com
Status: Started

Comment 12 by nhiroki@chromium.org, Jan 13 2016

Thank you!

Comment 13 by nhiroki@chromium.org, Jan 22 2016

Re: the chromestatus dashboard, how about expanding the scope of the entry to all APIs that accept CacheQueryOptions, namely, Cache.{match,matchAll,delete,keys} and CacheStorage.match? I think it's better to ship the option for them all at once in order to avoid developers' confusion.

Comment 14 by jinho.b...@samsung.com, Jan 22 2016

#13,

Done!

Thank you.

Comment 15 by bugdroid1@chromium.org, Feb 16 2016

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8cd93e585ae7f85b41a54b9328657ac0ab48eedb

commit 8cd93e585ae7f85b41a54b9328657ac0ab48eedb
Author: jinho.bang <jinho.bang@samsung.com>
Date: Tue Feb 16 03:06:42 2016

CacheStorage: Add ignoreSearch option to cache.matchAll().

The option that speicifies whether the matching process should ignore the query
string in the url. If the option is true, query string of request url would be
ignored when performaing a match.

Spec:
  https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#cache-query-options-dictionary

BUG= 520784 

Review URL: https://codereview.chromium.org/1578363009

Cr-Commit-Position: refs/heads/master@{#375508}

[modify] http://crrev.com/8cd93e585ae7f85b41a54b9328657ac0ab48eedb/content/browser/cache_storage/cache_storage_cache.cc
[modify] http://crrev.com/8cd93e585ae7f85b41a54b9328657ac0ab48eedb/content/browser/cache_storage/cache_storage_cache.h
[modify] http://crrev.com/8cd93e585ae7f85b41a54b9328657ac0ab48eedb/content/browser/cache_storage/cache_storage_cache_unittest.cc
[modify] http://crrev.com/8cd93e585ae7f85b41a54b9328657ac0ab48eedb/content/browser/cache_storage/cache_storage_dispatcher_host.cc
[add] http://crrev.com/8cd93e585ae7f85b41a54b9328657ac0ab48eedb/third_party/WebKit/LayoutTests/http/tests/cachestorage/resources/ignore-search-with-credentials-iframe.html
[add] http://crrev.com/8cd93e585ae7f85b41a54b9328657ac0ab48eedb/third_party/WebKit/LayoutTests/http/tests/cachestorage/resources/ignore-search-with-credentials-worker.js
[modify] http://crrev.com/8cd93e585ae7f85b41a54b9328657ac0ab48eedb/third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-matchAll.js
[modify] http://crrev.com/8cd93e585ae7f85b41a54b9328657ac0ab48eedb/third_party/WebKit/LayoutTests/http/tests/cachestorage/serviceworker/cache-matchAll-expected.txt
[add] http://crrev.com/8cd93e585ae7f85b41a54b9328657ac0ab48eedb/third_party/WebKit/LayoutTests/http/tests/cachestorage/serviceworker/ignore-search-with-credentials.html
[modify] http://crrev.com/8cd93e585ae7f85b41a54b9328657ac0ab48eedb/third_party/WebKit/LayoutTests/http/tests/cachestorage/window/cache-match-expected.txt
[modify] http://crrev.com/8cd93e585ae7f85b41a54b9328657ac0ab48eedb/third_party/WebKit/LayoutTests/http/tests/cachestorage/window/cache-matchAll-expected.txt
[modify] http://crrev.com/8cd93e585ae7f85b41a54b9328657ac0ab48eedb/third_party/WebKit/LayoutTests/http/tests/cachestorage/worker/cache-match-expected.txt
[modify] http://crrev.com/8cd93e585ae7f85b41a54b9328657ac0ab48eedb/third_party/WebKit/LayoutTests/http/tests/cachestorage/worker/cache-matchAll-expected.txt
[modify] http://crrev.com/8cd93e585ae7f85b41a54b9328657ac0ab48eedb/third_party/WebKit/Source/modules/cachestorage/Cache.cpp
[modify] http://crrev.com/8cd93e585ae7f85b41a54b9328657ac0ab48eedb/third_party/WebKit/Source/modules/cachestorage/CacheStorage.cpp
[modify] http://crrev.com/8cd93e585ae7f85b41a54b9328657ac0ab48eedb/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in

Comment 16 by jmedley@chromium.org, Feb 22 2016

Labels: -M-49

Comment 18 by tauryy...@gmail.com, May 30 2016

It looks like CacheStorage && CacheQueryOptions are implemented in Chrome 51 and later but ignoreSearch:true option is doesn't work with CacheStorage.match() method at all.
Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#cache-storage-match

Comment 19 by nhiroki@chromium.org, May 30 2016

tauryyard: 'ignoreSearch' is still being implemented. Please see https://www.chromestatus.com/features/5915801131417600 for tracking the release status.

jinho.bang: Any updates on this? Do you have enough bandwidth to implement this?

Comment 20 by nekr.fab...@gmail.com, May 30 2016

There is polyfill for ignoreSearch, btw: https://github.com/NekR/sw-cache-options

Comment 21 by jinho.b...@samsung.com, May 31 2016

nhiroki@,

Ah, Sorry for delay. I have a progress CL.
If you are okay, I'll upload a new patch set in this weekend.

Comment 22 by bugdroid1@chromium.org, Aug 9 2016

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e6a4575cd18e48c18e7d5f5183e2fc17e537b725

commit e6a4575cd18e48c18e7d5f5183e2fc17e537b725
Author: jinho.bang <jinho.bang@samsung.com>
Date: Tue Aug 09 23:38:36 2016

CacheStorage: Expand cache.keys() method.

Current implementation of the method is always return all of keys even if it
takes optional parameters. According to ServiceWorkerSpec[1], the method should
take a request object and cache query options[2] as optional parameters.
This CL is expanding the method as per the spec.

[1] http://slightlyoff.github.io/ServiceWorker/spec/service_worker/#cache-keys
[2] Currenly, only supports ignoreSearch option like matchAll(), delete()

BUG= 520784 

Review-Url: https://codereview.chromium.org/1719103002
Cr-Commit-Position: refs/heads/master@{#410868}

[modify] https://crrev.com/e6a4575cd18e48c18e7d5f5183e2fc17e537b725/content/browser/cache_storage/cache_storage_cache.cc
[modify] https://crrev.com/e6a4575cd18e48c18e7d5f5183e2fc17e537b725/content/browser/cache_storage/cache_storage_cache.h
[modify] https://crrev.com/e6a4575cd18e48c18e7d5f5183e2fc17e537b725/content/browser/cache_storage/cache_storage_cache_unittest.cc
[modify] https://crrev.com/e6a4575cd18e48c18e7d5f5183e2fc17e537b725/content/browser/cache_storage/cache_storage_dispatcher_host.cc
[modify] https://crrev.com/e6a4575cd18e48c18e7d5f5183e2fc17e537b725/content/renderer/cache_storage/cache_storage_dispatcher.cc
[modify] https://crrev.com/e6a4575cd18e48c18e7d5f5183e2fc17e537b725/content/renderer/cache_storage/cache_storage_dispatcher.h
[modify] https://crrev.com/e6a4575cd18e48c18e7d5f5183e2fc17e537b725/third_party/WebKit/LayoutTests/http/tests/cachestorage/resources/test-helpers.js
[add] https://crrev.com/e6a4575cd18e48c18e7d5f5183e2fc17e537b725/third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-keys.js
[add] https://crrev.com/e6a4575cd18e48c18e7d5f5183e2fc17e537b725/third_party/WebKit/LayoutTests/http/tests/cachestorage/serviceworker/cache-keys.html
[modify] https://crrev.com/e6a4575cd18e48c18e7d5f5183e2fc17e537b725/third_party/WebKit/LayoutTests/http/tests/cachestorage/serviceworker/ignore-search-with-credentials.html
[modify] https://crrev.com/e6a4575cd18e48c18e7d5f5183e2fc17e537b725/third_party/WebKit/Source/modules/cachestorage/Cache.cpp
[modify] https://crrev.com/e6a4575cd18e48c18e7d5f5183e2fc17e537b725/third_party/WebKit/Source/modules/cachestorage/CacheTest.cpp
[modify] https://crrev.com/e6a4575cd18e48c18e7d5f5183e2fc17e537b725/third_party/WebKit/Source/modules/cachestorage/InspectorCacheStorageAgent.cpp
[modify] https://crrev.com/e6a4575cd18e48c18e7d5f5183e2fc17e537b725/third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerCache.h

Comment 23 by bugdroid1@chromium.org, Aug 22 2016

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4694c8366a320f4fa85f4781f8e2fad565d62ef4

commit 4694c8366a320f4fa85f4781f8e2fad565d62ef4
Author: jkarlin <jkarlin@chromium.org>
Date: Mon Aug 22 18:48:56 2016

[CacheStorage] Use QueryCache everywhere

This CL consolidates many code paths around the new QueryCache method. This CL:

1) Optimizes QueryCache in the case that only one lookup needs to be performed
2) Adds vary checking (and ignore_vary support) to QueryCache (previously was only in match)
3) Changes Cache::Delete to use QueryCache
4) Changes Cache::Put to doom rather than call Delete before inserting
5) Changes Cache::Match to use QueryCache (via MatchAll)
6) Adds unittests for all query params
7) Adds layout tests for all query params
8) Removes the RuntimeEnabledFeature for the ignoreQuery option

As a consequence of using QueryCache everywhere, options are fully supported and tested.

For reviewers: the good stuff is in cache_storage_cache.cc, most of the rest is testing.

BUG= 631978 , 426309 , 499216 , 482256 , 520784 

Review-Url: https://codereview.chromium.org/2242883002
Cr-Commit-Position: refs/heads/master@{#413487}

[modify] https://crrev.com/4694c8366a320f4fa85f4781f8e2fad565d62ef4/content/browser/cache_storage/cache_storage.cc
[modify] https://crrev.com/4694c8366a320f4fa85f4781f8e2fad565d62ef4/content/browser/cache_storage/cache_storage.h
[modify] https://crrev.com/4694c8366a320f4fa85f4781f8e2fad565d62ef4/content/browser/cache_storage/cache_storage_cache.cc
[modify] https://crrev.com/4694c8366a320f4fa85f4781f8e2fad565d62ef4/content/browser/cache_storage/cache_storage_cache.h
[modify] https://crrev.com/4694c8366a320f4fa85f4781f8e2fad565d62ef4/content/browser/cache_storage/cache_storage_cache_unittest.cc
[modify] https://crrev.com/4694c8366a320f4fa85f4781f8e2fad565d62ef4/content/browser/cache_storage/cache_storage_dispatcher_host.cc
[modify] https://crrev.com/4694c8366a320f4fa85f4781f8e2fad565d62ef4/content/browser/cache_storage/cache_storage_manager.cc
[modify] https://crrev.com/4694c8366a320f4fa85f4781f8e2fad565d62ef4/content/browser/cache_storage/cache_storage_manager.h
[modify] https://crrev.com/4694c8366a320f4fa85f4781f8e2fad565d62ef4/content/browser/cache_storage/cache_storage_manager_unittest.cc
[modify] https://crrev.com/4694c8366a320f4fa85f4781f8e2fad565d62ef4/content/browser/service_worker/service_worker_browsertest.cc
[modify] https://crrev.com/4694c8366a320f4fa85f4781f8e2fad565d62ef4/content/common/cache_storage/cache_storage_types.h
[modify] https://crrev.com/4694c8366a320f4fa85f4781f8e2fad565d62ef4/third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-delete.js
[modify] https://crrev.com/4694c8366a320f4fa85f4781f8e2fad565d62ef4/third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-keys.js
[modify] https://crrev.com/4694c8366a320f4fa85f4781f8e2fad565d62ef4/third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-match.js
[modify] https://crrev.com/4694c8366a320f4fa85f4781f8e2fad565d62ef4/third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-matchAll.js
[modify] https://crrev.com/4694c8366a320f4fa85f4781f8e2fad565d62ef4/third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-storage-match.js
[modify] https://crrev.com/4694c8366a320f4fa85f4781f8e2fad565d62ef4/third_party/WebKit/LayoutTests/http/tests/cachestorage/serviceworker/cache-matchAll-expected.txt
[modify] https://crrev.com/4694c8366a320f4fa85f4781f8e2fad565d62ef4/third_party/WebKit/LayoutTests/http/tests/cachestorage/window/cache-matchAll-expected.txt
[modify] https://crrev.com/4694c8366a320f4fa85f4781f8e2fad565d62ef4/third_party/WebKit/LayoutTests/http/tests/cachestorage/worker/cache-matchAll-expected.txt
[modify] https://crrev.com/4694c8366a320f4fa85f4781f8e2fad565d62ef4/third_party/WebKit/Source/modules/cachestorage/Cache.cpp
[modify] https://crrev.com/4694c8366a320f4fa85f4781f8e2fad565d62ef4/third_party/WebKit/Source/modules/cachestorage/CacheStorage.cpp
[modify] https://crrev.com/4694c8366a320f4fa85f4781f8e2fad565d62ef4/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in

Comment 24 by jkarlin@chromium.org, Aug 23 2016

Status: Fixed (was: Started)

Sign in to add a comment