CacheStorage: Support multiple batch operations

CacheStorage: Support multiple batch operations

This is groundwork for supporting cache.addAll() [1]. The current backend
implementation assumes that a cache IPC message contains just one batch
operation, so the backend cannot handle multiple batch operations issued
by cache.addAll().

To support them, this CL introduces CacheStorageCache::BatchOperation which
is an entry point for put/delete operations and roughly corresponds to the
BatchCacheOperations algorithm in the spec [2].

According to the spec, batch operations should atomically be done, but this CL
does not support it yet.

[1] https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#cache-addAll
[2] https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#batch-cache-operations-algorithm

BUG=426928, 427652
TEST=content_unittests --gtest_filter=CacheStorage*
TEST=run-webkit-tests http/tests/cachestorage/

Committed: https://crrev.com/d04e05cd1fafc6c666ff07a30f94bca781fe5d60
Cr-Commit-Position: refs/heads/master@{#329121}

[nhiroki, 2015-05-07 07:41:18]

Patchset #1 (id:1) has been deleted

[nhiroki, 2015-05-07 07:41:25]

Patchset #1 (id:20001) has been deleted

[nhiroki, 2015-05-07 07:41:32]

Patchset #1 (id:40001) has been deleted

[nhiroki, 2015-05-07 07:41:38]

Patchset #1 (id:60001) has been deleted

[nhiroki, 2015-05-07 07:41:44]

Patchset #1 (id:80001) has been deleted

[nhiroki, 2015-05-07 07:41:50]

Patchset #1 (id:100001) has been deleted

[nhiroki, 2015-05-07 07:59:15]

Patchset #1 (id:120001) has been deleted

[nhiroki, 2015-05-07 08:01:51]

Patchset #1 (id:140001) has been deleted

[nhiroki, 2015-05-07 08:01:59]

Patchset #1 (id:160001) has been deleted

[nhiroki, 2015-05-07 08:02:05]

Patchset #1 (id:180001) has been deleted

[nhiroki, 2015-05-07 08:08:45]

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

[nhiroki, 2015-05-07 08:13:09]

Hi jkarlin@, can you review this?

If this is on the right track, I'll add unittests for multiple put operations.
Thanks!

[jkarlin, 2015-05-07 13:06:16]

Nice. Looking forward to the unittests.

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

https://codereview.chromium.org/1113303003/diff/200001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:167: class
BatchOperationManager : public base::RefCounted<BatchOperationManager> {
Why not use a BarrierClosure instead? An example very similar to this is
CacheStorage::MatchAllCaches.

https://codereview.chromium.org/1113303003/diff/200001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:173: void
OnOperationComplete(CacheStorageCache::ErrorType error) {
DCHECK_GE(0, num_pending_operations_)

https://codereview.chromium.org/1113303003/diff/200001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:176: if
(--num_pending_operations_ == 0 ||
This will cause num_pending_operations_ to go negative if the constructor was
called with |num_pending_operations| = 0.

https://codereview.chromium.org/1113303003/diff/200001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:483: NOTREACHED();
Run the callback with an error here.

[nhiroki, 2015-05-08 03:45:12]

Patchset #2 (id:220001) has been deleted

[nhiroki, 2015-05-08 04:00:58]

Thank you for reviewing! Added tests. Can you take another look?

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

https://codereview.chromium.org/1113303003/diff/200001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:167: class
BatchOperationManager : public base::RefCounted<BatchOperationManager> {
On 2015/05/07 13:06:16, jkarlin wrote:
> Why not use a BarrierClosure instead? An example very similar to this is
> CacheStorage::MatchAllCaches.

Ah, I didn't know MatchAllCaches does the similar thing. That way looks good. I
replaced BatchOperationManager with BarrierClosure.

https://codereview.chromium.org/1113303003/diff/200001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:173: void
OnOperationComplete(CacheStorageCache::ErrorType error) {
On 2015/05/07 13:06:16, jkarlin wrote:
> DCHECK_GE(0, num_pending_operations_)

(Removed this part)

https://codereview.chromium.org/1113303003/diff/200001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:176: if
(--num_pending_operations_ == 0 ||
On 2015/05/07 13:06:16, jkarlin wrote:
> This will cause num_pending_operations_ to go negative if the constructor was
> called with |num_pending_operations| = 0.

(Removed this part)

https://codereview.chromium.org/1113303003/diff/200001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:483: NOTREACHED();
On 2015/05/07 13:06:16, jkarlin wrote:
> Run the callback with an error here.

Done.

According to the spec, this case should return "TypeError", but currently
CacheStorageCache::ErrorType does not support the type. As a workaround
"ERROR_TYPE_STORAGE" is passed.

https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#ba...

[jkarlin, 2015-05-08 11:45:43]

Almost there, just a nit and a couple more tests requested.

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

https://codereview.chromium.org/1113303003/diff/240001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.h:81: void
BatchDidOperation(const base::Closure& barrier_closure,
nit: Suggest BatchDidOneOperation. I hope to change the matchall callback's name
similarly.

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

https://codereview.chromium.org/1113303003/diff/240001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache_unittest.cc:568: 
Also add a test that if the first batch operation (of several) fails it still
returns an error.

https://codereview.chromium.org/1113303003/diff/240001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache_unittest.cc:568: 
Add a test to show that you can mix delete and put operations in the same batch
call.

[jkarlin, 2015-05-08 12:04:26]

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

https://codereview.chromium.org/1113303003/diff/240001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.h:78: // algorithm in the
spec.
Add a TODO and a corresponding crbug to make this function atomic.

[nhiroki, 2015-05-08 15:09:02]

Thank you for reviewing! This contains only comment replies. I'll update the
patch later.

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

https://codereview.chromium.org/1113303003/diff/240001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache_unittest.cc:568: 
On 2015/05/08 11:45:43, jkarlin wrote:
> Add a test to show that you can mix delete and put operations in the same
batch
> call.

The current implementation has "DCHECK_EQ(1u, operations.size())" in
CacheStorageCache::BatchOperation() to avoid such mixed operations.

I'm not really sure that we need to support mixed operations because the spec
does not define the case. I suppose that mixed operations make it difficult to
handle batch results because a delete operation can return NOT_FOUND as a
success case.

https://codereview.chromium.org/1113303003/diff/240001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache_unittest.cc:568: 
On 2015/05/08 11:45:43, jkarlin wrote:
> Also add a test that if the first batch operation (of several) fails it still
> returns an error. 

Do you have any ideas to fail only one operation in the batch? I tried to add
such a test using an invalid request/response whose protocol is not supported
(eg. "ftp://") but the current backend implementation does not check the
protocol and successfully completes the operation...

(If there isn't a reasonable way, is it acceptable to revisit this after adding
those security checks in the backend?)

[jkarlin, 2015-05-08 15:27:27]

lgtm with nits

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

https://codereview.chromium.org/1113303003/diff/240001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.h:78: // algorithm in the
spec.
Add a comment that operations can't be mixed. Describe what the function does a
bit more (rather than just pointing to spec). Describe what errors are returned
and when.

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

https://codereview.chromium.org/1113303003/diff/240001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache_unittest.cc:568: 
On 2015/05/08 15:09:01, nhiroki wrote:
> On 2015/05/08 11:45:43, jkarlin wrote:
> > Add a test to show that you can mix delete and put operations in the same
> batch
> > call.
> 
> The current implementation has "DCHECK_EQ(1u, operations.size())" in
> CacheStorageCache::BatchOperation() to avoid such mixed operations.
> 
> I'm not really sure that we need to support mixed operations because the spec
> does not define the case. I suppose that mixed operations make it difficult to
> handle batch results because a delete operation can return NOT_FOUND as a
> success case.

Good point. Never mind on this test.

https://codereview.chromium.org/1113303003/diff/240001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache_unittest.cc:568: 
On 2015/05/08 15:09:01, nhiroki wrote:
> On 2015/05/08 11:45:43, jkarlin wrote:
> > Also add a test that if the first batch operation (of several) fails it
still
> > returns an error. 
> 
> Do you have any ideas to fail only one operation in the batch? I tried to add
> such a test using an invalid request/response whose protocol is not supported
> (eg. "ftp://") but the current backend implementation does not check the
> protocol and successfully completes the operation...

> 
> (If there isn't a reasonable way, is it acceptable to revisit this after
adding
> those security checks in the backend?)

Yes, please add a TODO to add the test and add a bug to add the security checks
on the browser side.

[nhiroki, 2015-05-11 07:35:04]

Thank you! Updated.

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

https://codereview.chromium.org/1113303003/diff/240001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.h:78: // algorithm in the
spec.
On 2015/05/08 12:04:25, jkarlin wrote:
> Add a TODO and a corresponding crbug to make this function atomic.

Done.

https://codereview.chromium.org/1113303003/diff/240001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.h:78: // algorithm in the
spec.
On 2015/05/08 15:27:27, jkarlin wrote:
> Add a comment that operations can't be mixed. Describe what the function does
a
> bit more (rather than just pointing to spec). Describe what errors are
returned
> and when.

Done.

https://codereview.chromium.org/1113303003/diff/240001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.h:81: void
BatchDidOperation(const base::Closure& barrier_closure,
On 2015/05/08 11:45:43, jkarlin wrote:
> nit: Suggest BatchDidOneOperation. I hope to change the matchall callback's
name
> similarly.

Done.

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

https://codereview.chromium.org/1113303003/diff/240001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache_unittest.cc:568: 
On 2015/05/08 15:27:27, jkarlin wrote:
> On 2015/05/08 15:09:01, nhiroki wrote:
> > On 2015/05/08 11:45:43, jkarlin wrote:
> > > Also add a test that if the first batch operation (of several) fails it
> still
> > > returns an error. 
> > 
> > Do you have any ideas to fail only one operation in the batch? I tried to
add
> > such a test using an invalid request/response whose protocol is not
supported
> > (eg. "ftp://") but the current backend implementation does not check the
> > protocol and successfully completes the operation...
> 
> > 
> > (If there isn't a reasonable way, is it acceptable to revisit this after
> adding
> > those security checks in the backend?)
> 
> Yes, please add a TODO to add the test and add a bug to add the security
checks
> on the browser side.

Done.

[nhiroki, 2015-05-11 07:35:24]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2015-05-11 07:35:24]

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/1113303003/#ps280001
(title: "add comments")

[commit-bot: I haz the power, 2015-05-11 07:40:28]

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

[commit-bot: I haz the power, 2015-05-11 08:42:23]

Committed patchset #4 (id:280001)

[commit-bot: I haz the power, 2015-05-11 08:43:20]

Patchset 4 (id:??) landed as
https://crrev.com/d04e05cd1fafc6c666ff07a30f94bca781fe5d60
Cr-Commit-Position: refs/heads/master@{#329121}