Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1694213002:
CacheStorage: Add ignoreSearch option to cache.delete(). (Closed)
|
Created:
4 years, 10 months ago by zino Modified:
4 years, 10 months ago 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. |
DescriptionCacheStorage: 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 #
Messages
Total messages: 25 (7 generated)
Description was changed from ========== Delete Impl BUG= ========== to ========== 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#ca... BUG=520784 ==========
jinho.bang@samsung.com changed reviewers: + nhiroki@chromium.org
PTAL. Thank you. https://codereview.chromium.org/1694213002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-delete.js (left): https://codereview.chromium.org/1694213002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-delete.js:67: var cache_entries = { I'm not sure but these codes looks unnecessary. https://codereview.chromium.org/1694213002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-delete.js:84: function prepopulated_cache_test(test_function, description) { This test doesn't run actually. So, I removed it.
Looks good. Nit comments only. https://codereview.chromium.org/1694213002/diff/20001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/1694213002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:1038: RemoveQueryParam(request->url), callback)); I'd prefer to lazily remove the query param at line 1072 because it'd be somewhat unclear for a sourcecode reader why we need to remove it here. https://codereview.chromium.org/1694213002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-delete.js (left): https://codereview.chromium.org/1694213002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-delete.js:67: var cache_entries = { On 2016/02/16 06:45:44, zino wrote: > I'm not sure but these codes looks unnecessary. Acknowledged. https://codereview.chromium.org/1694213002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-delete.js:84: function prepopulated_cache_test(test_function, description) { On 2016/02/16 06:45:44, zino wrote: > This test doesn't run actually. So, I removed it. Acknowledged. https://codereview.chromium.org/1694213002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-delete.js (right): https://codereview.chromium.org/1694213002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-delete.js:78: { ignoreSearch: true }); Can you add a test for the case where 'ignoreSearch: false' is explicitly specified? cache.delete(entries.a_with_query.request, { ignoreSearch: false })
Thank you for review. https://codereview.chromium.org/1694213002/diff/20001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/1694213002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:1038: RemoveQueryParam(request->url), callback)); On 2016/02/16 08:47:23, nhiroki (slow) wrote: > I'd prefer to lazily remove the query param at line 1072 because it'd be > somewhat unclear for a sourcecode reader why we need to remove it here. Yeah.. I can't understand why I did that.. Done. Thank you. https://codereview.chromium.org/1694213002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-delete.js (right): https://codereview.chromium.org/1694213002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-delete.js:78: { ignoreSearch: true }); On 2016/02/16 08:47:23, nhiroki (slow) wrote: > Can you add a test for the case where 'ignoreSearch: false' is explicitly > specified? > > cache.delete(entries.a_with_query.request, { ignoreSearch: false }) Done.
Can you add tests in cache_storage_cache_unittest.cc like MatchAll? (Sorry, I should have commented it in the first review round) https://codereview.chromium.org/1694213002/diff/40001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/1694213002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:1077: UpdateCacheSize(); I wonder if we need to clear |entries_context| before UpdateCacheSize(). DeleteDidOpenEntry() does so. Can you ask jkarlin@ who implemented the cache size calculation mechanism to double-check? See: https://codereview.chromium.org/1681653002
On 2016/02/16 09:51:48, nhiroki (slow) wrote: > Can you add tests in cache_storage_cache_unittest.cc like MatchAll? > > (Sorry, I should have commented it in the first review round) > > https://codereview.chromium.org/1694213002/diff/40001/content/browser/cache_s... > File content/browser/cache_storage/cache_storage_cache.cc (right): > > https://codereview.chromium.org/1694213002/diff/40001/content/browser/cache_s... > content/browser/cache_storage/cache_storage_cache.cc:1077: UpdateCacheSize(); > I wonder if we need to clear |entries_context| before UpdateCacheSize(). > DeleteDidOpenEntry() does so. Can you ask jkarlin@ who implemented the cache > size calculation mechanism to double-check? > > See: https://codereview.chromium.org/1681653002 okay no problem. (I had to write the test first.. :) ) I'll write the tests and check it.
jinho.bang@samsung.com changed reviewers: + jkarlin@chromium.org
On 2016/02/16 09:53:34, zino wrote: > On 2016/02/16 09:51:48, nhiroki (slow) wrote: > > Can you add tests in cache_storage_cache_unittest.cc like MatchAll? > > > > (Sorry, I should have commented it in the first review round) > > > > > https://codereview.chromium.org/1694213002/diff/40001/content/browser/cache_s... > > File content/browser/cache_storage/cache_storage_cache.cc (right): > > > > > https://codereview.chromium.org/1694213002/diff/40001/content/browser/cache_s... > > content/browser/cache_storage/cache_storage_cache.cc:1077: UpdateCacheSize(); > > I wonder if we need to clear |entries_context| before UpdateCacheSize(). > > DeleteDidOpenEntry() does so. Can you ask jkarlin@ who implemented the cache > > size calculation mechanism to double-check? > > > > See: https://codereview.chromium.org/1681653002 > > okay no problem. (I had to write the test first.. :) ) > I'll write the tests and check it. + jkarlin@ as reviewer. Could you give me some input?
https://codereview.chromium.org/1694213002/diff/40001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/1694213002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:1077: UpdateCacheSize(); On 2016/02/16 09:51:47, nhiroki (slow) wrote: > I wonder if we need to clear |entries_context| before UpdateCacheSize(). > DeleteDidOpenEntry() does so. Can you ask jkarlin@ who implemented the cache > size calculation mechanism to double-check? > > See: https://codereview.chromium.org/1681653002 It does need to be cleared first. UpdateCacheSize() will run synchronously in the case of an in-memory cache backend (incognito mode). If the entries are still open here then they won't be deleted from the cache before UpdateCacheSize runs. This would have been caught by a CacheStorageCacheTestP unittest as those run both disk and in-memory backends. +1 for unittest request :) https://codereview.chromium.org/1694213002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-delete.js (right): https://codereview.chromium.org/1694213002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-delete.js:101: // method has the same behavior when not taking ignoreSearch option. Perhaps: // cache.delete()'s behavior should be the same if ignoreSearch is not provided or if ignoreSearch is false. https://codereview.chromium.org/1694213002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-delete.js:113: 'Cache.delete with ignoreSearch option (when is specified as false)'); s/when is/when it is/
Thank you for review. PTAL. https://codereview.chromium.org/1694213002/diff/40001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/1694213002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:1077: UpdateCacheSize(); On 2016/02/16 14:54:37, jkarlin (OOO til 24th) wrote: > On 2016/02/16 09:51:47, nhiroki (slow) wrote: > > I wonder if we need to clear |entries_context| before UpdateCacheSize(). > > DeleteDidOpenEntry() does so. Can you ask jkarlin@ who implemented the cache > > size calculation mechanism to double-check? > > > > See: https://codereview.chromium.org/1681653002 > > It does need to be cleared first. UpdateCacheSize() will run synchronously in > the case of an in-memory cache backend (incognito mode). If the entries are > still open here then they won't be deleted from the cache before UpdateCacheSize > runs. > > This would have been caught by a CacheStorageCacheTestP unittest as those run > both disk and in-memory backends. +1 for unittest request :) Done. https://codereview.chromium.org/1694213002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-delete.js (right): https://codereview.chromium.org/1694213002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-delete.js:101: // method has the same behavior when not taking ignoreSearch option. On 2016/02/16 14:54:37, jkarlin (OOO til 24th) wrote: > Perhaps: > // cache.delete()'s behavior should be the same if ignoreSearch is not provided > or if ignoreSearch is false. Done. https://codereview.chromium.org/1694213002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-delete.js:113: 'Cache.delete with ignoreSearch option (when is specified as false)'); On 2016/02/16 14:54:37, jkarlin (OOO til 24th) wrote: > s/when is/when it is/ Done.
lgtm
On 2016/02/20 01:42:24, nhiroki (slow until Feb. 24) wrote: > lgtm Can I wait for jkarlin@'s review?
On 2016/02/23 12:54:35, zino wrote: > On 2016/02/20 01:42:24, nhiroki (slow until Feb. 24) wrote: > > lgtm > > Can I wait for jkarlin@'s review? Should I..
https://codereview.chromium.org/1694213002/diff/60001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache_unittest.cc (right): https://codereview.chromium.org/1694213002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache_unittest.cc:413: CacheStorageCacheQueryParams()) { I see that default arguments are now allowed by the style guide, cool. https://codereview.chromium.org/1694213002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache_unittest.cc:1045: EXPECT_LT(0, Size()); Prefer to use Keys() instead of Size() in this test to determine exactly what is in the cache. https://codereview.chromium.org/1694213002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache_unittest.cc:1055: // The following delete operation will remove both of body_reqeust_ and typo: s/body_reqeust_/body_request_/ https://codereview.chromium.org/1694213002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache_unittest.cc:1060: } Add a second test that deletes with default options to verify that only one is deleted.
Thank you for review. Please take a look again. https://codereview.chromium.org/1694213002/diff/60001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache_unittest.cc (right): https://codereview.chromium.org/1694213002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache_unittest.cc:1045: EXPECT_LT(0, Size()); On 2016/02/24 16:35:34, jkarlin wrote: > Prefer to use Keys() instead of Size() in this test to determine exactly what is > in the cache. Done. https://codereview.chromium.org/1694213002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache_unittest.cc:1055: // The following delete operation will remove both of body_reqeust_ and On 2016/02/24 16:35:34, jkarlin wrote: > typo: s/body_reqeust_/body_request_/ Nice catch! Thank you. Done. https://codereview.chromium.org/1694213002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache_unittest.cc:1060: } On 2016/02/24 16:35:34, jkarlin wrote: > Add a second test that deletes with default options to verify that only one is > deleted. Done.
lgtm https://codereview.chromium.org/1694213002/diff/80001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache_unittest.cc (right): https://codereview.chromium.org/1694213002/diff/80001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache_unittest.cc:949: EXPECT_TRUE(Keys()); This check before the puts is unnecessary https://codereview.chromium.org/1694213002/diff/80001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache_unittest.cc:979: EXPECT_EQ(0u, callback_strings_.size()); Ditto
Thanks. https://codereview.chromium.org/1694213002/diff/80001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache_unittest.cc (right): https://codereview.chromium.org/1694213002/diff/80001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache_unittest.cc:949: EXPECT_TRUE(Keys()); On 2016/02/24 18:14:03, jkarlin wrote: > This check before the puts is unnecessary Done. https://codereview.chromium.org/1694213002/diff/80001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache_unittest.cc:979: EXPECT_EQ(0u, callback_strings_.size()); On 2016/02/24 18:14:03, jkarlin wrote: > Ditto Done.
The CQ bit was checked by jinho.bang@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from nhiroki@chromium.org, jkarlin@chromium.org Link to the patchset: https://codereview.chromium.org/1694213002/#ps100001 (title: "Remove unnecessary check")
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
Message was sent while issue was closed.
Description was changed from ========== 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#ca... BUG=520784 ========== to ========== 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#ca... BUG=520784 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== 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#ca... BUG=520784 ========== to ========== 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#ca... BUG=520784 Committed: https://crrev.com/a248727a88e8e99042cf8d6fed0dfba77433f3f5 Cr-Commit-Position: refs/heads/master@{#377396} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/a248727a88e8e99042cf8d6fed0dfba77433f3f5 Cr-Commit-Position: refs/heads/master@{#377396} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
