Part 2: WIP Refactor Webview graphics related code

Part 2: Refactor Android WebView graphics code

1. Change the object ownership to be:
AwContents -> BrowserViewRenderer -> SharedRendererState ->
HardwareRenderer
(The left object owns the right object.)

2. Move DrawGL, InitializeHardwareIfNeeded, ReleaseHardwareIfNeeded from
AwContents to SharedRendererState.

BUG=418346
Committed: https://crrev.com/c4a6fc020e5de65add90f1755519b4228036cbae
Cr-Commit-Position: refs/heads/master@{#302011}

[hush (inactive), 2014-10-15 01:42:29]

Patchset #1 (id:1) has been deleted

[hush (inactive), 2014-10-15 04:02:18]

Patchset #1 (id:20001) has been deleted

[hush (inactive), 2014-10-15 04:08:29]

Patchset #1 (id:40001) has been deleted

[hush (inactive), 2014-10-15 04:13:36]

Patchset #1 (id:60001) has been deleted

[hush (inactive), 2014-10-15 04:14:31]

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

[hush (inactive), 2014-10-15 04:14:32]

This based on https://codereview.chromium.org/655813004/ Patch Set 1.

[hush (inactive), 2014-10-23 02:28:40]

ps2: based on ps2 of sister CL.

I compiled webview using upstream code and some basic smoke tests with a few
webview apps. Seems to work. I will do more tests later.

[hush (inactive), 2014-10-25 03:06:25]

I haven't tested PS 3 yet...

[hush (inactive), 2014-10-25 03:11:04]

Patchset #3 (id:120001) has been deleted

[hush (inactive), 2014-10-28 00:57:16]

On 2014/10/25 03:06:25, hush wrote:
> I haven't tested PS 3 yet...

I just flashed PS3 and tested a few apps (gmail, books, magazine and vice). And
they all seem to work.

So PTAL PS3.

[boliu, 2014-10-28 19:49:44]

https://codereview.chromium.org/653173004/diff/140001/android_webview/browser...
File android_webview/browser/browser_view_renderer.cc (left):

https://codereview.chromium.org/653173004/diff/140001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:757:
ReturnUnusedResource(shared_renderer_state_.PassCompositorFrame());
This isn't quite the same anymore.

This used to return the resource to child compositor. Now it's just hanging
around in SRS::returned_resources_

I think if we need PassCompositorFrame on both threads, we should just have two
versions of it for now. The one on UI can be called PassUncommittedFrameOnUI()
or something like that to distinguish them.

In the future, we can refactor CommitFrame to be on SRS, which pushes the frame
into HR.

https://codereview.chromium.org/653173004/diff/140001/android_webview/browser...
File android_webview/browser/browser_view_renderer.h (right):

https://codereview.chromium.org/653173004/diff/140001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:110: void
InvalidateOnFunctorDestroy();
nit: Put this next to UpdateParentDrawContraints/DidSkipCommitFrame, ie the rest
of methods called by SRS

https://codereview.chromium.org/653173004/diff/140001/android_webview/browser...
File android_webview/browser/shared_renderer_state.cc (right):

https://codereview.chromium.org/653173004/diff/140001/android_webview/browser...
android_webview/browser/shared_renderer_state.cc:333:
SetInsideHardwareRelease(true);
Keep using a scoped var to enforce this. Now it can be a private inner class.

https://codereview.chromium.org/653173004/diff/140001/android_webview/browser...
android_webview/browser/shared_renderer_state.cc:352: // hardware_renderer_ is
reset on RT?
Technically yes, but that would mean also locking around setters to
hardware_renderer_. I'm ok with removing this DCHECK but keep the one in the
destructor.

https://codereview.chromium.org/653173004/diff/140001/android_webview/browser...
android_webview/browser/shared_renderer_state.cc:366: // Flush any invoke
functors that's caused by OnDetachedFromWindow.
s/OnDetachedFromWindow/ReleaseHardware/

https://codereview.chromium.org/653173004/diff/140001/android_webview/browser...
File android_webview/browser/shared_renderer_state.h (right):

https://codereview.chromium.org/653173004/diff/140001/android_webview/browser...
android_webview/browser/shared_renderer_state.h:89: GLViewRendererManager::Key
renderer_manager_key_;
This is both UI and RT

Although...I think this can almost be moved to RT exclusively. But let's do that
after we have tests.

https://codereview.chromium.org/653173004/diff/140001/android_webview/native/...
File android_webview/native/aw_contents.h (right):

https://codereview.chromium.org/653173004/diff/140001/android_webview/native/...
android_webview/native/aw_contents.h:246: SharedRendererState*
shared_renderer_state_;
At _some_ point in the future, we should be able to remove this pointer as well.

[hush (inactive), 2014-10-29 19:53:06]

Patchset #4 (id:160001) has been deleted

[hush (inactive), 2014-10-29 19:57:04]

https://codereview.chromium.org/653173004/diff/140001/android_webview/browser...
File android_webview/browser/browser_view_renderer.cc (left):

https://codereview.chromium.org/653173004/diff/140001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:757:
ReturnUnusedResource(shared_renderer_state_.PassCompositorFrame());
On 2014/10/28 19:49:43, boliu wrote:
> This isn't quite the same anymore.
> 
> This used to return the resource to child compositor. Now it's just hanging
> around in SRS::returned_resources_
> 
> I think if we need PassCompositorFrame on both threads, we should just have
two
> versions of it for now. The one on UI can be called PassUncommittedFrameOnUI()
> or something like that to distinguish them.
> 
> In the future, we can refactor CommitFrame to be on SRS, which pushes the
frame
> into HR.

The resources in SRS::returned_resources_ will be returned the next time
ReturnResourceFromParent() is called, I think. So it is not a correctness
problem?
Anyways I just added PassUncommittedFrameOnUI() as suggested, which has the same
implementation of PassCompositorFrame(). So it is kind of weird.

https://codereview.chromium.org/653173004/diff/140001/android_webview/browser...
File android_webview/browser/browser_view_renderer.h (right):

https://codereview.chromium.org/653173004/diff/140001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:110: void
InvalidateOnFunctorDestroy();
On 2014/10/28 19:49:43, boliu wrote:
> nit: Put this next to UpdateParentDrawContraints/DidSkipCommitFrame, ie the
rest
> of methods called by SRS

Done.

https://codereview.chromium.org/653173004/diff/140001/android_webview/browser...
File android_webview/browser/shared_renderer_state.cc (right):

https://codereview.chromium.org/653173004/diff/140001/android_webview/browser...
android_webview/browser/shared_renderer_state.cc:333:
SetInsideHardwareRelease(true);
On 2014/10/28 19:49:43, boliu wrote:
> Keep using a scoped var to enforce this. Now it can be a private inner class.

Done.

https://codereview.chromium.org/653173004/diff/140001/android_webview/browser...
android_webview/browser/shared_renderer_state.cc:352: // hardware_renderer_ is
reset on RT?
On 2014/10/28 19:49:43, boliu wrote:
> Technically yes, but that would mean also locking around setters to
> hardware_renderer_. I'm ok with removing this DCHECK but keep the one in the
> destructor.

Removed this DCHECK.

https://codereview.chromium.org/653173004/diff/140001/android_webview/browser...
android_webview/browser/shared_renderer_state.cc:366: // Flush any invoke
functors that's caused by OnDetachedFromWindow.
On 2014/10/28 19:49:43, boliu wrote:
> s/OnDetachedFromWindow/ReleaseHardware/

Done.

https://codereview.chromium.org/653173004/diff/140001/android_webview/browser...
File android_webview/browser/shared_renderer_state.h (right):

https://codereview.chromium.org/653173004/diff/140001/android_webview/browser...
android_webview/browser/shared_renderer_state.h:89: GLViewRendererManager::Key
renderer_manager_key_;
On 2014/10/28 19:49:43, boliu wrote:
> This is both UI and RT
> 
> Although...I think this can almost be moved to RT exclusively. But let's do
that
> after we have tests.

I see. Make the pushback and removal of the key happen on RT.
I will add a TODO here.

https://codereview.chromium.org/653173004/diff/140001/android_webview/native/...
File android_webview/native/aw_contents.h (right):

https://codereview.chromium.org/653173004/diff/140001/android_webview/native/...
android_webview/native/aw_contents.h:246: SharedRendererState*
shared_renderer_state_;
On 2014/10/28 19:49:43, boliu wrote:
> At _some_ point in the future, we should be able to remove this pointer as
well.

Yes, hopefully. Added a TODO here.
I guess one major thing is DrawGL functor registration.
Right now AwContents::DrawGL is registered as the functor. To remove this
pointer, we need to register SharedRendererState::DrawGL instead? So that
AwContents does not need to forward the call to SharedRendererState. But that
breaks the assumption that AwContents being the boundary between java and native
and exposes the existence of SRS to android frameworks somehow.

[boliu, 2014-10-29 22:40:33]

lgtm

remember to remove 3 in description

https://codereview.chromium.org/653173004/diff/140001/android_webview/native/...
File android_webview/native/aw_contents.h (right):

https://codereview.chromium.org/653173004/diff/140001/android_webview/native/...
android_webview/native/aw_contents.h:246: SharedRendererState*
shared_renderer_state_;
On 2014/10/29 19:57:04, hush wrote:
> On 2014/10/28 19:49:43, boliu wrote:
> > At _some_ point in the future, we should be able to remove this pointer as
> well.
> 
> Yes, hopefully. Added a TODO here.
> I guess one major thing is DrawGL functor registration.
> Right now AwContents::DrawGL is registered as the functor. To remove this
> pointer, we need to register SharedRendererState::DrawGL instead?

Yeah that's the idea I have in my head.

> So that
> AwContents does not need to forward the call to SharedRendererState. But that
> breaks the assumption that AwContents being the boundary between java and
native
> and exposes the existence of SRS to android frameworks somehow.

The other side of functor isn't java though. Also to frameworks, it's just an
opaque pointer, we are not exposing anything.

[hush (inactive), 2014-10-30 00:46:37]

https://codereview.chromium.org/653173004/diff/140001/android_webview/native/...
File android_webview/native/aw_contents.h (right):

https://codereview.chromium.org/653173004/diff/140001/android_webview/native/...
android_webview/native/aw_contents.h:246: SharedRendererState*
shared_renderer_state_;
On 2014/10/29 22:40:33, boliu wrote:
> On 2014/10/29 19:57:04, hush wrote:
> > On 2014/10/28 19:49:43, boliu wrote:
> > > At _some_ point in the future, we should be able to remove this pointer as
> > well.
> > 
> > Yes, hopefully. Added a TODO here.
> > I guess one major thing is DrawGL functor registration.
> > Right now AwContents::DrawGL is registered as the functor. To remove this
> > pointer, we need to register SharedRendererState::DrawGL instead?
> 
> Yeah that's the idea I have in my head.
> 
> > So that
> > AwContents does not need to forward the call to SharedRendererState. But
that
> > breaks the assumption that AwContents being the boundary between java and
> native
> > and exposes the existence of SRS to android frameworks somehow.
> 
> The other side of functor isn't java though. Also to frameworks, it's just an
> opaque pointer, we are not exposing anything.

filed https://code.google.com/p/chromium/issues/detail?id=428550 for it.

[hush (inactive), 2014-10-30 00:46:41]

The CQ bit was checked by hush@chromium.org

[commit-bot: I haz the power, 2014-10-30 00:48:31]

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

[commit-bot: I haz the power, 2014-10-30 01:34:33]

Committed patchset #4 (id:180001)

[commit-bot: I haz the power, 2014-10-30 01:35:00]

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