CacheStorage: Implement Cache.matchAll()

CacheStorage: Implement Cache.matchAll()

This CL implements Cache.matchAll(). When an optional request is omitted,
CacheStorageCache iterates all responses in this cache and returns them. When
the optional request is given, the backend runs an existing implementation
for Cache.match().

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

(1) Blink: https://codereview.chromium.org/1268463003/
(2) Chromium: THIS PATCH
(3) Blink: https://codereview.chromium.org/1262773003/

BUG=428363
Committed: https://crrev.com/43e720b11886c21e531607e8152e30a23d6eaac9
Cr-Commit-Position: refs/heads/master@{#342359}

[nhiroki, 2015-08-03 10:47:45]

Patchset #4 (id:60001) has been deleted

[nhiroki, 2015-08-03 11:06:20]

Patchset #1 (id:1) has been deleted

[nhiroki, 2015-08-04 10:12:27]

Patchset #1 (id:20001) has been deleted

[nhiroki, 2015-08-04 10:12:32]

Patchset #1 (id:40001) has been deleted

[nhiroki, 2015-08-04 10:12:37]

Patchset #1 (id:80001) has been deleted

[nhiroki, 2015-08-04 10:19:56]

Patchset #1 (id:100001) has been deleted

[nhiroki, 2015-08-04 10:33:38]

nhiroki@chromium.org changed reviewers:
+ jkarlin@chromium.org

[nhiroki, 2015-08-04 10:33:39]

Hi jkarlin@, can you review this?

[jkarlin, 2015-08-04 13:25:07]

This looks good. Just a couple places we could reduce code duplication.

https://codereview.chromium.org/1248003004/diff/120001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_cache.cc (right):

https://codereview.chromium.org/1248003004/diff/120001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:624: void
CacheStorageCache::MatchAllDidOpenNextEntry(
This method should go, instead make a common utility function that MatchAll and
Keys can share to open all of the entries in a vector or list.

https://codereview.chromium.org/1248003004/diff/120001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:694: ServiceWorkerResponse
response(
The code below should be combined with MatchDidReadMetadata somehow. No need for
the duplication.

https://codereview.chromium.org/1248003004/diff/120001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_cache_unittest.cc (right):

https://codereview.chromium.org/1248003004/diff/120001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache_unittest.cc:663:
EXPECT_STREQ("OK", responses[0].status_text.c_str());
A "bool ResponsesEqual(const ServiceWorkerResponse&, const
ServiceWorkerResponse&) const" function would really simplify these tests.

[nhiroki, 2015-08-05 08:01:50]

Patchset #2 (id:140001) has been deleted

[nhiroki, 2015-08-05 08:03:04]

Thank you for reviewing. Updated. Can you take another look?

https://codereview.chromium.org/1248003004/diff/120001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_cache.cc (right):

https://codereview.chromium.org/1248003004/diff/120001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:624: void
CacheStorageCache::MatchAllDidOpenNextEntry(
On 2015/08/04 13:25:07, jkarlin wrote:
> This method should go, instead make a common utility function that MatchAll
and
> Keys can share to open all of the entries in a vector or list.

Done.

https://codereview.chromium.org/1248003004/diff/120001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:694: ServiceWorkerResponse
response(
On 2015/08/04 13:25:07, jkarlin wrote:
> The code below should be combined with MatchDidReadMetadata somehow. No need
for
> the duplication.

Done.

https://codereview.chromium.org/1248003004/diff/120001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_cache_unittest.cc (right):

https://codereview.chromium.org/1248003004/diff/120001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache_unittest.cc:663:
EXPECT_STREQ("OK", responses[0].status_text.c_str());
On 2015/08/04 13:25:07, jkarlin wrote:
> A "bool ResponsesEqual(const ServiceWorkerResponse&, const
> ServiceWorkerResponse&) const" function would really simplify these tests.

Done.

[jkarlin, 2015-08-05 12:09:05]

Almost there.

https://codereview.chromium.org/1248003004/diff/180001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_cache.cc (right):

https://codereview.chromium.org/1248003004/diff/180001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:179: void
PopulateServiceWorkerResponse(const CacheMetadata& metadata,
Perhaps rename to PopularResponseMetadata or
PopulateServiceWorkerResponseMetadata since it doesn't populate the body.

https://codereview.chromium.org/1248003004/diff/180001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:205: if (enumerated_entry)
Why is this in the for loop now?

https://codereview.chromium.org/1248003004/diff/180001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:231:
std::vector<ServiceWorkerResponse> out_responses;
scoped_ptr<std::vector<ServiceWorkerResponse>> to prevent copying the responses?

https://codereview.chromium.org/1248003004/diff/180001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:732: // Create a blob with
the response body data.
The block of code below is duplicated from MatchDidReadMetadata. Please merge
into a function.

https://codereview.chromium.org/1248003004/diff/180001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_cache.h (right):

https://codereview.chromium.org/1248003004/diff/180001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.h:52: const
std::vector<ServiceWorkerResponse>&,
Let's make this a scoped_ptr<std::vector<ServiceWorkerResponse>> to reduce
copying the responses. That makes it consistent with Keys as well.

https://codereview.chromium.org/1248003004/diff/180001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.h:74: // Returns
CACHE_STORAGE_OK and all responses in this cache. If there is no
s/there is no response/there are no responses/

https://codereview.chromium.org/1248003004/diff/180001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_cache_unittest.cc (right):

https://codereview.chromium.org/1248003004/diff/180001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache_unittest.cc:146: if
(actual.blob_uuid.empty())
Can you compare the blob contents of the two responses with
CacheStorageCacheTest::CopyBody here? That would simplify the tests further.
You'd have to move CacheStorageCacheTest::CopyBody into the unnamed namespace,
which is better anyway.

[nhiroki, 2015-08-06 03:34:24]

Patchset #4 (id:200001) has been deleted

[nhiroki, 2015-08-06 03:36:53]

Thank you for your comments! Updated.

https://codereview.chromium.org/1248003004/diff/180001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_cache.cc (right):

https://codereview.chromium.org/1248003004/diff/180001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:179: void
PopulateServiceWorkerResponse(const CacheMetadata& metadata,
On 2015/08/05 12:09:05, jkarlin wrote:
> Perhaps rename to PopularResponseMetadata or
> PopulateServiceWorkerResponseMetadata since it doesn't populate the body.

Good point. Renamed this.

https://codereview.chromium.org/1248003004/diff/180001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:205: if (enumerated_entry)
On 2015/08/05 12:09:05, jkarlin wrote:
> Why is this in the for loop now?

Oh..., this is just a mistake. Fixed. Thanks!

https://codereview.chromium.org/1248003004/diff/180001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:231:
std::vector<ServiceWorkerResponse> out_responses;
On 2015/08/05 12:09:05, jkarlin wrote:
> scoped_ptr<std::vector<ServiceWorkerResponse>> to prevent copying the
responses?

Done.

https://codereview.chromium.org/1248003004/diff/180001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:732: // Create a blob with
the response body data.
On 2015/08/05 12:09:05, jkarlin wrote:
> The block of code below is duplicated from MatchDidReadMetadata. Please merge
> into a function.

Factored out the block into PopulateResponseBody().

https://codereview.chromium.org/1248003004/diff/180001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_cache.h (right):

https://codereview.chromium.org/1248003004/diff/180001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.h:52: const
std::vector<ServiceWorkerResponse>&,
On 2015/08/05 12:09:05, jkarlin wrote:
> Let's make this a scoped_ptr<std::vector<ServiceWorkerResponse>> to reduce
> copying the responses. That makes it consistent with Keys as well.

Done.

https://codereview.chromium.org/1248003004/diff/180001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.h:74: // Returns
CACHE_STORAGE_OK and all responses in this cache. If there is no
On 2015/08/05 12:09:05, jkarlin wrote:
> s/there is no response/there are no responses/

Done.

https://codereview.chromium.org/1248003004/diff/180001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_cache_unittest.cc (right):

https://codereview.chromium.org/1248003004/diff/180001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache_unittest.cc:146: if
(actual.blob_uuid.empty())
On 2015/08/05 12:09:05, jkarlin wrote:
> Can you compare the blob contents of the two responses with
> CacheStorageCacheTest::CopyBody here? That would simplify the tests further.
> You'd have to move CacheStorageCacheTest::CopyBody into the unnamed namespace,
> which is better anyway.

Calling CopyBody() here might complicate this function because it requires two
more arguments (expected_body and actual_body_handle). Instead, I renamed this
function to "ResponseMetadataEqual()" and introduced a separate function
"ResponseBodiesEqual()" for comparing the blob contents. How does this look?

[jkarlin, 2015-08-06 15:11:13]

lgtm with changes

https://codereview.chromium.org/1248003004/diff/220001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_cache.h (right):

https://codereview.chromium.org/1248003004/diff/220001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.h:54:
ScopedVector<storage::BlobDataHandle>)>;
Likewise, please make this a scoped_ptr<std::vector<storage::BlobDataHandle>>?

https://codereview.chromium.org/1248003004/diff/220001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_cache_unittest.cc (right):

https://codereview.chromium.org/1248003004/diff/220001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache_unittest.cc:189: if
(expected_body != actual_body)
return expected_body == actual_body;

[nhiroki, 2015-08-07 13:10:41]

Patchset #6 (id:260001) has been deleted

[nhiroki, 2015-08-07 13:12:16]

Thank you!

https://codereview.chromium.org/1248003004/diff/220001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_cache.h (right):

https://codereview.chromium.org/1248003004/diff/220001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.h:54:
ScopedVector<storage::BlobDataHandle>)>;
On 2015/08/06 15:11:13, jkarlin wrote:
> Likewise, please make this a scoped_ptr<std::vector<storage::BlobDataHandle>>?

Done.

https://codereview.chromium.org/1248003004/diff/220001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_cache_unittest.cc (right):

https://codereview.chromium.org/1248003004/diff/220001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache_unittest.cc:189: if
(expected_body != actual_body)
On 2015/08/06 15:11:13, jkarlin wrote:
> return expected_body == actual_body;

Done.

[nhiroki, 2015-08-07 13:12:25]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2015-08-07 13:12:26]

The patchset sent to the CQ was uploaded after l-g-t-m from jkarlin@chromium.org
Link to the patchset: https://codereview.chromium.org/1248003004/#ps280001
(title: "replace ScopedVector<BlobDataHandle> with
scoped_ptr<std::vector<BlobDataHandle>>")

[commit-bot: I haz the power, 2015-08-07 13:12:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1248003004/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1248003004/280001

[commit-bot: I haz the power, 2015-08-07 14:33:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-07 14:33:54]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[nhiroki, 2015-08-07 14:35:12]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2015-08-07 14:35:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1248003004/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1248003004/280001

[commit-bot: I haz the power, 2015-08-07 15:39:19]

Committed patchset #6 (id:280001)

[commit-bot: I haz the power, 2015-08-07 15:40:07]

Patchset 6 (id:??) landed as
https://crrev.com/43e720b11886c21e531607e8152e30a23d6eaac9
Cr-Commit-Position: refs/heads/master@{#342359}