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

Issue 2242883002: [CacheStorage] Use QueryCache everywhere (Closed)

Created:
2 years, 9 months ago by jkarlin
Modified:
2 years, 9 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.

Description

[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 Committed: https://crrev.com/4694c8366a320f4fa85f4781f8e2fad565d62ef4 Cr-Commit-Position: refs/heads/master@{#413487}

Patch Set 1 #

Patch Set 2 : Fixes #

Patch Set 3 : Windows fix #

Patch Set 4 : Rebase #

Patch Set 5 : Fix memory leak #

Patch Set 6 : Put dooms instead of deletes #

Patch Set 7 : Unit tests of options #

Patch Set 8 : Layout tests #

Patch Set 9 : Nits #

Total comments: 26

Patch Set 10 : Address comments from PS9 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+772 lines, -265 lines) Patch
M content/browser/cache_storage/cache_storage.h View 1 2 3 4 5 6 7 8 9 4 chunks +4 lines, -0 lines 0 comments Download
M content/browser/cache_storage/cache_storage.cc View 1 2 3 4 5 6 7 8 9 7 chunks +8 lines, -3 lines 0 comments Download
M content/browser/cache_storage/cache_storage_cache.h View 1 2 3 4 5 6 7 8 6 chunks +22 lines, -19 lines 0 comments Download
M content/browser/cache_storage/cache_storage_cache.cc View 1 2 3 4 5 6 7 8 9 11 chunks +109 lines, -146 lines 0 comments Download
M content/browser/cache_storage/cache_storage_cache_unittest.cc View 1 2 3 4 5 6 10 chunks +174 lines, -24 lines 0 comments Download
M content/browser/cache_storage/cache_storage_dispatcher_host.cc View 4 chunks +4 lines, -4 lines 0 comments Download
M content/browser/cache_storage/cache_storage_manager.h View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
M content/browser/cache_storage/cache_storage_manager.cc View 1 chunk +5 lines, -2 lines 0 comments Download
M content/browser/cache_storage/cache_storage_manager_unittest.cc View 1 2 3 4 5 6 7 8 9 6 chunks +165 lines, -25 lines 0 comments Download
M content/browser/service_worker/service_worker_browsertest.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M content/common/cache_storage/cache_storage_types.h View 1 2 3 1 chunk +3 lines, -3 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-delete.js View 1 2 3 4 5 6 7 1 chunk +31 lines, -2 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-keys.js View 1 2 3 4 5 6 7 8 9 1 chunk +56 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-match.js View 1 2 3 4 5 6 7 8 1 chunk +50 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-matchAll.js View 1 2 3 4 5 6 7 8 9 2 chunks +53 lines, -2 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-storage-match.js View 1 2 3 4 5 6 7 8 1 chunk +75 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/cachestorage/serviceworker/cache-matchAll-expected.txt View 1 2 3 4 5 6 7 1 chunk +3 lines, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/cachestorage/window/cache-matchAll-expected.txt View 1 2 3 4 5 6 7 2 chunks +3 lines, -2 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/cachestorage/worker/cache-matchAll-expected.txt View 1 2 3 4 5 6 7 2 chunks +3 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/modules/cachestorage/Cache.cpp View 1 2 3 4 5 6 7 8 9 7 chunks +1 line, -16 lines 0 comments Download
M third_party/WebKit/Source/modules/cachestorage/CacheStorage.cpp View 1 2 3 4 5 6 7 8 9 3 chunks +0 lines, -12 lines 0 comments Download
M third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in View 1 2 3 4 5 6 7 8 9 1 chunk +0 lines, -1 line 0 comments Download

Depends on Patchset:

Messages

Total messages: 67 (57 generated)
jkarlin
nhiroki@ PTAL at everything cmumford@ FYI
2 years, 9 months ago (2016-08-16 18:28:33 UTC) #42
nhiroki
LGTM https://codereview.chromium.org/2242883002/diff/160001/content/browser/cache_storage/cache_storage.cc File content/browser/cache_storage/cache_storage.cc (right): https://codereview.chromium.org/2242883002/diff/160001/content/browser/cache_storage/cache_storage.cc#newcode611 content/browser/cache_storage/cache_storage.cc:611: blank line https://codereview.chromium.org/2242883002/diff/160001/content/browser/cache_storage/cache_storage.h File content/browser/cache_storage/cache_storage.h (right): https://codereview.chromium.org/2242883002/diff/160001/content/browser/cache_storage/cache_storage.h#newcode103 content/browser/cache_storage/cache_storage.h:103: ...
2 years, 9 months ago (2016-08-18 00:05:43 UTC) #45
jkarlin
dglazkov@chromium.org: Please review changes in RuntimeEnabledFeatures.in, thanks! Some context: These CacheStorage query options have been ...
2 years, 9 months ago (2016-08-18 14:08:24 UTC) #50
jkarlin
dglazkov@ PTAL, thanks!
2 years, 9 months ago (2016-08-19 11:20:31 UTC) #51
jkarlin
+jochen@ PTAL at runtimeenabledfeatures.in. The idl is unchanged, this CL just makes the (lgtm'd long ...
2 years, 9 months ago (2016-08-22 12:47:43 UTC) #57
jochen (gone - plz use gerrit)
lgtm
2 years, 9 months ago (2016-08-22 15:51:36 UTC) #59
jkarlin
Thanks!
2 years, 9 months ago (2016-08-22 16:27:23 UTC) #60
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2242883002/200001
2 years, 9 months ago (2016-08-22 16:28:02 UTC) #63
commit-bot: I haz the power
Committed patchset #10 (id:200001)
2 years, 9 months ago (2016-08-22 18:49:12 UTC) #65
commit-bot: I haz the power
2 years, 9 months ago (2016-08-22 18:50:50 UTC) #67
Message was sent while issue was closed.
Patchset 10 (id:??) landed as
https://crrev.com/4694c8366a320f4fa85f4781f8e2fad565d62ef4
Cr-Commit-Position: refs/heads/master@{#413487}

Powered by Google App Engine
This is Rietveld 408576698