[CacheStorage] Simpler way to wrap callbacks to run pending operation completions

[CacheStorage] Simpler way to wrap callbacks to run pending operation completions

CacheStorage (and BackgroundSync) wrap callbacks in order to ensure that the scheduler
is bumped after running the callback. This resulted in lots of helper functions. This
CL cleans up the helper functions with the help of template magic.

BUG=629592
Committed: https://crrev.com/3555fc8a7880f6c132292689694aa1925dee461b
Cr-Commit-Position: refs/heads/master@{#406668}

[jkarlin, 2016-07-19 19:03:08]

jkarlin@chromium.org changed reviewers:
+ iclelland@chromium.org, michaeln@chromium.org

[jkarlin, 2016-07-19 19:03:09]

michaeln@ PTAL at cache_storage/

iclelland@ PTAL at background_sync/

Thanks!

[jkarlin, 2016-07-19 19:04:23]

Description was changed from

==========
[CacheStorage] Simpler way to wrap callbacks to run pending operation
completions

CacheStorage (and BackgroundSync) wrap callbacks in order to ensure that the
scheduler is bumped after running the callback. This resulted in lots of
extraneous helper functions. This can all be cleaned up with some template
functions.

BUG=
==========

to

==========
[CacheStorage] Simpler way to wrap callbacks to run pending operation
completions

CacheStorage (and BackgroundSync) wrap callbacks in order to ensure that the
scheduler
is bumped after running the callback. This resulted in lots of helper functions.
This
CL cleans up the helper functions with the help of template magic.
==========

[jkarlin, 2016-07-19 19:05:49]

Description was changed from

==========
[CacheStorage] Simpler way to wrap callbacks to run pending operation
completions

CacheStorage (and BackgroundSync) wrap callbacks in order to ensure that the
scheduler
is bumped after running the callback. This resulted in lots of helper functions.
This
CL cleans up the helper functions with the help of template magic.
==========

to

==========
[CacheStorage] Simpler way to wrap callbacks to run pending operation
completions

CacheStorage (and BackgroundSync) wrap callbacks in order to ensure that the
scheduler
is bumped after running the callback. This resulted in lots of helper functions.
This
CL cleans up the helper functions with the help of template magic.

BUG=629592
==========

[jkarlin, 2016-07-19 19:05:54]

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

[commit-bot: I haz the power, 2016-07-19 19:07:20]

Dry run: 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, 2016-07-19 20:14:26]

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

[commit-bot: I haz the power, 2016-07-19 20:14:27]

Dry run: This issue passed the CQ dry run.

[michaeln, 2016-07-19 23:45:25]

nice cleanup

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

https://codereview.chromium.org/2167483002/diff/1/content/browser/cache_stora...
content/browser/cache_storage/cache_storage_scheduler.h:54: R
RunNextContinuation(const base::Callback<R(Args...)>& callback,
should this function return a value of type 'R'? how does this compile? R must
always be void. seems like it from the callsiites.

would this template work with just parametrizing Args?

template <typename... Args>
void RunNextContinuation(const base::Callback<void(Args...)>& ...

ditto for the WrapCallback method?

[jkarlin, 2016-07-20 00:51:13]

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

[jkarlin, 2016-07-20 00:51:51]

PTAL

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

https://codereview.chromium.org/2167483002/diff/1/content/browser/cache_stora...
content/browser/cache_storage/cache_storage_scheduler.h:54: R
RunNextContinuation(const base::Callback<R(Args...)>& callback,
On 2016/07/19 23:45:25, michaeln wrote:
> should this function return a value of type 'R'? how does this compile? R must
> always be void. seems like it from the callsiites.
> 
> would this template work with just parametrizing Args?
> 
> template <typename... Args>
> void RunNextContinuation(const base::Callback<void(Args...)>& ...
> 
> ditto for the WrapCallback method?

Good catch on specifying void. Done.

[commit-bot: I haz the power, 2016-07-20 00:52:03]

Dry run: 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, 2016-07-20 01:03:32]

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

[commit-bot: I haz the power, 2016-07-20 01:03:33]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jkarlin, 2016-07-20 02:30:26]

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

[commit-bot: I haz the power, 2016-07-20 02:30:50]

Dry run: 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, 2016-07-20 04:15:44]

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

[commit-bot: I haz the power, 2016-07-20 04:15:46]

Dry run: This issue passed the CQ dry run.

[jkarlin, 2016-07-20 11:17:46]

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

[jkarlin, 2016-07-20 11:18:16]

Removed iclelland@ as he is OOO. 

michaeln@ PTAL at everything, thanks!

[michaeln, 2016-07-20 18:42:16]

lgtm % comment

https://codereview.chromium.org/2167483002/diff/40001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_scheduler.h (right):

https://codereview.chromium.org/2167483002/diff/40001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_scheduler.h:42: // Wraps |callback|
to first call CompleteOperationAndRunNext before running.
Comment is a little misleading, maybe you meant to say "before returning", the
comment sounds like its a head patch instead of a tail patch. I think you don't
really need to describe the order of execution because that should be obvious.
Mabye just...

... to also call CompleteOperationAndRunNext.

[jkarlin, 2016-07-20 19:07:31]

Thanks, done.

https://codereview.chromium.org/2167483002/diff/40001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_scheduler.h (right):

https://codereview.chromium.org/2167483002/diff/40001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_scheduler.h:42: // Wraps |callback|
to first call CompleteOperationAndRunNext before running.
On 2016/07/20 18:42:16, michaeln wrote:
> Comment is a little misleading, maybe you meant to say "before returning", the
> comment sounds like its a head patch instead of a tail patch. I think you
don't
> really need to describe the order of execution because that should be obvious.
> Mabye just...
> 
> ... to also call CompleteOperationAndRunNext.
> 

Done.

[jkarlin, 2016-07-20 19:07:48]

The CQ bit was checked by jkarlin@chromium.org

[jkarlin, 2016-07-20 19:07:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org
Link to the patchset: https://codereview.chromium.org/2167483002/#ps60001
(title: "Comment clarification")

[commit-bot: I haz the power, 2016-07-20 19:08:25]

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, 2016-07-20 19:40:41]

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

[commit-bot: I haz the power, 2016-07-20 19:40:42]

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

[jkarlin, 2016-07-20 20:34:38]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2016-07-20 20:35:15]

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, 2016-07-20 21:00:00]

Description was changed from

==========
[CacheStorage] Simpler way to wrap callbacks to run pending operation
completions

CacheStorage (and BackgroundSync) wrap callbacks in order to ensure that the
scheduler
is bumped after running the callback. This resulted in lots of helper functions.
This
CL cleans up the helper functions with the help of template magic.

BUG=629592
==========

to

==========
[CacheStorage] Simpler way to wrap callbacks to run pending operation
completions

CacheStorage (and BackgroundSync) wrap callbacks in order to ensure that the
scheduler
is bumped after running the callback. This resulted in lots of helper functions.
This
CL cleans up the helper functions with the help of template magic.

BUG=629592
==========

[commit-bot: I haz the power, 2016-07-20 21:00:02]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-20 21:02:10]

Description was changed from

==========
[CacheStorage] Simpler way to wrap callbacks to run pending operation
completions

CacheStorage (and BackgroundSync) wrap callbacks in order to ensure that the
scheduler
is bumped after running the callback. This resulted in lots of helper functions.
This
CL cleans up the helper functions with the help of template magic.

BUG=629592
==========

to

==========
[CacheStorage] Simpler way to wrap callbacks to run pending operation
completions

CacheStorage (and BackgroundSync) wrap callbacks in order to ensure that the
scheduler
is bumped after running the callback. This resulted in lots of helper functions.
This
CL cleans up the helper functions with the help of template magic.

BUG=629592

Committed: https://crrev.com/3555fc8a7880f6c132292689694aa1925dee461b
Cr-Commit-Position: refs/heads/master@{#406668}
==========

[commit-bot: I haz the power, 2016-07-20 21:02:11]

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