Add layerTreeBuildHelper to control layer tree in ContentViewRenderView

Make it possible to extend ContentViewRenderView

Changes to ContentViewRenderView that will enable to us to extend
functionality.

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

[David Trainor- moved to gerrit, 2014-03-18 18:33:28]

lgtm

https://chromiumcodereview.appspot.com/201583004/diff/1/content/browser/andro...
File content/browser/android/content_view_render_view.h (right):

https://chromiumcodereview.appspot.com/201583004/diff/1/content/browser/andro...
content/browser/android/content_view_render_view.h:30: jlong
native_content_view);
Fix indenting

https://chromiumcodereview.appspot.com/201583004/diff/1/content/browser/andro...
content/browser/android/content_view_render_view.h:43: protected:
New line before protected

https://chromiumcodereview.appspot.com/201583004/diff/1/content/browser/andro...
content/browser/android/content_view_render_view.h:48: private:
New line before private

[Yusuf, 2014-03-19 17:35:09]

https://codereview.chromium.org/201583004/diff/1/content/browser/android/cont...
File content/browser/android/content_view_render_view.h (right):

https://codereview.chromium.org/201583004/diff/1/content/browser/android/cont...
content/browser/android/content_view_render_view.h:30: jlong
native_content_view);
On 2014/03/18 18:33:28, David Trainor wrote:
> Fix indenting

Done.

https://codereview.chromium.org/201583004/diff/1/content/browser/android/cont...
content/browser/android/content_view_render_view.h:43: protected:
On 2014/03/18 18:33:28, David Trainor wrote:
> New line before protected

Done.

https://codereview.chromium.org/201583004/diff/1/content/browser/android/cont...
content/browser/android/content_view_render_view.h:48: private:
On 2014/03/18 18:33:28, David Trainor wrote:
> New line before private

Done.

[Yusuf, 2014-03-19 17:35:12]

The CQ bit was checked by yusufo@chromium.org

[commit-bot: I haz the power, 2014-03-19 17:36:09]

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

[commit-bot: I haz the power, 2014-03-19 18:12:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-19 18:12:56]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Yusuf, 2014-03-19 18:17:12]

yfriedman@ for .../android approval

[Yaron, 2014-03-19 19:00:12]

This seems pretty sievers-y so adding him. Have a comment on internal CL too.

https://codereview.chromium.org/201583004/diff/20001/content/browser/android/...
File content/browser/android/content_view_render_view.cc (right):

https://codereview.chromium.org/201583004/diff/20001/content/browser/android/...
content/browser/android/content_view_render_view.cc:57: new
ContentViewRenderView(env, obj, root_window);
Hmm, this mirrors TabAndroid but it's a dangerous pattern. At a minimum, there
should be a comment why this isn't a leak (since it's not obvious) however I'd
like to see if there's another way.

https://codereview.chromium.org/201583004/diff/20001/content/browser/android/...
File content/browser/android/content_view_render_view.h (right):

https://codereview.chromium.org/201583004/diff/20001/content/browser/android/...
content/browser/android/content_view_render_view.h:29: virtual void
SetCurrentContentView(JNIEnv* env,
Crossing JNI+subclasses simultaneously seems risky and something we've tried to
avoid. It's also challenging cause you're crossing repositories.

[no sievers, 2014-03-20 22:35:18]

Am fine with landing whatever if it helps Yusuf.

I don't really like ContentViewRenderView, but that's really my fault :)

What I want to do is split this up into a more proper API:
- the scheduling (incl. vsync) logic should be more hidden and move into the
compositor or somewhere and be shared everywhere
- the layer handling should also move out of here and be left to the app

[Yusuf, 2014-04-04 21:30:09]

Uploaded new patch to add a helper class. PTAL

[Yaron, 2014-04-10 01:26:09]

Still thinking of the best way to do this but have one proposal inline

https://codereview.chromium.org/201583004/diff/40001/content/browser/android/...
File content/browser/android/content_view_render_view.cc (right):

https://codereview.chromium.org/201583004/diff/40001/content/browser/android/...
content/browser/android/content_view_render_view.cc:53: if
(!native_build_helper)
Can you CHECK this? Why can this be 0?

https://codereview.chromium.org/201583004/diff/40001/content/browser/android/...
content/browser/android/content_view_render_view.cc:142:
compositor_->SetRootLayer(root_layer_);
I think this should stay in SetCurrentContentView, right after setting the root
layer.

https://codereview.chromium.org/201583004/diff/40001/content/browser/android/...
File content/browser/android/layer_tree_build_helper.cc (right):

https://codereview.chromium.org/201583004/diff/40001/content/browser/android/...
content/browser/android/layer_tree_build_helper.cc:25: ContentViewCoreImpl*
content_view) {
Does this need ContentViewCoreImpl? Seems like upstream and downstream they're
both replicating the logic to handle content_view's layer. 

Can you instead get ContentView's layer, pass that into GetLayerTree as a layer
with name "content_layer_root) or something. Then, the subclass can choose that
it wants to insert the content layer somewhere insides its tree instead of
re-using as root?

https://codereview.chromium.org/201583004/diff/40001/content/browser/android/...
content/browser/android/layer_tree_build_helper.cc:27: root_layer_ =
content_view->GetLayer();
I don't know if this is a real issue but if you are passed a content view once,
and then not passed a content view in a subsequent GetLayerTree, you're not
going to update and instead keep pointing back to the previous one's layer.

https://codereview.chromium.org/201583004/diff/40001/content/browser/android/...
File content/browser/android/layer_tree_build_helper.h (right):

https://codereview.chromium.org/201583004/diff/40001/content/browser/android/...
content/browser/android/layer_tree_build_helper.h:19: class LayerTreeBuildHelper
{
Needs comment.

[Yusuf, 2014-04-23 00:16:14]

https://codereview.chromium.org/201583004/diff/40001/content/browser/android/...
File content/browser/android/content_view_render_view.cc (right):

https://codereview.chromium.org/201583004/diff/40001/content/browser/android/...
content/browser/android/content_view_render_view.cc:53: if
(!native_build_helper)
On 2014/04/10 01:26:09, Yaron wrote:
> Can you CHECK this? Why can this be 0?

Done.

https://codereview.chromium.org/201583004/diff/40001/content/browser/android/...
content/browser/android/content_view_render_view.cc:142:
compositor_->SetRootLayer(root_layer_);
On 2014/04/10 01:26:09, Yaron wrote:
> I think this should stay in SetCurrentContentView, right after setting the
root
> layer.

Done.

https://codereview.chromium.org/201583004/diff/40001/content/browser/android/...
File content/browser/android/layer_tree_build_helper.cc (right):

https://codereview.chromium.org/201583004/diff/40001/content/browser/android/...
content/browser/android/layer_tree_build_helper.cc:25: ContentViewCoreImpl*
content_view) {
On 2014/04/10 01:26:09, Yaron wrote:
> Does this need ContentViewCoreImpl? Seems like upstream and downstream they're
> both replicating the logic to handle content_view's layer. 
> 
> Can you instead get ContentView's layer, pass that into GetLayerTree as a
layer
> with name "content_layer_root) or something. Then, the subclass can choose
that
> it wants to insert the content layer somewhere insides its tree instead of
> re-using as root?

Done.

https://codereview.chromium.org/201583004/diff/40001/content/browser/android/...
content/browser/android/layer_tree_build_helper.cc:27: root_layer_ =
content_view->GetLayer();
On 2014/04/10 01:26:09, Yaron wrote:
> I don't know if this is a real issue but if you are passed a content view
once,
> and then not passed a content view in a subsequent GetLayerTree, you're not
> going to update and instead keep pointing back to the previous one's layer.

Done.

https://codereview.chromium.org/201583004/diff/40001/content/browser/android/...
File content/browser/android/layer_tree_build_helper.h (right):

https://codereview.chromium.org/201583004/diff/40001/content/browser/android/...
content/browser/android/layer_tree_build_helper.h:19: class LayerTreeBuildHelper
{
On 2014/04/10 01:26:09, Yaron wrote:
> Needs comment.

Done.

[Yaron, 2014-04-23 18:54:42]

https://codereview.chromium.org/201583004/diff/70001/content/browser/android/...
File content/browser/android/content_view_render_view.cc (right):

https://codereview.chromium.org/201583004/diff/70001/content/browser/android/...
content/browser/android/content_view_render_view.cc:18: #include
"content/public/browser/android/layer_tree_build_helper.h"
i think you forgot to "git add" these :)

https://codereview.chromium.org/201583004/diff/70001/content/browser/android/...
content/browser/android/content_view_render_view.cc:79: root_layer_ =
layer_tree_build_helper_->GetLayerTree(content_view);
I thought I suggested to pass they layer in right here. (not the CVCI)

[Yusuf, 2014-04-23 20:00:03]

https://codereview.chromium.org/201583004/diff/70001/content/browser/android/...
File content/browser/android/content_view_render_view.cc (right):

https://codereview.chromium.org/201583004/diff/70001/content/browser/android/...
content/browser/android/content_view_render_view.cc:18: #include
"content/public/browser/android/layer_tree_build_helper.h"
Oops. Sorry about that.

On 2014/04/23 18:54:43, Yaron wrote:
> i think you forgot to "git add" these :)

https://codereview.chromium.org/201583004/diff/70001/content/browser/android/...
content/browser/android/content_view_render_view.cc:79: root_layer_ =
layer_tree_build_helper_->GetLayerTree(content_view);
On 2014/04/23 18:54:43, Yaron wrote:
> I thought I suggested to pass they layer in right here. (not the CVCI)

Done.

[Yaron, 2014-04-23 20:57:45]

https://codereview.chromium.org/201583004/diff/90001/content/browser/android/...
File content/browser/android/content_view_render_view.cc (right):

https://codereview.chromium.org/201583004/diff/90001/content/browser/android/...
content/browser/android/content_view_render_view.cc:79: root_layer_ =
Actually, do we need a ref? Isn't it sufficient to pass to the compositor?

https://codereview.chromium.org/201583004/diff/90001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
(right):

https://codereview.chromium.org/201583004/diff/90001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:46:
protected ContentView mCurrentContentView;
Is this change still necessary? You haven't re-uploaded internal but I don't
think it should be

https://codereview.chromium.org/201583004/diff/90001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:270:
*
Update

https://codereview.chromium.org/201583004/diff/90001/content/public/browser/a...
File content/public/browser/android/layer_tree_build_helper.h (right):

https://codereview.chromium.org/201583004/diff/90001/content/public/browser/a...
content/public/browser/android/layer_tree_build_helper.h:21: class
LayerTreeBuildHelper {
http://www.chromium.org/developers/content-module/content-api

Should be an interface with an impl not in public (sorry for not being clear
before)

[Yusuf, 2014-04-24 00:06:03]

Sorry my checkout got messed up, so I had to rebase.

https://codereview.chromium.org/201583004/diff/90001/content/browser/android/...
File content/browser/android/content_view_render_view.cc (right):

https://codereview.chromium.org/201583004/diff/90001/content/browser/android/...
content/browser/android/content_view_render_view.cc:79: root_layer_ =
On 2014/04/23 20:57:46, Yaron wrote:
> Actually, do we need a ref? Isn't it sufficient to pass to the compositor?

Done.

https://codereview.chromium.org/201583004/diff/90001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
(right):

https://codereview.chromium.org/201583004/diff/90001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:46:
protected ContentView mCurrentContentView;
Yes. I will need this in extending classes to route touch events.

On 2014/04/23 20:57:46, Yaron wrote:
> Is this change still necessary? You haven't re-uploaded internal but I don't
> think it should be

https://codereview.chromium.org/201583004/diff/90001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:270:
*
On 2014/04/23 20:57:46, Yaron wrote:
> Update

Done.

https://codereview.chromium.org/201583004/diff/90001/content/public/browser/a...
File content/public/browser/android/layer_tree_build_helper.h (right):

https://codereview.chromium.org/201583004/diff/90001/content/public/browser/a...
content/public/browser/android/layer_tree_build_helper.h:21: class
LayerTreeBuildHelper {
On 2014/04/23 20:57:46, Yaron wrote:
> http://www.chromium.org/developers/content-module/content-api
> 
> Should be an interface with an impl not in public (sorry for not being clear
> before)

Done.

[Yaron, 2014-04-24 19:47:10]

getting close

https://codereview.chromium.org/201583004/diff/130001/content/browser/android...
File content/browser/android/content_view_render_view.cc (right):

https://codereview.chromium.org/201583004/diff/130001/content/browser/android...
content/browser/android/content_view_render_view.cc:40:
layer_tree_build_helper_(new LayerTreeBuildHelperImpl()),
This is problematic. You're leaking this original copy if someone gives you a
LayerTreeBuilder, however it looks like you aren't taking ownership of the one
provided to you. Need to clean up ownership model but I want to see the updated
intenral CL.

https://codereview.chromium.org/201583004/diff/130001/content/public/browser/...
File content/public/browser/android/layer_tree_build_helper.h (right):

https://codereview.chromium.org/201583004/diff/130001/content/public/browser/...
content/public/browser/android/layer_tree_build_helper.h:20: 
nit: ws

https://codereview.chromium.org/201583004/diff/130001/content/public/browser/...
content/public/browser/android/layer_tree_build_helper.h:28:
scoped_refptr<cc::Layer> root_layer_;
Move to impl.

[Yusuf, 2014-04-28 23:18:46]

https://codereview.chromium.org/201583004/diff/130001/content/browser/android...
File content/browser/android/content_view_render_view.cc (right):

https://codereview.chromium.org/201583004/diff/130001/content/browser/android...
content/browser/android/content_view_render_view.cc:40:
layer_tree_build_helper_(new LayerTreeBuildHelperImpl()),
Moved the ownership to content_view_render_view.

On 2014/04/24 19:47:11, Yaron wrote:
> This is problematic. You're leaking this original copy if someone gives you a
> LayerTreeBuilder, however it looks like you aren't taking ownership of the one
> provided to you. Need to clean up ownership model but I want to see the
updated
> intenral CL.

https://codereview.chromium.org/201583004/diff/130001/content/public/browser/...
File content/public/browser/android/layer_tree_build_helper.h (right):

https://codereview.chromium.org/201583004/diff/130001/content/public/browser/...
content/public/browser/android/layer_tree_build_helper.h:20: 
On 2014/04/24 19:47:11, Yaron wrote:
> nit: ws

Done.

[Yaron, 2014-04-29 04:24:01]

lgtm

https://codereview.chromium.org/201583004/diff/150001/content/browser/android...
File content/browser/android/layer_tree_build_helper_impl.h (right):

https://codereview.chromium.org/201583004/diff/150001/content/browser/android...
content/browser/android/layer_tree_build_helper_impl.h:16: class Compositor;
Remove both oft hese

https://codereview.chromium.org/201583004/diff/150001/content/public/browser/...
File content/public/browser/android/layer_tree_build_helper.h (right):

https://codereview.chromium.org/201583004/diff/150001/content/public/browser/...
content/public/browser/android/layer_tree_build_helper.h:1: // Copyright 2014
The Chromium Authors. All rights reserved.
I don't see this added in gyp.

https://codereview.chromium.org/201583004/diff/150001/content/public/browser/...
content/public/browser/android/layer_tree_build_helper.h:25:
DISALLOW_COPY_AND_ASSIGN(LayerTreeBuildHelper);
Add "private:" label.

[Yusuf, 2014-04-29 17:39:33]

https://codereview.chromium.org/201583004/diff/150001/content/browser/android...
File content/browser/android/layer_tree_build_helper_impl.h (right):

https://codereview.chromium.org/201583004/diff/150001/content/browser/android...
content/browser/android/layer_tree_build_helper_impl.h:16: class Compositor;
On 2014/04/29 04:24:02, Yaron wrote:
> Remove both oft hese

Done.

https://codereview.chromium.org/201583004/diff/150001/content/public/browser/...
File content/public/browser/android/layer_tree_build_helper.h (right):

https://codereview.chromium.org/201583004/diff/150001/content/public/browser/...
content/public/browser/android/layer_tree_build_helper.h:1: // Copyright 2014
The Chromium Authors. All rights reserved.
On 2014/04/29 04:24:02, Yaron wrote:
> I don't see this added in gyp.

Done.

https://codereview.chromium.org/201583004/diff/150001/content/public/browser/...
content/public/browser/android/layer_tree_build_helper.h:25:
DISALLOW_COPY_AND_ASSIGN(LayerTreeBuildHelper);
On 2014/04/29 04:24:02, Yaron wrote:
> Add "private:" label.

Done.

[Yusuf, 2014-04-29 17:39:38]

The CQ bit was checked by yusufo@chromium.org

[commit-bot: I haz the power, 2014-04-29 17:40:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yusufo@chromium.org/201583004/170001

[commit-bot: I haz the power, 2014-04-29 23:30:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-29 23:30:22]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium

[Yusuf, 2014-04-30 00:17:23]

The CQ bit was checked by yusufo@chromium.org

[commit-bot: I haz the power, 2014-04-30 00:21:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yusufo@chromium.org/201583004/170001

[commit-bot: I haz the power, 2014-04-30 05:00:02]

Change committed as 267071

[no sievers, 2014-05-05 20:03:59]

https://codereview.chromium.org/201583004/diff/170001/content/browser/android...
File content/browser/android/layer_tree_build_helper_impl.cc (right):

https://codereview.chromium.org/201583004/diff/170001/content/browser/android...
content/browser/android/layer_tree_build_helper_impl.cc:15:
root_layer_->SetBackgroundColor(SK_ColorWHITE);
This layer does not do anything since the bounds are 0.