Android: Extend ViewAndroid

Android: Extend ViewAndroid

This moves cc::Layer ownership from
RenderWidgetHostViewAndroid and ContentViewCore to
ViewAndroid.

Also, ContentViewCore changes from 'is a' ViewAndroid to
'has a' ViewAndroid.

RenderWidgetHostViewAndroid also gets its own ViewAndroid now.
For getting to the WindowAndroid or ViewAndroidDelegate it
will ask its parent.

BUG=624666, 598880, 581521, 626765
TBR=dtrainor@chromium.org
NOTRY=True

Committed: https://crrev.com/e6343711183c73be584725b1b0b7f70a60dd2a5b
Cr-Commit-Position: refs/heads/master@{#404740}

[no sievers, 2016-07-07 00:12:39]

Description was changed from

==========
Android: Extend ViewAndroid

This moves cc::Layer ownership from
RenderWidgetHostViewAndroid and ContentViewCore to
ViewAndroid.

Also, ContentViewCore changes from 'is a' ViewAndroid to
'has a' ViewAndroid.

RenderWidgetHostViewAndroid also gets its own ViewAndroid now.
For getting to the WindowAndroid or ViewAndroidDelegate it
will ask its parent.

BUG=624666,598880
==========

to

==========
Android: Extend ViewAndroid

This moves cc::Layer ownership from
RenderWidgetHostViewAndroid and ContentViewCore to
ViewAndroid.

Also, ContentViewCore changes from 'is a' ViewAndroid to
'has a' ViewAndroid.

RenderWidgetHostViewAndroid also gets its own ViewAndroid now.
For getting to the WindowAndroid or ViewAndroidDelegate it
will ask its parent.

BUG=624666,598880
==========

[no sievers, 2016-07-07 00:12:39]

sievers@chromium.org changed reviewers:
+ yusufo@chromium.org

[no sievers, 2016-07-07 00:13:52]

Yusuf, ptal!

[no sievers, 2016-07-07 00:15:58]

Description was changed from

==========
Android: Extend ViewAndroid

This moves cc::Layer ownership from
RenderWidgetHostViewAndroid and ContentViewCore to
ViewAndroid.

Also, ContentViewCore changes from 'is a' ViewAndroid to
'has a' ViewAndroid.

RenderWidgetHostViewAndroid also gets its own ViewAndroid now.
For getting to the WindowAndroid or ViewAndroidDelegate it
will ask its parent.

BUG=624666,598880
==========

to

==========
Android: Extend ViewAndroid

This moves cc::Layer ownership from
RenderWidgetHostViewAndroid and ContentViewCore to
ViewAndroid.

Also, ContentViewCore changes from 'is a' ViewAndroid to
'has a' ViewAndroid.

RenderWidgetHostViewAndroid also gets its own ViewAndroid now.
For getting to the WindowAndroid or ViewAndroidDelegate it
will ask its parent.

BUG=624666,598880,581521
==========

[Yusuf, 2016-07-08 16:35:06]

https://codereview.chromium.org/2122403002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2122403002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:434: gfx::Size
bounds = view_.GetLayer()->bounds();
If I am getting this right, this is the biggest change in this CL right? layer_
now has a single owner_ and we don't have to worry about making sure RWHV and
CVC are talking about the same layer. Or was that already the case somehow?

https://codereview.chromium.org/2122403002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.h (right):

https://codereview.chromium.org/2122403002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.h:345: mutable
ui::ViewAndroid view_;
Where do we initialize this? and also in destroy should there be any cleanup
related with this.

https://codereview.chromium.org/2122403002/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_view_android.cc (right):

https://codereview.chromium.org/2122403002/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_view_android.cc:70: // TODO(sievers):
This should return null.
is this a placeholder for now?

https://codereview.chromium.org/2122403002/diff/20001/content/public/browser/...
File content/public/browser/android/content_view_core.h (right):

https://codereview.chromium.org/2122403002/diff/20001/content/public/browser/...
content/public/browser/android/content_view_core.h:62: virtual cc::Layer*
GetLayer() const = 0;
should getViewAndroid be here too?

https://codereview.chromium.org/2122403002/diff/20001/ui/android/view_android.cc
File ui/android/view_android.cc (right):

https://codereview.chromium.org/2122403002/diff/20001/ui/android/view_android...
ui/android/view_android.cc:18: ViewAndroid::ViewAndroid(const JavaRef<jobject>&
delegate,
I feel like we need a comment paragraph somewhere that explains the current
state of who owns what and how they are connected. But can't decide what the
right place for it is. Probably either in CVCImpl or RWHVAndroid somewhere.

https://codereview.chromium.org/2122403002/diff/20001/ui/android/view_android...
ui/android/view_android.cc:35: void ViewAndroid::AddChild(ViewAndroid* child) {
these(parent_ being accessible this much) worry me a bit. Also when we add a
ViewAndroid as a child, what do we do about the root_window in it? Should that
not be checked/removed/processed somehow?

[no sievers, 2016-07-08 18:29:05]

https://codereview.chromium.org/2122403002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2122403002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:434: gfx::Size
bounds = view_.GetLayer()->bounds();
On 2016/07/08 16:35:05, Yusuf wrote:
> If I am getting this right, this is the biggest change in this CL right?
layer_
> now has a single owner_ and we don't have to worry about making sure RWHV and
> CVC are talking about the same layer. Or was that already the case somehow?

Both CVC and RWHVA already had their own layer. The reason that CVC has its own
layer is so that during navigation (and what not) we don't have to talk to
something higher up to switch out the layer, but the application can have a
consistent layer for the WebContents. Oh and also the CVC has a SolidColorLayer
so that we get something with the most recent background color in case the RWHV
doesn't have a frame in its layer.

As far as the biggest change is concerned, it's a bit subtle but shouldn't
change anything functionally (I have to double-check interstitials yet). But
having this sort-of double-linked hierarchy between the web contents/CVC and the
render-side object I hope will allow avoiding some lifetime (i.e.
use-after-free) issues. Also I want to grow ViewAndroid to handle a few other
things (like touch event forwarding, see crbug.com/622847) so therefore it needs
to have pointers both ways (down for the events, up for the delegated stuff).

Also ContentViewCore is supposed to go away (crbug.com/598880) since it doesn't
have clear semantics in terms of what it is (or what is is responsible for i.e.
it's more of a collection of random things related to the WebContents) and there
are probably existing other places where all the code and logic could move to.

https://codereview.chromium.org/2122403002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.h (right):

https://codereview.chromium.org/2122403002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.h:345: mutable
ui::ViewAndroid view_;
On 2016/07/08 16:35:05, Yusuf wrote:
> Where do we initialize this? and also in destroy should there be any cleanup
> related with this.

We set the |view_|'s layer still at the same time that we used to create or
destroy the former |layer_|. I *tried* to not make any major changes at least as
far as functional flow or lifetime is concerned.

See SetLayer() and also AddChild()/RemoveChild() (for making sure the layer here
still reaches the same WindowAndroid and Delegate at all times as before the
patch) in the .cc file.

https://codereview.chromium.org/2122403002/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_view_android.cc (right):

https://codereview.chromium.org/2122403002/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_view_android.cc:70: // TODO(sievers):
This should return null.
On 2016/07/08 16:35:05, Yusuf wrote:
> is this a placeholder for now?

This is from web_contents_view.h:

 // Returns the native widget that contains the contents of the tab.
  virtual gfx::NativeView GetNativeView() const = 0;

  // Returns the native widget with the main content of the tab (i.e. the main
  // render view host, though there may be many popups in the tab as children of
  // the container).
  virtual gfx::NativeView GetContentNativeView() const = 0;

So I think it's probably more correct to return the RWHV's view only here as
long as it exists and null otherwise instead of falling back to reach up to the
container view.

But I haven't look what the implications of that are in detail, and thought it'd
be nice to therefore avoid the behavioral change in this patch (this way we'd
still get to the WindowAndroid if somebody used this API for that even if RWHV
is null).

https://codereview.chromium.org/2122403002/diff/20001/content/public/browser/...
File content/public/browser/android/content_view_core.h (right):

https://codereview.chromium.org/2122403002/diff/20001/content/public/browser/...
content/public/browser/android/content_view_core.h:62: virtual cc::Layer*
GetLayer() const = 0;
On 2016/07/08 16:35:05, Yusuf wrote:
> should getViewAndroid be here too?

I really want to get rid of CVC :)

I'm thinking since this interface is pretty small, it might even be feasible to
eliminate this one first. All the users of this would have to be converted to
use something else to get to the functionality, such as ViewAndroid,
WebContents(View) or something else.

https://codereview.chromium.org/2122403002/diff/20001/ui/android/view_android.cc
File ui/android/view_android.cc (right):

https://codereview.chromium.org/2122403002/diff/20001/ui/android/view_android...
ui/android/view_android.cc:18: ViewAndroid::ViewAndroid(const JavaRef<jobject>&
delegate,
On 2016/07/08 16:35:05, Yusuf wrote:
> I feel like we need a comment paragraph somewhere that explains the current
> state of who owns what and how they are connected. But can't decide what the
> right place for it is. Probably either in CVCImpl or RWHVAndroid somewhere.

I'm guessing it needs to evolve a bit but I can put some comments in the header
here.

Also see my other comments for what direction this is possibly going into. And
the discussion in https://codereview.chromium.org/2103243002/ where it's about
potentially bubbling up the adding removing of anchor views from the lower
native layer to the embedder through ViewAndroid (and its delegate).

https://codereview.chromium.org/2122403002/diff/20001/ui/android/view_android...
ui/android/view_android.cc:35: void ViewAndroid::AddChild(ViewAndroid* child) {
On 2016/07/08 16:35:05, Yusuf wrote:
> these(parent_ being accessible this much) worry me a bit. Also when we add a
> ViewAndroid as a child, what do we do about the root_window in it? Should that
> not be checked/removed/processed somehow?

Good point. We could check that a child being added has no root window set.
 
Would it make sense if WindowAndroid was also a ViewAndroid so that it's
literally just the |parent_| at the root?

[no sievers, 2016-07-08 19:22:10]

https://codereview.chromium.org/2122403002/diff/20001/ui/android/view_android.cc
File ui/android/view_android.cc (right):

https://codereview.chromium.org/2122403002/diff/20001/ui/android/view_android...
ui/android/view_android.cc:18: ViewAndroid::ViewAndroid(const JavaRef<jobject>&
delegate,
On 2016/07/08 16:35:05, Yusuf wrote:
> I feel like we need a comment paragraph somewhere that explains the current
> state of who owns what and how they are connected. But can't decide what the
> right place for it is. Probably either in CVCImpl or RWHVAndroid somewhere.

Done.

https://codereview.chromium.org/2122403002/diff/20001/ui/android/view_android...
ui/android/view_android.cc:35: void ViewAndroid::AddChild(ViewAndroid* child) {
On 2016/07/08 18:29:04, sievers wrote:
> On 2016/07/08 16:35:05, Yusuf wrote:
> > these(parent_ being accessible this much) worry me a bit. Also when we add a
> > ViewAndroid as a child, what do we do about the root_window in it? Should
that
> > not be checked/removed/processed somehow?
> 
> Good point. We could check that a child being added has no root window set.
>  
> Would it make sense if WindowAndroid was also a ViewAndroid so that it's
> literally just the |parent_| at the root?

Done.

[no sievers, 2016-07-08 20:06:49]

Description was changed from

==========
Android: Extend ViewAndroid

This moves cc::Layer ownership from
RenderWidgetHostViewAndroid and ContentViewCore to
ViewAndroid.

Also, ContentViewCore changes from 'is a' ViewAndroid to
'has a' ViewAndroid.

RenderWidgetHostViewAndroid also gets its own ViewAndroid now.
For getting to the WindowAndroid or ViewAndroidDelegate it
will ask its parent.

BUG=624666,598880,581521
==========

to

==========
Android: Extend ViewAndroid

This moves cc::Layer ownership from
RenderWidgetHostViewAndroid and ContentViewCore to
ViewAndroid.

Also, ContentViewCore changes from 'is a' ViewAndroid to
'has a' ViewAndroid.

RenderWidgetHostViewAndroid also gets its own ViewAndroid now.
For getting to the WindowAndroid or ViewAndroidDelegate it
will ask its parent.

BUG=624666,598880,581521,626765
==========

[Yusuf, 2016-07-08 23:06:01]

lgtm other than a naming nit.

Thanks Daniel!

https://codereview.chromium.org/2122403002/diff/20001/ui/android/view_android.cc
File ui/android/view_android.cc (right):

https://codereview.chromium.org/2122403002/diff/20001/ui/android/view_android...
ui/android/view_android.cc:35: void ViewAndroid::AddChild(ViewAndroid* child) {
On 2016/07/08 18:29:04, sievers wrote:
> On 2016/07/08 16:35:05, Yusuf wrote:
> > these(parent_ being accessible this much) worry me a bit. Also when we add a
> > ViewAndroid as a child, what do we do about the root_window in it? Should
that
> > not be checked/removed/processed somehow?
> 
> Good point. We could check that a child being added has no root window set.
>  
> Would it make sense if WindowAndroid was also a ViewAndroid so that it's
> literally just the |parent_| at the root?

I would say yes it would make sense for WindowAndroid to be a ViewAndroid, but
is it the case in any of the other platforms NativeWindow/View pairs? That might
be a reason to not to do it if there are no precedents.

Even if there are feel free to do that in a follow up patch.

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

https://codereview.chromium.org/2122403002/diff/40001/content/browser/android...
content/browser/android/content_view_core_impl.cc:875: return
view_android_.GetLayer();
CVC and RWHVAndroid are both Android specific, we call ViewAndroid view_android_
here view_ in RWHVAndroid. I think having the same name would make things better
readable. No strong feelings about which name.

[no sievers, 2016-07-08 23:24:22]

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

https://codereview.chromium.org/2122403002/diff/40001/content/browser/android...
content/browser/android/content_view_core_impl.cc:875: return
view_android_.GetLayer();
On 2016/07/08 23:06:00, Yusuf wrote:
> CVC and RWHVAndroid are both Android specific, we call ViewAndroid
view_android_
> here view_ in RWHVAndroid. I think having the same name would make things
better
> readable. No strong feelings about which name.

True. I've made it CVC::view_ since 'android' is a bit verbose in an
Android-only source file.

[no sievers, 2016-07-08 23:24:33]

The CQ bit was checked by sievers@chromium.org

[no sievers, 2016-07-08 23:24:33]

The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org
Link to the patchset: https://codereview.chromium.org/2122403002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2016-07-08 23:25:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-08 23:34:10]

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

[commit-bot: I haz the power, 2016-07-08 23:34:11]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[no sievers, 2016-07-08 23:41:37]

sievers@chromium.org changed reviewers:
+ dtrainor@chromium.org

[no sievers, 2016-07-08 23:41:50]

+dtrainor for OWNERS stamp

[no sievers, 2016-07-11 18:54:21]

Description was changed from

==========
Android: Extend ViewAndroid

This moves cc::Layer ownership from
RenderWidgetHostViewAndroid and ContentViewCore to
ViewAndroid.

Also, ContentViewCore changes from 'is a' ViewAndroid to
'has a' ViewAndroid.

RenderWidgetHostViewAndroid also gets its own ViewAndroid now.
For getting to the WindowAndroid or ViewAndroidDelegate it
will ask its parent.

BUG=624666,598880,581521,626765
==========

to

==========
Android: Extend ViewAndroid

This moves cc::Layer ownership from
RenderWidgetHostViewAndroid and ContentViewCore to
ViewAndroid.

Also, ContentViewCore changes from 'is a' ViewAndroid to
'has a' ViewAndroid.

RenderWidgetHostViewAndroid also gets its own ViewAndroid now.
For getting to the WindowAndroid or ViewAndroidDelegate it
will ask its parent.

BUG=624666,598880,581521,626765
TBR=dtrainor@chromium.org
==========

[no sievers, 2016-07-11 18:54:49]

On 2016/07/08 23:41:50, sievers wrote:
> +dtrainor for OWNERS stamp

TBR dtrainor@ per offline chat... there'll be a bunch of follow-up work anyways
to address any potential comments.

[no sievers, 2016-07-11 18:54:59]

The CQ bit was checked by sievers@chromium.org

[commit-bot: I haz the power, 2016-07-11 18:55:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-11 22:26:06]

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

[commit-bot: I haz the power, 2016-07-11 22:26:07]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[no sievers, 2016-07-11 22:27:53]

Description was changed from

==========
Android: Extend ViewAndroid

This moves cc::Layer ownership from
RenderWidgetHostViewAndroid and ContentViewCore to
ViewAndroid.

Also, ContentViewCore changes from 'is a' ViewAndroid to
'has a' ViewAndroid.

RenderWidgetHostViewAndroid also gets its own ViewAndroid now.
For getting to the WindowAndroid or ViewAndroidDelegate it
will ask its parent.

BUG=624666,598880,581521,626765
TBR=dtrainor@chromium.org
==========

to

==========
Android: Extend ViewAndroid

This moves cc::Layer ownership from
RenderWidgetHostViewAndroid and ContentViewCore to
ViewAndroid.

Also, ContentViewCore changes from 'is a' ViewAndroid to
'has a' ViewAndroid.

RenderWidgetHostViewAndroid also gets its own ViewAndroid now.
For getting to the WindowAndroid or ViewAndroidDelegate it
will ask its parent.

BUG=624666,598880,581521,626765
TBR=dtrainor@chromium.org
NOTRY=True
==========

[no sievers, 2016-07-11 22:28:41]

The CQ bit was checked by sievers@chromium.org

[commit-bot: I haz the power, 2016-07-11 22:29:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-11 22:35:44]

Description was changed from

==========
Android: Extend ViewAndroid

This moves cc::Layer ownership from
RenderWidgetHostViewAndroid and ContentViewCore to
ViewAndroid.

Also, ContentViewCore changes from 'is a' ViewAndroid to
'has a' ViewAndroid.

RenderWidgetHostViewAndroid also gets its own ViewAndroid now.
For getting to the WindowAndroid or ViewAndroidDelegate it
will ask its parent.

BUG=624666,598880,581521,626765
TBR=dtrainor@chromium.org
NOTRY=True
==========

to

==========
Android: Extend ViewAndroid

This moves cc::Layer ownership from
RenderWidgetHostViewAndroid and ContentViewCore to
ViewAndroid.

Also, ContentViewCore changes from 'is a' ViewAndroid to
'has a' ViewAndroid.

RenderWidgetHostViewAndroid also gets its own ViewAndroid now.
For getting to the WindowAndroid or ViewAndroidDelegate it
will ask its parent.

BUG=624666,598880,581521,626765
TBR=dtrainor@chromium.org
NOTRY=True
==========

[commit-bot: I haz the power, 2016-07-11 22:35:48]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-11 22:35:51]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-11 22:37:21]

Description was changed from

==========
Android: Extend ViewAndroid

This moves cc::Layer ownership from
RenderWidgetHostViewAndroid and ContentViewCore to
ViewAndroid.

Also, ContentViewCore changes from 'is a' ViewAndroid to
'has a' ViewAndroid.

RenderWidgetHostViewAndroid also gets its own ViewAndroid now.
For getting to the WindowAndroid or ViewAndroidDelegate it
will ask its parent.

BUG=624666,598880,581521,626765
TBR=dtrainor@chromium.org
NOTRY=True
==========

to

==========
Android: Extend ViewAndroid

This moves cc::Layer ownership from
RenderWidgetHostViewAndroid and ContentViewCore to
ViewAndroid.

Also, ContentViewCore changes from 'is a' ViewAndroid to
'has a' ViewAndroid.

RenderWidgetHostViewAndroid also gets its own ViewAndroid now.
For getting to the WindowAndroid or ViewAndroidDelegate it
will ask its parent.

BUG=624666,598880,581521,626765
TBR=dtrainor@chromium.org
NOTRY=True

Committed: https://crrev.com/e6343711183c73be584725b1b0b7f70a60dd2a5b
Cr-Commit-Position: refs/heads/master@{#404740}
==========

[commit-bot: I haz the power, 2016-07-11 22:37:23]

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

[Khushal, 2016-07-12 00:34:02]

https://codereview.chromium.org/2122403002/diff/80001/content/browser/android...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/2122403002/diff/80001/content/browser/android...
content/browser/android/content_view_core_impl.cc:841: if
(!view_.GetLayer()->children().empty())
Any reason for changing this here? The root layer should still draw only when we
have no delegated layers right?

[no sievers, 2016-07-12 00:35:27]

https://codereview.chromium.org/2122403002/diff/80001/content/browser/android...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/2122403002/diff/80001/content/browser/android...
content/browser/android/content_view_core_impl.cc:841: if
(!view_.GetLayer()->children().empty())
On 2016/07/12 00:34:02, Khushal wrote:
> Any reason for changing this here? The root layer should still draw only when
we
> have no delegated layers right?

err very nice catch, thanks. I think I broke this during rebase.