services/ui: Allow injecting the IO thread task runner for gpu channel.

services/ui: Allow injecting the IO thread task runner for gpu channel.

Instead of always creating a new thread, allow injecting the IO thread
task runner into the ui::GpuService for the gpu::GpuChannelHost it
creates. This allows chrome browser and renderer processes to inject
their existing IO thread's task runner. A new thread is created only for
other mus apps that do not have their own IO thread.

BUG=638647
Committed: https://crrev.com/75f7dde87bfea8b3eab80814e320c2c528d77224
Cr-Commit-Position: refs/heads/master@{#413583}

[sadrul, 2016-08-18 03:26:03]

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

[commit-bot: I haz the power, 2016-08-18 03:26:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sadrul, 2016-08-18 03:27:15]

sadrul@chromium.org changed reviewers:
+ fsamuel@chromium.org

[commit-bot: I haz the power, 2016-08-18 04:13:58]

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

[commit-bot: I haz the power, 2016-08-18 04:13:59]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2016-08-18 11:00:56]

https://codereview.chromium.org/2249413004/diff/1/services/ui/public/cpp/gpu_...
File services/ui/public/cpp/gpu_service.h (right):

https://codereview.chromium.org/2249413004/diff/1/services/ui/public/cpp/gpu_...
services/ui/public/cpp/gpu_service.h:44:
scoped_refptr<base::SingleThreadTaskRunner> task_runner);
Can this not be passed in Initialize to avoid giving the impression that the
IOThreadTaskRunner can change at any time?

Also optional nit: GpuService::Initialize sounds weird because it actually
creates the object rather than initialize an already constructed object.

It would be nice to call this GpuService::Create, but optional I guess

[sadrul, 2016-08-18 12:53:20]

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

[commit-bot: I haz the power, 2016-08-18 12:53:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sadrul, 2016-08-18 12:56:52]

sadrul@chromium.org changed reviewers:
+ sky@chromium.org

[sadrul, 2016-08-18 12:56:53]

+sky@ for owner

https://codereview.chromium.org/2249413004/diff/1/services/ui/public/cpp/gpu_...
File services/ui/public/cpp/gpu_service.h (right):

https://codereview.chromium.org/2249413004/diff/1/services/ui/public/cpp/gpu_...
services/ui/public/cpp/gpu_service.h:44:
scoped_refptr<base::SingleThreadTaskRunner> task_runner);
On 2016/08/18 11:00:56, Fady Samuel wrote:
> Can this not be passed in Initialize to avoid giving the impression that the
> IOThreadTaskRunner can change at any time?

It's not straight forward, because for the chrome browser, the GpuService is
created by the views::WindowManagerConnection object. So we would have to plumb
the task-runner through that to GpuService. This is cleaner.
SetIOThreadTaskRunner() already has a DCHECK() to make sure the task runner
cannot be changed once set. I have added some messaging to the DCHECK() to
clarify that as well.

> 
> Also optional nit: GpuService::Initialize sounds weird because it actually
> creates the object rather than initialize an already constructed object.
> 
> It would be nice to call this GpuService::Create, but optional I guess

I have disliked Initialize() myself. Renamed to Create().

[Fady Samuel, 2016-08-18 13:05:39]

https://codereview.chromium.org/2249413004/diff/1/services/ui/public/cpp/gpu_...
File services/ui/public/cpp/gpu_service.h (right):

https://codereview.chromium.org/2249413004/diff/1/services/ui/public/cpp/gpu_...
services/ui/public/cpp/gpu_service.h:44:
scoped_refptr<base::SingleThreadTaskRunner> task_runner);
On 2016/08/18 12:56:53, sadrul wrote:
> On 2016/08/18 11:00:56, Fady Samuel wrote:
> > Can this not be passed in Initialize to avoid giving the impression that the
> > IOThreadTaskRunner can change at any time?
> 
> It's not straight forward, because for the chrome browser, the GpuService is
> created by the views::WindowManagerConnection object. So we would have to
plumb
> the task-runner through that to GpuService. This is cleaner.
> SetIOThreadTaskRunner() already has a DCHECK() to make sure the task runner
> cannot be changed once set. I have added some messaging to the DCHECK() to
> clarify that as well.
> 
> > 
> > Also optional nit: GpuService::Initialize sounds weird because it actually
> > creates the object rather than initialize an already constructed object.
> > 
> > It would be nice to call this GpuService::Create, but optional I guess
> 
> I have disliked Initialize() myself. Renamed to Create().

LGTM but I still feel like this is brittle because we could accidentally create
an IOThread if someone calls GetIOThreadTaskRunner() too early before
SetIOThreadTaskRunner is called

[Fady Samuel, 2016-08-18 13:11:51]

On 2016/08/18 13:05:39, Fady Samuel wrote:
>
https://codereview.chromium.org/2249413004/diff/1/services/ui/public/cpp/gpu_...
> File services/ui/public/cpp/gpu_service.h (right):
> 
>
https://codereview.chromium.org/2249413004/diff/1/services/ui/public/cpp/gpu_...
> services/ui/public/cpp/gpu_service.h:44:
> scoped_refptr<base::SingleThreadTaskRunner> task_runner);
> On 2016/08/18 12:56:53, sadrul wrote:
> > On 2016/08/18 11:00:56, Fady Samuel wrote:
> > > Can this not be passed in Initialize to avoid giving the impression that
the
> > > IOThreadTaskRunner can change at any time?
> > 
> > It's not straight forward, because for the chrome browser, the GpuService is
> > created by the views::WindowManagerConnection object. So we would have to
> plumb
> > the task-runner through that to GpuService. This is cleaner.
> > SetIOThreadTaskRunner() already has a DCHECK() to make sure the task runner
> > cannot be changed once set. I have added some messaging to the DCHECK() to
> > clarify that as well.
> > 
> > > 
> > > Also optional nit: GpuService::Initialize sounds weird because it actually
> > > creates the object rather than initialize an already constructed object.
> > > 
> > > It would be nice to call this GpuService::Create, but optional I guess
> > 
> > I have disliked Initialize() myself. Renamed to Create().
> 
> LGTM but I still feel like this is brittle because we could accidentally
create
> an IOThread if someone calls GetIOThreadTaskRunner() too early before
> SetIOThreadTaskRunner is called

Actually, correcting myself. In that case, if GpuService::SetIOThreadTaskRunner
is called later, it will DCHECK so LGTM++. This seems reasonable.

[commit-bot: I haz the power, 2016-08-18 13:47:05]

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

[commit-bot: I haz the power, 2016-08-18 13:47:06]

Dry run: This issue passed the CQ dry run.

[sky, 2016-08-18 18:08:48]

LGTM

[sadrul, 2016-08-18 19:31:14]

The CQ bit was checked by sadrul@chromium.org

[commit-bot: I haz the power, 2016-08-18 19:32: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-08-18 19:39:38]

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

[commit-bot: I haz the power, 2016-08-18 19:39:40]

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

[sadrul, 2016-08-18 19:44:01]

sadrul@chromium.org changed reviewers:
+ ben@chromium.org

[sadrul, 2016-08-18 19:44:02]

+ben@ for content/renderer change

[sadrul, 2016-08-22 06:10:58]

sadrul@chromium.org changed reviewers:
+ piman@chromium.org

[sadrul, 2016-08-22 06:10:59]

+piman@ for content/renderer changes since ben@ is ooo

[piman, 2016-08-22 19:42:04]

https://codereview.chromium.org/2249413004/diff/20001/services/ui/public/cpp/...
File services/ui/public/cpp/gpu_service.cc (right):

https://codereview.chromium.org/2249413004/diff/20001/services/ui/public/cpp/...
services/ui/public/cpp/gpu_service.cc:59: DCHECK(!io_task_runner_)
Would it make sense instead to pass the task runner explicitly to Create, and
creating the thread in the constructor if the task runner is null?

In particular, I'm concerned that GetIOThreadTaskRunner can be called on any
thread, making the logic there unsafe.

[sadrul, 2016-08-22 20:05:55]

https://codereview.chromium.org/2249413004/diff/20001/services/ui/public/cpp/...
File services/ui/public/cpp/gpu_service.cc (right):

https://codereview.chromium.org/2249413004/diff/20001/services/ui/public/cpp/...
services/ui/public/cpp/gpu_service.cc:59: DCHECK(!io_task_runner_)
On 2016/08/22 19:42:03, piman wrote:
> Would it make sense instead to pass the task runner explicitly to Create, and
> creating the thread in the constructor if the task runner is null?
> 
> In particular, I'm concerned that GetIOThreadTaskRunner can be called on any
> thread, making the logic there unsafe.

Ah, interesting. Yes, Fady brought up the issue earlier. It's not entirely
straight forward because of how GpuService is created for the chrome browser
process (see
https://codereview.chromium.org/2249413004/diff/1/services/ui/public/cpp/gpu_...).
Would it make more sense if I create this thread in EstablishGpuChannel[Sync]
instead?

[piman, 2016-08-22 20:31:50]

https://codereview.chromium.org/2249413004/diff/20001/services/ui/public/cpp/...
File services/ui/public/cpp/gpu_service.cc (right):

https://codereview.chromium.org/2249413004/diff/20001/services/ui/public/cpp/...
services/ui/public/cpp/gpu_service.cc:59: DCHECK(!io_task_runner_)
On 2016/08/22 20:05:54, sadrul wrote:
> On 2016/08/22 19:42:03, piman wrote:
> > Would it make sense instead to pass the task runner explicitly to Create,
and
> > creating the thread in the constructor if the task runner is null?
> > 
> > In particular, I'm concerned that GetIOThreadTaskRunner can be called on any
> > thread, making the logic there unsafe.
> 
> Ah, interesting. Yes, Fady brought up the issue earlier. It's not entirely
> straight forward because of how GpuService is created for the chrome browser
> process (see
>
https://codereview.chromium.org/2249413004/diff/1/services/ui/public/cpp/gpu_...).
> Would it make more sense if I create this thread in EstablishGpuChannel[Sync]
> instead?

I feel like plumbing the task runner through WindowManagerConnection is easy
enough, and feels (to me) cleaner than the Law-of-Demeter-cringy logic in
ChromeBrowserMainExtraPartsViews::MojoShellConnectionStarted - but I guess
opinions can differ on that.


I think deferring creating the thread to EstablishGpuChannel* would work, but
then I would prefer making GetIOThreadTaskRunner private (and even making the
GpuChannelHostFactory inheritance private, but that's iffy by the style guide) -
making it clear that it's only called by the GpuChannelHost, which by
construction would only be after the thread is created.

But in general, I prefer "obviously correct" code rather than defending with
documentation.

[sadrul, 2016-08-22 21:15:24]

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

[commit-bot: I haz the power, 2016-08-22 21:16:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sadrul, 2016-08-22 21:20:27]

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

[sadrul, 2016-08-22 21:20:38]

https://codereview.chromium.org/2249413004/diff/20001/services/ui/public/cpp/...
File services/ui/public/cpp/gpu_service.cc (right):

https://codereview.chromium.org/2249413004/diff/20001/services/ui/public/cpp/...
services/ui/public/cpp/gpu_service.cc:59: DCHECK(!io_task_runner_)
On 2016/08/22 20:31:50, piman wrote:
> On 2016/08/22 20:05:54, sadrul wrote:
> > On 2016/08/22 19:42:03, piman wrote:
> > > Would it make sense instead to pass the task runner explicitly to Create,
> and
> > > creating the thread in the constructor if the task runner is null?
> > > 
> > > In particular, I'm concerned that GetIOThreadTaskRunner can be called on
any
> > > thread, making the logic there unsafe.
> > 
> > Ah, interesting. Yes, Fady brought up the issue earlier. It's not entirely
> > straight forward because of how GpuService is created for the chrome browser
> > process (see
> >
>
https://codereview.chromium.org/2249413004/diff/1/services/ui/public/cpp/gpu_...).
> > Would it make more sense if I create this thread in
EstablishGpuChannel[Sync]
> > instead?
> 
> I feel like plumbing the task runner through WindowManagerConnection is easy
> enough, and feels (to me) cleaner than the Law-of-Demeter-cringy logic in
> ChromeBrowserMainExtraPartsViews::MojoShellConnectionStarted - but I guess
> opinions can differ on that.
> 
> 
> I think deferring creating the thread to EstablishGpuChannel* would work, but
> then I would prefer making GetIOThreadTaskRunner private (and even making the
> GpuChannelHostFactory inheritance private, but that's iffy by the style guide)
-
> making it clear that it's only called by the GpuChannelHost, which by
> construction would only be after the thread is created.
> 
> But in general, I prefer "obviously correct" code rather than defending with
> documentation.

Agreed. And contrary to popular meme, obviously correct is the best kind of
correct. Done.

[commit-bot: I haz the power, 2016-08-22 21:21:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[piman, 2016-08-22 21:50:47]

LGTM, thanks!

[commit-bot: I haz the power, 2016-08-22 23:26:01]

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

[commit-bot: I haz the power, 2016-08-22 23:26:03]

Dry run: This issue passed the CQ dry run.

[sadrul, 2016-08-22 23:31:32]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2016-08-22 23:31:33]

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

[commit-bot: I haz the power, 2016-08-22 23:31:56]

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-08-22 23:37:49]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-22 23:40:59]

Description was changed from

==========
services/ui: Allow injecting the IO thread task runner for gpu channel.

Instead of always creating a new thread, allow injecting the IO thread
task runner into the ui::GpuService for the gpu::GpuChannelHost it
creates. This allows chrome browser and renderer processes to inject
their existing IO thread's task runner. A new thread is created only for
other mus apps that do not have their own IO thread.

BUG=638647
==========

to

==========
services/ui: Allow injecting the IO thread task runner for gpu channel.

Instead of always creating a new thread, allow injecting the IO thread
task runner into the ui::GpuService for the gpu::GpuChannelHost it
creates. This allows chrome browser and renderer processes to inject
their existing IO thread's task runner. A new thread is created only for
other mus apps that do not have their own IO thread.

BUG=638647

Committed: https://crrev.com/75f7dde87bfea8b3eab80814e320c2c528d77224
Cr-Commit-Position: refs/heads/master@{#413583}
==========

[commit-bot: I haz the power, 2016-08-22 23:41:00]

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