aw: Refactor hardware init

aw: Refactor hardware init

Move inserting the initialization task to first OnDraw with hardware
canvas rather than OnAttach. Also insert view into GLViewRendererManager
here.

Refactor DeferredGpuCommandService into its own file, and have
AwContents interact with it. Move ScopedAppGLStateRestore into
AwContents as well. This meant that the header cannot include any GL
files, so required a refactor there.

Also discovered GLViewRendererManager::PushBack is not thread safe
during refactoring. Fix that as well.

BUG=344087
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269461

[boliu, 2014-05-08 16:02:47]

hold off review for this for now, I want to make some more changes

[boliu, 2014-05-08 20:47:48]

ok, ready for review

[sgurun-gerrit only, 2014-05-08 22:55:11]

https://codereview.chromium.org/273703008/diff/20001/android_webview/browser/...
File android_webview/browser/gl_view_renderer_manager.cc (right):

https://codereview.chromium.org/273703008/diff/20001/android_webview/browser/...
android_webview/browser/gl_view_renderer_manager.cc:29: AutoLock
auto_lock(lock_);
did you need to add this because of the refactoring or are you fixing a bug? if
there is a known bug, either use a separate CL with a better explanation of the
bug or mention it in the commit message.

https://codereview.chromium.org/273703008/diff/20001/android_webview/native/a...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/273703008/diff/20001/android_webview/native/a...
android_webview/native/aw_contents.cc:886: if (is_hardware_accelerated)
why is moving the initialization to here a better way?

[boliu, 2014-05-08 23:03:47]

https://codereview.chromium.org/273703008/diff/20001/android_webview/browser/...
File android_webview/browser/gl_view_renderer_manager.cc (right):

https://codereview.chromium.org/273703008/diff/20001/android_webview/browser/...
android_webview/browser/gl_view_renderer_manager.cc:29: AutoLock
auto_lock(lock_);
On 2014/05/08 22:55:12, sgurun wrote:
> did you need to add this because of the refactoring or are you fixing a bug?
if
> there is a known bug, either use a separate CL with a better explanation of
the
> bug or mention it in the commit message.

Just an unrelated bug found while refactoring. Updated CL description.

https://codereview.chromium.org/273703008/diff/20001/android_webview/native/a...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/273703008/diff/20001/android_webview/native/a...
android_webview/native/aw_contents.cc:886: if (is_hardware_accelerated)
On 2014/05/08 22:55:12, sgurun wrote:
> why is moving the initialization to here a better way?

It didn't really move, it's just preparation for ubercomp. I'll explain a bit.
So hardware life time is somethign like this:

1. Attach (to hardware tree)
2. OnDraw (with hardware canvas)
3. Functor (on render thread)
4. Detach

Of course 2 and 3 can repeat as many times as necessary.

Previously, before this patch, we initialized hardware lazily in 3. We don't
need to do anything in 2 because there was nothing to do.

But in ubercomp world, we do need to do something in 2. This also precents a
natural point to initialize hardware on UI thread, so moved the bit in 1 to 2 as
well.

Ehh...makes sense...?

[sgurun-gerrit only, 2014-05-08 23:09:59]

On 2014/05/08 23:03:47, boliu wrote:
>
https://codereview.chromium.org/273703008/diff/20001/android_webview/browser/...
> File android_webview/browser/gl_view_renderer_manager.cc (right):
> 
>
https://codereview.chromium.org/273703008/diff/20001/android_webview/browser/...
> android_webview/browser/gl_view_renderer_manager.cc:29: AutoLock
> auto_lock(lock_);
> On 2014/05/08 22:55:12, sgurun wrote:
> > did you need to add this because of the refactoring or are you fixing a bug?
> if
> > there is a known bug, either use a separate CL with a better explanation of
> the
> > bug or mention it in the commit message.
> 
> Just an unrelated bug found while refactoring. Updated CL description.
> 
>
https://codereview.chromium.org/273703008/diff/20001/android_webview/native/a...
> File android_webview/native/aw_contents.cc (right):
> 
>
https://codereview.chromium.org/273703008/diff/20001/android_webview/native/a...
> android_webview/native/aw_contents.cc:886: if (is_hardware_accelerated)
> On 2014/05/08 22:55:12, sgurun wrote:
> > why is moving the initialization to here a better way?
> 
> It didn't really move, it's just preparation for ubercomp. I'll explain a bit.
> So hardware life time is somethign like this:
> 
> 1. Attach (to hardware tree)
> 2. OnDraw (with hardware canvas)
> 3. Functor (on render thread)
> 4. Detach
> 
> Of course 2 and 3 can repeat as many times as necessary.
> 
> Previously, before this patch, we initialized hardware lazily in 3. We don't
> need to do anything in 2 because there was nothing to do.
> 
> But in ubercomp world, we do need to do something in 2. This also precents a
> natural point to initialize hardware on UI thread, so moved the bit in 1 to 2
as
> well.
> 
> Ehh...makes sense...?

Isn't  it better to do it in 1 then rather then checking for it each time ondraw
is called.

[boliu, 2014-05-08 23:13:11]

On 2014/05/08 23:09:59, sgurun wrote:
> On 2014/05/08 23:03:47, boliu wrote:
> >
>
https://codereview.chromium.org/273703008/diff/20001/android_webview/browser/...
> > File android_webview/browser/gl_view_renderer_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/273703008/diff/20001/android_webview/browser/...
> > android_webview/browser/gl_view_renderer_manager.cc:29: AutoLock
> > auto_lock(lock_);
> > On 2014/05/08 22:55:12, sgurun wrote:
> > > did you need to add this because of the refactoring or are you fixing a
bug?
> > if
> > > there is a known bug, either use a separate CL with a better explanation
of
> > the
> > > bug or mention it in the commit message.
> > 
> > Just an unrelated bug found while refactoring. Updated CL description.
> > 
> >
>
https://codereview.chromium.org/273703008/diff/20001/android_webview/native/a...
> > File android_webview/native/aw_contents.cc (right):
> > 
> >
>
https://codereview.chromium.org/273703008/diff/20001/android_webview/native/a...
> > android_webview/native/aw_contents.cc:886: if (is_hardware_accelerated)
> > On 2014/05/08 22:55:12, sgurun wrote:
> > > why is moving the initialization to here a better way?
> > 
> > It didn't really move, it's just preparation for ubercomp. I'll explain a
bit.
> > So hardware life time is somethign like this:
> > 
> > 1. Attach (to hardware tree)
> > 2. OnDraw (with hardware canvas)
> > 3. Functor (on render thread)
> > 4. Detach
> > 
> > Of course 2 and 3 can repeat as many times as necessary.
> > 
> > Previously, before this patch, we initialized hardware lazily in 3. We don't
> > need to do anything in 2 because there was nothing to do.
> > 
> > But in ubercomp world, we do need to do something in 2. This also precents a
> > natural point to initialize hardware on UI thread, so moved the bit in 1 to
2
> as
> > well.
> > 
> > Ehh...makes sense...?
> 
> Isn't  it better to do it in 1 then rather then checking for it each time
ondraw
> is called.

Nope, that was an explicit decision to not do it in 1. We've had apps that
attaches webview to a hardware view tree, but never makes it visible so never
does a single hardware draw. In those cases, initializing hardware is a total
waste.

Note that this behavior did not change in this patch. The removed code in
OnDetached explicitly said it only inserts the task, but not schedule it.

[sgurun-gerrit only, 2014-05-08 23:18:29]

On 2014/05/08 23:13:11, boliu wrote:
> On 2014/05/08 23:09:59, sgurun wrote:
> > On 2014/05/08 23:03:47, boliu wrote:
> > >
> >
>
https://codereview.chromium.org/273703008/diff/20001/android_webview/browser/...
> > > File android_webview/browser/gl_view_renderer_manager.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/273703008/diff/20001/android_webview/browser/...
> > > android_webview/browser/gl_view_renderer_manager.cc:29: AutoLock
> > > auto_lock(lock_);
> > > On 2014/05/08 22:55:12, sgurun wrote:
> > > > did you need to add this because of the refactoring or are you fixing a
> bug?
> > > if
> > > > there is a known bug, either use a separate CL with a better explanation
> of
> > > the
> > > > bug or mention it in the commit message.
> > > 
> > > Just an unrelated bug found while refactoring. Updated CL description.
> > > 
> > >
> >
>
https://codereview.chromium.org/273703008/diff/20001/android_webview/native/a...
> > > File android_webview/native/aw_contents.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/273703008/diff/20001/android_webview/native/a...
> > > android_webview/native/aw_contents.cc:886: if (is_hardware_accelerated)
> > > On 2014/05/08 22:55:12, sgurun wrote:
> > > > why is moving the initialization to here a better way?
> > > 
> > > It didn't really move, it's just preparation for ubercomp. I'll explain a
> bit.
> > > So hardware life time is somethign like this:
> > > 
> > > 1. Attach (to hardware tree)
> > > 2. OnDraw (with hardware canvas)
> > > 3. Functor (on render thread)
> > > 4. Detach
> > > 
> > > Of course 2 and 3 can repeat as many times as necessary.
> > > 
> > > Previously, before this patch, we initialized hardware lazily in 3. We
don't
> > > need to do anything in 2 because there was nothing to do.
> > > 
> > > But in ubercomp world, we do need to do something in 2. This also precents
a
> > > natural point to initialize hardware on UI thread, so moved the bit in 1
to
> 2
> > as
> > > well.
> > > 
> > > Ehh...makes sense...?
> > 
> > Isn't  it better to do it in 1 then rather then checking for it each time
> ondraw
> > is called.
> 
> Nope, that was an explicit decision to not do it in 1. We've had apps that
> attaches webview to a hardware view tree, but never makes it visible so never
> does a single hardware draw. In those cases, initializing hardware is a total
> waste.
> 
> Note that this behavior did not change in this patch. The removed code in
> OnDetached explicitly said it only inserts the task, but not schedule it.

lgtm

[boliu, 2014-05-08 23:20:24]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2014-05-08 23:27:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/boliu@chromium.org/273703008/20001

[commit-bot: I haz the power, 2014-05-09 23:16:49]

Change committed as 269461