Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(78)

Issue 1694213002: CacheStorage: Add ignoreSearch option to cache.delete(). (Closed)

Created:
3 years, 1 month ago by zino
Modified:
3 years ago
Reviewers:
jkarlin, nhiroki
CC:
chromium-reviews, darin-cc_chromium.org, jam, jkarlin+watch_chromium.org, nhiroki
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.delete(). If using cache.delete() with ignoreSearch option, delete all caches that match request url without query string from cache storage. Spec: https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#cache-query-options-dictionary BUG=520784 Committed: https://crrev.com/a248727a88e8e99042cf8d6fed0dfba77433f3f5 Cr-Commit-Position: refs/heads/master@{#377396}

Patch Set 1 #

Patch Set 2 : #

Total comments: 8

Patch Set 3 : #

Total comments: 7

Patch Set 4 : #

Total comments: 7

Patch Set 5 : addressed comments #

Total comments: 4

Patch Set 6 : Remove unnecessary check #

Unified diffs Side-by-side diffs Delta from patch set Stats (+153 lines, -28 lines) Patch
M content/browser/cache_storage/cache_storage_cache.h View 1 2 3 4 1 chunk +6 lines, -0 lines 0 comments Download
M content/browser/cache_storage/cache_storage_cache.cc View 1 2 3 4 3 chunks +36 lines, -2 lines 0 comments Download
M content/browser/cache_storage/cache_storage_cache_unittest.cc View 1 2 3 4 5 2 chunks +59 lines, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-delete.js View 1 2 3 1 chunk +44 lines, -25 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/cachestorage/serviceworker/ignore-search-with-credentials.html View 1 1 chunk +8 lines, -0 lines 0 comments Download

Messages

Total messages: 25 (7 generated)
zino
PTAL. Thank you. https://codereview.chromium.org/1694213002/diff/20001/third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-delete.js File third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-delete.js (left): https://codereview.chromium.org/1694213002/diff/20001/third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-delete.js#oldcode67 third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-delete.js:67: var cache_entries = { I'm not ...
3 years, 1 month ago (2016-02-16 06:45:44 UTC) #3
nhiroki
Looks good. Nit comments only. https://codereview.chromium.org/1694213002/diff/20001/content/browser/cache_storage/cache_storage_cache.cc File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/1694213002/diff/20001/content/browser/cache_storage/cache_storage_cache.cc#newcode1038 content/browser/cache_storage/cache_storage_cache.cc:1038: RemoveQueryParam(request->url), callback)); I'd prefer ...
3 years, 1 month ago (2016-02-16 08:47:23 UTC) #4
zino
Thank you for review. https://codereview.chromium.org/1694213002/diff/20001/content/browser/cache_storage/cache_storage_cache.cc File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/1694213002/diff/20001/content/browser/cache_storage/cache_storage_cache.cc#newcode1038 content/browser/cache_storage/cache_storage_cache.cc:1038: RemoveQueryParam(request->url), callback)); On 2016/02/16 08:47:23, ...
3 years, 1 month ago (2016-02-16 09:23:09 UTC) #5
nhiroki
Can you add tests in cache_storage_cache_unittest.cc like MatchAll? (Sorry, I should have commented it in ...
3 years, 1 month ago (2016-02-16 09:51:48 UTC) #6
zino
On 2016/02/16 09:51:48, nhiroki (slow) wrote: > Can you add tests in cache_storage_cache_unittest.cc like MatchAll? ...
3 years, 1 month ago (2016-02-16 09:53:34 UTC) #7
zino
On 2016/02/16 09:53:34, zino wrote: > On 2016/02/16 09:51:48, nhiroki (slow) wrote: > > Can ...
3 years, 1 month ago (2016-02-16 10:01:59 UTC) #9
jkarlin
https://codereview.chromium.org/1694213002/diff/40001/content/browser/cache_storage/cache_storage_cache.cc File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/1694213002/diff/40001/content/browser/cache_storage/cache_storage_cache.cc#newcode1077 content/browser/cache_storage/cache_storage_cache.cc:1077: UpdateCacheSize(); On 2016/02/16 09:51:47, nhiroki (slow) wrote: > I ...
3 years, 1 month ago (2016-02-16 14:54:37 UTC) #10
zino
Thank you for review. PTAL. https://codereview.chromium.org/1694213002/diff/40001/content/browser/cache_storage/cache_storage_cache.cc File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/1694213002/diff/40001/content/browser/cache_storage/cache_storage_cache.cc#newcode1077 content/browser/cache_storage/cache_storage_cache.cc:1077: UpdateCacheSize(); On 2016/02/16 14:54:37, ...
3 years ago (2016-02-18 08:12:48 UTC) #11
nhiroki
lgtm
3 years ago (2016-02-20 01:42:24 UTC) #12
zino
On 2016/02/20 01:42:24, nhiroki (slow until Feb. 24) wrote: > lgtm Can I wait for ...
3 years ago (2016-02-23 12:54:35 UTC) #13
zino
On 2016/02/23 12:54:35, zino wrote: > On 2016/02/20 01:42:24, nhiroki (slow until Feb. 24) wrote: ...
3 years ago (2016-02-23 12:54:50 UTC) #14
jkarlin
https://codereview.chromium.org/1694213002/diff/60001/content/browser/cache_storage/cache_storage_cache_unittest.cc File content/browser/cache_storage/cache_storage_cache_unittest.cc (right): https://codereview.chromium.org/1694213002/diff/60001/content/browser/cache_storage/cache_storage_cache_unittest.cc#newcode413 content/browser/cache_storage/cache_storage_cache_unittest.cc:413: CacheStorageCacheQueryParams()) { I see that default arguments are now ...
3 years ago (2016-02-24 16:35:34 UTC) #15
zino
Thank you for review. Please take a look again. https://codereview.chromium.org/1694213002/diff/60001/content/browser/cache_storage/cache_storage_cache_unittest.cc File content/browser/cache_storage/cache_storage_cache_unittest.cc (right): https://codereview.chromium.org/1694213002/diff/60001/content/browser/cache_storage/cache_storage_cache_unittest.cc#newcode1045 content/browser/cache_storage/cache_storage_cache_unittest.cc:1045: ...
3 years ago (2016-02-24 18:05:22 UTC) #16
jkarlin
lgtm https://codereview.chromium.org/1694213002/diff/80001/content/browser/cache_storage/cache_storage_cache_unittest.cc File content/browser/cache_storage/cache_storage_cache_unittest.cc (right): https://codereview.chromium.org/1694213002/diff/80001/content/browser/cache_storage/cache_storage_cache_unittest.cc#newcode949 content/browser/cache_storage/cache_storage_cache_unittest.cc:949: EXPECT_TRUE(Keys()); This check before the puts is unnecessary ...
3 years ago (2016-02-24 18:14:03 UTC) #17
zino
Thanks. https://codereview.chromium.org/1694213002/diff/80001/content/browser/cache_storage/cache_storage_cache_unittest.cc File content/browser/cache_storage/cache_storage_cache_unittest.cc (right): https://codereview.chromium.org/1694213002/diff/80001/content/browser/cache_storage/cache_storage_cache_unittest.cc#newcode949 content/browser/cache_storage/cache_storage_cache_unittest.cc:949: EXPECT_TRUE(Keys()); On 2016/02/24 18:14:03, jkarlin wrote: > This ...
3 years ago (2016-02-24 18:17:58 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1694213002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1694213002/100001
3 years ago (2016-02-24 18:23:59 UTC) #21
commit-bot: I haz the power
Committed patchset #6 (id:100001)
3 years ago (2016-02-24 22:09:20 UTC) #23
commit-bot: I haz the power
3 years ago (2016-02-24 22:11:10 UTC) #25
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/a248727a88e8e99042cf8d6fed0dfba77433f3f5
Cr-Commit-Position: refs/heads/master@{#377396}

Powered by Google App Engine
This is Rietveld 408576698