Share SyncPointManager between ipc and in-process

Share SyncPointManager between ipc and in-process

Allow SyncPointManager instance to be set, and set the same
instance in Android WebView. Also remove ref-counting and just
guarantee SyncPointManager outlives running message loop.

Also make SyncPointManager thread safe, and callbacks are no longer
guaranteed to happen on the same thread. Make sure command buffer
implementations handle it correctly.

BUG=509702
Committed: https://crrev.com/c5befe7d0b86bb4996017605f1edc4bfa1c327f8
Cr-Commit-Position: refs/heads/master@{#339712}

[boliu, 2015-07-15 16:08:28]

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

[boliu, 2015-07-15 16:08:29]

ptal

net -50 lines!

[no sievers, 2015-07-15 21:51:40]

sievers@chromium.org changed reviewers:
+ jbauman@chromium.org

[no sievers, 2015-07-15 21:53:27]

Looks fine but +jbauman to confirm that it's fine to make this global and that
we'd never want to have multiple sync point namespaces.

Note that this for Android WebView to be able to use sync points across GPU
threads.

[jbauman, 2015-07-15 23:35:24]

lgtm. Having only one SyncPointManager makes sense to me, though I could imagine
doing weird things with multiple ones with mojo.

[boliu, 2015-07-15 23:43:48]

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

[boliu, 2015-07-15 23:43:49]

+msw for components/view_manager

[msw, 2015-07-16 19:59:10]

components/view_manager lgtm. CC'ing sky for FYI.

[piman, 2015-07-16 20:20:50]

Drive-by: making this a singleton seems like a regression... why can't we keep
dependency injection? We can still remove the refcounting parts, provided we
enforce single-ownership at a higher level. Needing a default constructor (for
LazyInstance) also forces the condvar broadcast even on single-threaded cases,
which seems like a regression too.

[boliu, 2015-07-16 21:26:31]

How does this sound

On 2015/07/16 20:20:50, piman (Very slow to review) wrote:
> Drive-by: making this a singleton seems like a regression... why can't we keep
> dependency injection? We can still remove the refcounting parts, provided we
> enforce single-ownership at a higher level.

I guess the idea that it's a singleton doesn't have to run so deep. Eg keep
GpuChannelManager::sync_point_manager, and everything else should keep calling
that.

GpuChannelManager::sync_point_manager can still be implemented as GetInstance.

> Needing a default constructor (for
> LazyInstance) also forces the condvar broadcast even on single-threaded cases,
> which seems like a regression too.

Could check kEnableThreadedTextureMailboxes?

Previous patch sets checked that for both thread checks and supporting wait, and
it was causing problems for thread checks because not everything sets that
correctly. But using it for wait support only should be fine.

[no sievers, 2015-07-16 21:53:41]

On 2015/07/16 21:26:31, boliu wrote:> 
> > Needing a default constructor (for
> > LazyInstance) also forces the condvar broadcast even on single-threaded
cases,
> > which seems like a regression too.
> 
> Could check kEnableThreadedTextureMailboxes?

Actually it has to broadcast the cond. var regardless for
InProcessCommandBuffer.
Even the tests (like cc_ or compositor_unittests) would block the client thread
on that.

[boliu, 2015-07-16 21:56:18]

On 2015/07/16 21:53:41, sievers wrote:
> On 2015/07/16 21:26:31, boliu wrote:> 
> > > Needing a default constructor (for
> > > LazyInstance) also forces the condvar broadcast even on single-threaded
> cases,
> > > which seems like a regression too.
> > 
> > Could check kEnableThreadedTextureMailboxes?
> 
> Actually it has to broadcast the cond. var regardless for
> InProcessCommandBuffer.
> Even the tests (like cc_ or compositor_unittests) would block the client
thread
> on that.

Those are still single threaded though, right? ie Wait is called after the sync
point has already been retired, so then there is no need to wait on the
condition var.

So... s/wait support/multi-threaded wait support/

[chromium-reviews, 2015-07-16 23:55:58]

On Thu, Jul 16, 2015 at 2:26 PM, <boliu@chromium.org> wrote:

> How does this sound
>
> On 2015/07/16 20:20:50, piman (Very slow to review) wrote:
>
>> Drive-by: making this a singleton seems like a regression... why can't we
>> keep
>> dependency injection? We can still remove the refcounting parts, provided
>> we
>> enforce single-ownership at a higher level.
>>
>
> I guess the idea that it's a singleton doesn't have to run so deep. Eg keep
> GpuChannelManager::sync_point_manager, and everything else should keep
> calling
> that.
>
> GpuChannelManager::sync_point_manager can still be implemented as
> GetInstance.


That's better, but is there a problem with having webview inject it into
InProcessCommandBuffer, say, via the InProcessCommandBuffer::Service?


>
>  Needing a default constructor (for
>> LazyInstance) also forces the condvar broadcast even on single-threaded
>> cases,
>> which seems like a regression too.
>>
>
> Could check kEnableThreadedTextureMailboxes?
>
> Previous patch sets checked that for both thread checks and supporting
> wait, and
> it was causing problems for thread checks because not everything sets that
> correctly. But using it for wait support only should be fine.
>
> https://codereview.chromium.org/1231263003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[boliu, 2015-07-17 00:01:37]

On 2015/07/16 23:55:58, chromium-reviews wrote:
> On Thu, Jul 16, 2015 at 2:26 PM, <mailto:boliu@chromium.org> wrote:
> 
> > How does this sound
> >
> > On 2015/07/16 20:20:50, piman (Very slow to review) wrote:
> >
> >> Drive-by: making this a singleton seems like a regression... why can't we
> >> keep
> >> dependency injection? We can still remove the refcounting parts, provided
> >> we
> >> enforce single-ownership at a higher level.
> >>
> >
> > I guess the idea that it's a singleton doesn't have to run so deep. Eg keep
> > GpuChannelManager::sync_point_manager, and everything else should keep
> > calling
> > that.
> >
> > GpuChannelManager::sync_point_manager can still be implemented as
> > GetInstance.
> 
> 
> That's better, but is there a problem with having webview inject it into
> InProcessCommandBuffer, say, via the InProcessCommandBuffer::Service?

Goal of this patch is to share the same SyncPointManager instance between
InProcessCommandBuffer and GpuCommandBufferStub classes. There is no easy way to
inject the instance there.

And end goal is to have webview use GpuCommandBufferStub and classes on chrome's
gpu thread, and get rid of most of the "if (SynchronousCompositorFactory ...)"
checks.

(InProcessCommandBuffer is not really a problem. SyncPointManager is already a
global LazyInstance there)

> 
> 
> >
> >  Needing a default constructor (for
> >> LazyInstance) also forces the condvar broadcast even on single-threaded
> >> cases,
> >> which seems like a regression too.
> >>
> >
> > Could check kEnableThreadedTextureMailboxes?
> >
> > Previous patch sets checked that for both thread checks and supporting
> > wait, and
> > it was causing problems for thread checks because not everything sets that
> > correctly. But using it for wait support only should be fine.
> >
> > https://codereview.chromium.org/1231263003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[boliu, 2015-07-17 00:16:26]

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

[chromium-reviews, 2015-07-17 00:27:42]

On Thu, Jul 16, 2015 at 5:01 PM, <boliu@chromium.org> wrote:

> On 2015/07/16 23:55:58, chromium-reviews wrote:
>
>> On Thu, Jul 16, 2015 at 2:26 PM, <mailto:boliu@chromium.org> wrote:
>>
>
>  > How does this sound
>> >
>> > On 2015/07/16 20:20:50, piman (Very slow to review) wrote:
>> >
>> >> Drive-by: making this a singleton seems like a regression... why can't
>> we
>> >> keep
>> >> dependency injection? We can still remove the refcounting parts,
>> provided
>> >> we
>> >> enforce single-ownership at a higher level.
>> >>
>> >
>> > I guess the idea that it's a singleton doesn't have to run so deep. Eg
>> keep
>> > GpuChannelManager::sync_point_manager, and everything else should keep
>> > calling
>> > that.
>> >
>> > GpuChannelManager::sync_point_manager can still be implemented as
>> > GetInstance.
>>
>
>
>  That's better, but is there a problem with having webview inject it into
>> InProcessCommandBuffer, say, via the InProcessCommandBuffer::Service?
>>
>
> Goal of this patch is to share the same SyncPointManager instance between
> InProcessCommandBuffer and GpuCommandBufferStub classes.


Understood.


> There is no easy way to inject the instance there.
>

Why? Sorry to be a pain, I'd like to understand what makes it hard. It
seems that eventually you want webview create a GpuThread, so you can
inject the SyncPointManager there, and pass it from the GpuThread to the
GpuChannelManager.
The same way you webview passes a InProcessCommandBuffer::Service to
the InProcessCommandBuffer, and if that includes the SyncPointManager
(instead of the lazy instance there), you're done.


> And end goal is to have webview use GpuCommandBufferStub and classes on
> chrome's
> gpu thread, and get rid of most of the "if (SynchronousCompositorFactory
> ...)"
> checks.
>
> (InProcessCommandBuffer is not really a problem. SyncPointManager is
> already a
> global LazyInstance there)
>
>
>
>  >
>> >  Needing a default constructor (for
>> >> LazyInstance) also forces the condvar broadcast even on single-threaded
>> >> cases,
>> >> which seems like a regression too.
>> >>
>> >
>> > Could check kEnableThreadedTextureMailboxes?
>> >
>> > Previous patch sets checked that for both thread checks and supporting
>> > wait, and
>> > it was causing problems for thread checks because not everything sets
>> that
>> > correctly. But using it for wait support only should be fine.
>> >
>> > https://codereview.chromium.org/1231263003/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
> https://codereview.chromium.org/1231263003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[boliu, 2015-07-17 00:40:42]

On 2015/07/17 00:27:42, chromium-reviews wrote:
> On Thu, Jul 16, 2015 at 5:01 PM, <mailto:boliu@chromium.org> wrote:
> 
> > On 2015/07/16 23:55:58, chromium-reviews wrote:
> >
> >> On Thu, Jul 16, 2015 at 2:26 PM, <mailto:boliu@chromium.org> wrote:
> >>
> >
> >  > How does this sound
> >> >
> >> > On 2015/07/16 20:20:50, piman (Very slow to review) wrote:
> >> >
> >> >> Drive-by: making this a singleton seems like a regression... why can't
> >> we
> >> >> keep
> >> >> dependency injection? We can still remove the refcounting parts,
> >> provided
> >> >> we
> >> >> enforce single-ownership at a higher level.
> >> >>
> >> >
> >> > I guess the idea that it's a singleton doesn't have to run so deep. Eg
> >> keep
> >> > GpuChannelManager::sync_point_manager, and everything else should keep
> >> > calling
> >> > that.
> >> >
> >> > GpuChannelManager::sync_point_manager can still be implemented as
> >> > GetInstance.
> >>
> >
> >
> >  That's better, but is there a problem with having webview inject it into
> >> InProcessCommandBuffer, say, via the InProcessCommandBuffer::Service?
> >>
> >
> > Goal of this patch is to share the same SyncPointManager instance between
> > InProcessCommandBuffer and GpuCommandBufferStub classes.
> 
> 
> Understood.
> 
> 
> > There is no easy way to inject the instance there.
> >
> 
> Why? Sorry to be a pain, I'd like to understand what makes it hard. It
> seems that eventually you want webview create a GpuThread, so you can
> inject the SyncPointManager there, and pass it from the GpuThread to the
> GpuChannelManager.

Webview won't need to customize creating GpuChildThread though (beyond make sure
it's in-process). This is not the thread that's shared between webview and
android. Adding plumbing to create a custom GpuThread for webview just to share
SyncPointManager seems a bit overkill..

> The same way you webview passes a InProcessCommandBuffer::Service to
> the InProcessCommandBuffer, and if that includes the SyncPointManager
> (instead of the lazy instance there), you're done.

Tagent. The service is passed through client classes, which makes sense only
because it's in-process. For ipc one, injection will need to be through some
other path.

> 
> 
> > And end goal is to have webview use GpuCommandBufferStub and classes on
> > chrome's
> > gpu thread, and get rid of most of the "if (SynchronousCompositorFactory
> > ...)"
> > checks.
> >
> > (InProcessCommandBuffer is not really a problem. SyncPointManager is
> > already a
> > global LazyInstance there)
> >
> >
> >
> >  >
> >> >  Needing a default constructor (for
> >> >> LazyInstance) also forces the condvar broadcast even on single-threaded
> >> >> cases,
> >> >> which seems like a regression too.
> >> >>
> >> >
> >> > Could check kEnableThreadedTextureMailboxes?
> >> >
> >> > Previous patch sets checked that for both thread checks and supporting
> >> > wait, and
> >> > it was causing problems for thread checks because not everything sets
> >> that
> >> > correctly. But using it for wait support only should be fine.
> >> >
> >> > https://codereview.chromium.org/1231263003/
> >> >
> >>
> >
> >  To unsubscribe from this group and stop receiving emails from it, send an
> >>
> > email
> >
> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >>
> >
> >
> > https://codereview.chromium.org/1231263003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[piman, 2015-07-17 01:06:00]

On Thu, Jul 16, 2015 at 5:40 PM, <boliu@chromium.org> wrote:

> On 2015/07/17 00:27:42, chromium-reviews wrote:
>
>  On Thu, Jul 16, 2015 at 5:01 PM, <mailto:boliu@chromium.org> wrote:
>>
>
>  > On 2015/07/16 23:55:58, chromium-reviews wrote:
>> >
>> >> On Thu, Jul 16, 2015 at 2:26 PM, <mailto:boliu@chromium.org> wrote:
>> >>
>> >
>> >  > How does this sound
>> >> >
>> >> > On 2015/07/16 20:20:50, piman (Very slow to review) wrote:
>> >> >
>> >> >> Drive-by: making this a singleton seems like a regression... why
>> can't
>> >> we
>> >> >> keep
>> >> >> dependency injection? We can still remove the refcounting parts,
>> >> provided
>> >> >> we
>> >> >> enforce single-ownership at a higher level.
>> >> >>
>> >> >
>> >> > I guess the idea that it's a singleton doesn't have to run so deep.
>> Eg
>> >> keep
>> >> > GpuChannelManager::sync_point_manager, and everything else should
>> keep
>> >> > calling
>> >> > that.
>> >> >
>> >> > GpuChannelManager::sync_point_manager can still be implemented as
>> >> > GetInstance.
>> >>
>> >
>> >
>> >  That's better, but is there a problem with having webview inject it
>> into
>> >> InProcessCommandBuffer, say, via the InProcessCommandBuffer::Service?
>> >>
>> >
>> > Goal of this patch is to share the same SyncPointManager instance
>> between
>> > InProcessCommandBuffer and GpuCommandBufferStub classes.
>>
>
>
>  Understood.
>>
>
>
>  > There is no easy way to inject the instance there.
>> >
>>
>
>  Why? Sorry to be a pain, I'd like to understand what makes it hard. It
>> seems that eventually you want webview create a GpuThread, so you can
>> inject the SyncPointManager there, and pass it from the GpuThread to the
>> GpuChannelManager.
>>
>
> Webview won't need to customize creating GpuChildThread though (beyond
> make sure
> it's in-process). This is not the thread that's shared between webview and
> android. Adding plumbing to create a custom GpuThread for webview just to
> share
> SyncPointManager seems a bit overkill..


Passing parameters through globals is a hack, so I'm not sure what's best?
:)

 The same way you webview passes a InProcessCommandBuffer::Service to
>> the InProcessCommandBuffer, and if that includes the SyncPointManager
>> (instead of the lazy instance there), you're done.
>>
>
> Tagent. The service is passed through client classes, which makes sense
> only
> because it's in-process. For ipc one, injection will need to be through
> some
> other path.


The IPC one would be handled by GpuThread.


>
>
>
>
>  > And end goal is to have webview use GpuCommandBufferStub and classes on
>> > chrome's
>> > gpu thread, and get rid of most of the "if (SynchronousCompositorFactory
>> > ...)"
>> > checks.
>> >
>> > (InProcessCommandBuffer is not really a problem. SyncPointManager is
>> > already a
>> > global LazyInstance there)
>> >
>> >
>> >
>> >  >
>> >> >  Needing a default constructor (for
>> >> >> LazyInstance) also forces the condvar broadcast even on
>> single-threaded
>> >> >> cases,
>> >> >> which seems like a regression too.
>> >> >>
>> >> >
>> >> > Could check kEnableThreadedTextureMailboxes?
>> >> >
>> >> > Previous patch sets checked that for both thread checks and
>> supporting
>> >> > wait, and
>> >> > it was causing problems for thread checks because not everything sets
>> >> that
>> >> > correctly. But using it for wait support only should be fine.
>> >> >
>> >> > https://codereview.chromium.org/1231263003/
>> >> >
>> >>
>> >
>> >  To unsubscribe from this group and stop receiving emails from it, send
>> an
>> >>
>> > email
>> >
>> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
>> >>
>> >
>> >
>> > https://codereview.chromium.org/1231263003/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
> https://codereview.chromium.org/1231263003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[boliu, 2015-07-17 20:20:37]

PS8 adds all the plumbing to inject the SyncPointManager instance in android
webview.

The only sketchy part is I had to include content/gpu/in_process_gpu_thread.h
from content/browser/android/in_process.

[boliu, 2015-07-17 23:17:12]

piman: ptal

I think all the bots should be green now

[piman, 2015-07-20 22:26:11]

LGTM, thanks

https://codereview.chromium.org/1231263003/diff/200001/content/gpu/gpu_child_...
File content/gpu/gpu_child_thread.h (right):

https://codereview.chromium.org/1231263003/diff/200001/content/gpu/gpu_child_...
content/gpu/gpu_child_thread.h:97: scoped_ptr<gpu::SyncPointManager>
sync_point_manager_;
This works, or you can also have it owned at a higher level (GpuMain).

[boliu, 2015-07-20 23:58:05]

https://codereview.chromium.org/1231263003/diff/200001/content/gpu/gpu_child_...
File content/gpu/gpu_child_thread.h (right):

https://codereview.chromium.org/1231263003/diff/200001/content/gpu/gpu_child_...
content/gpu/gpu_child_thread.h:97: scoped_ptr<gpu::SyncPointManager>
sync_point_manager_;
On 2015/07/20 22:26:11, piman (Very slow to review) wrote:
> This works, or you can also have it owned at a higher level (GpuMain).

Ahh yes, that's much better. Gets rid of this _overrid weirdness.

[boliu, 2015-07-20 23:59:01]

msw and/or sky: want to look over components/view_manager again? There have been
changes since approval

[msw, 2015-07-21 00:46:03]

https://codereview.chromium.org/1231263003/diff/240001/components/view_manage...
File components/view_manager/gles2/command_buffer_impl.cc (right):

https://codereview.chromium.org/1231263003/diff/240001/components/view_manage...
components/view_manager/gles2/command_buffer_impl.cc:113:
base::Unretained(sync_point_manager_), sync_point));
What guarantees that |sync_point_manager_| exists when the callback is run?

[boliu, 2015-07-21 00:57:19]

https://codereview.chromium.org/1231263003/diff/240001/components/view_manage...
File components/view_manager/gles2/command_buffer_impl.cc (right):

https://codereview.chromium.org/1231263003/diff/240001/components/view_manage...
components/view_manager/gles2/command_buffer_impl.cc:113:
base::Unretained(sync_point_manager_), sync_point));
On 2015/07/21 00:46:03, msw wrote:
> What guarantees that |sync_point_manager_| exists when the callback is run?

Hmm, I'm not sure actually.

It outlives the "control thread", but I'm guessing driver_task_runner is not the
control thread. And I don't know mojo threads ordering/ownership etc at all..

One not very pretty option is to add a ref counted wrapper..

[piman, 2015-07-21 02:44:05]

On Mon, Jul 20, 2015 at 5:57 PM, <boliu@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1231263003/diff/240001/components/view_manage...
> File components/view_manager/gles2/command_buffer_impl.cc (right):
>
>
>
https://codereview.chromium.org/1231263003/diff/240001/components/view_manage...
> components/view_manager/gles2/command_buffer_impl.cc:113:
> base::Unretained(sync_point_manager_), sync_point));
> On 2015/07/21 00:46:03, msw wrote:
>
>> What guarantees that |sync_point_manager_| exists when the callback is
>>
> run?
>
> Hmm, I'm not sure actually.
>
> It outlives the "control thread", but I'm guessing driver_task_runner is
> not the control thread. And I don't know mojo threads ordering/ownership
> etc at all..
>
> One not very pretty option is to add a ref counted wrapper..
>

I think we just need to make sure the GpuState outlives
the CommandBufferDriver.

Couldn't CommandBufferDriver take a scoped_refptr<GpuState> instead of a
raw pointer to each of its fields?


>
> https://codereview.chromium.org/1231263003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[boliu, 2015-07-21 17:34:36]

msw: ptal

On 2015/07/21 02:44:05, piman (Very slow to review) wrote:
> I think we just need to make sure the GpuState outlives
> the CommandBufferDriver.
> 
> Couldn't CommandBufferDriver take a scoped_refptr<GpuState> instead of a
> raw pointer to each of its fields?

Good suggestion! Thanks. I updated both CommandBufferDriver and
CommandBufferImpl to take GpuState directly. Object lifetime a lot cleaner now
imo.

One thing is should GpuState be RefCounted*ThreadSafe*? GLShareGroup is only
RefCounted, so it's safe, or it was broken already.

[piman, 2015-07-21 17:51:02]

lgtm

[msw, 2015-07-21 17:53:01]

Nice, components/view_manager/ lgtm with nits.

https://codereview.chromium.org/1231263003/diff/280001/components/view_manage...
File components/view_manager/gles2/command_buffer_driver.h (right):

https://codereview.chromium.org/1231263003/diff/280001/components/view_manage...
components/view_manager/gles2/command_buffer_driver.h:22: class
SyncPointManager;
nit: remove this.

https://codereview.chromium.org/1231263003/diff/280001/components/view_manage...
components/view_manager/gles2/command_buffer_driver.h:25: class MailboxManager;
nit: remove this.

https://codereview.chromium.org/1231263003/diff/280001/components/view_manage...
components/view_manager/gles2/command_buffer_driver.h:31: class GLShareGroup;
nit: remove this.

https://codereview.chromium.org/1231263003/diff/280001/components/view_manage...
components/view_manager/gles2/command_buffer_driver.h:36: class GpuState;
nit: blank line above?

https://codereview.chromium.org/1231263003/diff/280001/components/view_manage...
File components/view_manager/gles2/command_buffer_impl.h (right):

https://codereview.chromium.org/1231263003/diff/280001/components/view_manage...
components/view_manager/gles2/command_buffer_impl.h:15: class SyncPointManager;
nit: remove this.

https://codereview.chromium.org/1231263003/diff/280001/components/view_manage...
File components/view_manager/gles2/gpu_impl.cc (right):

https://codereview.chromium.org/1231263003/diff/280001/components/view_manage...
components/view_manager/gles2/gpu_impl.cc:9: #include
"gpu/command_buffer/service/mailbox_manager.h"
nit: remove this.

https://codereview.chromium.org/1231263003/diff/280001/components/view_manage...
components/view_manager/gles2/gpu_impl.cc:10: #include
"gpu/command_buffer/service/mailbox_manager_impl.h"
nit: remove this.

https://codereview.chromium.org/1231263003/diff/280001/components/view_manage...
components/view_manager/gles2/gpu_impl.cc:11: #include
"gpu/command_buffer/service/sync_point_manager.h"
nit: remove this.

https://codereview.chromium.org/1231263003/diff/280001/components/view_manage...
components/view_manager/gles2/gpu_impl.cc:12: #include
"mojo/converters/geometry/geometry_type_converters.h"
nit: remove if not needed.

https://codereview.chromium.org/1231263003/diff/280001/components/view_manage...
components/view_manager/gles2/gpu_impl.cc:13: #include "ui/gl/gl_share_group.h"
nit: remove if not needed.

https://codereview.chromium.org/1231263003/diff/280001/components/view_manage...
components/view_manager/gles2/gpu_impl.cc:14: #include "ui/gl/gl_surface.h"
nit: remove if not needed.

[boliu, 2015-07-21 18:03:08]

All unused forward declarations and includes removed. This includes a bunch from
gpu_impl.h which wasn't touched before, so didn't show up in review.

[boliu, 2015-07-21 18:04:20]

The CQ bit was checked by boliu@chromium.org

[boliu, 2015-07-21 18:04:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbauman@chromium.org, msw@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1231263003/#ps300001
(title: "remove forward decl/includes")

[msw, 2015-07-21 18:05:15]

thanks lgtm

[commit-bot: I haz the power, 2015-07-21 18:06:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1231263003/300001

[commit-bot: I haz the power, 2015-07-21 19:09:05]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2015-07-21 19:09:54]

Patchset 16 (id:??) landed as
https://crrev.com/c5befe7d0b86bb4996017605f1edc4bfa1c327f8
Cr-Commit-Position: refs/heads/master@{#339712}