Build mojo_shell on Android

Build mojo_shell on Android

This CL contains a basic skeleton for building mojo_shell on Android. I've
introduced a mojo_shell_apk target that builds an APK that prints a simple
message and loads libmojo_shell.so. Currently libmojo_shell doesn't do
anything. A future CL will do write up something interested on the C++ side.

R=bulach@chromium.org, darin@chromium.org, joth@chromium.org

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

[abarth-chromium, 2013-10-24 01:25:53]

Thoughts on who should review this CL?

[darin (slow to review), 2013-10-24 03:52:46]

LGTM

https://codereview.chromium.org/38993002/diff/20001/mojo/mojo.gyp
File mojo/mojo.gyp (right):

https://codereview.chromium.org/38993002/diff/20001/mojo/mojo.gyp#newcode200
mojo/mojo.gyp:200: 'target_name': 'sample_app',
nit: indentation

https://codereview.chromium.org/38993002/diff/20001/mojo/shell/android/shell_...
File mojo/shell/android/shell_apk/AndroidManifest.xml (right):

https://codereview.chromium.org/38993002/diff/20001/mojo/shell/android/shell_...
mojo/shell/android/shell_apk/AndroidManifest.xml:27: <uses-permission
android:name="android.permission.ACCESS_COARSE_LOCATION"/>
I guess this is just like the set of permissions that ContentShell on Android
requests. OK.

https://codereview.chromium.org/38993002/diff/20001/mojo/shell/android/shell_...
File
mojo/shell/android/shell_apk/src/org/chromium/mojo_shell_apk/LibraryLoader.java
(right):

https://codereview.chromium.org/38993002/diff/20001/mojo/shell/android/shell_...
mojo/shell/android/shell_apk/src/org/chromium/mojo_shell_apk/LibraryLoader.java:16:
return;
nit: indentation

https://codereview.chromium.org/38993002/diff/20001/mojo/shell/android/shell_...
File mojo/shell/android/shell_library_loader.cc (right):

https://codereview.chromium.org/38993002/diff/20001/mojo/shell/android/shell_...
mojo/shell/android/shell_library_loader.cc:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
At some point the tooling will tell you that you don't need the "(c)" in the
copyright block, or it may just annoyingly remove them on commit, which will
cause merge conflicts later on :-/

[darin (slow to review), 2013-10-24 03:54:13]

On 2013/10/24 03:52:46, darin wrote:
> LGTM

^^^ I meant that un-authoritatively. Feel free to commit and iterate.
You might ask joth@ for a real code review.

[abarth-chromium, 2013-10-24 04:02:15]

https://codereview.chromium.org/38993002/diff/20001/mojo/shell/android/shell_...
File mojo/shell/android/shell_apk/AndroidManifest.xml (right):

https://codereview.chromium.org/38993002/diff/20001/mojo/shell/android/shell_...
mojo/shell/android/shell_apk/AndroidManifest.xml:27: <uses-permission
android:name="android.permission.ACCESS_COARSE_LOCATION"/>
On 2013/10/24 03:52:47, darin wrote:
> I guess this is just like the set of permissions that ContentShell on Android
> requests. OK.

Yeah, we can start with a minimal set and expand if you prefer.

https://codereview.chromium.org/38993002/diff/20001/mojo/shell/android/shell_...
File mojo/shell/android/shell_library_loader.cc (right):

https://codereview.chromium.org/38993002/diff/20001/mojo/shell/android/shell_...
mojo/shell/android/shell_library_loader.cc:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2013/10/24 03:52:47, darin wrote:
> At some point the tooling will tell you that you don't need the "(c)" in the
> copyright block, or it may just annoyingly remove them on commit, which will
> cause merge conflicts later on :-/

ok

[abarth-chromium, 2013-10-24 04:02:57]

+joth

Would you be willing to take a look at this CL?  Thanks!

[joth, 2013-10-24 04:37:57]

lgtm with minor a comment for the androidy bits.
+bulach who's had most experience in bringing up new test target / shells (I
think - or will know who else to copy in)

https://codereview.chromium.org/38993002/diff/150001/mojo/shell/android/shell...
File mojo/shell/android/shell_apk/AndroidManifest.xml (right):

https://codereview.chromium.org/38993002/diff/150001/mojo/shell/android/shell...
mojo/shell/android/shell_apk/AndroidManifest.xml:35: <uses-permission
android:name="android.permission.WRITE_EXTERNAL_STORAGE"/>
I'd wager none of these are needed (yet|ever) for mojo shell. While it probably
doesn't matter for a test app, best practice is to only put capabilities in that
you need (helps catch e.g. adding dependency on a restricted function inside a
basic library function)

https://codereview.chromium.org/38993002/diff/150001/mojo/shell/android/shell...
File mojo/shell/android/shell_apk/res/layout/mojo_shell_activity.xml (right):

https://codereview.chromium.org/38993002/diff/150001/mojo/shell/android/shell...
mojo/shell/android/shell_apk/res/layout/mojo_shell_activity.xml:3: <!--
Copyright (c) 2012 The Chromium Authors. All rights reserved.
2013

https://codereview.chromium.org/38993002/diff/150001/mojo/shell/android/shell...
File
mojo/shell/android/shell_apk/src/org/chromium/mojo_shell_apk/LibraryLoader.java
(right):

https://codereview.chromium.org/38993002/diff/150001/mojo/shell/android/shell...
mojo/shell/android/shell_apk/src/org/chromium/mojo_shell_apk/LibraryLoader.java:18:
System.loadLibrary("mojo_shell");
Probably not a blocker at this point, but assuming you want to use command line,
JNI calls, etc I expect you'll want to use
org/chromium/content/app/LibraryLoader.java for this in future, implying that'll
need factoring out to a lower layer.

https://codereview.chromium.org/38993002/diff/150001/mojo/shell/android/shell...
File
mojo/shell/android/shell_apk/src/org/chromium/mojo_shell_apk/MojoShellApplication.java
(right):

https://codereview.chromium.org/38993002/diff/150001/mojo/shell/android/shell...
mojo/shell/android/shell_apk/src/org/chromium/mojo_shell_apk/MojoShellApplication.java:7:
import org.chromium.base.BaseChromiumApplication;
oh!, kind of funky we have "Application" concept in src/base but there it is.

[bulach, 2013-10-24 12:58:03]

lgtm with joth's comments and some more info here.. most of my suggestions can
potentially be done later, but I suppose the sooner we get the pieces needed
from "content/" the simpler it'll be to continue extending mojo. thanks!

https://codereview.chromium.org/38993002/diff/150001/mojo/shell/android/shell...
File
mojo/shell/android/shell_apk/src/org/chromium/mojo_shell_apk/LibraryLoader.java
(right):

https://codereview.chromium.org/38993002/diff/150001/mojo/shell/android/shell...
mojo/shell/android/shell_apk/src/org/chromium/mojo_shell_apk/LibraryLoader.java:18:
System.loadLibrary("mojo_shell");
On 2013/10/24 04:37:57, joth wrote:
> Probably not a blocker at this point, but assuming you want to use command
line,
> JNI calls, etc I expect you'll want to use
> org/chromium/content/app/LibraryLoader.java for this in future, implying
that'll
> need factoring out to a lower layer.

plus all the threading issues :)

just FYI, things to keep in mind for mojo:
- the library is normally loaded in a background thread
---- except for things that require synchronous initialization, like "android
content providers"
- main thread in java does all the java-UI initialization
- notification comes back to the main thread, initialization proceeds
- recently, the monolithic "main()" was split in multiple smaller tasks so that
the java UI is never blocked on those.

https://codereview.chromium.org/38993002/diff/150001/mojo/shell/android/shell...
File
mojo/shell/android/shell_apk/src/org/chromium/mojo_shell_apk/MojoShellActivity.java
(right):

https://codereview.chromium.org/38993002/diff/150001/mojo/shell/android/shell...
mojo/shell/android/shell_apk/src/org/chromium/mojo_shell_apk/MojoShellActivity.java:7:
import java.lang.UnsatisfiedLinkError;
nit: no need, afaict java.lang is implicit.

https://codereview.chromium.org/38993002/diff/150001/mojo/shell/android/shell...
mojo/shell/android/shell_apk/src/org/chromium/mojo_shell_apk/MojoShellActivity.java:28:
LibraryLoader.ensureInitialized();
yeah, not saying to do now, but just highlighting that the "real deal" does not
initialize this way... if mojo is to become an example on how to have a base
multi-process app :) we should probably change this soon.

also, it needs to call into base::android::InitApplicationContext()...
this is now exposed at the content level,

ContentMain.initApplicationContext(getApplicationContext()), mojo (shell) would
probably require something similar..

[abarth-chromium, 2013-10-24 13:43:05]

Thanks for taking a look!

https://codereview.chromium.org/38993002/diff/150001/mojo/shell/android/shell...
File mojo/shell/android/shell_apk/AndroidManifest.xml (right):

https://codereview.chromium.org/38993002/diff/150001/mojo/shell/android/shell...
mojo/shell/android/shell_apk/AndroidManifest.xml:35: <uses-permission
android:name="android.permission.WRITE_EXTERNAL_STORAGE"/>
On 2013/10/24 04:37:57, joth wrote:
> I'd wager none of these are needed (yet|ever) for mojo shell. While it
probably
> doesn't matter for a test app, best practice is to only put capabilities in
that
> you need (helps catch e.g. adding dependency on a restricted function inside a
> basic library function)

Ok, I'll trim this down a bit.  We will eventually need all of these because
we'll eventually want to host Content inside Mojo, but that's a long ways off.

https://codereview.chromium.org/38993002/diff/150001/mojo/shell/android/shell...
File
mojo/shell/android/shell_apk/src/org/chromium/mojo_shell_apk/LibraryLoader.java
(right):

https://codereview.chromium.org/38993002/diff/150001/mojo/shell/android/shell...
mojo/shell/android/shell_apk/src/org/chromium/mojo_shell_apk/LibraryLoader.java:18:
System.loadLibrary("mojo_shell");
On 2013/10/24 04:37:57, joth wrote:
> Probably not a blocker at this point, but assuming you want to use command
line,
> JNI calls, etc I expect you'll want to use
> org/chromium/content/app/LibraryLoader.java for this in future, implying
that'll
> need factoring out to a lower layer.

Yes.  We've been avoiding introducing a dependency from Content to Mojo, but we
should either add that dependency or move this sort of class to a lower layer.

https://codereview.chromium.org/38993002/diff/150001/mojo/shell/android/shell...
mojo/shell/android/shell_apk/src/org/chromium/mojo_shell_apk/LibraryLoader.java:18:
System.loadLibrary("mojo_shell");
On 2013/10/24 12:58:04, bulach wrote:
> just FYI, things to keep in mind for mojo:

Thanks, that's very useful information.  At this stage, we'll trying to get
something basic running.  We'll likely need to work through those sorts of
issues once we get the basics up and running.

https://codereview.chromium.org/38993002/diff/150001/mojo/shell/android/shell...
File
mojo/shell/android/shell_apk/src/org/chromium/mojo_shell_apk/MojoShellActivity.java
(right):

https://codereview.chromium.org/38993002/diff/150001/mojo/shell/android/shell...
mojo/shell/android/shell_apk/src/org/chromium/mojo_shell_apk/MojoShellActivity.java:7:
import java.lang.UnsatisfiedLinkError;
On 2013/10/24 12:58:04, bulach wrote:
> nit: no need, afaict java.lang is implicit.

Oh, that's nice.  I'm a Java noob.

https://codereview.chromium.org/38993002/diff/150001/mojo/shell/android/shell...
mojo/shell/android/shell_apk/src/org/chromium/mojo_shell_apk/MojoShellActivity.java:28:
LibraryLoader.ensureInitialized();
On 2013/10/24 12:58:04, bulach wrote:
> yeah, not saying to do now, but just highlighting that the "real deal" does
not
> initialize this way... if mojo is to become an example on how to have a base
> multi-process app :) we should probably change this soon.

I'm sorry, but I didn't understand this comment.  What should we change this to?

> also, it needs to call into base::android::InitApplicationContext()...
> this is now exposed at the content level,
> 
> ContentMain.initApplicationContext(getApplicationContext()), mojo (shell)
would
> probably require something similar..

Yeah, we aren't calling "main" type functions on the C++ side yet.  I was going
to work through that in the next CL.  We'll obviously need to do that soon.  :)

https://codereview.chromium.org/38993002/diff/150001/mojo/shell/android/shell...
File
mojo/shell/android/shell_apk/src/org/chromium/mojo_shell_apk/MojoShellApplication.java
(right):

https://codereview.chromium.org/38993002/diff/150001/mojo/shell/android/shell...
mojo/shell/android/shell_apk/src/org/chromium/mojo_shell_apk/MojoShellApplication.java:7:
import org.chromium.base.BaseChromiumApplication;
On 2013/10/24 04:37:57, joth wrote:
> oh!, kind of funky we have "Application" concept in src/base but there it is.

It doesn't seem to do much, but I thought it was better to use it.

[bulach, 2013-10-24 16:27:57]

still lgtm

https://codereview.chromium.org/38993002/diff/150001/mojo/shell/android/shell...
File
mojo/shell/android/shell_apk/src/org/chromium/mojo_shell_apk/MojoShellActivity.java
(right):

https://codereview.chromium.org/38993002/diff/150001/mojo/shell/android/shell...
mojo/shell/android/shell_apk/src/org/chromium/mojo_shell_apk/MojoShellActivity.java:28:
LibraryLoader.ensureInitialized();
On 2013/10/24 13:43:05, abarth wrote:
> On 2013/10/24 12:58:04, bulach wrote:
> > yeah, not saying to do now, but just highlighting that the "real deal" does
> not
> > initialize this way... if mojo is to become an example on how to have a base
> > multi-process app :) we should probably change this soon.
> 
> I'm sorry, but I didn't understand this comment.  What should we change this
to?

ops, sorry I wasn't clear.. :)
it's perfectly fine to keep as is for the time being..
As mentioned before, we'd need to extract the building blocks from content up
here at some point...
specifically, the "ensureInitialized" would be blocking.
At the "onCreate" level, we'd want to kick off loading the library in a separate
thread, do the java initialization here, then listen for a callback to proceed
with further initialization once the library has been loaded...
the great thing about mojo is that will help clarify and make it more generic,
right now each one of our "shells" (content, chromium, chrome) have their own
mechanisms, so I'm really looking forward to this being a major force in
clarifying / refactoring such building blocks!

> 
> > also, it needs to call into base::android::InitApplicationContext()...
> > this is now exposed at the content level,
> > 
> > ContentMain.initApplicationContext(getApplicationContext()), mojo (shell)
> would
> > probably require something similar..
> 
> Yeah, we aren't calling "main" type functions on the C++ side yet.  I was
going
> to work through that in the next CL.  We'll obviously need to do that soon. 
:)

[abarth-chromium, 2013-10-24 16:32:18]

On 2013/10/24 16:27:57, bulach wrote:
> ops, sorry I wasn't clear.. :)
> it's perfectly fine to keep as is for the time being..
> As mentioned before, we'd need to extract the building blocks from content up
> here at some point...
> specifically, the "ensureInitialized" would be blocking.
> At the "onCreate" level, we'd want to kick off loading the library in a
separate
> thread, do the java initialization here, then listen for a callback to proceed
> with further initialization once the library has been loaded...
> the great thing about mojo is that will help clarify and make it more generic,
> right now each one of our "shells" (content, chromium, chrome) have their own
> mechanisms, so I'm really looking forward to this being a major force in
> clarifying / refactoring such building blocks!

Ah, that makes sense.  If I ask nicely, would you be willing to help with that
part?  You understand this part of the system much better than I do.  :)

[commit-bot: I haz the power, 2013-10-24 16:39:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abarth@chromium.org/38993002/360001

[bulach, 2013-10-24 17:21:00]

On 2013/10/24 16:32:18, abarth wrote:
> On 2013/10/24 16:27:57, bulach wrote:
> > ops, sorry I wasn't clear.. :)
> > it's perfectly fine to keep as is for the time being..
> > As mentioned before, we'd need to extract the building blocks from content
up
> > here at some point...
> > specifically, the "ensureInitialized" would be blocking.
> > At the "onCreate" level, we'd want to kick off loading the library in a
> separate
> > thread, do the java initialization here, then listen for a callback to
proceed
> > with further initialization once the library has been loaded...
> > the great thing about mojo is that will help clarify and make it more
generic,
> > right now each one of our "shells" (content, chromium, chrome) have their
own
> > mechanisms, so I'm really looking forward to this being a major force in
> > clarifying / refactoring such building blocks!
> 
> Ah, that makes sense.  If I ask nicely, would you be willing to help with that
> part?  You understand this part of the system much better than I do.  :)

sure!! :) I'll be ooo next week though, but aberent@ will be back by then.. he
did most of this library loading / async startup of lately, I'm sure he can help
even more effectively... ;) I'll drop him a note so he has some context...

[abarth-chromium, 2013-10-24 20:29:41]

Committed patchset #6 manually as r230795 (presubmit successful).