Introduce ViewRoot forwarding input/view events to native

Introduce ViewRoot forwarding input/view events to native

This CL introduces a new interface ViewRoot to forward Java
view/input events down to native with a view to replacing
the flow through ContentViewCore eventually.

In native side ViewRoot is a subclass of ViewAndroid. It acts
as the root of a ViewAndroid tree, and receives touch/view events
from embedders. It has a reference to WindowAndroid, and paritially
takes over the role it played as the tree root. Therefore WindowAndroid
no longer inherits from ViewAndroid. It effectively reverts
https://crrev.com/2136373002.

ViewRoot is mapped 1:1 to WindowAndroid and shared across all the contents.
WebView is an exception where all activities share the WA instance.

ViewRoot, ViewAndroid, and ViewClient are put together to adopt
the pattern "chain of responsibility" dispatching the events to
the classes implementing |ViewClient| along the view hierarchy, and
conditionally stopping the processing when required.

BUG=671401

[Jinsuk Kim, 2016-12-22 05:32:47]

The CQ bit was checked by jinsukkim@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 05:32:59]

Dry run: 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-12-22 05:34:57]

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

[commit-bot: I haz the power, 2016-12-22 05:34:58]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Jinsuk Kim, 2016-12-22 06:28:30]

The CQ bit was checked by jinsukkim@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 06:28:37]

Dry run: 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-12-22 06:52:08]

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

[commit-bot: I haz the power, 2016-12-22 06:52:09]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Jinsuk Kim, 2016-12-22 09:54:46]

Patchset #1 (id:1) has been deleted

[Jinsuk Kim, 2016-12-22 10:23:16]

Description was changed from

==========
Make ViewRoot the top of ViewAndroid tree

This CL introduce a native class ViewRoot, a subclass of ViewAndroid. It acts as
the root of a ViewAndroid tree, and receives touch/view events from embedders.
It has a reference to WindowAndroid, and paritially takes over the role it
played as a root.

WindowAndroid which will no longer inherit from ViewAndroid - so the CL
effectively reverts https://crrev.com/2136373002.

Embedders are responsible for passing in a ViewRoot when creating a new
WebContents.

Also fixes a bug caused by using a main content's ViewRoot for overlay panel's.


BUG=671401, 671967
==========

to

==========
Make ViewRoot the top of ViewAndroid tree

This CL introduces a native class ViewRoot, a subclass of ViewAndroid.
It acts as the root of a ViewAndroid tree, and receives touch/view
events from embedders. It has a reference to WindowAndroid, and paritially takes
over the role it played as the tree root.

WindowAndroid will no longer inherit from ViewAndroid. It effectively
reverts https://crrev.com/2136373002.

Embedders are responsible for passing in a ViewRoot when creating a new
WebContents.

Also fixes a bug caused by using a main content's ViewRoot for overlay panel's.


BUG=671401, 671967
==========

[Jinsuk Kim, 2016-12-22 10:23:40]

Description was changed from

==========
Make ViewRoot the top of ViewAndroid tree

This CL introduces a native class ViewRoot, a subclass of ViewAndroid.
It acts as the root of a ViewAndroid tree, and receives touch/view
events from embedders. It has a reference to WindowAndroid, and paritially takes
over the role it played as the tree root.

WindowAndroid will no longer inherit from ViewAndroid. It effectively
reverts https://crrev.com/2136373002.

Embedders are responsible for passing in a ViewRoot when creating a new
WebContents.

Also fixes a bug caused by using a main content's ViewRoot for overlay panel's.


BUG=671401, 671967
==========

to

==========
Make ViewRoot the top of ViewAndroid tree

This CL introduces a native class ViewRoot, a subclass of ViewAndroid.
It acts as the root of a ViewAndroid tree, and receives touch/view
events from embedders. It has a reference to WindowAndroid, and 
paritially takes over the role it played as the tree root.

WindowAndroid will no longer inherit from ViewAndroid. It effectively
reverts https://crrev.com/2136373002.

Embedders are responsible for passing in a ViewRoot when creating a new
WebContents.

Also fixes a bug caused by using a main content's ViewRoot for overlay panel's.


BUG=671401, 671967
==========

[Jinsuk Kim, 2016-12-22 10:24:04]

Description was changed from

==========
Make ViewRoot the top of ViewAndroid tree

This CL introduces a native class ViewRoot, a subclass of ViewAndroid.
It acts as the root of a ViewAndroid tree, and receives touch/view
events from embedders. It has a reference to WindowAndroid, and 
paritially takes over the role it played as the tree root.

WindowAndroid will no longer inherit from ViewAndroid. It effectively
reverts https://crrev.com/2136373002.

Embedders are responsible for passing in a ViewRoot when creating a new
WebContents.

Also fixes a bug caused by using a main content's ViewRoot for overlay panel's.


BUG=671401, 671967
==========

to

==========
Make ViewRoot the top of ViewAndroid tree

This CL introduces a native class ViewRoot, a subclass of ViewAndroid.
It acts as the root of a ViewAndroid tree, and receives touch/view
events from embedders. It has a reference to WindowAndroid, and 
paritially takes over the role it played as the tree root.

WindowAndroid will no longer inherit from ViewAndroid. It effectively
reverts https://crrev.com/2136373002.

Embedders are responsible for passing in a ViewRoot when creating
a new WebContents.

Also fixes a bug caused by using a main content's ViewRoot for
overlay panel's.


BUG=671401, 671967
==========

[Jinsuk Kim, 2016-12-22 10:24:19]

The CQ bit was checked by jinsukkim@chromium.org to run a CQ dry run

[Jinsuk Kim, 2016-12-22 10:24:30]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-12-22 10:24:35]

Dry run: 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-12-22 11:43:10]

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

[commit-bot: I haz the power, 2016-12-22 11:43:11]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Jinsuk Kim, 2017-01-03 08:05:13]

Patchset #1 (id:40001) has been deleted

[Jinsuk Kim, 2017-01-03 08:05:20]

Patchset #1 (id:60001) has been deleted

[Jinsuk Kim, 2017-01-03 08:07:16]

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

[Jinsuk Kim, 2017-01-03 08:07:17]

..still got a few failing tests but would like to get your initial feedback.
Please take a look.

Note that blimp-side changes are just to make buildbot happy.

[Jinsuk Kim, 2017-01-03 08:08:33]

The CQ bit was checked by jinsukkim@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-03 08:08:45]

Dry run: 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, 2017-01-03 09:51:48]

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

[commit-bot: I haz the power, 2017-01-03 09:51:49]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[boliu, 2017-01-03 19:16:11]

pretty good overall

said in comments below, but overall suggestion is just make ViewRoot a parallel
object to WindowAndroid in content layer and above. No need to invent new
lifetime or ownership for ViewRoot yet.

https://codereview.chromium.org/2595263002/diff/80001/content/browser/devtool...
File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right):

https://codereview.chromium.org/2595263002/diff/80001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_browsertest.cc:613:
std::unique_ptr<ui::ViewRoot> view_root(new ui::ViewRoot());
it's not clear to me why this test is special, we should make sure *all* tests
set up ViewRoot correctly, even if it's not required to complete the test

https://codereview.chromium.org/2595263002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/compositor_impl_android.h (right):

https://codereview.chromium.org/2595263002/diff/80001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.h:131:
scoped_refptr<cc::Layer> root_layer_;
this used to live in WindowAndroid? then perhaps this class should hold a
ViewRoot now instead this?

https://codereview.chromium.org/2595263002/diff/80001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
(right):

https://codereview.chromium.org/2595263002/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:341:
public void initViewRoot(ViewRoot viewRoot);
I think ViewRoot should be set and stored where WindowAndroid is set and stored,
eg add new param to CVC.initialize/updateWindowAndroid. This probably applies to
the content embedder as well. No need to invent new lifetime rules for ViewRoot

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

https://codereview.chromium.org/2595263002/diff/80001/ui/android/view_android...
ui/android/view_android.cc:194: bool ViewAndroid::HasViewRootInTreeHierarchy() {
I think this linear time check is unnecessary now. The new, more strict
invariant is that ViewRoot is never added as a child of another View. And that's
super easy to enforce in AddChild.

https://codereview.chromium.org/2595263002/diff/80001/ui/android/view_android.h
File ui/android/view_android.h (right):

https://codereview.chromium.org/2595263002/diff/80001/ui/android/view_android...
ui/android/view_android.h:69: virtual void SetWindowAndroid(WindowAndroid*
window);
This should not be needed.

First this seems to be a method that should be on ViewRoot only, not View.

For both chrome and webview, WindowAndroid and ViewRoot should be set *together*
everywhere. And the mapping from a ViewRoot to WindowAndroid should never have
to change. This applies to tab reparenting in chrome as well, ie when
reparenting to a new WindowAndroid, should start using a new ViewRoot as well.
So there should be no need for SetWindowAndroid in ViewRoot either.

https://codereview.chromium.org/2595263002/diff/80001/ui/android/view_android...
ui/android/view_android.h:94: void OnPhysicalBackingSizeChangedInternal(int
width, int height);
the rename is to avoid name collision with ViewRoot, right?

maybe this should be protected then?

https://codereview.chromium.org/2595263002/diff/80001/ui/android/view_android...
ui/android/view_android.h:97: // Returns the root of this hierarchy.
not quite, returns the ViewRoot of the hierarchy, but null if hierarchy isn't
attached to a ViewRoot

https://codereview.chromium.org/2595263002/diff/80001/ui/android/view_root.h
File ui/android/view_root.h (right):

https://codereview.chromium.org/2595263002/diff/80001/ui/android/view_root.h#...
ui/android/view_root.h:1: // Copyright 2016 The Chromium Authors. All rights
reserved.
new year!

https://codereview.chromium.org/2595263002/diff/80001/ui/android/view_root.h#...
ui/android/view_root.h:25: ViewRoot();
Instead of this, have a "CreateForTesting" method, that also creates the java
part as well. See WindowAndroid::CreateForTesting for example.

https://codereview.chromium.org/2595263002/diff/80001/ui/android/view_root.h#...
ui/android/view_root.h:37: const base::android::JavaParamRef<jobject>& obj);
nit: indent is wrong here and below

https://codereview.chromium.org/2595263002/diff/80001/ui/android/view_root.h#...
ui/android/view_root.h:47: JavaObjectWeakGlobalRef java_obj_;
only looking at ui/ code right now, and it's not clear to me what the ownership
relationship is between native and java ViewRoot. eg what's responsible for
deleting the native side?

And whatever the ownership relationship is (other than relying on gc), this can
be a strong reference.

https://codereview.chromium.org/2595263002/diff/80001/ui/snapshot/snapshot_an...
File ui/snapshot/snapshot_android.cc (right):

https://codereview.chromium.org/2595263002/diff/80001/ui/snapshot/snapshot_an...
ui/snapshot/snapshot_android.cc:46: view.SetWindowAndroid(window);
err, this is just wrong..

This code can talk to DisplayAndroidManager directly, so maybe should do that.

So add a DisplayAndroidManager::GetInstance method, which can get the screen and
just downcast it. Then add a method to DisplayAndroidManager that just takes a
WindowAndroid.

This does beg the question, long term, should gfx::NativeWindow be redefined to
ViewRoot. I'm not sure what the answer is though.. whatever is convenient I
guess.

[boliu, 2017-01-03 19:17:01]

oh, also, I didn't look at chrome/chromecast/blimp

[Jinsuk Kim, 2017-01-04 10:41:19]

Patchset #2 (id:100001) has been deleted

[Jinsuk Kim, 2017-01-04 10:45:02]

https://codereview.chromium.org/2595263002/diff/80001/content/browser/devtool...
File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right):

https://codereview.chromium.org/2595263002/diff/80001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_browsertest.cc:613:
std::unique_ptr<ui::ViewRoot> view_root(new ui::ViewRoot());
On 2017/01/03 19:16:11, boliu wrote:
> it's not clear to me why this test is special, we should make sure *all* tests
> set up ViewRoot correctly, even if it's not required to complete the test

It was a wrong change added for temporary glitch. Removed.

https://codereview.chromium.org/2595263002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/compositor_impl_android.h (right):

https://codereview.chromium.org/2595263002/diff/80001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.h:131:
scoped_refptr<cc::Layer> root_layer_;
On 2017/01/03 19:16:11, boliu wrote:
> this used to live in WindowAndroid? then perhaps this class should hold a
> ViewRoot now instead this?

Compositor is shared among all content (so ViewRoot) instances. It makes sense
to use the layer in WindowAndroid but there can be multiple ViewRoots. This is
basically reverting the CL mentioned in the description.

https://codereview.chromium.org/2595263002/diff/80001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
(right):

https://codereview.chromium.org/2595263002/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:341:
public void initViewRoot(ViewRoot viewRoot);
On 2017/01/03 19:16:11, boliu wrote:
> I think ViewRoot should be set and stored where WindowAndroid is set and
stored,
> eg add new param to CVC.initialize/updateWindowAndroid. This probably applies
to
> the content embedder as well. No need to invent new lifetime rules for
ViewRoot

Done. It's just that I'm the one who put the comment in CVC telling people not
to add new stuff there... didn't want to do exactly that...

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

https://codereview.chromium.org/2595263002/diff/80001/ui/android/view_android...
ui/android/view_android.cc:194: bool ViewAndroid::HasViewRootInTreeHierarchy() {
On 2017/01/03 19:16:11, boliu wrote:
> I think this linear time check is unnecessary now. The new, more strict
> invariant is that ViewRoot is never added as a child of another View. And
that's
> super easy to enforce in AddChild.

Done.

https://codereview.chromium.org/2595263002/diff/80001/ui/android/view_android.h
File ui/android/view_android.h (right):

https://codereview.chromium.org/2595263002/diff/80001/ui/android/view_android...
ui/android/view_android.h:69: virtual void SetWindowAndroid(WindowAndroid*
window);
On 2017/01/03 19:16:11, boliu wrote:
> This should not be needed.
> 
> First this seems to be a method that should be on ViewRoot only, not View.
> 
> For both chrome and webview, WindowAndroid and ViewRoot should be set
*together*
> everywhere. And the mapping from a ViewRoot to WindowAndroid should never have
> to change. This applies to tab reparenting in chrome as well, ie when
> reparenting to a new WindowAndroid, should start using a new ViewRoot as well.
> So there should be no need for SetWindowAndroid in ViewRoot either.

Done. Is this suggestion about the immutable pairing between ViewRoot -
WindowAndroid to make it easy to track their relationship?

https://codereview.chromium.org/2595263002/diff/80001/ui/android/view_android...
ui/android/view_android.h:94: void OnPhysicalBackingSizeChangedInternal(int
width, int height);
On 2017/01/03 19:16:11, boliu wrote:
> the rename is to avoid name collision with ViewRoot, right?
> 
> maybe this should be protected then?

Done.

https://codereview.chromium.org/2595263002/diff/80001/ui/android/view_android...
ui/android/view_android.h:97: // Returns the root of this hierarchy.
On 2017/01/03 19:16:11, boliu wrote:
> not quite, returns the ViewRoot of the hierarchy, but null if hierarchy isn't
> attached to a ViewRoot

Done.

https://codereview.chromium.org/2595263002/diff/80001/ui/android/view_root.h
File ui/android/view_root.h (right):

https://codereview.chromium.org/2595263002/diff/80001/ui/android/view_root.h#...
ui/android/view_root.h:1: // Copyright 2016 The Chromium Authors. All rights
reserved.
On 2017/01/03 19:16:11, boliu wrote:
> new year!

Done.

https://codereview.chromium.org/2595263002/diff/80001/ui/android/view_root.h#...
ui/android/view_root.h:25: ViewRoot();
On 2017/01/03 19:16:11, boliu wrote:
> Instead of this, have a "CreateForTesting" method, that also creates the java
> part as well. See WindowAndroid::CreateForTesting for example.

It's also used in blimp. Replaced it with ViewRoot(WindowAndroid*).

https://codereview.chromium.org/2595263002/diff/80001/ui/android/view_root.h#...
ui/android/view_root.h:37: const base::android::JavaParamRef<jobject>& obj);
On 2017/01/03 19:16:11, boliu wrote:
> nit: indent is wrong here and below

Done.

https://codereview.chromium.org/2595263002/diff/80001/ui/android/view_root.h#...
ui/android/view_root.h:47: JavaObjectWeakGlobalRef java_obj_;
On 2017/01/03 19:16:11, boliu wrote:
> only looking at ui/ code right now, and it's not clear to me what the
ownership
> relationship is between native and java ViewRoot. eg what's responsible for
> deleting the native side?
> 

Updated to destroy the native side when all of its children are removed.
ViewRoot always starts with adding one child, so the destruction gets triggered
when the associated web content is destroyed.


> And whatever the ownership relationship is (other than relying on gc), this
can
> be a strong reference.

Any suggestion to avoid potential issue is welcome. I don't have a good idea...
now its use is limited to accessing a single call to Java (onDestroyNativeView).

https://codereview.chromium.org/2595263002/diff/80001/ui/snapshot/snapshot_an...
File ui/snapshot/snapshot_android.cc (right):

https://codereview.chromium.org/2595263002/diff/80001/ui/snapshot/snapshot_an...
ui/snapshot/snapshot_android.cc:46: view.SetWindowAndroid(window);
On 2017/01/03 19:16:11, boliu wrote:
> err, this is just wrong..
> 
> This code can talk to DisplayAndroidManager directly, so maybe should do that.
> 
> So add a DisplayAndroidManager::GetInstance method, which can get the screen
and
> just downcast it. Then add a method to DisplayAndroidManager that just takes a
> WindowAndroid.
> 

I have a problem getting this to build(undefined reference to
'ui::DisplayAndroidManager::GetDisplayNearestWindowAndroid(ui::WindowAndroid*)
const'). Will fix this soon.

> This does beg the question, long term, should gfx::NativeWindow be redefined
to
> ViewRoot. I'm not sure what the answer is though.. whatever is convenient I
> guess.

[boliu, 2017-01-04 17:45:10]

replying to comments

https://codereview.chromium.org/2595263002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/compositor_impl_android.h (right):

https://codereview.chromium.org/2595263002/diff/80001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.h:131:
scoped_refptr<cc::Layer> root_layer_;
On 2017/01/04 10:45:02, Jinsuk Kim wrote:
> On 2017/01/03 19:16:11, boliu wrote:
> > this used to live in WindowAndroid? then perhaps this class should hold a
> > ViewRoot now instead this?
> 
> Compositor is shared among all content (so ViewRoot) instances. It makes sense
> to use the layer in WindowAndroid but there can be multiple ViewRoots. This is
> basically reverting the CL mentioned in the description.

Well.. webview doesn't use this compositor, and everywhere else, WindowAndroid
and ViewRoot is 1-to-1, so it's probably not too bad to just use ViewRoot?

I think should note this assumption in a comment here, but I think it's the
right thing to do. Plus the decision of which ViewRoot is passed here happens in
content embedder layer, it doesn't require weird logic like use the ViewRoot
from the first WebContents

https://codereview.chromium.org/2595263002/diff/80001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
(right):

https://codereview.chromium.org/2595263002/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:341:
public void initViewRoot(ViewRoot viewRoot);
On 2017/01/04 10:45:02, Jinsuk Kim wrote:
> On 2017/01/03 19:16:11, boliu wrote:
> > I think ViewRoot should be set and stored where WindowAndroid is set and
> stored,
> > eg add new param to CVC.initialize/updateWindowAndroid. This probably
applies
> to
> > the content embedder as well. No need to invent new lifetime rules for
> ViewRoot
> 
> Done. It's just that I'm the one who put the comment in CVC telling people not
> to add new stuff there... didn't want to do exactly that...

That's true.. you can move WindowAndroid out of CVC first

https://codereview.chromium.org/2595263002/diff/80001/ui/android/view_android.h
File ui/android/view_android.h (right):

https://codereview.chromium.org/2595263002/diff/80001/ui/android/view_android...
ui/android/view_android.h:69: virtual void SetWindowAndroid(WindowAndroid*
window);
On 2017/01/04 10:45:02, Jinsuk Kim wrote:
> On 2017/01/03 19:16:11, boliu wrote:
> > This should not be needed.
> > 
> > First this seems to be a method that should be on ViewRoot only, not View.
> > 
> > For both chrome and webview, WindowAndroid and ViewRoot should be set
> *together*
> > everywhere. And the mapping from a ViewRoot to WindowAndroid should never
have
> > to change. This applies to tab reparenting in chrome as well, ie when
> > reparenting to a new WindowAndroid, should start using a new ViewRoot as
well.
> > So there should be no need for SetWindowAndroid in ViewRoot either.
> 
> Done. Is this suggestion about the immutable pairing between ViewRoot -
> WindowAndroid to make it easy to track their relationship? 

Yeah. Also to keep things simple and avoid unnecessary complexity.

https://codereview.chromium.org/2595263002/diff/80001/ui/android/view_root.h
File ui/android/view_root.h (right):

https://codereview.chromium.org/2595263002/diff/80001/ui/android/view_root.h#...
ui/android/view_root.h:47: JavaObjectWeakGlobalRef java_obj_;
On 2017/01/04 10:45:02, Jinsuk Kim wrote:
> On 2017/01/03 19:16:11, boliu wrote:
> > only looking at ui/ code right now, and it's not clear to me what the
> ownership
> > relationship is between native and java ViewRoot. eg what's responsible for
> > deleting the native side?
> > 
> 
> Updated to destroy the native side when all of its children are removed.
> ViewRoot always starts with adding one child, so the destruction gets
triggered
> when the associated web content is destroyed.

That seems wrong for chrome, where we expect the ViewRoot to stay even after all
the tabs are closed? Probably should just make it like WindowAndroid, have an
explicit destroy method on java that cleans up the native side.

> 
> 
> > And whatever the ownership relationship is (other than relying on gc), this
> can
> > be a strong reference.
> 
> Any suggestion to avoid potential issue is welcome. I don't have a good
idea...
> now its use is limited to accessing a single call to Java
(onDestroyNativeView).

Using a strong ref here is ok.

[boliu, 2017-01-04 18:58:06]

https://codereview.chromium.org/2595263002/diff/80001/ui/snapshot/snapshot_an...
File ui/snapshot/snapshot_android.cc (right):

https://codereview.chromium.org/2595263002/diff/80001/ui/snapshot/snapshot_an...
ui/snapshot/snapshot_android.cc:46: view.SetWindowAndroid(window);
On 2017/01/04 10:45:02, Jinsuk Kim wrote:
> On 2017/01/03 19:16:11, boliu wrote:
> > err, this is just wrong..
> > 
> > This code can talk to DisplayAndroidManager directly, so maybe should do
that.
> > 
> > So add a DisplayAndroidManager::GetInstance method, which can get the screen
> and
> > just downcast it. Then add a method to DisplayAndroidManager that just takes
a
> > WindowAndroid.
> > 
> 
> I have a problem getting this to build(undefined reference to
> 'ui::DisplayAndroidManager::GetDisplayNearestWindowAndroid(ui::WindowAndroid*)
> const'). Will fix this soon.
> 
> > This does beg the question, long term, should gfx::NativeWindow be redefined
> to
> > ViewRoot. I'm not sure what the answer is though.. whatever is convenient I
> > guess.
> 

Need to add UI_ANDROID_EXPORT to DisplayAndroidManager

https://codereview.chromium.org/2595263002/diff/120001/content/browser/androi...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/2595263002/diff/120001/content/browser/androi...
content/browser/android/content_view_core_impl.cc:280: ui::WindowAndroid* window
=
should DCHECK that window and view root are either both null or both not null

actually.. perhaps should pass in view_root only since can get the window
android from the view root? Probably should rename the method :)

https://codereview.chromium.org/2595263002/diff/120001/content/browser/androi...
content/browser/android/content_view_core_impl.cc:282: if (window ==
GetWindowAndroid())
this is not enough, what if view_root is different?

https://codereview.chromium.org/2595263002/diff/120001/content/browser/androi...
content/browser/android/content_view_core_impl.cc:285: view->RemoveFromParent();
Remove/AddChild should stay where they used to be

https://codereview.chromium.org/2595263002/diff/120001/content/browser/androi...
content/browser/android/content_view_core_impl.cc:286:
ui::ViewRoot::FromJavaObject(env, jview_root)->AddChild(view);
probably should match WindowAndroid pattern here too, ie have a getNativePointer
method on the java class

https://codereview.chromium.org/2595263002/diff/120001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/2595263002/diff/120001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:587:
WindowAndroid windowAndroid, ViewRoot viewRoot) {
idea.. maybe should just pass in view root everywhere in content, since you can
get to the window from view root?

https://codereview.chromium.org/2595263002/diff/120001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
(right):

https://codereview.chromium.org/2595263002/diff/120001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:127:
public void setCurrentContent(ContentViewCore contentViewCore, ViewRoot
viewRoot) {
seems like this should be unnecessary, you can get the ViewRoot from CVC, right?

https://codereview.chromium.org/2595263002/diff/120001/ui/android/display_and...
File ui/android/display_android_manager.h (right):

https://codereview.chromium.org/2595263002/diff/120001/ui/android/display_and...
ui/android/display_android_manager.h:20: display::Display
GetDisplayNearestWindowAndroid(
add a comment that this is here because NativeWindow is not a NativeView on
android

https://codereview.chromium.org/2595263002/diff/120001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/base/ViewRoot.java (right):

https://codereview.chromium.org/2595263002/diff/120001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/base/ViewRoot.java:19: public static
ViewRoot create(WindowAndroid window) {
return null if window is null? or could throw an except too..

https://codereview.chromium.org/2595263002/diff/120001/ui/android/view_androi...
File ui/android/view_android.cc (right):

https://codereview.chromium.org/2595263002/diff/120001/ui/android/view_androi...
ui/android/view_android.cc:97: DCHECK(child->GetViewRoot() != child);  //
ViewRoot cannot be a child.
just have a "bool IsViewRoot" method instead

https://codereview.chromium.org/2595263002/diff/120001/ui/android/view_android.h
File ui/android/view_android.h (right):

https://codereview.chromium.org/2595263002/diff/120001/ui/android/view_androi...
ui/android/view_android.h:68: virtual void AddChild(ViewAndroid* child);
hmm, why does this need to be virtual?

https://codereview.chromium.org/2595263002/diff/120001/ui/android/view_androi...
ui/android/view_android.h:94: virtual void RemoveChild(ViewAndroid* child);
this doesn't need to be virtual or protected if ViewRoot is destroyed explicitly

https://codereview.chromium.org/2595263002/diff/120001/ui/android/view_root.cc
File ui/android/view_root.cc (right):

https://codereview.chromium.org/2595263002/diff/120001/ui/android/view_root.c...
ui/android/view_root.cc:85: }  // namespace ui
nit: empty line above

https://codereview.chromium.org/2595263002/diff/120001/ui/android/view_root.h
File ui/android/view_root.h (right):

https://codereview.chromium.org/2595263002/diff/120001/ui/android/view_root.h...
ui/android/view_root.h:25: ViewRoot(WindowAndroid* window_android);
explicit

Also I don't understand why blimp is special about this. Doesn't it need a java
ViewRoot as well to forward events? Again, ViewRoot should just be a parallel
object to WindowAndroid. It's ok to hack something in blimp code, but it's not
ok that hack spills out into other code.

https://codereview.chromium.org/2595263002/diff/120001/ui/android/view_root.h...
ui/android/view_root.h:36: static ViewRoot* FromJavaObject(JNIEnv* env,
style again here and below: c++ requires multi-line arguments to be lined up
vertically

https://codereview.chromium.org/2595263002/diff/120001/ui/snapshot/snapshot_a...
File ui/snapshot/snapshot_android.cc (right):

https://codereview.chromium.org/2595263002/diff/120001/ui/snapshot/snapshot_a...
ui/snapshot/snapshot_android.cc:47:
static_cast<DisplayAndroidManager*>(display::Screen::GetScreen());
cast should happen in DisplayAndroidManager, maybe should just have a
DisplayAndroidManager::GetInstance?

[Jinsuk Kim, 2017-01-05 05:39:45]

Patchset #3 (id:140001) has been deleted

[Jinsuk Kim, 2017-01-05 05:48:56]

The CQ bit was checked by jinsukkim@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 05:49:21]

Dry run: 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, 2017-01-05 06:28:13]

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

[commit-bot: I haz the power, 2017-01-05 06:28:14]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Jinsuk Kim, 2017-01-05 06:57:27]

Patchset #3 (id:160001) has been deleted

[Jinsuk Kim, 2017-01-05 08:18:20]

Patchset #4 (id:200001) has been deleted

[Jinsuk Kim, 2017-01-05 08:18:31]

Patchset #3 (id:180001) has been deleted

[Jinsuk Kim, 2017-01-05 10:18:46]

Patchset #3 (id:220001) has been deleted

[Jinsuk Kim, 2017-01-05 10:18:58]

Patchset #3 (id:240001) has been deleted

[Jinsuk Kim, 2017-01-05 11:03:13]

https://codereview.chromium.org/2595263002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/compositor_impl_android.h (right):

https://codereview.chromium.org/2595263002/diff/80001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.h:131:
scoped_refptr<cc::Layer> root_layer_;
On 2017/01/04 17:45:10, boliu wrote:
> On 2017/01/04 10:45:02, Jinsuk Kim wrote:
> > On 2017/01/03 19:16:11, boliu wrote:
> > > this used to live in WindowAndroid? then perhaps this class should hold a
> > > ViewRoot now instead this?
> > 
> > Compositor is shared among all content (so ViewRoot) instances. It makes
sense
> > to use the layer in WindowAndroid but there can be multiple ViewRoots. This
is
> > basically reverting the CL mentioned in the description.
> 
> Well.. webview doesn't use this compositor, and everywhere else, WindowAndroid
> and ViewRoot is 1-to-1, so it's probably not too bad to just use ViewRoot?
> 
> I think should note this assumption in a comment here, but I think it's the
> right thing to do. Plus the decision of which ViewRoot is passed here happens
in
> content embedder layer, it doesn't require weird logic like use the ViewRoot
> from the first WebContents

Learned the hard way that Chrome also can have multiple ViewRoots under a
WindowAndroid when it uses overlay panel. This makes it tricky to use ViewRoot
here, because it switches dynamically depending on which content it works for,
while the root layer here means to be static, per-WindowAndroid object, created
at construction time and used it in many places with the need to update. So I'm
still not sure if it is better to switch to ViewRoot and use the underlying cc
layer. |SetRootLayer()| for instance may not work as expected if the root layer
keeps changing.

https://codereview.chromium.org/2595263002/diff/80001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
(right):

https://codereview.chromium.org/2595263002/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:341:
public void initViewRoot(ViewRoot viewRoot);
On 2017/01/04 17:45:10, boliu wrote:
> On 2017/01/04 10:45:02, Jinsuk Kim wrote:
> > On 2017/01/03 19:16:11, boliu wrote:
> > > I think ViewRoot should be set and stored where WindowAndroid is set and
> > stored,
> > > eg add new param to CVC.initialize/updateWindowAndroid. This probably
> applies
> > to
> > > the content embedder as well. No need to invent new lifetime rules for
> > ViewRoot
> > 
> > Done. It's just that I'm the one who put the comment in CVC telling people
not
> > to add new stuff there... didn't want to do exactly that...
> 
> That's true.. you can move WindowAndroid out of CVC first

I like your suggestion s/WindowAndroid/ViewRoot passed into CVC.

https://codereview.chromium.org/2595263002/diff/80001/ui/android/view_android.h
File ui/android/view_android.h (right):

https://codereview.chromium.org/2595263002/diff/80001/ui/android/view_android...
ui/android/view_android.h:69: virtual void SetWindowAndroid(WindowAndroid*
window);
On 2017/01/04 17:45:10, boliu wrote:
> On 2017/01/04 10:45:02, Jinsuk Kim wrote:
> > On 2017/01/03 19:16:11, boliu wrote:
> > > This should not be needed.
> > > 
> > > First this seems to be a method that should be on ViewRoot only, not View.
> > > 
> > > For both chrome and webview, WindowAndroid and ViewRoot should be set
> > *together*
> > > everywhere. And the mapping from a ViewRoot to WindowAndroid should never
> have
> > > to change. This applies to tab reparenting in chrome as well, ie when
> > > reparenting to a new WindowAndroid, should start using a new ViewRoot as
> well.
> > > So there should be no need for SetWindowAndroid in ViewRoot either.
> > 
> > Done. Is this suggestion about the immutable pairing between ViewRoot -
> > WindowAndroid to make it easy to track their relationship? 
> 
> Yeah. Also to keep things simple and avoid unnecessary complexity.

Acknowledged.

https://codereview.chromium.org/2595263002/diff/80001/ui/android/view_root.h
File ui/android/view_root.h (right):

https://codereview.chromium.org/2595263002/diff/80001/ui/android/view_root.h#...
ui/android/view_root.h:47: JavaObjectWeakGlobalRef java_obj_;
On 2017/01/04 17:45:10, boliu wrote:
> On 2017/01/04 10:45:02, Jinsuk Kim wrote:
> > On 2017/01/03 19:16:11, boliu wrote:
> > > only looking at ui/ code right now, and it's not clear to me what the
> > ownership
> > > relationship is between native and java ViewRoot. eg what's responsible
for
> > > deleting the native side?
> > > 
> > 
> > Updated to destroy the native side when all of its children are removed.
> > ViewRoot always starts with adding one child, so the destruction gets
> triggered
> > when the associated web content is destroyed.
> 
> That seems wrong for chrome, where we expect the ViewRoot to stay even after
all
> the tabs are closed? Probably should just make it like WindowAndroid, have an
> explicit destroy method on java that cleans up the native side

Done.

> > 
> > 
> > > And whatever the ownership relationship is (other than relying on gc),
this
> > can
> > > be a strong reference.
> > 
> > Any suggestion to avoid potential issue is welcome. I don't have a good
> idea...
> > now its use is limited to accessing a single call to Java
> (onDestroyNativeView).
> 
> Using a strong ref here is ok.

Now that onDestroyNative is gone, java obj is not needed any more atm.

https://codereview.chromium.org/2595263002/diff/80001/ui/snapshot/snapshot_an...
File ui/snapshot/snapshot_android.cc (right):

https://codereview.chromium.org/2595263002/diff/80001/ui/snapshot/snapshot_an...
ui/snapshot/snapshot_android.cc:46: view.SetWindowAndroid(window);
On 2017/01/04 18:58:05, boliu wrote:
> On 2017/01/04 10:45:02, Jinsuk Kim wrote:
> > On 2017/01/03 19:16:11, boliu wrote:
> > > err, this is just wrong..
> > > 
> > > This code can talk to DisplayAndroidManager directly, so maybe should do
> that.
> > > 
> > > So add a DisplayAndroidManager::GetInstance method, which can get the
screen
> > and
> > > just downcast it. Then add a method to DisplayAndroidManager that just
takes
> a
> > > WindowAndroid.
> > > 
> > 
> > I have a problem getting this to build(undefined reference to
> >
'ui::DisplayAndroidManager::GetDisplayNearestWindowAndroid(ui::WindowAndroid*)
> > const'). Will fix this soon.
> > 
> > > This does beg the question, long term, should gfx::NativeWindow be
redefined
> > to
> > > ViewRoot. I'm not sure what the answer is though.. whatever is convenient
I
> > > guess.
> > 
> 
> Need to add UI_ANDROID_EXPORT to DisplayAndroidManager

Ah

https://codereview.chromium.org/2595263002/diff/120001/content/browser/androi...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/2595263002/diff/120001/content/browser/androi...
content/browser/android/content_view_core_impl.cc:280: ui::WindowAndroid* window
=
On 2017/01/04 18:58:06, boliu wrote:
> should DCHECK that window and view root are either both null or both not null
> 
> actually.. perhaps should pass in view_root only since can get the window
> android from the view root? Probably should rename the method :)

Renamed to UpdateViewRoot.

https://codereview.chromium.org/2595263002/diff/120001/content/browser/androi...
content/browser/android/content_view_core_impl.cc:282: if (window ==
GetWindowAndroid())
On 2017/01/04 18:58:05, boliu wrote:
> this is not enough, what if view_root is different?

Used view_root instead. ViewRoot and WA always come in pair now, so this will do
the job.

https://codereview.chromium.org/2595263002/diff/120001/content/browser/androi...
content/browser/android/content_view_core_impl.cc:285: view->RemoveFromParent();
On 2017/01/04 18:58:06, boliu wrote:
> Remove/AddChild should stay where they used to be

Done.

https://codereview.chromium.org/2595263002/diff/120001/content/browser/androi...
content/browser/android/content_view_core_impl.cc:286:
ui::ViewRoot::FromJavaObject(env, jview_root)->AddChild(view);
On 2017/01/04 18:58:05, boliu wrote:
> probably should match WindowAndroid pattern here too, ie have a
getNativePointer
> method on the java class

Done.

https://codereview.chromium.org/2595263002/diff/120001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/2595263002/diff/120001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:587:
WindowAndroid windowAndroid, ViewRoot viewRoot) {
On 2017/01/04 18:58:06, boliu wrote:
> idea.. maybe should just pass in view root everywhere in content, since you
can
> get to the window from view root?

Nice idea.

https://codereview.chromium.org/2595263002/diff/120001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
(right):

https://codereview.chromium.org/2595263002/diff/120001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:127:
public void setCurrentContent(ContentViewCore contentViewCore, ViewRoot
viewRoot) {
On 2017/01/04 18:58:06, boliu wrote:
> seems like this should be unnecessary, you can get the ViewRoot from CVC,
right?

Again, this is how I struggled not to add a new method/variable in CVC...

https://codereview.chromium.org/2595263002/diff/120001/ui/android/display_and...
File ui/android/display_android_manager.h (right):

https://codereview.chromium.org/2595263002/diff/120001/ui/android/display_and...
ui/android/display_android_manager.h:20: display::Display
GetDisplayNearestWindowAndroid(
On 2017/01/04 18:58:06, boliu wrote:
> add a comment that this is here because NativeWindow is not a NativeView on
> android

Done.

https://codereview.chromium.org/2595263002/diff/120001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/base/ViewRoot.java (right):

https://codereview.chromium.org/2595263002/diff/120001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/base/ViewRoot.java:19: public static
ViewRoot create(WindowAndroid window) {
On 2017/01/04 18:58:06, boliu wrote:
> return null if window is null? or could throw an except too..

Done. Chose to throw an exception.

https://codereview.chromium.org/2595263002/diff/120001/ui/android/view_androi...
File ui/android/view_android.cc (right):

https://codereview.chromium.org/2595263002/diff/120001/ui/android/view_androi...
ui/android/view_android.cc:97: DCHECK(child->GetViewRoot() != child);  //
ViewRoot cannot be a child.
On 2017/01/04 18:58:06, boliu wrote:
> just have a "bool IsViewRoot" method instead

Done.

https://codereview.chromium.org/2595263002/diff/120001/ui/android/view_android.h
File ui/android/view_android.h (right):

https://codereview.chromium.org/2595263002/diff/120001/ui/android/view_androi...
ui/android/view_android.h:68: virtual void AddChild(ViewAndroid* child);
On 2017/01/04 18:58:06, boliu wrote:
> hmm, why does this need to be virtual?

Ah forgot to remove it after some testing. Done.

https://codereview.chromium.org/2595263002/diff/120001/ui/android/view_androi...
ui/android/view_android.h:94: virtual void RemoveChild(ViewAndroid* child);
On 2017/01/04 18:58:06, boliu wrote:
> this doesn't need to be virtual or protected if ViewRoot is destroyed
explicitly

Removed virtual and turned to private.

https://codereview.chromium.org/2595263002/diff/120001/ui/android/view_root.cc
File ui/android/view_root.cc (right):

https://codereview.chromium.org/2595263002/diff/120001/ui/android/view_root.c...
ui/android/view_root.cc:85: }  // namespace ui
On 2017/01/04 18:58:06, boliu wrote:
> nit: empty line above

Done.

https://codereview.chromium.org/2595263002/diff/120001/ui/android/view_root.h
File ui/android/view_root.h (right):

https://codereview.chromium.org/2595263002/diff/120001/ui/android/view_root.h...
ui/android/view_root.h:25: ViewRoot(WindowAndroid* window_android);
On 2017/01/04 18:58:06, boliu wrote:
> explicit
> 
> Also I don't understand why blimp is special about this. Doesn't it need a
java
> ViewRoot as well to forward events? Again, ViewRoot should just be a parallel
> object to WindowAndroid. It's ok to hack something in blimp code, but it's not
> ok that hack spills out into other code.

My bad - should have avoided the quick hack.

So far ViewRoot handles only physical backing size which is not needed for Blimp
(yet). The constructor can be removed.

https://codereview.chromium.org/2595263002/diff/120001/ui/android/view_root.h...
ui/android/view_root.h:36: static ViewRoot* FromJavaObject(JNIEnv* env,
On 2017/01/04 18:58:06, boliu wrote:
> style again here and below: c++ requires multi-line arguments to be lined up
> vertically

Done.

https://codereview.chromium.org/2595263002/diff/120001/ui/snapshot/snapshot_a...
File ui/snapshot/snapshot_android.cc (right):

https://codereview.chromium.org/2595263002/diff/120001/ui/snapshot/snapshot_a...
ui/snapshot/snapshot_android.cc:47:
static_cast<DisplayAndroidManager*>(display::Screen::GetScreen());
On 2017/01/04 18:58:06, boliu wrote:
> cast should happen in DisplayAndroidManager, maybe should just have a
> DisplayAndroidManager::GetInstance?

Done.

[boliu, 2017-01-05 19:09:27]

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

[boliu, 2017-01-05 19:09:29]

+ted: Question about viewroot in chrome. Should OverlayPanel get its own
viewroot? In my head overlay should just be its own subtree without a root, but
then I don't know much about overlay panels in chrome

https://codereview.chromium.org/2595263002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/compositor_impl_android.h (right):

https://codereview.chromium.org/2595263002/diff/80001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.h:131:
scoped_refptr<cc::Layer> root_layer_;
On 2017/01/05 11:03:12, Jinsuk Kim wrote:
> On 2017/01/04 17:45:10, boliu wrote:
> > On 2017/01/04 10:45:02, Jinsuk Kim wrote:
> > > On 2017/01/03 19:16:11, boliu wrote:
> > > > this used to live in WindowAndroid? then perhaps this class should hold
a
> > > > ViewRoot now instead this?
> > > 
> > > Compositor is shared among all content (so ViewRoot) instances. It makes
> sense
> > > to use the layer in WindowAndroid but there can be multiple ViewRoots.
This
> is
> > > basically reverting the CL mentioned in the description.
> > 
> > Well.. webview doesn't use this compositor, and everywhere else,
WindowAndroid
> > and ViewRoot is 1-to-1, so it's probably not too bad to just use ViewRoot?
> > 
> > I think should note this assumption in a comment here, but I think it's the
> > right thing to do. Plus the decision of which ViewRoot is passed here
happens
> in
> > content embedder layer, it doesn't require weird logic like use the ViewRoot
> > from the first WebContents
> 
> Learned the hard way that Chrome also can have multiple ViewRoots under a
> WindowAndroid when it uses overlay panel. This makes it tricky to use ViewRoot
> here, because it switches dynamically depending on which content it works for,
> while the root layer here means to be static, per-WindowAndroid object,
created
> at construction time and used it in many places with the need to update. So
I'm
> still not sure if it is better to switch to ViewRoot and use the underlying cc
> layer. |SetRootLayer()| for instance may not work as expected if the root
layer
> keeps changing.

Overlay panel like contextual search? Maybe we should discuss this in the next
meeting, or just add ted to this. But in my head, I don't think an overlay panel
should get its own ViewRoot, it's just another part of a non-root subtree.. But
then again, I don't know much about overlay panels in chrome

https://codereview.chromium.org/2595263002/diff/210038/android_webview/java/s...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/2595263002/diff/210038/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:1206:
mViewRoot.destroy();
Finding the right place to call destroy is tricky for webview. This doesn't work
if webview is garbage collected without being destroyed first (which is
allowed..) The right place to handle that is pass the ViewRoot into
AwContentsDestroyRunnable, and call destroy in run()

https://codereview.chromium.org/2595263002/diff/210038/content/browser/androi...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/2595263002/diff/210038/content/browser/androi...
content/browser/android/content_view_core_impl.cc:284: if (GetWindowAndroid()) {
should be view->GetViewRoot() now

https://codereview.chromium.org/2595263002/diff/210038/content/browser/androi...
content/browser/android/content_view_core_impl.cc:1537: const
JavaParamRef<jobject>& jview_root,
use native pointer here as well

https://codereview.chromium.org/2595263002/diff/210038/content/browser/webui/...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/2595263002/diff/210038/content/browser/webui/...
content/browser/webui/url_data_manager_backend.cc:481:
base::PostTaskWithTraitsAndReply(
unrelated changes in this file?

https://codereview.chromium.org/2595263002/diff/210038/content/shell/android/...
File content/shell/android/java/src/org/chromium/content_shell/Shell.java
(right):

https://codereview.chromium.org/2595263002/diff/210038/content/shell/android/...
content/shell/android/java/src/org/chromium/content_shell/Shell.java:301:
webContents, ViewRoot.create(mWindow));
first question is what calls destroy?

Then after digging, the right thing here again is that ViewRoot should be
created *next* to where WindowAndroid is created, so in this case, in
ContentShellActivity. However, then also realized that WindowAndroid isn't
destroyed either, which is an existing bug.

I still haven't read other embedders (other than webview) yet, but make sure
destroy is called in all the places

https://codereview.chromium.org/2595263002/diff/210038/ui/android/display_and...
File ui/android/display_android_manager.h (right):

https://codereview.chromium.org/2595263002/diff/210038/ui/android/display_and...
ui/android/display_android_manager.h:21: static DisplayAndroidManager*
GetInstance();
Add a comment that prefer using Screen::GetScreen, and only use this to avoid
casting

https://codereview.chromium.org/2595263002/diff/210038/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/base/ViewRoot.java (right):

https://codereview.chromium.org/2595263002/diff/210038/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/base/ViewRoot.java:28: mNativeView =
nativeInit(window == null ? 0 : window.getNativePointer());
don't need null check here anymore

https://codereview.chromium.org/2595263002/diff/210038/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/base/ViewRoot.java:47: @CalledByNative
Remove ViewRoot::FromJavaObject entirely, and then this won't be needed anymore

https://codereview.chromium.org/2595263002/diff/210038/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/base/ViewRoot.java:52: public void destroy()
{
assert mNativeView != 0

https://codereview.chromium.org/2595263002/diff/210038/ui/android/view_android.h
File ui/android/view_android.h (right):

https://codereview.chromium.org/2595263002/diff/210038/ui/android/view_androi...
ui/android/view_android.h:71: // Adds this view as a child of another view.
just noticed this comment sounds backwards..

https://codereview.chromium.org/2595263002/diff/210038/ui/android/view_androi...
ui/android/view_android.h:72: void AddChild(ViewAndroid* child);
nit: avoid rearranging method order

https://codereview.chromium.org/2595263002/diff/210038/ui/android/view_root.cc
File ui/android/view_root.cc (right):

https://codereview.chromium.org/2595263002/diff/210038/ui/android/view_root.c...
ui/android/view_root.cc:28: const base::android::JavaParamRef<jobject>& obj,
don't these two params? then remember to mark constructor explicit

https://codereview.chromium.org/2595263002/diff/210038/ui/android/view_root.c...
ui/android/view_root.cc:29: jlong jwindow_android) {
nit: j prefix usually means a java type, that's not the case here, this is a
native pointer

[Ted C, 2017-01-05 19:17:14]

tedchoc@chromium.org changed reviewers:
+ dtrainor@chromium.org, mdjones@chromium.org

[Ted C, 2017-01-05 19:17:15]

+mdjones and +dtrainor for larger overlay panel thoughts as well.

But, I agree with you Bo.  I don't think they should have their own ViewRoot. 
ViewRoot is not supposed to be 1-1 with web contents.  Our ViewRoot is
essentially the CompositorView.  The overlay panel and normal web contents are
children of that view and therefore share the same ViewRoot.

[Jinsuk Kim, 2017-01-05 20:27:14]

On 2017/01/05 19:17:15, Ted C wrote:
> +mdjones and +dtrainor for larger overlay panel thoughts as well.
> 
> But, I agree with you Bo.  I don't think they should have their own ViewRoot. 
> ViewRoot is not supposed to be 1-1 with web contents.  Our ViewRoot is
> essentially the CompositorView.  The overlay panel and normal web contents are
> children of that view and therefore share the same ViewRoot.

From my view ViewRoot is used per content to forward events in place of CVC.
Overlay panel comes with its own content that needs events specifically
parameterized for it, which I think justifies having its own ViewRoot.
It's not clear to me how a ViewRoot can tell which web contents to talk to.
Please see CompositorViewHolder where I put a logic that chooses the right
ViewRoot for the content it is dealing with to set the physical backing size.
This might require further clarification on how ViewRoot should work
in terms of concept before going further.

[boliu, 2017-01-05 20:32:54]

On 2017/01/05 20:27:14, Jinsuk Kim wrote:
> On 2017/01/05 19:17:15, Ted C wrote:
> > +mdjones and +dtrainor for larger overlay panel thoughts as well.
> > 
> > But, I agree with you Bo.  I don't think they should have their own
ViewRoot. 
> > ViewRoot is not supposed to be 1-1 with web contents.  Our ViewRoot is
> > essentially the CompositorView.  The overlay panel and normal web contents
are
> > children of that view and therefore share the same ViewRoot.
> 
> From my view ViewRoot is used per content to forward events in place of CVC.
> Overlay panel comes with its own content that needs events specifically
> parameterized for it, which I think justifies having its own ViewRoot.
> It's not clear to me how a ViewRoot can tell which web contents to talk to.
> Please see CompositorViewHolder where I put a logic that chooses the right
> ViewRoot for the content it is dealing with to set the physical backing size.
> This might require further clarification on how ViewRoot should work
> in terms of concept before going further.

So looking that code, the current behavior is that if there's an overlay, then
the size change is only propagated to the overlay, and not the tab's webcontents
below? that seems odd to me, but ok..

but then the proper way to figure this out is probably something like, tell the
viewandroid of the tab's webcontents to *not* propagate size changes (and
possibly other things in the future)

[mdjones, 2017-01-05 22:21:49]

On 2017/01/05 19:17:15, Ted C wrote:
> +mdjones and +dtrainor for larger overlay panel thoughts as well.
> 
> But, I agree with you Bo.  I don't think they should have their own ViewRoot. 
> ViewRoot is not supposed to be 1-1 with web contents.  Our ViewRoot is
> essentially the CompositorView.  The overlay panel and normal web contents are
> children of that view and therefore share the same ViewRoot.

Conceptually I think there should be a single ViewRoot. For event routing,
touches will never go to the main page web contents if the overlay is showing.
Since we already have logic specific to overlays in CompositorViewHolder, it
seems like we should be able to do something similar for this.

[Jinsuk Kim, 2017-01-05 22:30:16]

https://codereview.chromium.org/2595263002/diff/210038/android_webview/java/s...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/2595263002/diff/210038/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:1206:
mViewRoot.destroy();
On 2017/01/05 19:09:28, boliu wrote:
> Finding the right place to call destroy is tricky for webview. This doesn't
work
> if webview is garbage collected without being destroyed first (which is
> allowed..) The right place to handle that is pass the ViewRoot into
> AwContentsDestroyRunnable, and call destroy in run()

Done. I'm still keeping the code nulling out mViewRoot below in this case where
mCleanupReference is not set.

https://codereview.chromium.org/2595263002/diff/210038/content/browser/androi...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/2595263002/diff/210038/content/browser/androi...
content/browser/android/content_view_core_impl.cc:284: if (GetWindowAndroid()) {
On 2017/01/05 19:09:28, boliu wrote:
> should be view->GetViewRoot() now

Done.

https://codereview.chromium.org/2595263002/diff/210038/content/browser/androi...
content/browser/android/content_view_core_impl.cc:1537: const
JavaParamRef<jobject>& jview_root,
On 2017/01/05 19:09:28, boliu wrote:
> use native pointer here as well

Done.

https://codereview.chromium.org/2595263002/diff/210038/content/browser/webui/...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/2595263002/diff/210038/content/browser/webui/...
content/browser/webui/url_data_manager_backend.cc:481:
base::PostTaskWithTraitsAndReply(
On 2017/01/05 19:09:28, boliu wrote:
> unrelated changes in this file?

No idea how this sneaked in... reverted.

https://codereview.chromium.org/2595263002/diff/210038/content/shell/android/...
File content/shell/android/java/src/org/chromium/content_shell/Shell.java
(right):

https://codereview.chromium.org/2595263002/diff/210038/content/shell/android/...
content/shell/android/java/src/org/chromium/content_shell/Shell.java:301:
webContents, ViewRoot.create(mWindow));
On 2017/01/05 19:09:28, boliu wrote:
> first question is what calls destroy?
> 
> Then after digging, the right thing here again is that ViewRoot should be
> created *next* to where WindowAndroid is created, so in this case, in
> ContentShellActivity. However, then also realized that WindowAndroid isn't
> destroyed either, which is an existing bug.
> 

Shouldn't it be created where 'content' is created? Though in this embedder WA :
VR is 1-1 there relationship is 1-multiple in general, and view root is created
per content.

But then in the latest comments I can see that the discussion you guys had kind
of concluded it is rather per-tab (or CompositorView) where there can be
multiple contents (overlay panel for contextual search, mobile view, etc) -
specifically for Chrome. 

I still can't get my head around how a single ViewRoot can deal with multiple
web contents. I'll come up with either a new patch or a new question soon.

> I still haven't read other embedders (other than webview) yet, but make sure
> destroy is called in all the places

Done. destroyed when Shell (and its CVC) is destroyed for now.

https://codereview.chromium.org/2595263002/diff/210038/ui/android/display_and...
File ui/android/display_android_manager.h (right):

https://codereview.chromium.org/2595263002/diff/210038/ui/android/display_and...
ui/android/display_android_manager.h:21: static DisplayAndroidManager*
GetInstance();
On 2017/01/05 19:09:28, boliu wrote:
> Add a comment that prefer using Screen::GetScreen, and only use this to avoid
> casting

Done.

https://codereview.chromium.org/2595263002/diff/210038/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/base/ViewRoot.java (right):

https://codereview.chromium.org/2595263002/diff/210038/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/base/ViewRoot.java:28: mNativeView =
nativeInit(window == null ? 0 : window.getNativePointer());
On 2017/01/05 19:09:29, boliu wrote:
> don't need null check here anymore

Done.

https://codereview.chromium.org/2595263002/diff/210038/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/base/ViewRoot.java:47: @CalledByNative
On 2017/01/05 19:09:29, boliu wrote:
> Remove ViewRoot::FromJavaObject entirely, and then this won't be needed
anymore

Now it's used to pass native pointer through CVC nativeInit(). Ended up taking
off @CalledByNative only.

https://codereview.chromium.org/2595263002/diff/210038/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/base/ViewRoot.java:52: public void destroy()
{
On 2017/01/05 19:09:29, boliu wrote:
> assert mNativeView != 0

Done.

https://codereview.chromium.org/2595263002/diff/210038/ui/android/view_android.h
File ui/android/view_android.h (right):

https://codereview.chromium.org/2595263002/diff/210038/ui/android/view_androi...
ui/android/view_android.h:71: // Adds this view as a child of another view.
On 2017/01/05 19:09:29, boliu wrote:
> just noticed this comment sounds backwards..

Done.

https://codereview.chromium.org/2595263002/diff/210038/ui/android/view_androi...
ui/android/view_android.h:72: void AddChild(ViewAndroid* child);
On 2017/01/05 19:09:29, boliu wrote:
> nit: avoid rearranging method order

Done.

https://codereview.chromium.org/2595263002/diff/210038/ui/android/view_root.cc
File ui/android/view_root.cc (right):

https://codereview.chromium.org/2595263002/diff/210038/ui/android/view_root.c...
ui/android/view_root.cc:28: const base::android::JavaParamRef<jobject>& obj,
On 2017/01/05 19:09:29, boliu wrote:
> don't these two params? then remember to mark constructor explicit

Done.

https://codereview.chromium.org/2595263002/diff/210038/ui/android/view_root.c...
ui/android/view_root.cc:29: jlong jwindow_android) {
On 2017/01/05 19:09:29, boliu wrote:
> nit: j prefix usually means a java type, that's not the case here, this is a
> native pointer

Done.

[boliu, 2017-01-05 22:39:38]

Ok, gonna continue this review, and remove other reviewers that got added here
just for the discussion.

If we have other things to discuss, we'll start a thread instead :p

https://codereview.chromium.org/2595263002/diff/210038/content/shell/android/...
File content/shell/android/java/src/org/chromium/content_shell/Shell.java
(right):

https://codereview.chromium.org/2595263002/diff/210038/content/shell/android/...
content/shell/android/java/src/org/chromium/content_shell/Shell.java:301:
webContents, ViewRoot.create(mWindow));
On 2017/01/05 22:30:15, Jinsuk Kim wrote:
> Shouldn't it be created where 'content' is created?

No, except webview. If it weren't for webview, ViewRoot and WindowAndroid would
be the same class. Which means other than webview, ViewRoot and WindowAndroid
should have the exact same life time.

[boliu, 2017-01-05 22:39:51]

boliu@chromium.org changed reviewers:
- dtrainor@chromium.org, mdjones@chromium.org, tedchoc@chromium.org

[David Trainor- moved to gerrit, 2017-01-05 22:44:12]

On 2017/01/05 20:32:54, boliu wrote:
> On 2017/01/05 20:27:14, Jinsuk Kim wrote:
> > On 2017/01/05 19:17:15, Ted C wrote:
> > > +mdjones and +dtrainor for larger overlay panel thoughts as well.
> > > 
> > > But, I agree with you Bo.  I don't think they should have their own
> ViewRoot. 
> > > ViewRoot is not supposed to be 1-1 with web contents.  Our ViewRoot is
> > > essentially the CompositorView.  The overlay panel and normal web contents
> are
> > > children of that view and therefore share the same ViewRoot.
> > 
> > From my view ViewRoot is used per content to forward events in place of CVC.
> > Overlay panel comes with its own content that needs events specifically
> > parameterized for it, which I think justifies having its own ViewRoot.
> > It's not clear to me how a ViewRoot can tell which web contents to talk to.
> > Please see CompositorViewHolder where I put a logic that chooses the right
> > ViewRoot for the content it is dealing with to set the physical backing
size.
> > This might require further clarification on how ViewRoot should work
> > in terms of concept before going further.

So, I think this is bit complicated because this is the first time we're really
hitting the case where we want to mix Java event handling and page content event
handling in the same content area.

In my head we have a single ViewRoot, but the ViewAndroid children would be
positioned/ordered so that touch events get propagated to the correct one.  Some
of these ViewAndroids would delegate events to WebContents, some would delegate
to UI logic (Java for now).  We're basically building an event dispatching view
hierarchy right?

Example overlay view hierarchy.  This might not be the best way to structure it,
but IIRC this is how the CC layers are constructed today.  Each of these is a
ViewAndroid, touch handling would check hit testing on the hierarchy until it
finds a match.

Root
 +--- Overlay/Layout Root Thing
           +--- Overlay Page Content (WebContents backed)
           +--- Overlay Top Bar (Java backed)
           +--- Overlay Background (Java backed)
           +--- Underlying Page Content (WebContents backed)

When the Overlay builds it's ViewAndroid hierarchy it needs to build some
ViewAndroids that have Java-backed touch handling delegates.  It can add
WebContents' ViewAndroid nodes wherever it wants.

There are a few tricky parts that we need to support for this (I'm sure this
list isn't comprehensive):
- Making a Java-backed ViewAndroid touch event delegate.
- Pinning gestures to a specific ViewAndroid (if a ViewAndroid gets touch down,
it should get all gestures until touch up).
- Sizing specific WebContents/ViewAndroid.  This is kind of separate, but we
should probably answer this sooner rather than later.  Do we drive the
ViewAndroid size from the rendered frame?  Or do we drive the renderer's size
from the ViewAndroid?

What do you think? :)  Does this make sense?

> 
> So looking that code, the current behavior is that if there's an overlay, then
> the size change is only propagated to the overlay, and not the tab's
webcontents
> below? that seems odd to me, but ok..
> 
> but then the proper way to figure this out is probably something like, tell
the
> viewandroid of the tab's webcontents to *not* propagate size changes (and
> possibly other things in the future)

[David Trainor- moved to gerrit, 2017-01-05 22:45:00]

On 2017/01/05 22:39:38, boliu wrote:
> Ok, gonna continue this review, and remove other reviewers that got added here
> just for the discussion.
> 
> If we have other things to discuss, we'll start a thread instead :p

Oops!

> 
>
https://codereview.chromium.org/2595263002/diff/210038/content/shell/android/...
> File content/shell/android/java/src/org/chromium/content_shell/Shell.java
> (right):
> 
>
https://codereview.chromium.org/2595263002/diff/210038/content/shell/android/...
> content/shell/android/java/src/org/chromium/content_shell/Shell.java:301:
> webContents, ViewRoot.create(mWindow));
> On 2017/01/05 22:30:15, Jinsuk Kim wrote:
> > Shouldn't it be created where 'content' is created?
> 
> No, except webview. If it weren't for webview, ViewRoot and WindowAndroid
would
> be the same class. Which means other than webview, ViewRoot and WindowAndroid
> should have the exact same life time.

[Jinsuk Kim, 2017-01-13 07:00:03]

Patchset #5 (id:290001) has been deleted

[Jinsuk Kim, 2017-01-13 07:40:58]

Description was changed from

==========
Make ViewRoot the top of ViewAndroid tree

This CL introduces a native class ViewRoot, a subclass of ViewAndroid.
It acts as the root of a ViewAndroid tree, and receives touch/view
events from embedders. It has a reference to WindowAndroid, and 
paritially takes over the role it played as the tree root.

WindowAndroid will no longer inherit from ViewAndroid. It effectively
reverts https://crrev.com/2136373002.

Embedders are responsible for passing in a ViewRoot when creating
a new WebContents.

Also fixes a bug caused by using a main content's ViewRoot for
overlay panel's.


BUG=671401, 671967
==========

to

==========
Introduce ViewRoot forwarding input/view events to native

This CL introduces a new interface ViewRoot to forward Java
view/input events down to native with a view to replacing
the flow through ContentViewCore eventually.

In native side ViewRoot is a subclass of ViewAndroid. It acts
as the root of a ViewAndroid tree, and receives touch/view events
from embedders. It has a reference to WindowAndroid, and paritially
takes over the role it played as the tree root. Therefore WindowAndroid
no longer inherits from ViewAndroid. It effectively reverts
https://crrev.com/2136373002.

ViewRoot is mapped 1:1 to WindowAndroid and shared across all the contents.
WebView is an exception where all activities share the WA instance.

ViewRoot, ViewAndroid, and ViewClient are put together to adopt
the pattern "chain of responsibility" dispatching the events to
the classes implementing |ViewClient| along the view hierarchy, and
conditionally stopping the processing when required.

|OnTouchEvent| is used for an example.

BUG=671401
==========

[Jinsuk Kim, 2017-01-13 07:42:21]

Patchset #5 (id:310001) has been deleted

[Jinsuk Kim, 2017-01-13 07:56:12]

Note for patch #5:

- Replaced the example from physical backing size to touch event.
- ViewRoot is created per compositor view, shared by all the contents handled on
it.
- CompositorImpl takes ViewRoot instead of gfx::NativeWindow which is
WindowAndroid. Most of smalle changes in many classes are due to plumbing
related to this.

Please note that touch event was not entirely taken over by ViewRoot but still
goes through CVC. Only because some states/variables used for processing touch
event are shared with other events (key, for instance). The others will be
migrated eventually. This CL aims to show how ViewRoot propagates events to its
children. I think the portion of touch event moved to ViewRoot is enough to show
that.

I'm planning to add basic geometry information like layout, size/position in a
follow-up CL. In this CL ContentView is still in use to receive events from
Android platform based on view geometry. I added a bare minimum logic of
|ViewAndroid| order management to decide the order in which the native views
receive events.

[boliu, 2017-01-13 19:36:23]

On 2017/01/13 07:56:12, Jinsuk Kim wrote:
> Note for patch #5:
> 
> - Replaced the example from physical backing size to touch event.
> - ViewRoot is created per compositor view, shared by all the contents handled
on
> it.
> - CompositorImpl takes ViewRoot instead of gfx::NativeWindow which is
> WindowAndroid. Most of smalle changes in many classes are due to plumbing
> related to this.
> 
> Please note that touch event was not entirely taken over by ViewRoot but still
> goes through CVC. Only because some states/variables used for processing touch
> event are shared with other events (key, for instance). The others will be
> migrated eventually. This CL aims to show how ViewRoot propagates events to
its
> children. I think the portion of touch event moved to ViewRoot is enough to
show
> that.
> 
> I'm planning to add basic geometry information like layout, size/position in a
> follow-up CL. In this CL ContentView is still in use to receive events from
> Android platform based on view geometry. I added a bare minimum logic of
> |ViewAndroid| order management to decide the order in which the native views
> receive events.

I think this is the wrong order to do things. We should add geometry data first,
ie have ViewAndroid mirror java views, then verify that all works. Then move
touch events over.

This CL as it stands doesn't actually do anything, except adding extra steps
between CVC and WCVA/RWHVA

[Jinsuk Kim, 2017-01-16 05:36:02]

On 2017/01/13 19:36:23, boliu wrote:
> > 
> > I'm planning to add basic geometry information like layout, size/position in
a
> > follow-up CL. In this CL ContentView is still in use to receive events from
> > Android platform based on view geometry. I added a bare minimum logic of
> > |ViewAndroid| order management to decide the order in which the native views
> > receive events.
> 
> I think this is the wrong order to do things. We should add geometry data
first,
> ie have ViewAndroid mirror java views, then verify that all works. Then move
> touch events over.
> 
> This CL as it stands doesn't actually do anything, except adding extra steps
> between CVC and WCVA/RWHVA

Added a inner class |Bounds| in |ViewAndroid| that manages the basic layout.
Put it together in this CL since what's already done in here (ViewRoot) was
useful to help test if the hit testing actually works.

ContentView is still in use, so offset for bounds needs to be zero as the event
coordinate is based on the view. Left a couple of TODO's for a reminder to
reflect
the offset from the top/left of the compositor view once ContentView is removed.
Verified the hit testing works as expected with main/overlay contents.

Take another look and let me know if this is the level of view geometry support
you expected in the first CL.

[Jinsuk Kim, 2017-01-16 06:44:38]

Patchset #6 (id:350001) has been deleted

[boliu, 2017-01-17 16:12:19]

On 2017/01/16 05:36:02, Jinsuk Kim wrote:
> On 2017/01/13 19:36:23, boliu wrote:
> > > 
> > > I'm planning to add basic geometry information like layout, size/position
in
> a
> > > follow-up CL. In this CL ContentView is still in use to receive events
from
> > > Android platform based on view geometry. I added a bare minimum logic of
> > > |ViewAndroid| order management to decide the order in which the native
views
> > > receive events.
> > 
> > I think this is the wrong order to do things. We should add geometry data
> first,
> > ie have ViewAndroid mirror java views, then verify that all works. Then move
> > touch events over.
> > 
> > This CL as it stands doesn't actually do anything, except adding extra steps
> > between CVC and WCVA/RWHVA
> 
> Added a inner class |Bounds| in |ViewAndroid| that manages the basic layout.
> Put it together in this CL since what's already done in here (ViewRoot) was
> useful to help test if the hit testing actually works.

Sure adding bounds is fine. But I think routing events through it should come
after we are sure viewandroid layout matches java views exactly. Flipping the
order just doesn't make sense to me, because then you have to get the layout
exactly right the first time, otherwise you'll be breaking something.

If you want to see if hit testing works, you can start writing unit tests for
view android tree, which given the amount of logic it will handle, is probably a
good idea now.

> 
> ContentView is still in use, so offset for bounds needs to be zero as the
event
> coordinate is based on the view. Left a couple of TODO's for a reminder to
> reflect
> the offset from the top/left of the compositor view once ContentView is
removed.
> Verified the hit testing works as expected with main/overlay contents.
> 
> Take another look and let me know if this is the level of view geometry
support
> you expected in the first CL.

[boliu, 2017-01-17 16:17:28]

a few comments on view_android

https://codereview.chromium.org/2595263002/diff/370001/ui/android/view_androi...
File ui/android/view_android.cc (right):

https://codereview.chromium.org/2595263002/diff/370001/ui/android/view_androi...
ui/android/view_android.cc:133: child->SetBounds(origin, Bounds::kMatchParent,
Bounds::kMatchParent);
this makes no sense to me, why are children automatically switched to
matchparent?

https://codereview.chromium.org/2595263002/diff/370001/ui/android/view_androi...
ui/android/view_android.cc:227: if (child->OnTouchEventInternal(event,
is_touch_handle_event))
when recursing, need to translate the event by -origin

https://codereview.chromium.org/2595263002/diff/370001/ui/android/view_android.h
File ui/android/view_android.h (right):

https://codereview.chromium.org/2595263002/diff/370001/ui/android/view_androi...
ui/android/view_android.h:61: class Bounds {
there's gfx::Rect, don't need to create your own class

[Jinsuk Kim, 2017-01-17 23:32:48]

> Sure adding bounds is fine. But I think routing events through it should come
> after we are sure viewandroid layout matches java views exactly. Flipping the
> order just doesn't make sense to me, because then you have to get the layout
> exactly right the first time, otherwise you'll be breaking something.

Maybe I'm missing something - I'm not clear how different you want it to be
handled. The way it is doing now is that ViewRoot is passed touch events, then
it passes them to each child which performs hit testing using its bounds to
check if it should handle them, which I believe is as expected. What is
'flipped'?

https://codereview.chromium.org/2595263002/diff/370001/ui/android/view_androi...
File ui/android/view_android.cc (right):

https://codereview.chromium.org/2595263002/diff/370001/ui/android/view_androi...
ui/android/view_android.cc:133: child->SetBounds(origin, Bounds::kMatchParent,
Bounds::kMatchParent);
On 2017/01/17 16:17:28, boliu wrote:
> this makes no sense to me, why are children automatically switched to
> matchparent?

VA for RWHVA is supposed to have the same layout as that of WCVA. So if the
event matches WCVA then it should also match RWHVA.

https://codereview.chromium.org/2595263002/diff/370001/ui/android/view_androi...
ui/android/view_android.cc:227: if (child->OnTouchEventInternal(event,
is_touch_handle_event))
On 2017/01/17 16:17:28, boliu wrote:
> when recursing, need to translate the event by -origin

As I mentioned in the TODO, origin remains zero at this point. Will reflect it
as the TODO's are resolved.

https://codereview.chromium.org/2595263002/diff/370001/ui/android/view_android.h
File ui/android/view_android.h (right):

https://codereview.chromium.org/2595263002/diff/370001/ui/android/view_androi...
ui/android/view_android.h:61: class Bounds {
On 2017/01/17 16:17:28, boliu wrote:
> there's gfx::Rect, don't need to create your own class

Used it at first but switched to my own in order to be able to let
width_/height_ have meta param like kMatchParent. Could have gfx::Rect plus
separate variables for those meta param but then I think it's just a matter of
choices.

[Jinsuk Kim, 2017-01-18 02:38:20]

Chatted offline and clarified a few things. Will introduce a command line
switch, implement ViewAndroid layout that mirrors Java view first. Changes will
be put behind the switch.

[Jinsuk Kim, 2017-01-23 08:54:52]

Description was changed from

==========
Introduce ViewRoot forwarding input/view events to native

This CL introduces a new interface ViewRoot to forward Java
view/input events down to native with a view to replacing
the flow through ContentViewCore eventually.

In native side ViewRoot is a subclass of ViewAndroid. It acts
as the root of a ViewAndroid tree, and receives touch/view events
from embedders. It has a reference to WindowAndroid, and paritially
takes over the role it played as the tree root. Therefore WindowAndroid
no longer inherits from ViewAndroid. It effectively reverts
https://crrev.com/2136373002.

ViewRoot is mapped 1:1 to WindowAndroid and shared across all the contents.
WebView is an exception where all activities share the WA instance.

ViewRoot, ViewAndroid, and ViewClient are put together to adopt
the pattern "chain of responsibility" dispatching the events to
the classes implementing |ViewClient| along the view hierarchy, and
conditionally stopping the processing when required.

|OnTouchEvent| is used for an example.

BUG=671401
==========

to

==========
Introduce ViewRoot forwarding input/view events to native

This CL introduces a new interface ViewRoot to forward Java
view/input events down to native with a view to replacing
the flow through ContentViewCore eventually.

In native side ViewRoot is a subclass of ViewAndroid. It acts
as the root of a ViewAndroid tree, and receives touch/view events
from embedders. It has a reference to WindowAndroid, and paritially
takes over the role it played as the tree root. Therefore WindowAndroid
no longer inherits from ViewAndroid. It effectively reverts
https://crrev.com/2136373002.

ViewRoot is mapped 1:1 to WindowAndroid and shared across all the contents.
WebView is an exception where all activities share the WA instance.

ViewRoot, ViewAndroid, and ViewClient are put together to adopt
the pattern "chain of responsibility" dispatching the events to
the classes implementing |ViewClient| along the view hierarchy, and
conditionally stopping the processing when required.

BUG=671401
==========

[Jinsuk Kim, 2017-01-23 09:00:14]

Updates:
- Introduced a new command line switch
- Set the bounds of ViewAndroid correctly mirroring that of Java ContentView.
Added unit tests.

ViewRoot is not in action for any events yet, though |OnTouchEvent| is in the
new interface. Will keep the upcoming changes behind the command line switch.
Let me know if you want this CL to be split into multiple ones. I included the
ViewAndroid layout/bounds in this CL because ViewRoot makes it more convenient
to do unit tests for ViewAndroid hit testing.

[boliu, 2017-01-23 21:27:19]

On 2017/01/23 09:00:14, Jinsuk Kim wrote:
> Updates:
> - Introduced a new command line switch
> - Set the bounds of ViewAndroid correctly mirroring that of Java ContentView.
> Added unit tests.
> 
> ViewRoot is not in action for any events yet, though |OnTouchEvent| is in the
> new interface. Will keep the upcoming changes behind the command line switch.
> Let me know if you want this CL to be split into multiple ones. I included the
> ViewAndroid layout/bounds in this CL because ViewRoot makes it more convenient
> to do unit tests for ViewAndroid hit testing.

If we have a command line switch, we no longer need to have everything in a
single CL. Is it possible to break this up into smaller pieces to make reviewing
easier?