NOT FOR REVIEW: Aura on Android - content shell compiles

NOT FOR REVIEW: Aura on Android - content shell compiles

This is on top of the changes in https://codereview.chromium.org/1256053002/

Major changes:
1. Using shell_views, got rid of shell_android.cc.
2. Got rid of ShellManager, and stripped down Shell.java. I was planning to potentially expose the native Shell::CreateNewWindow() method through Shell.java since we don't have ShellManager::launchSHell anymore - see review comment in ContentShellActivity.finishInitialization.
3. BrowserStartupController sets Activity as the "application context". This is done so that PlatformWindowAndroid::Show() can initialize the Java PlatformWindowAndroid, which sets itself as the content view on the activity.
4. Added WindowTreeHostAndroid - basically a copy of WindowTreeHostOzone with a couple of small changes.


BUG=513795

[mfomitchev, 2015-08-12 22:18:46]

https://codereview.chromium.org/1284343005/diff/1/content/shell/android/aura_...
File content/shell/android/aura_shell_apk/res/layout/content_shell_activity.xml
(right):

https://codereview.chromium.org/1284343005/diff/1/content/shell/android/aura_...
content/shell/android/aura_shell_apk/res/layout/content_shell_activity.xml:10:
<org.chromium.aura_content_shell_apk.ShellManager
Ok, using ShellManager here is clearly wrong. I assume we can probably just get
rid of this layout file since PlatformWindowAndroid is going to set itself as
the content view anyway.

https://codereview.chromium.org/1284343005/diff/1/content/shell/android/aura_...
File
content/shell/android/aura_shell_apk/src/org/chromium/aura_content_shell_apk/ContentShellActivity.java
(right):

https://codereview.chromium.org/1284343005/diff/1/content/shell/android/aura_...
content/shell/android/aura_shell_apk/src/org/chromium/aura_content_shell_apk/ContentShellActivity.java:133:
//        mShellManager.launchShell(shellUrl);
So.. I was thinking whether we need to Shell::CreateNewWindow here?
The shell is also launched via ShellBrowserMainParts::PreMainMessageLoopRun() ->
ShellBrowserMainParts::InitializeMessageLoopContext() ->
Shell::CreateNewWindow.. So I don't fully understand if this is also needed?

[Yaron, 2015-08-13 13:40:06]

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

[Yaron, 2015-08-13 13:40:07]

https://codereview.chromium.org/1284343005/diff/1/content/shell/android/aura_...
File
content/shell/android/aura_shell_apk/src/org/chromium/aura_content_shell_apk/ContentShellActivity.java
(right):

https://codereview.chromium.org/1284343005/diff/1/content/shell/android/aura_...
content/shell/android/aura_shell_apk/src/org/chromium/aura_content_shell_apk/ContentShellActivity.java:133:
//        mShellManager.launchShell(shellUrl);
On 2015/08/12 22:18:46, mfomitchev wrote:
> So.. I was thinking whether we need to Shell::CreateNewWindow here?
> The shell is also launched via ShellBrowserMainParts::PreMainMessageLoopRun()
->
> ShellBrowserMainParts::InitializeMessageLoopContext() ->
> Shell::CreateNewWindow.. So I don't fully understand if this is also needed?

It could be that we don't need this at all. This call is a callback from
BrowserStartupController.StartupCallback. The functions you're referencing were
just executed so it could be that in this flow, the callback is unnecesary and
we can do it all as part of native initialization. However, ShellManager might
have the wrong state in that case.

[mfomitchev, 2015-08-13 15:33:04]

https://codereview.chromium.org/1284343005/diff/1/content/shell/android/aura_...
File
content/shell/android/aura_shell_apk/src/org/chromium/aura_content_shell_apk/ContentShellActivity.java
(right):

https://codereview.chromium.org/1284343005/diff/1/content/shell/android/aura_...
content/shell/android/aura_shell_apk/src/org/chromium/aura_content_shell_apk/ContentShellActivity.java:133:
//        mShellManager.launchShell(shellUrl);
Let me see if I understand this correctly. 

First we have ContentShellActivity::onCreate() called. This calls
BrowserStartupController.startBrowserProcessesAsync. That in turn asynchronously
does resource extraction and some other setup stuff. After that completes we set
ApplicationContext to be the Activity. And only after this completes will we
call BrowserMain, which would call
ShellBrowserMainParts::InitializeMessageLoopContext() and subsequently
Shell::CreateNewWindow?

If this is accurate, then Shell::CreateNewWindow is guaranteed to be called
after the activity is set as the ApplicationContext (and never earlier), so
PlatformWindowAndroid should get initialized correctly and set itself as the
activity's ContentView. So yeah - doesn't seem like this is needed. And we don't
have ShellManager, so we don't have to worry about it's state. In case we need
to update the state somewhere else (Shell?), perhaps we can override 
setContentView? Although I don't see the need the Java Shell class either at
this point..

[sadrul, 2015-08-13 18:31:28]

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

[sadrul, 2015-08-13 18:31:28]

non-review comment:

https://codereview.chromium.org/1284343005/diff/1/ui/aura/window_tree_host_an...
File ui/aura/window_tree_host_android.h (right):

https://codereview.chromium.org/1284343005/diff/1/ui/aura/window_tree_host_an...
ui/aura/window_tree_host_android.h:64:
DISALLOW_COPY_AND_ASSIGN(WindowTreeHostAndroid);
We should, of course, have a single WindowTreeHostCommon implementation that
uses a PlatformWindow (instead of three for ozone, win, and android).

[Yaron, 2015-08-13 18:54:03]

https://codereview.chromium.org/1284343005/diff/1/content/shell/android/aura_...
File
content/shell/android/aura_shell_apk/src/org/chromium/aura_content_shell_apk/ContentShellActivity.java
(right):

https://codereview.chromium.org/1284343005/diff/1/content/shell/android/aura_...
content/shell/android/aura_shell_apk/src/org/chromium/aura_content_shell_apk/ContentShellActivity.java:133:
//        mShellManager.launchShell(shellUrl);
On 2015/08/13 15:33:03, mfomitchev wrote:
> Let me see if I understand this correctly. 
> 
> First we have ContentShellActivity::onCreate() called. This calls
> BrowserStartupController.startBrowserProcessesAsync. That in turn
asynchronously
> does resource extraction and some other setup stuff. After that completes we
set
> ApplicationContext to be the Activity. And only after this completes will we
> call BrowserMain, which would call
> ShellBrowserMainParts::InitializeMessageLoopContext() and subsequently
> Shell::CreateNewWindow?

I think so, ya. It looks like BrowserStartupController invokes contentStart
which calls ContentMain.start which AFAIK results in BrowserMain.
> 
> If this is accurate, then Shell::CreateNewWindow is guaranteed to be called
> after the activity is set as the ApplicationContext (and never earlier), so
> PlatformWindowAndroid should get initialized correctly and set itself as the
> activity's ContentView. So yeah - doesn't seem like this is needed. And we
don't
> have ShellManager, so we don't have to worry about it's state. In case we need
> to update the state somewhere else (Shell?), perhaps we can override 
> setContentView? Although I don't see the need the Java Shell class either at
> this point..

It could be that it does less but it might still be needed. Something needs to
own and track the current WebContents/ContentViewCore

[mfomitchev, 2015-08-13 21:08:23]

https://codereview.chromium.org/1284343005/diff/1/content/shell/android/aura_...
File
content/shell/android/aura_shell_apk/src/org/chromium/aura_content_shell_apk/ContentShellActivity.java
(right):

https://codereview.chromium.org/1284343005/diff/1/content/shell/android/aura_...
content/shell/android/aura_shell_apk/src/org/chromium/aura_content_shell_apk/ContentShellActivity.java:133:
//        mShellManager.launchShell(shellUrl);
On 2015/08/13 18:54:03, Yaron wrote:
> On 2015/08/13 15:33:03, mfomitchev wrote:
> > Let me see if I understand this correctly. 
> > 
> > First we have ContentShellActivity::onCreate() called. This calls
> > BrowserStartupController.startBrowserProcessesAsync. That in turn
> asynchronously
> > does resource extraction and some other setup stuff. After that completes we
> set
> > ApplicationContext to be the Activity. And only after this completes will we
> > call BrowserMain, which would call
> > ShellBrowserMainParts::InitializeMessageLoopContext() and subsequently
> > Shell::CreateNewWindow?
> 
> I think so, ya. It looks like BrowserStartupController invokes contentStart
> which calls ContentMain.start which AFAIK results in BrowserMain.
> > 
> > If this is accurate, then Shell::CreateNewWindow is guaranteed to be called
> > after the activity is set as the ApplicationContext (and never earlier), so
> > PlatformWindowAndroid should get initialized correctly and set itself as the
> > activity's ContentView. So yeah - doesn't seem like this is needed. And we
> don't
> > have ShellManager, so we don't have to worry about it's state. In case we
need
> > to update the state somewhere else (Shell?), perhaps we can override 
> > setContentView? Although I don't see the need the Java Shell class either at
> > this point..
> 
> It could be that it does less but it might still be needed. Something needs to
> own and track the current WebContents/ContentViewCore

At this point I don't even have ContentViewCore.. and I don't have
web_contents_android.cc - not sure if this means I also don't have
WebContents.java - where is that controlled?
It seemed to me most of the stuff in there was for stuff we don't need or for
functionality we could do without for the initital content shell prototype.

https://codereview.chromium.org/1284343005/diff/1/ui/aura/window_tree_host_an...
File ui/aura/window_tree_host_android.h (right):

https://codereview.chromium.org/1284343005/diff/1/ui/aura/window_tree_host_an...
ui/aura/window_tree_host_android.h:64:
DISALLOW_COPY_AND_ASSIGN(WindowTreeHostAndroid);
On 2015/08/13 18:31:28, sadrul wrote:
> We should, of course, have a single WindowTreeHostCommon implementation that
> uses a PlatformWindow (instead of three for ozone, win, and android).

Oh yeah, for sure. The only real difference between WTHAndroid and WTHOzone is
the constructor, and it seems WindowTreeHostWin is pretty similar as well. Do
you suggest we put it all in the same .cc file and just ifdef differing pieces?

[sadrul, 2015-08-13 21:33:20]

https://codereview.chromium.org/1284343005/diff/1/ui/aura/window_tree_host_an...
File ui/aura/window_tree_host_android.h (right):

https://codereview.chromium.org/1284343005/diff/1/ui/aura/window_tree_host_an...
ui/aura/window_tree_host_android.h:64:
DISALLOW_COPY_AND_ASSIGN(WindowTreeHostAndroid);
On 2015/08/13 21:08:23, mfomitchev wrote:
> On 2015/08/13 18:31:28, sadrul wrote:
> > We should, of course, have a single WindowTreeHostCommon implementation that
> > uses a PlatformWindow (instead of three for ozone, win, and android).
> 
> Oh yeah, for sure. The only real difference between WTHAndroid and WTHOzone is
> the constructor, and it seems WindowTreeHostWin is pretty similar as well. Do
> you suggest we put it all in the same .cc file and just ifdef differing
pieces?

I think so, yeah. I played with the idea of introducing a PlatformWindowFactory
a long while back (https://codereview.chromium.org/403263002/), but we currently
do #ifdefs for view_manager
(https://code.google.com/p/chromium/codesearch#chromium/src/components/view_ma...).
We can do similar things here too.

[Yaron, 2015-08-14 15:10:24]

https://codereview.chromium.org/1284343005/diff/1/content/shell/android/aura_...
File
content/shell/android/aura_shell_apk/src/org/chromium/aura_content_shell_apk/ContentShellActivity.java
(right):

https://codereview.chromium.org/1284343005/diff/1/content/shell/android/aura_...
content/shell/android/aura_shell_apk/src/org/chromium/aura_content_shell_apk/ContentShellActivity.java:133:
//        mShellManager.launchShell(shellUrl);
On 2015/08/13 21:08:23, mfomitchev wrote:
> On 2015/08/13 18:54:03, Yaron wrote:
> > On 2015/08/13 15:33:03, mfomitchev wrote:
> > > Let me see if I understand this correctly. 
> > > 
> > > First we have ContentShellActivity::onCreate() called. This calls
> > > BrowserStartupController.startBrowserProcessesAsync. That in turn
> > asynchronously
> > > does resource extraction and some other setup stuff. After that completes
we
> > set
> > > ApplicationContext to be the Activity. And only after this completes will
we
> > > call BrowserMain, which would call
> > > ShellBrowserMainParts::InitializeMessageLoopContext() and subsequently
> > > Shell::CreateNewWindow?
> > 
> > I think so, ya. It looks like BrowserStartupController invokes contentStart
> > which calls ContentMain.start which AFAIK results in BrowserMain.
> > > 
> > > If this is accurate, then Shell::CreateNewWindow is guaranteed to be
called
> > > after the activity is set as the ApplicationContext (and never earlier),
so
> > > PlatformWindowAndroid should get initialized correctly and set itself as
the
> > > activity's ContentView. So yeah - doesn't seem like this is needed. And we
> > don't
> > > have ShellManager, so we don't have to worry about it's state. In case we
> need
> > > to update the state somewhere else (Shell?), perhaps we can override 
> > > setContentView? Although I don't see the need the Java Shell class either
at
> > > this point..
> > 
> > It could be that it does less but it might still be needed. Something needs
to
> > own and track the current WebContents/ContentViewCore
> 
> At this point I don't even have ContentViewCore.. and I don't have
> web_contents_android.cc - not sure if this means I also don't have
> WebContents.java - where is that controlled?

Ya, you don't have it it. It's created on demand in WebContentsImpl when asked
for the java counterpart.

> It seemed to me most of the stuff in there was for stuff we don't need or for
> functionality we could do without for the initital content shell prototype.

Ya, you might be right. If Aura is going to use Browser and chrome/browser/ui
you probably won't.

[no sievers, 2015-08-15 01:21:39]

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

[no sievers, 2015-08-15 01:21:40]

https://codereview.chromium.org/1284343005/diff/1/ui/aura/window_tree_host_an...
File ui/aura/window_tree_host_android.cc (right):

https://codereview.chromium.org/1284343005/diff/1/ui/aura/window_tree_host_an...
ui/aura/window_tree_host_android.cc:108: CreateCompositor(widget_);
Generally this might be good enough to get you going with --in-process-gpu.
But see comments in 513540 - I don't think we can really defer the compositor
creation until here.