Part 1: Refactor Android WebView graphics code

Part 1: Refactor Android WebView graphics code

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

HardwareRenderer will be owned by SharedRendererState in Part 2 CL:
https://codereview.chromium.org/653173004/.

2. Rename some functions in SharedRendererState to be clear about the
threads they are called.

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

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

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

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

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

[hush (inactive), 2014-10-15 03:24:05]

Patchset #1 (id:1) has been deleted

[hush (inactive), 2014-10-15 03:26:24]

Part 2 will be about moving the DrawGL and hardware initialization/release part
from aw_contents into compositor_proxy. It will also make compositor_proxy
return the unused frame resources.

[hush (inactive), 2014-10-16 18:15:31]

what do you think of the names btw? I'm not sure of a better name for
"BrowserViewRender"

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

got conflicts in almost every file during the rebase... I did some basic tests
together with Part 2. Things seem to work.

[hush (inactive), 2014-10-23 02:33:45]

Patchset #2 (id:40001) has been deleted

[hush (inactive), 2014-10-23 02:39:38]

Patchset #2 (id:60001) has been deleted

[boliu, 2014-10-23 16:40:06]

I think we can have the proxy be created and owned by BVR. Then if AwContents
wants to use the proxy, it reaches through BVR.

Also in cc, there's no interface between LayerTreeHost (BVR) and ThreadProxy
(CompositorProxy), so I wonder if we need one too, needs more thought.

And this is the opportunity to pick better names, at least for the new classes.

https://codereview.chromium.org/655813004/diff/80001/android_webview/browser/...
File android_webview/browser/aw_gl_methods.h (right):

https://codereview.chromium.org/655813004/diff/80001/android_webview/browser/...
android_webview/browser/aw_gl_methods.h:15: class AwGLMethods {
virtual destructor

https://codereview.chromium.org/655813004/diff/80001/android_webview/browser/...
android_webview/browser/aw_gl_methods.h:26: virtual void
InvalidateOnFunctorDestroy() = 0;
Nothing calls this yet, right? Move it to the follow up patch then

https://codereview.chromium.org/655813004/diff/80001/android_webview/browser/...
android_webview/browser/aw_gl_methods.h:28: }
nit: space above and comment on closing brace

https://codereview.chromium.org/655813004/diff/80001/android_webview/browser/...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/655813004/diff/80001/android_webview/browser/...
android_webview/browser/browser_view_renderer.cc:437: // Unused resource held by
proxy_ will be returned by the proxy.
Can we not do this in this CL?

If there's something depends on this change, pull it out in to a separate CL and
land that first

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

https://codereview.chromium.org/655813004/diff/80001/android_webview/browser/...
android_webview/browser/browser_view_renderer.h:65: AwGLMethods* gl_methods,
Hmm...sucks that we need both AwGLMethods and BrowserViewRendererClient here.

https://codereview.chromium.org/655813004/diff/80001/android_webview/browser/...
android_webview/browser/browser_view_renderer.h:197: BrowserViewRendererProxy*
proxy_;
Should BVR create/own the proxy? (When all the functor stuff is moved to proxy,
maybe that's in the follow up?)

https://codereview.chromium.org/655813004/diff/80001/android_webview/browser/...
File android_webview/browser/browser_view_renderer_proxy.h (right):

https://codereview.chromium.org/655813004/diff/80001/android_webview/browser/...
android_webview/browser/browser_view_renderer_proxy.h:16: class
BrowserViewRendererProxy {
virtual destructor

https://codereview.chromium.org/655813004/diff/80001/android_webview/browser/...
android_webview/browser/browser_view_renderer_proxy.h:23: virtual const
ParentCompositorDrawConstraints GetParentDrawConstraints()
first const is redundant, you are passing by value here

https://codereview.chromium.org/655813004/diff/80001/android_webview/browser/...
File android_webview/browser/compositor_proxy.h (right):

https://codereview.chromium.org/655813004/diff/80001/android_webview/browser/...
android_webview/browser/compositor_proxy.h:41: ~CompositorProxy();
virtual

https://codereview.chromium.org/655813004/diff/80001/android_webview/browser/...
android_webview/browser/compositor_proxy.h:103: BrowserViewRenderer*
browser_view_renderer_;
this is in the "ui only" category, move it up there

https://codereview.chromium.org/655813004/diff/80001/android_webview/browser/...
File android_webview/browser/hardware_renderer_proxy.h (right):

https://codereview.chromium.org/655813004/diff/80001/android_webview/browser/...
android_webview/browser/hardware_renderer_proxy.h:15: class
HardwareRendererProxy {
virtual destructor

https://codereview.chromium.org/655813004/diff/80001/android_webview/browser/...
android_webview/browser/hardware_renderer_proxy.h:17: virtual gfx::Vector2d
GetScrollOffset() = 0;
nit, can we add OnRT to the end of all these function calls, to make
distinguishing them easier, just by name

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

https://codereview.chromium.org/655813004/diff/80001/android_webview/native/a...
android_webview/native/aw_contents.cc:174:
compositor_proxy_.SetBrowserViewRenderer(&browser_view_renderer_);
need to set it to null in the destructor, and in theory null check every single
use of the pointer in the proxy

Maybe we can find a better way to structure it to avoid this.

[hush (inactive), 2014-10-24 21:31:18]

Patchset #3 (id:100001) has been deleted

[hush (inactive), 2014-10-24 21:31:34]

https://codereview.chromium.org/655813004/diff/80001/android_webview/browser/...
File android_webview/browser/aw_gl_methods.h (right):

https://codereview.chromium.org/655813004/diff/80001/android_webview/browser/...
android_webview/browser/aw_gl_methods.h:15: class AwGLMethods {
On 2014/10/23 16:40:05, boliu wrote:
> virtual destructor

No AwGLMethods anymore in the next patch...

https://codereview.chromium.org/655813004/diff/80001/android_webview/browser/...
android_webview/browser/aw_gl_methods.h:26: virtual void
InvalidateOnFunctorDestroy() = 0;
On 2014/10/23 16:40:05, boliu wrote:
> Nothing calls this yet, right? Move it to the follow up patch then

No AwGLMethods anymore in the next patch...

https://codereview.chromium.org/655813004/diff/80001/android_webview/browser/...
android_webview/browser/aw_gl_methods.h:28: }
On 2014/10/23 16:40:05, boliu wrote:
> nit: space above and comment on closing brace

No AwGLMethods anymore in the next patch...

https://codereview.chromium.org/655813004/diff/80001/android_webview/browser/...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/655813004/diff/80001/android_webview/browser/...
android_webview/browser/browser_view_renderer.cc:437: // Unused resource held by
proxy_ will be returned by the proxy.
On 2014/10/23 16:40:05, boliu wrote:
> Can we not do this in this CL?
> 
> If there's something depends on this change, pull it out in to a separate CL
and
> land that first

I put this back. I guess I will do this in part 2, where I move DrawGL and
hardware initialization/release from AwContents to SharedRendererState

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

https://codereview.chromium.org/655813004/diff/80001/android_webview/browser/...
android_webview/browser/browser_view_renderer.h:65: AwGLMethods* gl_methods,
On 2014/10/23 16:40:05, boliu wrote:
> Hmm...sucks that we need both AwGLMethods and BrowserViewRendererClient here.

In the next pathc, BVR is owned by AwContents and keeps a 
BrowserViewRendererClient pointer. And there is no AwGLMethods pointer anymore

https://codereview.chromium.org/655813004/diff/80001/android_webview/browser/...
android_webview/browser/browser_view_renderer.h:197: BrowserViewRendererProxy*
proxy_;
On 2014/10/23 16:40:05, boliu wrote:
> Should BVR create/own the proxy? (When all the functor stuff is moved to
proxy,
> maybe that's in the follow up?)


In the next patch, BVR owns proxy (currently still named SharedRendererState)

https://codereview.chromium.org/655813004/diff/80001/android_webview/browser/...
File android_webview/browser/browser_view_renderer_proxy.h (right):

https://codereview.chromium.org/655813004/diff/80001/android_webview/browser/...
android_webview/browser/browser_view_renderer_proxy.h:16: class
BrowserViewRendererProxy {
On 2014/10/23 16:40:05, boliu wrote:
> virtual destructor

No BrowserViewRendererProxy anymore...

https://codereview.chromium.org/655813004/diff/80001/android_webview/browser/...
android_webview/browser/browser_view_renderer_proxy.h:23: virtual const
ParentCompositorDrawConstraints GetParentDrawConstraints()
On 2014/10/23 16:40:05, boliu wrote:
> first const is redundant, you are passing by value here

Changed this in SRS

https://codereview.chromium.org/655813004/diff/80001/android_webview/browser/...
File android_webview/browser/compositor_proxy.h (right):

https://codereview.chromium.org/655813004/diff/80001/android_webview/browser/...
android_webview/browser/compositor_proxy.h:103: BrowserViewRenderer*
browser_view_renderer_;
On 2014/10/23 16:40:05, boliu wrote:
> this is in the "ui only" category, move it up there

Done.

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

https://codereview.chromium.org/655813004/diff/80001/android_webview/native/a...
android_webview/native/aw_contents.cc:174:
compositor_proxy_.SetBrowserViewRenderer(&browser_view_renderer_);
On 2014/10/23 16:40:05, boliu wrote:
> need to set it to null in the destructor, and in theory null check every
single
> use of the pointer in the proxy
> 
> Maybe we can find a better way to structure it to avoid this.

yeah. We changed the object ownership model as discussed. So no need to do this
anymore

[hush (inactive), 2014-10-24 21:37:41]

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

https://codereview.chromium.org/655813004/diff/120001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:450:
DCHECK(shared_renderer_state_->ReturnedResourcesEmpty());
We don't need this because we have an equivalent DCHECK here:
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/br...

[boliu, 2014-10-24 22:12:02]

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

https://codereview.chromium.org/655813004/diff/120001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:450:
DCHECK(shared_renderer_state_->ReturnedResourcesEmpty());
On 2014/10/24 21:37:41, hush wrote:
> We don't need this because we have an equivalent DCHECK here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/br...

Keep this for now.

It's not quite the same. What if we decide to rewrite ReturnResourceFromParent
or something in the future.

https://codereview.chromium.org/655813004/diff/120001/android_webview/browser...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/655813004/diff/120001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:76:
content::BrowserThread::GetMessageLoopProxyForThread(
pass in ui_task_runner

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

https://codereview.chromium.org/655813004/diff/120001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:193: // BrowserViewRenderer owns
SharedRendererState.
Don't need this, that's obvious.

[boliu, 2014-10-24 22:12:44]

the SHS owns HR part isn't true yet, so update the description

[hush (inactive), 2014-10-24 22:30:15]

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

https://codereview.chromium.org/655813004/diff/120001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:450:
DCHECK(shared_renderer_state_->ReturnedResourcesEmpty());
On 2014/10/24 22:12:01, boliu wrote:
> On 2014/10/24 21:37:41, hush wrote:
> > We don't need this because we have an equivalent DCHECK here:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/br...
> 
> Keep this for now.
> 
> It's not quite the same. What if we decide to rewrite ReturnResourceFromParent
> or something in the future.

ok

https://codereview.chromium.org/655813004/diff/120001/android_webview/browser...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/655813004/diff/120001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:76:
content::BrowserThread::GetMessageLoopProxyForThread(
On 2014/10/24 22:12:01, boliu wrote:
> pass in ui_task_runner

SRS needs a base::MessageLoopProxy, which is a child class of
base::SingleThreadTaskRunner.

To do so, I will update SRS constructor to take const
scoped_refptr<base::SingleThreadTaskRunner>&

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

https://codereview.chromium.org/655813004/diff/120001/android_webview/browser...
android_webview/browser/browser_view_renderer.h:193: // BrowserViewRenderer owns
SharedRendererState.
On 2014/10/24 22:12:01, boliu wrote:
> Don't need this, that's obvious.

Done.

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

lgtm weeeeeeeeeeeee!!

[hush (inactive), 2014-10-24 22:37:10]

The CQ bit was checked by hush@chromium.org

[hush (inactive), 2014-10-24 22:39:20]

On 2014/10/24 22:33:29, boliu wrote:
> lgtm weeeeeeeeeeeee!!

cool! Thanks!

[commit-bot: I haz the power, 2014-10-24 22:40:58]

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

[commit-bot: I haz the power, 2014-10-24 23:51:43]

Committed patchset #4 (id:140001)

[commit-bot: I haz the power, 2014-10-24 23:52:15]

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