Convert PpdCache and PpdProvider to TaskScheduler.

Convert PpdCache and PpdProvider to TaskScheduler.

Part of the larger Chromium migration away from BrowserThreads.

BUG=734279, 667892
Review-Url: https://codereview.chromium.org/2939373003
Cr-Commit-Position: refs/heads/master@{#480963}
Committed: https://chromium.googlesource.com/chromium/src/+/4fb6e69738018eda7b9d387934d35188dd46da17

[skau, 2017-06-17 01:04:35]

skau@chromium.org changed reviewers:
+ gab@chromium.org, justincarlson@chromium.org

[skau, 2017-06-17 01:04:35]

justincarlson@: package OWNER
gab@: Task stuff

[skau, 2017-06-17 01:23:05]

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

[commit-bot: I haz the power, 2017-06-17 01:23:16]

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 02:00:53]

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

[commit-bot: I haz the power, 2017-06-17 02:00:54]

Dry run: 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_...)

[gab, 2017-06-19 15:13:22]

Thanks :)

https://codereview.chromium.org/2939373003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/ppd_provider_factory.cc (right):

https://codereview.chromium.org/2939373003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/ppd_provider_factory.cc:28: cache);
inline |cache| here? (or at least std::move() it)

https://codereview.chromium.org/2939373003/diff/20001/chromeos/printing/ppd_c...
File chromeos/printing/ppd_cache.cc (right):

https://codereview.chromium.org/2939373003/diff/20001/chromeos/printing/ppd_c...
chromeos/printing/ppd_cache.cc:60: store_task_runner_->PostTask(
It's weird to run read/write on different sequences, especially when binding
|this| as that will make access to |this| racy. Looks like the *Impl() methods
don't access member state though? Could they be anonymous methods? If so, that'd
be fine.

https://codereview.chromium.org/2939373003/diff/20001/chromeos/printing/ppd_c...
chromeos/printing/ppd_cache.cc:77: // Find implementation, runs on the i/o
thread.
Update comment here and below

https://codereview.chromium.org/2939373003/diff/20001/chromeos/printing/ppd_p...
File chromeos/printing/ppd_provider.cc (right):

https://codereview.chromium.org/2939373003/diff/20001/chromeos/printing/ppd_p...
chromeos/printing/ppd_provider.cc:211: {base::TaskPriority::USER_BLOCKING,
Are you sure this is USER_BLOCKING? It was previously using the FILE thread
which wasn't appropriate for user blocking work (was that a bug?)

[skau, 2017-06-19 23:30:14]

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

[commit-bot: I haz the power, 2017-06-19 23:30:46]

Dry run: CQ is trying da patch.

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

[skau, 2017-06-19 23:31:08]

Revisions to fix the tests and some simplifications.  PTAL.

https://codereview.chromium.org/2939373003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/ppd_provider_factory.cc (right):

https://codereview.chromium.org/2939373003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/ppd_provider_factory.cc:28: cache);
On 2017/06/19 15:13:21, gab wrote:
> inline |cache| here? (or at least std::move() it)

Done.

https://codereview.chromium.org/2939373003/diff/20001/chromeos/printing/ppd_c...
File chromeos/printing/ppd_cache.cc (right):

https://codereview.chromium.org/2939373003/diff/20001/chromeos/printing/ppd_c...
chromeos/printing/ppd_cache.cc:60: store_task_runner_->PostTask(
On 2017/06/19 15:13:21, gab wrote:
> It's weird to run read/write on different sequences, especially when binding
> |this| as that will make access to |this| racy. Looks like the *Impl() methods
> don't access member state though? Could they be anonymous methods? If so,
that'd
> be fine.

I was able to convert the tasks to anonymous methods.  The two operations have
different needs and they're not expected to be sequenced.

https://codereview.chromium.org/2939373003/diff/20001/chromeos/printing/ppd_c...
chromeos/printing/ppd_cache.cc:77: // Find implementation, runs on the i/o
thread.
On 2017/06/19 15:13:22, gab wrote:
> Update comment here and below

Done.

https://codereview.chromium.org/2939373003/diff/20001/chromeos/printing/ppd_p...
File chromeos/printing/ppd_provider.cc (right):

https://codereview.chromium.org/2939373003/diff/20001/chromeos/printing/ppd_p...
chromeos/printing/ppd_provider.cc:211: {base::TaskPriority::USER_BLOCKING,
On 2017/06/19 15:13:22, gab wrote:
> Are you sure this is USER_BLOCKING? It was previously using the FILE thread
> which wasn't appropriate for user blocking work (was that a bug?)

No.  This should have been USER_VISIBLE.  It is now fixed.

[commit-bot: I haz the power, 2017-06-20 05:31:46]

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

[commit-bot: I haz the power, 2017-06-20 05:31:48]

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

[skau, 2017-06-20 15:30:36]

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

[commit-bot: I haz the power, 2017-06-20 15:30:51]

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-20 16:11:27]

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

[commit-bot: I haz the power, 2017-06-20 16:11:28]

Dry run: This issue passed the CQ dry run.

[Carlson, 2017-06-20 18:57:33]

lgtm

Generally looks like a really nice cleanup.  Thanks for doing this!

https://codereview.chromium.org/2939373003/diff/60001/chromeos/printing/ppd_c...
File chromeos/printing/ppd_cache.cc (right):

https://codereview.chromium.org/2939373003/diff/60001/chromeos/printing/ppd_c...
chromeos/printing/ppd_cache.cc:55: MaybeCreateCache(cache_dir);
This patch makes lookups higher priorities than stores, which is a good idea. 
Maybe we should remove cache creation on Find, and just do it in Store?  

If the cache directory doesn't exist, just return a miss here?

[skau, 2017-06-20 20:23:10]

Thanks for the suggestion!

https://codereview.chromium.org/2939373003/diff/60001/chromeos/printing/ppd_c...
File chromeos/printing/ppd_cache.cc (right):

https://codereview.chromium.org/2939373003/diff/60001/chromeos/printing/ppd_c...
chromeos/printing/ppd_cache.cc:55: MaybeCreateCache(cache_dir);
On 2017/06/20 18:57:33, Carlson wrote:
> This patch makes lookups higher priorities than stores, which is a good idea. 
> Maybe we should remove cache creation on Find, and just do it in Store?  
> 
> If the cache directory doesn't exist, just return a miss here?
> 

That's a good idea!

[skau, 2017-06-20 20:25:01]

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

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

Dry run: CQ is trying da patch.

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

[gab, 2017-06-20 20:36:08]

Task API usage looks fine to me % comment below (yours to evaluate), haven't
cross-checked impl details, r-s lgtm.

https://codereview.chromium.org/2939373003/diff/60001/chromeos/printing/ppd_c...
File chromeos/printing/ppd_cache.cc (right):

https://codereview.chromium.org/2939373003/diff/60001/chromeos/printing/ppd_c...
chromeos/printing/ppd_cache.cc:65: if (file.ReadAtCurrentPos(buf.get(), len) ==
len) {
This allows concurrent read/writes to the same file (per using two sequences),
can't that result in IO errors?

[skau, 2017-06-20 21:02:57]

Description was changed from

==========
Convert PpdCache and PpdProvider to TaskScheduler.

Part of the larger Chromium migration away from BrowserThreads.

BUG=734279
==========

to

==========
Convert PpdCache and PpdProvider to TaskScheduler.

Part of the larger Chromium migration away from BrowserThreads.

BUG=734279, 667892
==========

[skau, 2017-06-20 21:10:24]

This won't happen in the current implementation.  I don't overwrite files that
have already been stored.  There is a small possibility of a false cache miss,
but that's not a situation that I'm concerned about.  Concurrent fetches of the
same file is not a use case that is currently relevant.

[skau, 2017-06-20 21:10:32]

The CQ bit was unchecked by skau@chromium.org

[skau, 2017-06-20 21:10:33]

The CQ bit was checked by skau@chromium.org

[skau, 2017-06-20 21:10:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
justincarlson@chromium.org
Link to the patchset: https://codereview.chromium.org/2939373003/#ps80001
(title: "find can skip dir creation")

[commit-bot: I haz the power, 2017-06-20 21:11:08]

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-20 21:27:56]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497993033081550,
"parent_rev": "156c7095f41e676fbdd780ead1450be6aaff6d53", "commit_rev":
"4fb6e69738018eda7b9d387934d35188dd46da17"}

[commit-bot: I haz the power, 2017-06-20 21:28:10]

Description was changed from

==========
Convert PpdCache and PpdProvider to TaskScheduler.

Part of the larger Chromium migration away from BrowserThreads.

BUG=734279, 667892
==========

to

==========
Convert PpdCache and PpdProvider to TaskScheduler.

Part of the larger Chromium migration away from BrowserThreads.

BUG=734279, 667892

Review-Url: https://codereview.chromium.org/2939373003
Cr-Commit-Position: refs/heads/master@{#480963}
Committed:
https://chromium.googlesource.com/chromium/src/+/4fb6e69738018eda7b9d387934d3...
==========

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

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

[gab, 2017-06-20 21:40:10]

On 2017/06/20 21:10:24, skau wrote:
> This won't happen in the current implementation.  I don't overwrite files that
> have already been stored.  There is a small possibility of a false cache miss,
> but that's not a situation that I'm concerned about.  Concurrent fetches of
the
> same file is not a use case that is currently relevant.

I see, probably worth documenting that in the code as it's an important
assumption for this threading model to be valid (i.e. so that someone doesn't
build on this further without while breaking this assumption).