Move the IconLoader to use the task scheduler.

Move the IconLoader to use the task scheduler.

BUG=689520
Review-Url: https://codereview.chromium.org/2953633002
Cr-Commit-Position: refs/heads/master@{#481719}
Committed: https://chromium.googlesource.com/chromium/src/+/f4d431c396f62c9c205ae13b18c5e17d55fd08f3

[Avi (use Gerrit), 2017-06-21 19:20:55]

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

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

Dry run: CQ is trying da patch.

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

[Avi (use Gerrit), 2017-06-21 20:31:32]

avi@chromium.org changed reviewers:
+ benhenry@chromium.org, sky@chromium.org

[Avi (use Gerrit), 2017-06-21 20:31:35]

Ben: Please review the conversion.
Scott: Please verify that I can use the Win32 icon calls on a random thread
picked by the task scheduler rather than being on the FILE thread, and please
take the overall review.

[commit-bot: I haz the power, 2017-06-21 20:39:38]

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

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

Dry run: This issue passed the CQ dry run.

[benhenry, 2017-06-21 21:55:14]

Description was changed from

==========
Move the IconLoader to use the task scheduler.

BUG=689520
==========

to

==========
Move the IconLoader to use the task scheduler.

BUG=689520
==========

[benhenry, 2017-06-21 21:55:14]

benhenry@google.com changed reviewers:
+ gab@chromium.org

[sky, 2017-06-22 00:41:33]

I believe the only requirement for windows is that COM is enabled. I can't
readily tell from this patch if that is the case. Is it?

[sky, 2017-06-22 00:43:25]

https://codereview.chromium.org/2953633002/diff/1/chrome/browser/icon_loader.cc
File chrome/browser/icon_loader.cc (right):

https://codereview.chromium.org/2953633002/diff/1/chrome/browser/icon_loader....
chrome/browser/icon_loader.cc:52: FROM_HERE, {base::MayBlock(),
base::TaskPriority::USER_VISIBLE},
optional: create a function to return the traits that this and line 28 use.

[Avi (use Gerrit), 2017-06-22 01:21:17]

https://codereview.chromium.org/2953633002/diff/1/chrome/browser/icon_loader.cc
File chrome/browser/icon_loader.cc (right):

https://codereview.chromium.org/2953633002/diff/1/chrome/browser/icon_loader....
chrome/browser/icon_loader.cc:52: FROM_HERE, {base::MayBlock(),
base::TaskPriority::USER_VISIBLE},
On 2017/06/22 00:43:25, sky wrote:
> optional: create a function to return the traits that this and line 28 use.

Ben: Is that the style for the new PostTask?

[Avi (use Gerrit), 2017-06-22 01:38:08]

On 2017/06/22 00:41:33, sky wrote:
> I believe the only requirement for windows is that COM is enabled. I can't
> readily tell from this patch if that is the case. Is it?

https://chromium.googlesource.com/chromium/src/+/master/docs/threading_and_ta...

has something about a "COM Single-Thread Apartment (STA) thread" but nothing
about COM in general. Task peeps, how do I go with this?

[benhenry, 2017-06-22 15:18:59]

On 2017/06/22 01:21:17, Avi (ping after 24h) wrote:
>
https://codereview.chromium.org/2953633002/diff/1/chrome/browser/icon_loader.cc
> File chrome/browser/icon_loader.cc (right):
> 
>
https://codereview.chromium.org/2953633002/diff/1/chrome/browser/icon_loader....
> chrome/browser/icon_loader.cc:52: FROM_HERE, {base::MayBlock(),
> base::TaskPriority::USER_VISIBLE},
> On 2017/06/22 00:43:25, sky wrote:
> > optional: create a function to return the traits that this and line 28 use.
> 
> Ben: Is that the style for the new PostTask?

I'm hoping gab@ can step in. I'm just helping as a TPM and am not the best
person to give cr feedback.

[Avi (use Gerrit), 2017-06-22 15:21:33]

On 2017/06/22 15:18:59, benhenry wrote:
> On 2017/06/22 01:21:17, Avi (ping after 24h) wrote:
> >
>
https://codereview.chromium.org/2953633002/diff/1/chrome/browser/icon_loader.cc
> > File chrome/browser/icon_loader.cc (right):
> > 
> >
>
https://codereview.chromium.org/2953633002/diff/1/chrome/browser/icon_loader....
> > chrome/browser/icon_loader.cc:52: FROM_HERE, {base::MayBlock(),
> > base::TaskPriority::USER_VISIBLE},
> > On 2017/06/22 00:43:25, sky wrote:
> > > optional: create a function to return the traits that this and line 28
use.
> > 
> > Ben: Is that the style for the new PostTask?
> 
> I'm hoping gab@ can step in. I'm just helping as a TPM and am not the best
> person to give cr feedback.

No problem. You tagged me in the spreadsheet, so I was going to you as a first
choice :)

Gabriel? Thoughts on 1) style of the traits and 2) getting a COM thread?

[gab, 2017-06-22 15:38:35]

https://codereview.chromium.org/2953633002/diff/1/chrome/browser/icon_loader.cc
File chrome/browser/icon_loader.cc (right):

https://codereview.chromium.org/2953633002/diff/1/chrome/browser/icon_loader....
chrome/browser/icon_loader.cc:49: ->PostTask(FROM_HERE, std::move(read_icon));
content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
std::move(read_icon));

https://codereview.chromium.org/2953633002/diff/1/chrome/browser/icon_loader....
chrome/browser/icon_loader.cc:51: base::PostTaskWithTraits(
If you need COM you'll need to (on OS_WIN):

base::CreateCOMSTATaskRunnerWithTraits({base::MayBlock(),
base::TaskPriority::USER_VISIBLE})->PostTask(...);

(we unfortunately don't have a simpler one-off base::PostTask API for COM tasks
-- intentionally for now because this pattern doesn't come back that often and
the COM STA is inherently a single-threaded environment so it's nice to have to
request that explicitly)

Note: we're working on providing stricter COM checks to make sure such an
environment is requested (https://chromium-review.googlesource.com/c/537994/).
We currently allow COM everywhere in the TaskScheduler for backward
compatibility [1] but are looking to remove that as using an STA on a thread
that doesn't pump system messages is error-prone.

[1]
https://cs.chromium.org/chromium/src/base/task_scheduler/scheduler_worker.cc?...

https://codereview.chromium.org/2953633002/diff/1/chrome/browser/icon_loader....
chrome/browser/icon_loader.cc:52: FROM_HERE, {base::MayBlock(),
base::TaskPriority::USER_VISIBLE},
On 2017/06/22 01:21:17, Avi (ping after 24h) wrote:
> On 2017/06/22 00:43:25, sky wrote:
> > optional: create a function to return the traits that this and line 28 use.
> 
> Ben: Is that the style for the new PostTask?

TaskTraits is constexpr, you can store them in a common variable in the
anonymous namespace if you wish to re-use.

[Avi (use Gerrit), 2017-06-22 19:22:20]

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

[commit-bot: I haz the power, 2017-06-22 19:22:38]

Dry run: CQ is trying da patch.

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

[Avi (use Gerrit), 2017-06-22 19:26:59]

I just uploaded a new version; it hasn't passed the trybots, but Gabriel, since
you're out tomorrow, I wanted to catch you.

I'm switching back to each platform returning what's needed. Are the task
runners generated by the Mac and Windows code lightweight enough to generate
each time like this? Or should I use a Singleton or static member variable to
cache them?

[commit-bot: I haz the power, 2017-06-22 19:47:25]

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

[commit-bot: I haz the power, 2017-06-22 19:47:26]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[gab, 2017-06-22 19:49:42]

https://codereview.chromium.org/2953633002/diff/20001/chrome/browser/icon_loa...
File chrome/browser/icon_loader.cc (right):

https://codereview.chromium.org/2953633002/diff/20001/chrome/browser/icon_loa...
chrome/browser/icon_loader.cc:27: base::PostTaskWithTraitsAndReply(
This is fine so long as IconLoader::Start() is only invoked once (should we
DCHECK that?), otherwise this results in a task posted in parallel and therefore
could result in many &IconLoader::ReadGroup invoked on the same |this| in
parallel.

(update: I guess that's already enforced by |delete this| in ReadIcon())

https://codereview.chromium.org/2953633002/diff/20001/chrome/browser/icon_loa...
chrome/browser/icon_loader.cc:45: ReadIconTaskRunner()->PostTask(
This is also fine because the execution model seems to be that ReadIcon()
invokes IconLoadedCallback and deletes |this|.

However, I'm noting that OnReadGroup() is merely a reply on the original
ThreadTaskRunnerHandle (per PostTaskAndReply) which then reposts ReadIcon()
which itself posts back to ThreadTaskRunnerHandle before deleting |this|, that
middle OnReadGroup() hop seems unnecessary.

Overall all of this is implicitly sequenced by IconLoader itself but you could
make this explicit by using a SequencedTaskRunner and posting ReadGroup() and
ReadIcon() sequentially -- and at that point I'm not sure why ReadGroup() isn't
just part of ReadIcon() (at least it can go in the same task?)

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

On 2017/06/22 19:26:59, Avi (ping after 24h) wrote:
> I just uploaded a new version; it hasn't passed the trybots, but Gabriel,
since
> you're out tomorrow, I wanted to catch you.
> 
> I'm switching back to each platform returning what's needed. Are the task
> runners generated by the Mac and Windows code lightweight enough to generate
> each time like this? Or should I use a Singleton or static member variable to
> cache them?

They're lightweight, but preference is usually to hold them as a member, however
in this case as highlighted below: IconLoader seems to be a "consume once =>
delete self" class so it doesn't matter beyond readability (I'd make it a
SequencedTaskRunner member for readability).

[Avi (use Gerrit), 2017-06-22 20:09:29]

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

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

Dry run: CQ is trying da patch.

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

[Avi (use Gerrit), 2017-06-22 20:31:23]

On 2017/06/22 19:51:13, gab (OOO Friday) wrote:
> On 2017/06/22 19:26:59, Avi (ping after 24h) wrote:
> > I just uploaded a new version; it hasn't passed the trybots, but Gabriel,
> since
> > you're out tomorrow, I wanted to catch you.
> > 
> > I'm switching back to each platform returning what's needed. Are the task
> > runners generated by the Mac and Windows code lightweight enough to generate
> > each time like this? Or should I use a Singleton or static member variable
to
> > cache them?
> 
> They're lightweight, but preference is usually to hold them as a member,
however
> in this case as highlighted below: IconLoader seems to be a "consume once =>
> delete self" class so it doesn't matter beyond readability (I'd make it a
> SequencedTaskRunner member for readability).

Would it make sense to make the SequencedTaskRunner object static across all
IconLoader objects for reuse?

[Avi (use Gerrit), 2017-06-22 20:31:45]

https://codereview.chromium.org/2953633002/diff/20001/chrome/browser/icon_loa...
File chrome/browser/icon_loader.cc (right):

https://codereview.chromium.org/2953633002/diff/20001/chrome/browser/icon_loa...
chrome/browser/icon_loader.cc:27: base::PostTaskWithTraitsAndReply(
On 2017/06/22 19:49:41, gab (OOO Friday) wrote:
> This is fine so long as IconLoader::Start() is only invoked once (should we
> DCHECK that?), otherwise this results in a task posted in parallel and
therefore
> could result in many &IconLoader::ReadGroup invoked on the same |this| in
> parallel.
> 
> (update: I guess that's already enforced by |delete this| in ReadIcon())

Acknowledged.

https://codereview.chromium.org/2953633002/diff/20001/chrome/browser/icon_loa...
chrome/browser/icon_loader.cc:45: ReadIconTaskRunner()->PostTask(
On 2017/06/22 19:49:41, gab (OOO Friday) wrote:
> This is also fine because the execution model seems to be that ReadIcon()
> invokes IconLoadedCallback and deletes |this|.
> 
> However, I'm noting that OnReadGroup() is merely a reply on the original
> ThreadTaskRunnerHandle (per PostTaskAndReply) which then reposts ReadIcon()
> which itself posts back to ThreadTaskRunnerHandle before deleting |this|, that
> middle OnReadGroup() hop seems unnecessary.
> 
> Overall all of this is implicitly sequenced by IconLoader itself but you could
> make this explicit by using a SequencedTaskRunner and posting ReadGroup() and
> ReadIcon() sequentially -- and at that point I'm not sure why ReadGroup()
isn't
> just part of ReadIcon() (at least it can go in the same task?)

On the Linux platforms, the group determination needs to be done off the UI
thread yet the icon reading needs to be done on the UI thread. If you can
suggest a better way to bounce between those threads I'd be happy to switch to
it.

[gab, 2017-06-22 20:45:16]

On 2017/06/22 20:31:45, Avi (ping after 24h) wrote:
>
https://codereview.chromium.org/2953633002/diff/20001/chrome/browser/icon_loa...
> File chrome/browser/icon_loader.cc (right):
> 
>
https://codereview.chromium.org/2953633002/diff/20001/chrome/browser/icon_loa...
> chrome/browser/icon_loader.cc:27: base::PostTaskWithTraitsAndReply(
> On 2017/06/22 19:49:41, gab (OOO Friday) wrote:
> > This is fine so long as IconLoader::Start() is only invoked once (should we
> > DCHECK that?), otherwise this results in a task posted in parallel and
> therefore
> > could result in many &IconLoader::ReadGroup invoked on the same |this| in
> > parallel.
> > 
> > (update: I guess that's already enforced by |delete this| in ReadIcon())
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/2953633002/diff/20001/chrome/browser/icon_loa...
> chrome/browser/icon_loader.cc:45: ReadIconTaskRunner()->PostTask(
> On 2017/06/22 19:49:41, gab (OOO Friday) wrote:
> > This is also fine because the execution model seems to be that ReadIcon()
> > invokes IconLoadedCallback and deletes |this|.
> > 
> > However, I'm noting that OnReadGroup() is merely a reply on the original
> > ThreadTaskRunnerHandle (per PostTaskAndReply) which then reposts ReadIcon()
> > which itself posts back to ThreadTaskRunnerHandle before deleting |this|,
that
> > middle OnReadGroup() hop seems unnecessary.
> > 
> > Overall all of this is implicitly sequenced by IconLoader itself but you
could
> > make this explicit by using a SequencedTaskRunner and posting ReadGroup()
and
> > ReadIcon() sequentially -- and at that point I'm not sure why ReadGroup()
> isn't
> > just part of ReadIcon() (at least it can go in the same task?)
> 
> On the Linux platforms, the group determination needs to be done off the UI
> thread yet the icon reading needs to be done on the UI thread. If you can
> suggest a better way to bounce between those threads I'd be happy to switch to
> it.

Oh I see... so you need ReadGroup() on FILE-equivalent, then ReadIcon() on
custom task runner (sometimes FILE-equivalent).

I don't think you need a static task runner, so long as reading multiple icons
in parallel is fine, I don't see a reason to unify the task runner and creating
the task runner is as lightweight as creating any in-memory class essentially
(it doesn't actually create any system resources).

Given that, what you have lgtm perhaps % comments to explain how the sequencing
works (i.e. they're one-off parallel tasks sequenced one after the other), and
you could save the extra hop to OnReadGroup() if instead of PostTaskAndReply you
just posted ReadIcon() to ReadIconTaskRunner() from ReadGroup() -- might also
help self-document the implicit sequencing.

[Avi (use Gerrit), 2017-06-22 20:56:01]

On 2017/06/22 20:45:16, gab (OOO Friday) wrote:
> On 2017/06/22 20:31:45, Avi (ping after 24h) wrote:
> >
>
https://codereview.chromium.org/2953633002/diff/20001/chrome/browser/icon_loa...
> > File chrome/browser/icon_loader.cc (right):
> > 
> >
>
https://codereview.chromium.org/2953633002/diff/20001/chrome/browser/icon_loa...
> > chrome/browser/icon_loader.cc:27: base::PostTaskWithTraitsAndReply(
> > On 2017/06/22 19:49:41, gab (OOO Friday) wrote:
> > > This is fine so long as IconLoader::Start() is only invoked once (should
we
> > > DCHECK that?), otherwise this results in a task posted in parallel and
> > therefore
> > > could result in many &IconLoader::ReadGroup invoked on the same |this| in
> > > parallel.
> > > 
> > > (update: I guess that's already enforced by |delete this| in ReadIcon())
> > 
> > Acknowledged.
> > 
> >
>
https://codereview.chromium.org/2953633002/diff/20001/chrome/browser/icon_loa...
> > chrome/browser/icon_loader.cc:45: ReadIconTaskRunner()->PostTask(
> > On 2017/06/22 19:49:41, gab (OOO Friday) wrote:
> > > This is also fine because the execution model seems to be that ReadIcon()
> > > invokes IconLoadedCallback and deletes |this|.
> > > 
> > > However, I'm noting that OnReadGroup() is merely a reply on the original
> > > ThreadTaskRunnerHandle (per PostTaskAndReply) which then reposts
ReadIcon()
> > > which itself posts back to ThreadTaskRunnerHandle before deleting |this|,
> that
> > > middle OnReadGroup() hop seems unnecessary.
> > > 
> > > Overall all of this is implicitly sequenced by IconLoader itself but you
> could
> > > make this explicit by using a SequencedTaskRunner and posting ReadGroup()
> and
> > > ReadIcon() sequentially -- and at that point I'm not sure why ReadGroup()
> > isn't
> > > just part of ReadIcon() (at least it can go in the same task?)
> > 
> > On the Linux platforms, the group determination needs to be done off the UI
> > thread yet the icon reading needs to be done on the UI thread. If you can
> > suggest a better way to bounce between those threads I'd be happy to switch
to
> > it.
> 
> Oh I see... so you need ReadGroup() on FILE-equivalent, then ReadIcon() on
> custom task runner (sometimes FILE-equivalent).
> 
> I don't think you need a static task runner, so long as reading multiple icons
> in parallel is fine, I don't see a reason to unify the task runner and
creating
> the task runner is as lightweight as creating any in-memory class essentially
> (it doesn't actually create any system resources).
> 
> Given that, what you have lgtm perhaps % comments to explain how the
sequencing
> works (i.e. they're one-off parallel tasks sequenced one after the other), and
> you could save the extra hop to OnReadGroup() if instead of PostTaskAndReply
you
> just posted ReadIcon() to ReadIconTaskRunner() from ReadGroup() -- might also
> help self-document the implicit sequencing.

Ah, good idea. Rev to come.

[Avi (use Gerrit), 2017-06-22 21:00:18]

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

[commit-bot: I haz the power, 2017-06-22 21:00:23]

Dry run: CQ is trying da patch.

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

[Avi (use Gerrit), 2017-06-22 21:01:55]

Scott, ping? WDYT?

[sky, 2017-06-22 21:20:50]

LGTM

https://codereview.chromium.org/2953633002/diff/60001/chrome/browser/icon_loa...
File chrome/browser/icon_loader.h (right):

https://codereview.chromium.org/2953633002/diff/60001/chrome/browser/icon_loa...
chrome/browser/icon_loader.h:71: static scoped_refptr<base::TaskRunner>
ReadIconTaskRunner();
Mild preference for GetReadIconTaskRunner() as read-icon-task-runner sounds like
a verb.

[Avi (use Gerrit), 2017-06-22 21:46:35]

https://codereview.chromium.org/2953633002/diff/60001/chrome/browser/icon_loa...
File chrome/browser/icon_loader.h (right):

https://codereview.chromium.org/2953633002/diff/60001/chrome/browser/icon_loa...
chrome/browser/icon_loader.h:71: static scoped_refptr<base::TaskRunner>
ReadIconTaskRunner();
On 2017/06/22 21:20:49, sky wrote:
> Mild preference for GetReadIconTaskRunner() as read-icon-task-runner sounds
like
> a verb.

Done.

[Avi (use Gerrit), 2017-06-22 22:06:01]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2017-06-22 22:06:02]

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

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

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-22 23:30:55]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498169161525390,
"parent_rev": "d3043821a09ddb2b598179f2c5363b281a4e857e", "commit_rev":
"f4d431c396f62c9c205ae13b18c5e17d55fd08f3"}

[commit-bot: I haz the power, 2017-06-22 23:31:08]

Description was changed from

==========
Move the IconLoader to use the task scheduler.

BUG=689520
==========

to

==========
Move the IconLoader to use the task scheduler.

BUG=689520

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

[commit-bot: I haz the power, 2017-06-22 23:31:10]

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