Clean up RenderWidget/RenderWidgetCompositor/WebKit interactions

Clean up RenderWidget/RenderWidgetCompositor/WebKit interactions

This routes communication between RenderWidget's compositing support
(RenderWidgetCompositor) and WebKit through the WebWidget interface
instead of going through WebLayerTreeView(Client). The goal is to
get rid of the WebLayerTreeViewClient interface completely, slim down
WebLayerTreeView, and have all compositing-related communication between
chromium and WebKit just go through WebWidget, since compositing mode
isn't really all that special any more.  RenderWidgetCompositor is responsible
for bridging between the compositor implementation in cc:: and RenderWidget
itself.

Interacts closely with http://webkit.org/b/109125

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

[jamesr, 2013-02-07 02:51:49]

Some of the WebKit unit tests don't work yet and I haven't figured out how to
stage this with https://bugs.webkit.org/show_bug.cgi?id=109125 yet, but here's a
taste of the future.

[piman, 2013-02-12 01:29:56]

LGTM in principle, if you can figure out how to land this :)

[jamesr, 2013-02-12 02:04:41]

On 2013/02/12 01:29:56, piman wrote:
> LGTM in principle, if you can figure out how to land this :)

I'm getting there!  I have a small pile of unit test / DRT related patches to
land, caseq is fixing up the instrumentation logic here:
https://bugs.webkit.org/show_bug.cgi?id=109192, and Eberhard has fixed the font
atlas stuff.  Then it's gravy time!

[jamesr, 2013-03-01 01:13:15]

On 2013/02/12 01:29:56, piman wrote:
> LGTM in principle, if you can figure out how to land this :)

After https://bugs.webkit.org/show_bug.cgi?id=109125 lands and rolls in this
should be good to go.  Mind giving it another once-over?

[jamesr, 2013-03-01 01:13:48]

FYI this patch also includes https://codereview.chromium.org/12377029/, which is
waiting on another WebKit roll, but I'll land those bits separately.

[piman, 2013-03-01 01:51:40]

LGTM+couple of nits

https://codereview.chromium.org/12226051/diff/10001/content/renderer/gpu/rend...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/12226051/diff/10001/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.cc:81:
scoped_ptr<RenderWidgetCompositor> comp(
nit: s/comp/compositor/ while you're here?

https://codereview.chromium.org/12226051/diff/10001/content/renderer/gpu/rend...
File content/renderer/gpu/render_widget_compositor.h (left):

https://codereview.chromium.org/12226051/diff/10001/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.h:62: virtual void
updateAnimations(double frame_begin_time);
nit: We could keep both of these as accessor functions (i.e. non virtual), so
that RenderWidget doesn't have to reach into the layer_tree_host. It is kinda
awkward to have both the RWC reach into widget_->webwidget() and RW reach into
compositor_->layer_tree_host(), that makes both of them very strongly coupled.

[jamesr, 2013-03-01 01:56:09]

On 2013/03/01 01:51:40, piman wrote:
> LGTM+couple of nits
> 
>
https://codereview.chromium.org/12226051/diff/10001/content/renderer/gpu/rend...
> File content/renderer/gpu/render_widget_compositor.cc (right):
> 
>
https://codereview.chromium.org/12226051/diff/10001/content/renderer/gpu/rend...
> content/renderer/gpu/render_widget_compositor.cc:81:
> scoped_ptr<RenderWidgetCompositor> comp(
> nit: s/comp/compositor/ while you're here?
> 
>
https://codereview.chromium.org/12226051/diff/10001/content/renderer/gpu/rend...
> File content/renderer/gpu/render_widget_compositor.h (left):
> 
>
https://codereview.chromium.org/12226051/diff/10001/content/renderer/gpu/rend...
> content/renderer/gpu/render_widget_compositor.h:62: virtual void
> updateAnimations(double frame_begin_time);
> nit: We could keep both of these as accessor functions (i.e. non virtual), so
> that RenderWidget doesn't have to reach into the layer_tree_host. It is kinda
> awkward to have both the RWC reach into widget_->webwidget() and RW reach into
> compositor_->layer_tree_host(), that makes both of them very strongly coupled.

OK - so Chromify the function names and make them non-virtual? SGTM.

[piman, 2013-03-01 01:57:49]

On Thu, Feb 28, 2013 at 5:56 PM, <jamesr@chromium.org> wrote:

> On 2013/03/01 01:51:40, piman wrote:
>
>> LGTM+couple of nits
>>
>
>
> https://codereview.chromium.**org/12226051/diff/10001/**
>
content/renderer/gpu/render_**widget_compositor.cc<https://codereview.chromium.org/12226051/diff/10001/content/renderer/gpu/render_widget_compositor.cc>
>
>> File content/renderer/gpu/render_**widget_compositor.cc (right):
>>
>
>
> https://codereview.chromium.**org/12226051/diff/10001/**
>
content/renderer/gpu/render_**widget_compositor.cc#newcode81<https://codereview.chromium.org/12226051/diff/10001/content/renderer/gpu/render_widget_compositor.cc#newcode81>
>
>> content/renderer/gpu/render_**widget_compositor.cc:81:
>> scoped_ptr<**RenderWidgetCompositor> comp(
>> nit: s/comp/compositor/ while you're here?
>>
>
>
> https://codereview.chromium.**org/12226051/diff/10001/**
>
content/renderer/gpu/render_**widget_compositor.h<https://codereview.chromium.org/12226051/diff/10001/content/renderer/gpu/render_widget_compositor.h>
>
>> File content/renderer/gpu/render_**widget_compositor.h (left):
>>
>
>
> https://codereview.chromium.**org/12226051/diff/10001/**
>
content/renderer/gpu/render_**widget_compositor.h#oldcode62<https://codereview.chromium.org/12226051/diff/10001/content/renderer/gpu/render_widget_compositor.h#oldcode62>
>
>> content/renderer/gpu/render_**widget_compositor.h:62: virtual void
>> updateAnimations(double frame_begin_time);
>> nit: We could keep both of these as accessor functions (i.e. non
>> virtual), so
>> that RenderWidget doesn't have to reach into the layer_tree_host. It is
>> kinda
>> awkward to have both the RWC reach into widget_->webwidget() and RW reach
>> into
>> compositor_->layer_tree_host()**, that makes both of them very strongly
>> coupled.
>>
>
> OK - so Chromify the function names and make them non-virtual? SGTM.
>

Yep, thanks!

Antoine


>
>
https://codereview.chromium.**org/12226051/<https://codereview.chromium.org/1...
>

[jamesr, 2013-03-01 22:36:05]

Done - PTAL

[piman, 2013-03-01 22:58:20]

lgtm

https://codereview.chromium.org/12226051/diff/14001/content/renderer/gpu/rend...
File content/renderer/gpu/render_widget_compositor.h (right):

https://codereview.chromium.org/12226051/diff/14001/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.h:32: cc::LayerTreeHost*
layer_tree_host() const { return layer_tree_host_.get(); }
nit: is it still used now / could we remove it?

[jamesr, 2013-03-01 23:00:45]

On 2013/03/01 22:58:20, piman wrote:
> lgtm
> 
>
https://codereview.chromium.org/12226051/diff/14001/content/renderer/gpu/rend...
> File content/renderer/gpu/render_widget_compositor.h (right):
> 
>
https://codereview.chromium.org/12226051/diff/14001/content/renderer/gpu/rend...
> content/renderer/gpu/render_widget_compositor.h:32: cc::LayerTreeHost*
> layer_tree_host() const { return layer_tree_host_.get(); }
> nit: is it still used now / could we remove it?

It's still used so that render_view_impl_android can reach in and make a
android-specific call (cc::LayerTreeHost::enableHidingTopControls).  We could
expose that and then get rid of this getter, but I think that'd be better as a
follow-up.

[piman, 2013-03-01 23:06:14]

On Fri, Mar 1, 2013 at 3:00 PM, <jamesr@chromium.org> wrote:

> On 2013/03/01 22:58:20, piman wrote:
>
>> lgtm
>>
>
>
> https://codereview.chromium.**org/12226051/diff/14001/**
>
content/renderer/gpu/render_**widget_compositor.h<https://codereview.chromium.org/12226051/diff/14001/content/renderer/gpu/render_widget_compositor.h>
>
>> File content/renderer/gpu/render_**widget_compositor.h (right):
>>
>
>
> https://codereview.chromium.**org/12226051/diff/14001/**
>
content/renderer/gpu/render_**widget_compositor.h#newcode32<https://codereview.chromium.org/12226051/diff/14001/content/renderer/gpu/render_widget_compositor.h#newcode32>
>
>> content/renderer/gpu/render_**widget_compositor.h:32: cc::LayerTreeHost*
>> layer_tree_host() const { return layer_tree_host_.get(); }
>> nit: is it still used now / could we remove it?
>>
>
> It's still used so that render_view_impl_android can reach in and make a
> android-specific call (cc::LayerTreeHost::**enableHidingTopControls).  We
> could
> expose that and then get rid of this getter, but I think that'd be better
> as a
> follow-up.
>

Ok, LGTM for now.


>
>
https://codereview.chromium.**org/12226051/<https://codereview.chromium.org/1...
>

[commit-bot: I haz the power, 2013-03-06 00:50:34]

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

[jamesr, 2013-03-06 01:34:21]

Turns out since I had to fix the android build anyway it was just as easy to
fully hide the cc::LayerTreeHost getter.

[commit-bot: I haz the power, 2013-03-06 01:44:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/12226051/31003

[commit-bot: I haz the power, 2013-03-06 04:38:32]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-03-06 05:09:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/12226051/37001

[commit-bot: I haz the power, 2013-03-06 09:11:21]

Change committed as 186379