CacheStorage: Migrate to BindOnce/OnceCallback/OnceClosure

CacheStorage: Migrate to BindOnce/OnceCallback/OnceClosure

Switch to base::BindOnce/OnceCallback/OnceClosure to simplify code
(movable types) and make the API more explicit.

base::AdaptCallbackForRepeating is used in a handful of places to
adapt quota manager and disk cache APIs that have not been converted,
and a few lingering base::Bind() calls remain where there are API 
dependencies (e.g. quota client, base::CancelableClosure). 

base::AdaptCallbackForRepeating is also used to handle batch
operations where "first caller wins" semantics are required,
simplifying the existing code.

BUG=734632
R=cmumford@chromium.org,tzik@chromium.org

Review-Url: https://codereview.chromium.org/2947753002
Cr-Commit-Position: refs/heads/master@{#481305}
Committed: https://chromium.googlesource.com/chromium/src/+/f43e59e39557ee55c2d74d600a10c0d61ae17e36

[jsbell, 2017-06-19 23:26:33]

The CQ bit was checked by jsbell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-19 23:27:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jsbell, 2017-06-19 23:44:30]

Description was changed from

==========
CacheStorage: Migrate to BindOnce/OnceCallback/OnceClosure

Switch to base::BindOnce/OnceCallback/OnceClosure to simplify code
(movable types) and make the API more explicit.

base::AdaptCallbackForRepeating is used in a handful of places to
adapt quota manager and disk cache APIs that have not been converted.

BUG=734632
R=cmumford@chromium.org,jkarlin@chromium.org,tzik@chromium.org
==========

to

==========
CacheStorage: Migrate to BindOnce/OnceCallback/OnceClosure

Switch to base::BindOnce/OnceCallback/OnceClosure to simplify code
(movable types) and make the API more explicit.

base::AdaptCallbackForRepeating is used in a handful of places to
adapt quota manager and disk cache APIs that have not been converted,
and a few lingering base::Bind() calls remain where there are API dependencies
(e.g. quota client, base::CancelableClosure)

BUG=734632
R=cmumford@chromium.org,jkarlin@chromium.org,tzik@chromium.org
==========

[jsbell, 2017-06-19 23:45:00]

This is NOT ready for final review, but initial feedback is welcome. A few notes
for myself...

https://codereview.chromium.org/2947753002/diff/1/content/browser/cache_stora...
File content/browser/cache_storage/cache_storage_cache.cc (right):

https://codereview.chromium.org/2947753002/diff/1/content/browser/cache_stora...
content/browser/cache_storage/cache_storage_cache.cc:480: // TODO(jsbell):
Untangle this hideous mess.
I need to tackle this before landing.

https://codereview.chromium.org/2947753002/diff/1/content/browser/cache_stora...
File content/browser/cache_storage/cache_storage_cache.h (left):

https://codereview.chromium.org/2947753002/diff/1/content/browser/cache_stora...
content/browser/cache_storage/cache_storage_cache.h:212: // Returns all entries
in this cache.
This appeared to be dead code - there are no implementations. Did I miss
something?

https://codereview.chromium.org/2947753002/diff/1/content/browser/cache_stora...
File content/browser/cache_storage/cache_storage_scheduler.h (right):

https://codereview.chromium.org/2947753002/diff/1/content/browser/cache_stora...
content/browser/cache_storage/cache_storage_scheduler.h:55:
base::RepeatingCallback<void(Args...)> WrapCallbackToRunNext(
The overload is necessary to support background_sync which uses an instance of
CacheStorageScheduler. That should be converted over as well, but it can be a
follow-up CL.

[jsbell, 2017-06-19 23:51:16]

One more note-to-self

https://codereview.chromium.org/2947753002/diff/1/content/browser/cache_stora...
File content/browser/cache_storage/cache_storage_manager.cc (right):

https://codereview.chromium.org/2947753002/diff/1/content/browser/cache_stora...
content/browser/cache_storage/cache_storage_manager.cc:338:
base::Bind(&ListOriginsOnTaskRunner, root_path_),
If you convert this to base::BindOnce() it fails to compile, because the `const
Callback& callback` can't be automagically converted to a OnceCallback; I assume
that's due to the constness but I haven't dug in.

[commit-bot: I haz the power, 2017-06-20 00:24:24]

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

[commit-bot: I haz the power, 2017-06-20 00:24:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jsbell, 2017-06-20 22:57:04]

The CQ bit was checked by jsbell@chromium.org to run a CQ dry run

[jsbell, 2017-06-20 22:57:20]

Description was changed from

==========
CacheStorage: Migrate to BindOnce/OnceCallback/OnceClosure

Switch to base::BindOnce/OnceCallback/OnceClosure to simplify code
(movable types) and make the API more explicit.

base::AdaptCallbackForRepeating is used in a handful of places to
adapt quota manager and disk cache APIs that have not been converted,
and a few lingering base::Bind() calls remain where there are API dependencies
(e.g. quota client, base::CancelableClosure)

BUG=734632
R=cmumford@chromium.org,jkarlin@chromium.org,tzik@chromium.org
==========

to

==========
CacheStorage: Migrate to BindOnce/OnceCallback/OnceClosure

Switch to base::BindOnce/OnceCallback/OnceClosure to simplify code
(movable types) and make the API more explicit.

base::AdaptCallbackForRepeating is used in a handful of places to
adapt quota manager and disk cache APIs that have not been converted,
and a few lingering base::Bind() calls remain where there are API 
dependencies (e.g. quota client, base::CancelableClosure)

BUG=734632
R=cmumford@chromium.org,jkarlin@chromium.org,tzik@chromium.org
==========

[commit-bot: I haz the power, 2017-06-20 22:57:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jsbell, 2017-06-20 22:58:09]

Description was changed from

==========
CacheStorage: Migrate to BindOnce/OnceCallback/OnceClosure

Switch to base::BindOnce/OnceCallback/OnceClosure to simplify code
(movable types) and make the API more explicit.

base::AdaptCallbackForRepeating is used in a handful of places to
adapt quota manager and disk cache APIs that have not been converted,
and a few lingering base::Bind() calls remain where there are API 
dependencies (e.g. quota client, base::CancelableClosure)

BUG=734632
R=cmumford@chromium.org,jkarlin@chromium.org,tzik@chromium.org
==========

to

==========
CacheStorage: Migrate to BindOnce/OnceCallback/OnceClosure

Switch to base::BindOnce/OnceCallback/OnceClosure to simplify code
(movable types) and make the API more explicit.

base::AdaptCallbackForRepeating is used in a handful of places to
adapt quota manager and disk cache APIs that have not been converted,
and a few lingering base::Bind() calls remain where there are API 
dependencies (e.g. quota client, base::CancelableClosure). It is also
used to handle batch operations where "first caller wins" semantics
are required, simplifying the existing code.

BUG=734632
R=cmumford@chromium.org,jkarlin@chromium.org,tzik@chromium.org
==========

[jsbell, 2017-06-20 23:00:56]

tzik@ - I think this is ready for a real review. Can you take a look?

https://codereview.chromium.org/2947753002/diff/1/content/browser/cache_stora...
File content/browser/cache_storage/cache_storage_cache.cc (right):

https://codereview.chromium.org/2947753002/diff/1/content/browser/cache_stora...
content/browser/cache_storage/cache_storage_cache.cc:480: // TODO(jsbell):
Untangle this hideous mess.
On 2017/06/19 23:45:00, jsbell wrote:
> I need to tackle this before landing.

I reworked this in the latest CL, although I retained a dependency on
base::AdaptCallbackForRepeating because it captures the desired behavior better
than even the existing code. We may want to basically fork a copy of it for use
here, but it's also possible we'll redo the whole batch logic here at some point
anyway.

[jsbell, 2017-06-20 23:01:44]

Description was changed from

==========
CacheStorage: Migrate to BindOnce/OnceCallback/OnceClosure

Switch to base::BindOnce/OnceCallback/OnceClosure to simplify code
(movable types) and make the API more explicit.

base::AdaptCallbackForRepeating is used in a handful of places to
adapt quota manager and disk cache APIs that have not been converted,
and a few lingering base::Bind() calls remain where there are API 
dependencies (e.g. quota client, base::CancelableClosure). It is also
used to handle batch operations where "first caller wins" semantics
are required, simplifying the existing code.

BUG=734632
R=cmumford@chromium.org,jkarlin@chromium.org,tzik@chromium.org
==========

to

==========
CacheStorage: Migrate to BindOnce/OnceCallback/OnceClosure

Switch to base::BindOnce/OnceCallback/OnceClosure to simplify code
(movable types) and make the API more explicit.

base::AdaptCallbackForRepeating is used in a handful of places to
adapt quota manager and disk cache APIs that have not been converted,
and a few lingering base::Bind() calls remain where there are API 
dependencies (e.g. quota client, base::CancelableClosure). 

base::AdaptCallbackForRepeating is also used to handle batch
operations where "first caller wins" semantics are required,
simplifying the existing code.

BUG=734632
R=cmumford@chromium.org,jkarlin@chromium.org,tzik@chromium.org
==========

[commit-bot: I haz the power, 2017-06-21 01:50:32]

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

[commit-bot: I haz the power, 2017-06-21 01:50:33]

Dry run: This issue passed the CQ dry run.

[tzik, 2017-06-21 07:11:01]

LGTM!

https://codereview.chromium.org/2947753002/diff/1/content/browser/cache_stora...
File content/browser/cache_storage/cache_storage.cc (right):

https://codereview.chromium.org/2947753002/diff/1/content/browser/cache_stora...
content/browser/cache_storage/cache_storage.cc:1077:
base::Passed(std::move(cache_handle)), barrier_closure,
base::Passed here is not needed.
# feel free to leave this, as I will replace this pattern of Passed by a
refactoring tool.

[jsbell, 2017-06-21 15:54:15]

thanks tzik@

cmumford@ - can you also review as a cache storage expert?

[jsbell, 2017-06-21 15:54:40]

jsbell@chromium.org changed reviewers:
- jkarlin@chromium.org

[jsbell, 2017-06-21 15:54:57]

Description was changed from

==========
CacheStorage: Migrate to BindOnce/OnceCallback/OnceClosure

Switch to base::BindOnce/OnceCallback/OnceClosure to simplify code
(movable types) and make the API more explicit.

base::AdaptCallbackForRepeating is used in a handful of places to
adapt quota manager and disk cache APIs that have not been converted,
and a few lingering base::Bind() calls remain where there are API 
dependencies (e.g. quota client, base::CancelableClosure). 

base::AdaptCallbackForRepeating is also used to handle batch
operations where "first caller wins" semantics are required,
simplifying the existing code.

BUG=734632
R=cmumford@chromium.org,jkarlin@chromium.org,tzik@chromium.org
==========

to

==========
CacheStorage: Migrate to BindOnce/OnceCallback/OnceClosure

Switch to base::BindOnce/OnceCallback/OnceClosure to simplify code
(movable types) and make the API more explicit.

base::AdaptCallbackForRepeating is used in a handful of places to
adapt quota manager and disk cache APIs that have not been converted,
and a few lingering base::Bind() calls remain where there are API 
dependencies (e.g. quota client, base::CancelableClosure). 

base::AdaptCallbackForRepeating is also used to handle batch
operations where "first caller wins" semantics are required,
simplifying the existing code.

BUG=734632
R=cmumford@chromium.org,tzik@chromium.org
==========

[cmumford, 2017-06-21 19:07:57]

https://codereview.chromium.org/2947753002/diff/20001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage.h (right):

https://codereview.chromium.org/2947753002/diff/20001/content/browser/cache_s...
content/browser/cache_storage/cache_storage.h:212: base::OnceClosure closure,
Should this be a RepeatingClosure?

https://codereview.chromium.org/2947753002/diff/20001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_cache.cc (right):

https://codereview.chromium.org/2947753002/diff/20001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_cache.cc:1481: int rv =
backend_->CalculateSizeOfAllEntries(size_callback);
Don't you lose copy elision by converting to a local variable? Do you need to
std::move(size_callback)?

[jsbell, 2017-06-21 20:38:16]

Thanks for the questions! This stuff is subtle and I'm still wrapping my head
around it too.

https://codereview.chromium.org/2947753002/diff/20001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage.h (right):

https://codereview.chromium.org/2947753002/diff/20001/content/browser/cache_s...
content/browser/cache_storage/cache_storage.h:212: base::OnceClosure closure,
On 2017/06/21 19:07:56, cmumford wrote:
> Should this be a RepeatingClosure?

This method only calls it once, so it can take a OnceClosure.

The caller does pass in a BarrierClosure but the casting from RepeatingClosure
to OnceClosure does the right thing - this callee is only allowed to invoke it
once.

https://codereview.chromium.org/2947753002/diff/20001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_cache.cc (right):

https://codereview.chromium.org/2947753002/diff/20001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_cache.cc:1481: int rv =
backend_->CalculateSizeOfAllEntries(size_callback);
On 2017/06/21 19:07:56, cmumford wrote:
> Don't you lose copy elision by converting to a local variable? Do you need to
> std::move(size_callback)?

Nope - because size_callback is a RepeatingClosure.

The subsequent std::move(size_callback) on line 1483 is unnecessary here (it
could just be `size_callback.Run(rv)`) but is harmless.

[cmumford, 2017-06-21 20:40:51]

lgtm

[jsbell, 2017-06-21 21:03:36]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2017-06-21 21:04:06]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-21 21:12:27]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1498079016312030,
"parent_rev": "5d17220520e4bd0a7121f0886a4d432d5ae5a1e4", "commit_rev":
"ad4c9c799ec849d30fed09da21213c240aa63b76"}

[commit-bot: I haz the power, 2017-06-21 21:12:42]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1498079016312030,
"parent_rev": "6ef13fcd8a45a85d2afc436c5ebd7d9fd435f4b5", "commit_rev":
"f43e59e39557ee55c2d74d600a10c0d61ae17e36"}

[commit-bot: I haz the power, 2017-06-21 21:12:57]

Description was changed from

==========
CacheStorage: Migrate to BindOnce/OnceCallback/OnceClosure

Switch to base::BindOnce/OnceCallback/OnceClosure to simplify code
(movable types) and make the API more explicit.

base::AdaptCallbackForRepeating is used in a handful of places to
adapt quota manager and disk cache APIs that have not been converted,
and a few lingering base::Bind() calls remain where there are API 
dependencies (e.g. quota client, base::CancelableClosure). 

base::AdaptCallbackForRepeating is also used to handle batch
operations where "first caller wins" semantics are required,
simplifying the existing code.

BUG=734632
R=cmumford@chromium.org,tzik@chromium.org
==========

to

==========
CacheStorage: Migrate to BindOnce/OnceCallback/OnceClosure

Switch to base::BindOnce/OnceCallback/OnceClosure to simplify code
(movable types) and make the API more explicit.

base::AdaptCallbackForRepeating is used in a handful of places to
adapt quota manager and disk cache APIs that have not been converted,
and a few lingering base::Bind() calls remain where there are API 
dependencies (e.g. quota client, base::CancelableClosure). 

base::AdaptCallbackForRepeating is also used to handle batch
operations where "first caller wins" semantics are required,
simplifying the existing code.

BUG=734632
R=cmumford@chromium.org,tzik@chromium.org

Review-Url: https://codereview.chromium.org/2947753002
Cr-Commit-Position: refs/heads/master@{#481305}
Committed:
https://chromium.googlesource.com/chromium/src/+/f43e59e39557ee55c2d74d600a10...
==========

[commit-bot: I haz the power, 2017-06-21 21:12:59]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/f43e59e39557ee55c2d74d600a10...