TaskScheduler: migrate appcache database over to use it.

TaskScheduler: migrate appcache database over to use it.

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

[michaeln, 2017-06-16 01:32:05]

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

[commit-bot: I haz the power, 2017-06-16 01:32:43]

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, 2017-06-16 02:01:40]

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

[commit-bot: I haz the power, 2017-06-16 02:01:41]

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

[michaeln, 2017-06-17 03:25:04]

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

[commit-bot: I haz the power, 2017-06-17 03:25:10]

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, 2017-06-17 04:26:39]

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

[commit-bot: I haz the power, 2017-06-17 04:26:39]

Dry run: This issue passed the CQ dry run.

[michaeln, 2017-06-19 18:10:28]

Description was changed from

==========
taskScheduler

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

to

==========
TaskScheduler: migrate appcache database over to use it.
==========

[michaeln, 2017-06-19 18:10:29]

michaeln@chromium.org changed reviewers:
+ gab@chromium.org

[michaeln, 2017-06-19 18:12:18]

hi gab, ptal

[michaeln, 2017-06-19 20:50:24]

michaeln@chromium.org changed reviewers:
+ jsbell@chromium.org

[gab, 2017-06-20 21:29:55]

lgtm % nits and further improvement suggestion, thanks!

https://codereview.chromium.org/2940023002/diff/40001/content/browser/appcach...
File content/browser/appcache/appcache_service_impl.cc (right):

https://codereview.chromium.org/2940023002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_service_impl.cc:414: db_task_runner_ =
base::CreateSequencedTaskRunnerWithTraits(
Initialize in initializer-list above.

https://codereview.chromium.org/2940023002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_service_impl.cc:436: const
scoped_refptr<base::SingleThreadTaskRunner>& cache_thread) {
Can we get rid of cache_thread as well in follow-up CL?

https://codereview.chromium.org/2940023002/diff/40001/content/browser/appcach...
File content/browser/appcache/appcache_storage_impl.cc (right):

https://codereview.chromium.org/2940023002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_storage_impl.cc:1450: db_task_runner_ =
db_task_runner;
std::move

https://codereview.chromium.org/2940023002/diff/40001/content/browser/appcach...
File content/browser/appcache/appcache_storage_impl_unittest.cc (right):

https://codereview.chromium.org/2940023002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_storage_impl_unittest.cc:382:
base::ThreadTaskRunnerHandle::Get()->PostTask(
This could perhaps use SequencedTaskRunnerHandle::Get()?

[michaeln, 2017-07-11 00:33:54]

https://codereview.chromium.org/2940023002/diff/40001/content/browser/appcach...
File content/browser/appcache/appcache_service_impl.cc (right):

https://codereview.chromium.org/2940023002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_service_impl.cc:414: db_task_runner_ =
base::CreateSequencedTaskRunnerWithTraits(
On 2017/06/20 21:29:54, gab (in-out til july17) wrote:
> Initialize in initializer-list above.

Done.

https://codereview.chromium.org/2940023002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_service_impl.cc:436: const
scoped_refptr<base::SingleThreadTaskRunner>& cache_thread) {
On 2017/06/20 21:29:54, gab (in-out til july17) wrote:
> Can we get rid of cache_thread as well in follow-up CL?

yup, i think somebody else is signed up to take care of the cache thread across
its multiple consumers, i'll check up on how that's going and/or see about
carving this part out of that

https://codereview.chromium.org/2940023002/diff/40001/content/browser/appcach...
File content/browser/appcache/appcache_storage_impl.cc (right):

https://codereview.chromium.org/2940023002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_storage_impl.cc:1450: db_task_runner_ =
db_task_runner;
On 2017/06/20 21:29:54, gab (in-out til july17) wrote:
> std::move

i don't think its meaningful since db_task_runner is a const ref?

https://codereview.chromium.org/2940023002/diff/40001/content/browser/appcach...
File content/browser/appcache/appcache_storage_impl_unittest.cc (right):

https://codereview.chromium.org/2940023002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_storage_impl_unittest.cc:382:
base::ThreadTaskRunnerHandle::Get()->PostTask(
On 2017/06/20 21:29:54, gab (in-out til july17) wrote:
> This could perhaps use SequencedTaskRunnerHandle::Get()?

Done throughout the file

[michaeln, 2017-07-11 00:34:11]

The CQ bit was checked by michaeln@chromium.org

[michaeln, 2017-07-11 00:34:12]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org
Link to the patchset: https://codereview.chromium.org/2940023002/#ps80001
(title: "comments + rebase")

[commit-bot: I haz the power, 2017-07-11 00:34:32]

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-07-11 02:11:29]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1499733251725930,
"parent_rev": "a87cc6b7d71388830b2c56fdc188dddb9b6ef9c1", "commit_rev":
"be6b241a17964378fb18c62efbb56de7b781a725"}

[commit-bot: I haz the power, 2017-07-11 02:11:46]

Description was changed from

==========
TaskScheduler: migrate appcache database over to use it.
==========

to

==========
TaskScheduler: migrate appcache database over to use it.

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

[commit-bot: I haz the power, 2017-07-11 02:11:47]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/be6b241a17964378fb18c62efbb5...

[boliu, 2017-07-24 19:23:29]

boliu@chromium.org changed reviewers:
+ boliu@chromium.org

[boliu, 2017-07-24 19:23:30]

Hi, I'm looking at migrating QuotaManager. And I found that AppCacheStorageImpl
uses QuotaManager. Is this currently broken, and QuotaManager and
AppCacheStorageImpl should really be using the same db sequence?

I can't quite tell but I *think* GetUsageAndQuota is called on the IO thread, in
which case AppCache and QuotaManager don't need to share the same db sequence?
Would be super confusing though, two task runners that sounds like they are the
same aren't..

[jsbell, 2017-07-24 19:55:50]

Drive by response... hopefully I get the details right.

On 2017/07/24 19:23:30, boliu wrote:
> Hi, I'm looking at migrating QuotaManager. And I found that
AppCacheStorageImpl
> uses QuotaManager. Is this currently broken, and QuotaManager and
> AppCacheStorageImpl should really be using the same db sequence?

QuotaManager and AppCache are talking to separate databases, so they should be
able to use separate sequences.

Much of the AppCache logic runs on the IO thread; its internal database work
(now) runs on the db_task_runner_ (c/o task scheduler). See
AppCacheStorageImpl::DatabaseTask::Schedule for an example (runs on the IO
thread, schedules work on the db_task_runner)

Similarly, QuotaManager expects to be called on the IO thread for the most part,
and schedules work against its own DB thread. (I assume that's what you're
migrating, not migrating QuotaManager off of IO?)

> I can't quite tell but I *think* GetUsageAndQuota is called on the IO thread

Yes. GetQuotaThenSchedule eventually calls Schedule which asserts sequence
affinity. More DCHECKs about sequence affinity in both QuotaManager and AppCache
would be wonderful.

> in
> which case AppCache and QuotaManager don't need to share the same db sequence?

Each has a separate database, which can run separately.

> Would be super confusing though, two task runners that sounds like they are
the
> same aren't..

Names are confusing, but since there's no global database for Chrome (or
content, or ...) it's somewhat implicit - if not obvious - that QuotaManager's
db_runner is for talking to the quota database, and AppCache's db_runner is for
talking to the appcache database.