Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(409)

Issue 1578363009: CacheStorage: Add ignoreSearch option to cache.matchAll(). (Closed)

Created:
4 years, 11 months ago by zino
Modified:
4 years, 10 months ago
Reviewers:
tkent, nhiroki
CC:
chromium-reviews, darin-cc_chromium.org, jam, jkarlin+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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 Committed: https://crrev.com/8cd93e585ae7f85b41a54b9328657ac0ab48eedb Cr-Commit-Position: refs/heads/master@{#375508}

Patch Set 1 #

Patch Set 2 : #

Total comments: 17

Patch Set 3 : addressed comments #

Total comments: 12

Patch Set 4 : #

Total comments: 18

Patch Set 5 : #

Total comments: 1

Patch Set 6 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+217 lines, -53 lines) Patch
M content/browser/cache_storage/cache_storage_cache.h View 1 2 3 4 5 2 chunks +7 lines, -5 lines 0 comments Download
M content/browser/cache_storage/cache_storage_cache.cc View 1 2 3 4 5 6 chunks +44 lines, -11 lines 0 comments Download
M content/browser/cache_storage/cache_storage_cache_unittest.cc View 1 2 3 4 5 5 chunks +53 lines, -4 lines 0 comments Download
M content/browser/cache_storage/cache_storage_dispatcher_host.cc View 1 2 3 2 chunks +8 lines, -0 lines 0 comments Download
A + third_party/WebKit/LayoutTests/http/tests/cachestorage/resources/ignore-search-with-credentials-iframe.html View 1 2 3 1 chunk +0 lines, -15 lines 0 comments Download
A third_party/WebKit/LayoutTests/http/tests/cachestorage/resources/ignore-search-with-credentials-worker.js View 1 2 3 4 1 chunk +13 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-matchAll.js View 1 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/cachestorage/serviceworker/cache-matchAll-expected.txt View 1 1 chunk +2 lines, -2 lines 0 comments Download
A third_party/WebKit/LayoutTests/http/tests/cachestorage/serviceworker/ignore-search-with-credentials.html View 1 2 3 4 1 chunk +79 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/cachestorage/window/cache-match-expected.txt View 1 2 3 1 chunk +0 lines, -2 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/cachestorage/window/cache-matchAll-expected.txt View 1 2 1 chunk +2 lines, -4 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/cachestorage/worker/cache-match-expected.txt View 1 2 3 1 chunk +0 lines, -2 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/cachestorage/worker/cache-matchAll-expected.txt View 1 2 1 chunk +2 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/modules/cachestorage/Cache.cpp View 1 2 3 4 5 3 chunks +3 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/modules/cachestorage/CacheStorage.cpp View 1 2 3 2 chunks +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 33 (13 generated)
zino
PTAL.
4 years, 11 months ago (2016-01-15 09:58:45 UTC) #4
zino
On 2016/01/15 09:58:45, zino wrote: > PTAL. FYI, we have to be able to use ...
4 years, 11 months ago (2016-01-15 10:02:40 UTC) #5
nhiroki
Thank you for working on this. https://codereview.chromium.org/1578363009/diff/20001/content/browser/cache_storage/cache_storage_cache.cc File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/1578363009/diff/20001/content/browser/cache_storage/cache_storage_cache.cc#newcode152 content/browser/cache_storage/cache_storage_cache.cc:152: request_url.host() == key.host() ...
4 years, 11 months ago (2016-01-18 04:29:59 UTC) #6
zino
I addressed your comments. Could you review again? Thank you! https://codereview.chromium.org/1578363009/diff/20001/content/browser/cache_storage/cache_storage_cache.cc File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/1578363009/diff/20001/content/browser/cache_storage/cache_storage_cache.cc#newcode152 ...
4 years, 11 months ago (2016-01-19 13:46:38 UTC) #9
nhiroki
https://codereview.chromium.org/1578363009/diff/20001/content/browser/cache_storage/cache_storage_cache.cc File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/1578363009/diff/20001/content/browser/cache_storage/cache_storage_cache.cc#newcode152 content/browser/cache_storage/cache_storage_cache.cc:152: request_url.host() == key.host() && request_url.path() == key.path(); On 2016/01/19 ...
4 years, 11 months ago (2016-01-20 08:56:02 UTC) #10
zino
PTAL. https://codereview.chromium.org/1578363009/diff/80001/content/browser/cache_storage/cache_storage_cache.cc File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/1578363009/diff/80001/content/browser/cache_storage/cache_storage_cache.cc#newcode150 content/browser/cache_storage/cache_storage_cache.cc:150: bool URLMatchWithoutQuery(const GURL& request_url, const GURL& key) { ...
4 years, 10 months ago (2016-02-03 12:04:48 UTC) #12
nhiroki
Sorry for the late reply. Looks good overall. https://codereview.chromium.org/1578363009/diff/120001/content/browser/cache_storage/cache_storage_cache_unittest.cc File content/browser/cache_storage/cache_storage_cache_unittest.cc (right): https://codereview.chromium.org/1578363009/diff/120001/content/browser/cache_storage/cache_storage_cache_unittest.cc#newcode388 content/browser/cache_storage/cache_storage_cache_unittest.cc:388: scoped_ptr<ServiceWorkerFetchRequest>(), ...
4 years, 10 months ago (2016-02-10 07:35:33 UTC) #13
zino
Thank you for review. https://codereview.chromium.org/1578363009/diff/120001/content/browser/cache_storage/cache_storage_cache_unittest.cc File content/browser/cache_storage/cache_storage_cache_unittest.cc (right): https://codereview.chromium.org/1578363009/diff/120001/content/browser/cache_storage/cache_storage_cache_unittest.cc#newcode388 content/browser/cache_storage/cache_storage_cache_unittest.cc:388: scoped_ptr<ServiceWorkerFetchRequest>(), CacheStorageCacheQueryParams(), On 2016/02/10 07:35:32, ...
4 years, 10 months ago (2016-02-11 03:35:05 UTC) #14
nhiroki
Can you upload the latest patchset? :)
4 years, 10 months ago (2016-02-12 04:18:33 UTC) #15
zino
On 2016/02/12 04:18:33, nhiroki (slow) wrote: > Can you upload the latest patchset? :) Ah, ...
4 years, 10 months ago (2016-02-12 08:28:41 UTC) #16
zino
+ tkent@ for Blink/Source/platform/RuntimeEnabledFeatures.in. PTAL. Thank you.
4 years, 10 months ago (2016-02-12 08:31:15 UTC) #18
zino
nhiroki@, PTAL. (if you forgot this CL.)
4 years, 10 months ago (2016-02-15 07:08:35 UTC) #19
nhiroki
LGTM! \o/ https://codereview.chromium.org/1578363009/diff/140001/third_party/WebKit/LayoutTests/http/tests/cachestorage/serviceworker/ignore-search-with-credentials.html File third_party/WebKit/LayoutTests/http/tests/cachestorage/serviceworker/ignore-search-with-credentials.html (right): https://codereview.chromium.org/1578363009/diff/140001/third_party/WebKit/LayoutTests/http/tests/cachestorage/serviceworker/ignore-search-with-credentials.html#newcode64 third_party/WebKit/LayoutTests/http/tests/cachestorage/serviceworker/ignore-search-with-credentials.html:64: // [1] https://fetch.spec.whatwg.org/#dom-request Good! Thank you for ...
4 years, 10 months ago (2016-02-15 08:55:10 UTC) #20
zino
tkent@, Could you review this? (Especially, need your stamp for third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in) Thank you.
4 years, 10 months ago (2016-02-15 09:07:29 UTC) #21
tkent
lgtm
4 years, 10 months ago (2016-02-15 23:02:29 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1578363009/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1578363009/140001
4 years, 10 months ago (2016-02-15 23:33:00 UTC) #24
commit-bot: I haz the power
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_compile_dbg_ng/builds/159475) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, ...
4 years, 10 months ago (2016-02-15 23:35:35 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1578363009/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1578363009/160001
4 years, 10 months ago (2016-02-16 01:39:37 UTC) #29
commit-bot: I haz the power
Committed patchset #6 (id:160001)
4 years, 10 months ago (2016-02-16 03:06:54 UTC) #31
commit-bot: I haz the power
4 years, 10 months ago (2016-02-16 22:51:14 UTC) #33
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/8cd93e585ae7f85b41a54b9328657ac0ab48eedb
Cr-Commit-Position: refs/heads/master@{#375508}

Powered by Google App Engine
This is Rietveld 408576698