Make ViewRoot the top of the ViewAndroid tree

Make ViewRoot the top of the ViewAndroid tree

ViewRoot takes over the role of WindowAndroid as the top of
view android tree hierarchy. WindowAndroid acts as Android
implementation of activity window only.

Also added |GerNearestDisplayWindowAndroid| to get around
the change that WindowAndroid is not inheriting
ViewAndroid(gfx::NativeView) any more.

BUG=671401

[Jinsuk Kim, 2017-02-10 10:10:10]

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

[commit-bot: I haz the power, 2017-02-10 10:10:22]

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-02-10 10:18:40]

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

[commit-bot: I haz the power, 2017-02-10 10:18:41]

Dry run: 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...)

[Jinsuk Kim, 2017-02-10 11:25:46]

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

[commit-bot: I haz the power, 2017-02-10 11:26:01]

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-02-10 11:33:27]

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

[commit-bot: I haz the power, 2017-02-10 11:33:28]

Dry run: 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...)

[Jinsuk Kim, 2017-02-11 04:16:30]

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

[commit-bot: I haz the power, 2017-02-11 04:17:01]

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-02-11 04:24:24]

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

[commit-bot: I haz the power, 2017-02-11 04:24:24]

Dry run: 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...)

[Jinsuk Kim, 2017-02-12 11:38:48]

Patchset #1 (id:1) has been deleted

[Jinsuk Kim, 2017-02-12 11:39:01]

Patchset #1 (id:20001) has been deleted

[Jinsuk Kim, 2017-02-12 11:39:12]

Patchset #1 (id:40001) has been deleted

[Jinsuk Kim, 2017-02-12 11:39:25]

Patchset #1 (id:60001) has been deleted

[Jinsuk Kim, 2017-02-12 11:40:16]

Description was changed from

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

ViewRoot takes over the role of WindowAndroid as the top of
view android tree hierarchy.

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

to

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

ViewRoot takes over the role of WindowAndroid as the top of
view android tree hierarchy. WindowAndroid acts as Android
implementation of activity window only.

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

[Jinsuk Kim, 2017-02-12 11:44:14]

Description was changed from

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

ViewRoot takes over the role of WindowAndroid as the top of
view android tree hierarchy. WindowAndroid acts as Android
implementation of activity window only.

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

to

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

ViewRoot takes over the role of WindowAndroid as the top of
view android tree hierarchy. WindowAndroid acts as Android
implementation of activity window only.

Also added |GerNearestDisplayWindowAndroid| to get around
the change that WindowAndroid is not inheriting
gfx::NativeWindow any more.

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

[Jinsuk Kim, 2017-02-12 11:57:17]

Description was changed from

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

ViewRoot takes over the role of WindowAndroid as the top of
view android tree hierarchy. WindowAndroid acts as Android
implementation of activity window only.

Also added |GerNearestDisplayWindowAndroid| to get around
the change that WindowAndroid is not inheriting
gfx::NativeWindow any more.

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

to

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

ViewRoot takes over the role of WindowAndroid as the top of
view android tree hierarchy. WindowAndroid acts as Android
implementation of activity window only.

Also added |GerNearestDisplayWindowAndroid| to get around
the change that WindowAndroid is not inheriting
ViewAndroid(gfx::NativeView) any more.

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

[Jinsuk Kim, 2017-02-12 11:58:13]

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

[Jinsuk Kim, 2017-02-12 11:58:14]

Note: Changes many files in chrome/ is for plumbing that passes ViewRoot 
instead of WindowAndroid (gfx::NativeWindow) to Compositor::Create.

[boliu, 2017-02-13 22:58:07]

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

[boliu, 2017-02-13 22:58:09]

overall pretty good

+oshima for ui/display, I have a question on screen.h

https://codereview.chromium.org/2688113002/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorObserverTestBase.java
(right):

https://codereview.chromium.org/2688113002/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorObserverTestBase.java:49:
mViewRoot = ViewRoot.create(windowAndroid);
ditto existing bug that's missing destroy

https://codereview.chromium.org/2688113002/diff/80001/chromecast/browser/andr...
File
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsActivity.java
(right):

https://codereview.chromium.org/2688113002/diff/80001/chromecast/browser/andr...
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsActivity.java:109:
mViewRoot = ViewRoot.create(mWindow);
errrrr, missing destroy

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

https://codereview.chromium.org/2688113002/diff/80001/content/browser/android...
content/browser/android/content_view_core_impl.cc:233:
GetViewAndroid()->SetLayer(cc::Layer::Create());
this code can just stay here, right? nothing here requires root/window?

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

https://codereview.chromium.org/2688113002/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:620:
WindowAndroid windowAndroid = viewRoot == null ? null :
viewRoot.getWindowAndroid();
nit: move this next to where viewRootPointer is defined

https://codereview.chromium.org/2688113002/diff/80001/content/public/browser/...
File content/public/browser/android/compositor.h (right):

https://codereview.chromium.org/2688113002/diff/80001/content/public/browser/...
content/public/browser/android/compositor.h:14: #include
"ui/gfx/native_widget_types.h"
still need this?

https://codereview.chromium.org/2688113002/diff/80001/content/shell/android/l...
File
content/shell/android/linker_test_apk/src/org/chromium/chromium_linker_test_apk/ChromiumLinkerTestActivity.java
(right):

https://codereview.chromium.org/2688113002/diff/80001/content/shell/android/l...
content/shell/android/linker_test_apk/src/org/chromium/chromium_linker_test_apk/ChromiumLinkerTestActivity.java:139:
mViewRoot = ViewRoot.create(mWindowAndroid);
this is an existing bug that the windowandroid here is never destroyed, but
should perhaps to find a place to destroy both window and viewroot?

https://codereview.chromium.org/2688113002/diff/80001/ui/android/DEPS
File ui/android/DEPS (right):

https://codereview.chromium.org/2688113002/diff/80001/ui/android/DEPS#newcode16
ui/android/DEPS:16: "+ui/events/android/motion_event_android.h",
hmm, why is this needed now?

https://codereview.chromium.org/2688113002/diff/80001/ui/android/display_andr...
File ui/android/display_android_manager.h (right):

https://codereview.chromium.org/2688113002/diff/80001/ui/android/display_andr...
ui/android/display_android_manager.h:17: class UI_ANDROID_EXPORT
DisplayAndroidManager : public display::ScreenBase {
does this need to be made public, if GetDisplayNearestWindowAndroid is being
added to Screen already?

https://codereview.chromium.org/2688113002/diff/80001/ui/display/screen.h
File ui/display/screen.h (right):

https://codereview.chromium.org/2688113002/diff/80001/ui/display/screen.h#new...
ui/display/screen.h:65: virtual display::Display GetDisplayNearestWindowAndroid(
just Android part in this name and overload the method. style guide more or less
allows this kind of overloading

also maybe instead of OS_ANDROID, have a WINDOW_IS_NOT_VIEW define? although I
dunno if that works for all platforms yet..

oshima: wdyt?

[oshima, 2017-02-14 00:26:43]

oshima@chromium.org changed reviewers:
+ benwells@chromium.org, tapted@chromium.org

[oshima, 2017-02-14 00:26:44]

https://codereview.chromium.org/2688113002/diff/80001/ui/display/screen.h
File ui/display/screen.h (right):

https://codereview.chromium.org/2688113002/diff/80001/ui/display/screen.h#new...
ui/display/screen.h:65: virtual display::Display GetDisplayNearestWindowAndroid(
On 2017/02/13 22:58:08, boliu wrote:
> just Android part in this name and overload the method. style guide more or
less
> allows this kind of overloading
> 
> also maybe instead of OS_ANDROID, have a WINDOW_IS_NOT_VIEW define? although I
> dunno if that works for all platforms yet..
> 
> oshima: wdyt?
+tapted@, benwells@

Maybe we should change this to NativeWindow to match the description and
name.They're the same on Aura. Only other platform they differ is mac/iOS. I
looked up mac code, and it may not be hard to change.

Ben, Trent, what do you think?

[tapted, 2017-02-14 00:58:00]

https://codereview.chromium.org/2688113002/diff/80001/ui/display/screen.h
File ui/display/screen.h (right):

https://codereview.chromium.org/2688113002/diff/80001/ui/display/screen.h#new...
ui/display/screen.h:65: virtual display::Display GetDisplayNearestWindowAndroid(
On 2017/02/14 00:26:44, oshima wrote:
> On 2017/02/13 22:58:08, boliu wrote:
> > just Android part in this name and overload the method. style guide more or
> less
> > allows this kind of overloading
> > 
> > also maybe instead of OS_ANDROID, have a WINDOW_IS_NOT_VIEW define? although
I
> > dunno if that works for all platforms yet..
> > 
> > oshima: wdyt?
> +tapted@, benwells@
> 
> Maybe we should change this to NativeWindow to match the description and
> name.They're the same on Aura. Only other platform they differ is mac/iOS. I
> looked up mac code, and it may not be hard to change.
> 
> Ben, Trent, what do you think?


Yeah in screen_mac this always uses [view window] -- there's no attempt to see
whether a subview is actually on a different display for the case where a window
spans multiple displays. So switching GetDisplayNearestWindow to take a
gfx::NativeWindow should work fine.



(Also, when this comes up on Mac, there is stuff in
chrome/browser/platform_util.h to help:

gfx::NativeWindow GetTopLevel(gfx::NativeView view);
gfx::NativeView GetViewForWindow(gfx::NativeWindow window);

in platform_util_android.cc, GetTopLevel() is `NOTIMPLEMENTED()` but returns
something anyway

GetViewForWindow() on android might be a missing symbol

I don't really see a problem with moving these two methods to ui/gfx if that's
needed. E.g., if we needed/wanted to provide a non-virtual overload to help with
this change (since you can't passing `[view window]` to satisfy mac requires
Objective-C which can't be put in a .cc file).  see also
http://crbug.com/327611#c9

[Jinsuk Kim, 2017-02-14 05:09:59]

https://codereview.chromium.org/2688113002/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorObserverTestBase.java
(right):

https://codereview.chromium.org/2688113002/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorObserverTestBase.java:49:
mViewRoot = ViewRoot.create(windowAndroid);
On 2017/02/13 22:58:08, boliu wrote:
> ditto existing bug that's missing destroy

Done.

https://codereview.chromium.org/2688113002/diff/80001/chromecast/browser/andr...
File
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsActivity.java
(right):

https://codereview.chromium.org/2688113002/diff/80001/chromecast/browser/andr...
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsActivity.java:109:
mViewRoot = ViewRoot.create(mWindow);
On 2017/02/13 22:58:08, boliu wrote:
> errrrr, missing destroy

Done. Put it in onDestroy()

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

https://codereview.chromium.org/2688113002/diff/80001/content/browser/android...
content/browser/android/content_view_core_impl.cc:233:
GetViewAndroid()->SetLayer(cc::Layer::Create());
On 2017/02/13 22:58:08, boliu wrote:
> this code can just stay here, right? nothing here requires root/window?

Yes I just wanted to keep all the initialization/setup for ViewAndroid/ViewRoot
in one place in Init(). Let me know if you want this back here.

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

https://codereview.chromium.org/2688113002/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:620:
WindowAndroid windowAndroid = viewRoot == null ? null :
viewRoot.getWindowAndroid();
On 2017/02/13 22:58:08, boliu wrote:
> nit: move this next to where viewRootPointer is defined

Done.

https://codereview.chromium.org/2688113002/diff/80001/content/public/browser/...
File content/public/browser/android/compositor.h (right):

https://codereview.chromium.org/2688113002/diff/80001/content/public/browser/...
content/public/browser/android/compositor.h:14: #include
"ui/gfx/native_widget_types.h"
On 2017/02/13 22:58:08, boliu wrote:
> still need this?

Removed and included <jni.h> that was included indirectly for |jobject|

https://codereview.chromium.org/2688113002/diff/80001/content/shell/android/l...
File
content/shell/android/linker_test_apk/src/org/chromium/chromium_linker_test_apk/ChromiumLinkerTestActivity.java
(right):

https://codereview.chromium.org/2688113002/diff/80001/content/shell/android/l...
content/shell/android/linker_test_apk/src/org/chromium/chromium_linker_test_apk/ChromiumLinkerTestActivity.java:139:
mViewRoot = ViewRoot.create(mWindowAndroid);
On 2017/02/13 22:58:08, boliu wrote:
> this is an existing bug that the windowandroid here is never destroyed, but
> should perhaps to find a place to destroy both window and viewroot?

Done. Added onDestroy() for that.

https://codereview.chromium.org/2688113002/diff/80001/ui/android/DEPS
File ui/android/DEPS (right):

https://codereview.chromium.org/2688113002/diff/80001/ui/android/DEPS#newcode16
ui/android/DEPS:16: "+ui/events/android/motion_event_android.h",
On 2017/02/13 22:58:08, boliu wrote:
> hmm, why is this needed now?

Should have been removed since I switched to using MotionEventData in ViewRoot.
Done.

https://codereview.chromium.org/2688113002/diff/80001/ui/android/display_andr...
File ui/android/display_android_manager.h (right):

https://codereview.chromium.org/2688113002/diff/80001/ui/android/display_andr...
ui/android/display_android_manager.h:17: class UI_ANDROID_EXPORT
DisplayAndroidManager : public display::ScreenBase {
On 2017/02/13 22:58:08, boliu wrote:
> does this need to be made public, if GetDisplayNearestWindowAndroid is being
> added to Screen already?

Not any more. Removed.

https://codereview.chromium.org/2688113002/diff/80001/ui/display/screen.h
File ui/display/screen.h (right):

https://codereview.chromium.org/2688113002/diff/80001/ui/display/screen.h#new...
ui/display/screen.h:65: virtual display::Display GetDisplayNearestWindowAndroid(
On 2017/02/14 00:58:00, tapted wrote:
> On 2017/02/14 00:26:44, oshima wrote:
> > On 2017/02/13 22:58:08, boliu wrote:
> > > just Android part in this name and overload the method. style guide more
or
> > less
> > > allows this kind of overloading
> > > 
> > > also maybe instead of OS_ANDROID, have a WINDOW_IS_NOT_VIEW define?
although
> I
> > > dunno if that works for all platforms yet..
> > > 
> > > oshima: wdyt?
> > +tapted@, benwells@
> > 
> > Maybe we should change this to NativeWindow to match the description and
> > name.They're the same on Aura. Only other platform they differ is mac/iOS. I
> > looked up mac code, and it may not be hard to change.
> > 
> > Ben, Trent, what do you think?
> 
> 
> Yeah in screen_mac this always uses [view window] -- there's no attempt to see
> whether a subview is actually on a different display for the case where a
window
> spans multiple displays. So switching GetDisplayNearestWindow to take a
> gfx::NativeWindow should work fine.
> 
> 
> 
> (Also, when this comes up on Mac, there is stuff in
> chrome/browser/platform_util.h to help:
> 
> gfx::NativeWindow GetTopLevel(gfx::NativeView view);
> gfx::NativeView GetViewForWindow(gfx::NativeWindow window);
> 
> in platform_util_android.cc, GetTopLevel() is `NOTIMPLEMENTED()` but returns
> something anyway
> 
> GetViewForWindow() on android might be a missing symbol
> 
> I don't really see a problem with moving these two methods to ui/gfx if that's
> needed. E.g., if we needed/wanted to provide a non-virtual overload to help
with
> this change (since you can't passing `[view window]` to satisfy mac requires
> Objective-C which can't be put in a .cc file).  see also
> http://crbug.com/327611#c9

I think it is probably better to take this part out of this CL and create a new
one replacing gfx::NativeView with gfx::NativeWindow for
GetDisplayNearestWindow. Let me do that.

[boliu, 2017-02-14 16:44:14]

I guess this needs to wait on that other CL now.

also probably should add mthiesse for the vr_shell stuff. That code confuses
me..

https://codereview.chromium.org/2688113002/diff/120001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorObserverTestBase.java
(right):

https://codereview.chromium.org/2688113002/diff/120001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorObserverTestBase.java:49:
mViewRoot.destroy();
probably should be runOnUiThreadBlocking to match setUp

[mthiesse, 2017-02-14 16:54:27]

mthiesse@chromium.org changed reviewers:
+ mthiesse@chromium.org

[mthiesse, 2017-02-14 16:54:28]

vr_shell/ lgtm with some minor comments.

https://codereview.chromium.org/2688113002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
(right):

https://codereview.chromium.org/2688113002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:96:
private ContentViewCore mContentCVC;
Remove mContentCVC

https://codereview.chromium.org/2688113002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:347:
mContentCVC.onAttachedToWindow();
mContentCVC is gone on ToT, we no longer rely on a CVC always existing.

Remove this line and the one below.