services/ui: Use a gpu::GpuChannelHost when creating ui::OutputSurface.

services/ui: Use a gpu::GpuChannelHost when creating ui::OutputSurface.

There are two users of ui::OutputSurface, and this change makes things more
correct for both. Because:

. renderer: RenderThreadImpl already creates the GpuChannelHost instance
  synchronously before creating the OutputSurface in non-mus. This does the same
  for mus, instead of potentially delaying the GpuChannelHost creation (or doing
  it in the compositor thread).

. ui or browser: Before this change, SurfaceContextFactory would create the
  OutputSurface, and immediately set it on the Compositor, leading to the
  synchronous creation of GpuChannelHost. This change just makes that more
  obvious, and creates the GpuChannelHost synchronously first before creating
  the OutputSurface.

This change also makes it possible to simplify ui::GpuService to not be usable
from multiple threads.

BUG=638647
Committed: https://crrev.com/12e259d5a209aa333c5cf748bffcad031e173786
Cr-Commit-Position: refs/heads/master@{#413011}

[sadrul, 2016-08-18 00:57:34]

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

[commit-bot: I haz the power, 2016-08-18 00:58:12]

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-08-18 01:17:36]

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

[commit-bot: I haz the power, 2016-08-18 01:17:37]

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

[sadrul, 2016-08-18 01:24:39]

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

[commit-bot: I haz the power, 2016-08-18 01:25:06]

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 02:08:05]

Description was changed from

==========
services/ui: Use a gpu::GpuChannelHost when creating ui::OutputSurface.

There are two users of ui::OutputSurface, and this change makes things more
correct for both. Because:

. renderer: RenderThreadImpl already creates the GpuChannelHost instance
  synchronously before creating the OutputSurface in non-mus. This does the same
  for mus, instead of potentially delaying the GpuChannelHost creation.

. ui or browser: Before this change, SurfaceContextFactory would create the
  OutputSurface, and immediately set it on the Compositor, leading to the
  synchronous creation of GpuChannelHost. This change just makes that more
  obvious, and creates the GpuChannelHost synchronously first before creating
  the OutputSurface.

This change also makes it possible to simplify ui::GpuService to not be usable
from multiple threads.

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

to

==========
services/ui: Use a gpu::GpuChannelHost when creating ui::OutputSurface.

There are two users of ui::OutputSurface, and this change makes things more
correct for both. Because:

. renderer: RenderThreadImpl already creates the GpuChannelHost instance
  synchronously before creating the OutputSurface in non-mus. This does the same
  for mus, instead of potentially delaying the GpuChannelHost creation (or doing
  it in the compositor thread).

. ui or browser: Before this change, SurfaceContextFactory would create the
  OutputSurface, and immediately set it on the Compositor, leading to the
  synchronous creation of GpuChannelHost. This change just makes that more
  obvious, and creates the GpuChannelHost synchronously first before creating
  the OutputSurface.

This change also makes it possible to simplify ui::GpuService to not be usable
from multiple threads.

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

[sadrul, 2016-08-18 02:09:21]

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

[sadrul, 2016-08-18 02:09:21]

piman@ Please review content/

sky@ Please review services/ui

[commit-bot: I haz the power, 2016-08-18 02:19:41]

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

[commit-bot: I haz the power, 2016-08-18 02:19:42]

Dry run: This issue passed the CQ dry run.

[piman, 2016-08-18 03:02:01]

lgtm

[sky, 2016-08-18 13:15:45]

https://codereview.chromium.org/2257693002/diff/20001/services/ui/demo/bitmap...
File services/ui/demo/bitmap_uploader.cc (right):

https://codereview.chromium.org/2257693002/diff/20001/services/ui/demo/bitmap...
services/ui/demo/bitmap_uploader.cc:43: gpu_service->EstablishGpuChannelSync());
Aren't these all rather expensive calls? Seems like an Init() better matches the
style guide.

https://codereview.chromium.org/2257693002/diff/20001/services/ui/demo/bitmap...
services/ui/demo/bitmap_uploader.cc:44: // CreateOffscreenContext() may return
null.
Update or remove comment?

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

https://codereview.chromium.org/2257693002/diff/20001/services/ui/public/cpp/...
services/ui/public/cpp/gles2_context.cc:80: std::unique_ptr<GLES2Context>
gles2_context(new GLES2Context);
This section is subtle and worth a comment. For example, why not create the
context if initialize succeeds?

[sadrul, 2016-08-18 14:59:18]

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

[commit-bot: I haz the power, 2016-08-18 14:59: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 14:59:49]

https://codereview.chromium.org/2257693002/diff/20001/services/ui/demo/bitmap...
File services/ui/demo/bitmap_uploader.cc (right):

https://codereview.chromium.org/2257693002/diff/20001/services/ui/demo/bitmap...
services/ui/demo/bitmap_uploader.cc:43: gpu_service->EstablishGpuChannelSync());
On 2016/08/18 13:15:45, sky wrote:
> Aren't these all rather expensive calls? Seems like an Init() better matches
the
> style guide.

Done.

https://codereview.chromium.org/2257693002/diff/20001/services/ui/demo/bitmap...
services/ui/demo/bitmap_uploader.cc:44: // CreateOffscreenContext() may return
null.
On 2016/08/18 13:15:45, sky wrote:
> Update or remove comment?

Done (removed)

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

https://codereview.chromium.org/2257693002/diff/20001/services/ui/public/cpp/...
services/ui/public/cpp/gles2_context.cc:80: std::unique_ptr<GLES2Context>
gles2_context(new GLES2Context);
On 2016/08/18 13:15:45, sky wrote:
> This section is subtle and worth a comment. For example, why not create the
> context if initialize succeeds?

We keep the context if it was successfully initialized, and destroy it if it
wasn't initialized due to some error.

[commit-bot: I haz the power, 2016-08-18 15:45:35]

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

[commit-bot: I haz the power, 2016-08-18 15:45:36]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sky, 2016-08-18 19:12:09]

LGTM

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

https://codereview.chromium.org/2257693002/diff/20001/services/ui/public/cpp/...
services/ui/public/cpp/gles2_context.cc:80: std::unique_ptr<GLES2Context>
gles2_context(new GLES2Context);
On 2016/08/18 14:59:48, sadrul wrote:
> On 2016/08/18 13:15:45, sky wrote:
> > This section is subtle and worth a comment. For example, why not create the
> > context if initialize succeeds?
> 
> We keep the context if it was successfully initialized, and destroy it if it
> wasn't initialized due to some error.

Makes sense, please add a comment to the code as well.

https://codereview.chromium.org/2257693002/diff/40001/services/ui/demo/bitmap...
File services/ui/demo/bitmap_uploader.h (right):

https://codereview.chromium.org/2257693002/diff/40001/services/ui/demo/bitmap...
services/ui/demo/bitmap_uploader.h:31: BitmapUploader(Window* window);
explicit

[sadrul, 2016-08-18 20:52:08]

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

[commit-bot: I haz the power, 2016-08-18 20:52:46]

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 20:53:28]

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

https://codereview.chromium.org/2257693002/diff/20001/services/ui/public/cpp/...
services/ui/public/cpp/gles2_context.cc:80: std::unique_ptr<GLES2Context>
gles2_context(new GLES2Context);
On 2016/08/18 19:12:09, sky wrote:
> On 2016/08/18 14:59:48, sadrul wrote:
> > On 2016/08/18 13:15:45, sky wrote:
> > > This section is subtle and worth a comment. For example, why not create
the
> > > context if initialize succeeds?
> > 
> > We keep the context if it was successfully initialized, and destroy it if it
> > wasn't initialized due to some error.
> 
> Makes sense, please add a comment to the code as well.

Done.

https://codereview.chromium.org/2257693002/diff/40001/services/ui/demo/bitmap...
File services/ui/demo/bitmap_uploader.h (right):

https://codereview.chromium.org/2257693002/diff/40001/services/ui/demo/bitmap...
services/ui/demo/bitmap_uploader.h:31: BitmapUploader(Window* window);
On 2016/08/18 19:12:09, sky wrote:
> explicit

Done.

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

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

[commit-bot: I haz the power, 2016-08-18 22:40:24]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sadrul, 2016-08-18 23:59:15]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2016-08-18 23:59:15]

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

[commit-bot: I haz the power, 2016-08-18 23:59:57]

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-19 01:37:17]

Description was changed from

==========
services/ui: Use a gpu::GpuChannelHost when creating ui::OutputSurface.

There are two users of ui::OutputSurface, and this change makes things more
correct for both. Because:

. renderer: RenderThreadImpl already creates the GpuChannelHost instance
  synchronously before creating the OutputSurface in non-mus. This does the same
  for mus, instead of potentially delaying the GpuChannelHost creation (or doing
  it in the compositor thread).

. ui or browser: Before this change, SurfaceContextFactory would create the
  OutputSurface, and immediately set it on the Compositor, leading to the
  synchronous creation of GpuChannelHost. This change just makes that more
  obvious, and creates the GpuChannelHost synchronously first before creating
  the OutputSurface.

This change also makes it possible to simplify ui::GpuService to not be usable
from multiple threads.

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

to

==========
services/ui: Use a gpu::GpuChannelHost when creating ui::OutputSurface.

There are two users of ui::OutputSurface, and this change makes things more
correct for both. Because:

. renderer: RenderThreadImpl already creates the GpuChannelHost instance
  synchronously before creating the OutputSurface in non-mus. This does the same
  for mus, instead of potentially delaying the GpuChannelHost creation (or doing
  it in the compositor thread).

. ui or browser: Before this change, SurfaceContextFactory would create the
  OutputSurface, and immediately set it on the Compositor, leading to the
  synchronous creation of GpuChannelHost. This change just makes that more
  obvious, and creates the GpuChannelHost synchronously first before creating
  the OutputSurface.

This change also makes it possible to simplify ui::GpuService to not be usable
from multiple threads.

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

[commit-bot: I haz the power, 2016-08-19 01:37:18]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-19 01:46:19]

Description was changed from

==========
services/ui: Use a gpu::GpuChannelHost when creating ui::OutputSurface.

There are two users of ui::OutputSurface, and this change makes things more
correct for both. Because:

. renderer: RenderThreadImpl already creates the GpuChannelHost instance
  synchronously before creating the OutputSurface in non-mus. This does the same
  for mus, instead of potentially delaying the GpuChannelHost creation (or doing
  it in the compositor thread).

. ui or browser: Before this change, SurfaceContextFactory would create the
  OutputSurface, and immediately set it on the Compositor, leading to the
  synchronous creation of GpuChannelHost. This change just makes that more
  obvious, and creates the GpuChannelHost synchronously first before creating
  the OutputSurface.

This change also makes it possible to simplify ui::GpuService to not be usable
from multiple threads.

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

to

==========
services/ui: Use a gpu::GpuChannelHost when creating ui::OutputSurface.

There are two users of ui::OutputSurface, and this change makes things more
correct for both. Because:

. renderer: RenderThreadImpl already creates the GpuChannelHost instance
  synchronously before creating the OutputSurface in non-mus. This does the same
  for mus, instead of potentially delaying the GpuChannelHost creation (or doing
  it in the compositor thread).

. ui or browser: Before this change, SurfaceContextFactory would create the
  OutputSurface, and immediately set it on the Compositor, leading to the
  synchronous creation of GpuChannelHost. This change just makes that more
  obvious, and creates the GpuChannelHost synchronously first before creating
  the OutputSurface.

This change also makes it possible to simplify ui::GpuService to not be usable
from multiple threads.

BUG=638647

Committed: https://crrev.com/12e259d5a209aa333c5cf748bffcad031e173786
Cr-Commit-Position: refs/heads/master@{#413011}
==========

[commit-bot: I haz the power, 2016-08-19 01:46:21]

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