Java content handler for Android.

Java content handler for Android.

This is a first version of the Java content handler for android. I
didn't have to touch base/, but I had to change a few BUILD.gn and
rules.gni files to make the thing compile and run correctly.

R=qsr@chromium.org, qsr
BUG=

Committed: https://chromium.googlesource.com/external/mojo/+/f2b541cc026bf5d63126bec23da6fd5f167db9a5

[qsr, 2015-02-05 16:50:08]

https://codereview.chromium.org/898853006/diff/1/build/android/gyp/javac.py
File build/android/gyp/javac.py (right):

https://codereview.chromium.org/898853006/diff/1/build/android/gyp/javac.py#n...
build/android/gyp/javac.py:109: def CreateManifest(manifest_path, classpath,
main_class=None, is_mojo=False):
What about adding a dict instead of this mojo specific thing and let any entries
be added?

https://codereview.chromium.org/898853006/diff/1/build/android/gyp/javac.py#n...
build/android/gyp/javac.py:192: help='The output JAR is a mojo application.')
And here you can have a repeated option that would be something like:

--manifest-entry=A=B

https://codereview.chromium.org/898853006/diff/1/build/config/android/interna...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/898853006/diff/1/build/config/android/interna...
build/config/android/internal_rules.gni:668: _mojo_app = false
And here and forward, just have a manifest_entries options that will be
something like:
manifest_entries = [
  'A=B',
  'C=D',
]

https://codereview.chromium.org/898853006/diff/1/examples/android/App.java
File examples/android/App.java (right):

https://codereview.chromium.org/898853006/diff/1/examples/android/App.java#ne...
examples/android/App.java:33: private static String sTAG = "AndroidTestApp";
We usually name this just TAG.

https://codereview.chromium.org/898853006/diff/1/examples/android/App.java#ne...
examples/android/App.java:54: mProviders.add(serviceProvider);
If you are not using exposedServices, you should close it yourself.

https://codereview.chromium.org/898853006/diff/1/examples/android/App.java#ne...
examples/android/App.java:58: public void requestQuit() {}
This should stop the runloop.

https://codereview.chromium.org/898853006/diff/1/examples/android/App.java#ne...
examples/android/App.java:61: public void close() {}
This should probably close mShell.

https://codereview.chromium.org/898853006/diff/1/examples/android/App.java#ne...
examples/android/App.java:98:
mManager.bind(mServiceClass.getConstructor().newInstance(), pipe);
Can you not to use reflection? You can define an  ServiceFactoryBinder as an
interface and pass an instance of this to the addService method.

https://codereview.chromium.org/898853006/diff/1/examples/android/App.java#ne...
examples/android/App.java:115: private InterfaceRequest<ServiceProvider>
mProvider;
You do not need to keep this object alive.

https://codereview.chromium.org/898853006/diff/1/examples/android/App.java#ne...
examples/android/App.java:136: }
You and to close the pipe if you do not bind it.

https://codereview.chromium.org/898853006/diff/1/examples/android/BUILD.gn
File examples/android/BUILD.gn (right):

https://codereview.chromium.org/898853006/diff/1/examples/android/BUILD.gn#ne...
examples/android/BUILD.gn:21: "//mojo/public/java:bindings",
Maybe those last 2 (at least the first one) should be added by the
mojo_java_android_application template?

https://codereview.chromium.org/898853006/diff/1/mojo/android/BUILD.gn
File mojo/android/BUILD.gn (right):

https://codereview.chromium.org/898853006/diff/1/mojo/android/BUILD.gn#newcode37
mojo/android/BUILD.gn:37: source_set("libsystem_java_src") {
You can let this with its previous name.

https://codereview.chromium.org/898853006/diff/1/mojo/android/system/base_run...
File mojo/android/system/base_run_loop.cc (right):

https://codereview.chromium.org/898853006/diff/1/mojo/android/system/base_run...
mojo/android/system/base_run_loop.cc:22: return (uintptr_t) message_loop;
Do not use C-cast. This should be a reinterpret_cast

https://codereview.chromium.org/898853006/diff/1/mojo/android/system/base_run...
mojo/android/system/base_run_loop.cc:48:
base::TimeDelta::FromMicroseconds(delay));*/
Either you do not need this method at all, or you probably want it to not be
completely commented.

https://codereview.chromium.org/898853006/diff/1/mojo/android/system/src/org/...
File mojo/android/system/src/org/chromium/mojo/system/impl/BaseRunLoop.java
(right):

https://codereview.chromium.org/898853006/diff/1/mojo/android/system/src/org/...
mojo/android/system/src/org/chromium/mojo/system/impl/BaseRunLoop.java:41:
nativeDeleteMessageLoop(mRunLoopID);
Hum... I need to think about this. This seems wrong, as we might want to run the
same runloop multiple time. You might want to make RunLoop closeable and delete
on close.

https://codereview.chromium.org/898853006/diff/1/mojo/public/java/system/src/...
File mojo/public/java/system/src/org/chromium/mojo/system/Core.java (right):

https://codereview.chromium.org/898853006/diff/1/mojo/public/java/system/src/...
mojo/public/java/system/src/org/chromium/mojo/system/Core.java:320: public void
setCurrentRunLoop(RunLoop runLoop);
Hum... What is the use case for this method? I think you can remove this one.

https://codereview.chromium.org/898853006/diff/1/mojo/public/java/system/src/...
File mojo/public/java/system/src/org/chromium/mojo/system/RunLoop.java (right):

https://codereview.chromium.org/898853006/diff/1/mojo/public/java/system/src/...
mojo/public/java/system/src/org/chromium/mojo/system/RunLoop.java:30: * be
executed. May be 0 for immediate execution.
immediate is ambiguous in this context. 0 still means it will be enqueued, not
run synchronously.

https://codereview.chromium.org/898853006/diff/1/mojo/public/mojo_application...
File mojo/public/mojo_application.gni (left):

https://codereview.chromium.org/898853006/diff/1/mojo/public/mojo_application...
mojo/public/mojo_application.gni:164: assert(defined(invoker.input_so))
What is the use case where you do not have an input_so ?

https://codereview.chromium.org/898853006/diff/1/services/android/BUILD.gn
File services/android/BUILD.gn (right):

https://codereview.chromium.org/898853006/diff/1/services/android/BUILD.gn#ne...
services/android/BUILD.gn:9: output_name = "java_handler"
Any reason you need an output_name?

https://codereview.chromium.org/898853006/diff/1/services/android/java_handle...
File services/android/java_handler.cc (right):

https://codereview.chromium.org/898853006/diff/1/services/android/java_handle...
services/android/java_handler.cc:90: // application context.
s/only if/because/

[etiennej, 2015-02-06 16:22:30]

https://codereview.chromium.org/898853006/diff/1/build/android/gyp/javac.py
File build/android/gyp/javac.py (right):

https://codereview.chromium.org/898853006/diff/1/build/android/gyp/javac.py#n...
build/android/gyp/javac.py:109: def CreateManifest(manifest_path, classpath,
main_class=None, is_mojo=False):
On 2015/02/05 16:50:06, qsr wrote:
> What about adding a dict instead of this mojo specific thing and let any
entries
> be added?

Done.

https://codereview.chromium.org/898853006/diff/1/build/android/gyp/javac.py#n...
build/android/gyp/javac.py:192: help='The output JAR is a mojo application.')
On 2015/02/05 16:50:07, qsr wrote:
> And here you can have a repeated option that would be something like:
> 
> --manifest-entry=A=B

Done.

https://codereview.chromium.org/898853006/diff/1/build/config/android/interna...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/898853006/diff/1/build/config/android/interna...
build/config/android/internal_rules.gni:668: _mojo_app = false
On 2015/02/05 16:50:07, qsr wrote:
> And here and forward, just have a manifest_entries options that will be
> something like:
> manifest_entries = [
>   'A=B',
>   'C=D',
> ]

Done.

https://codereview.chromium.org/898853006/diff/1/examples/android/App.java
File examples/android/App.java (right):

https://codereview.chromium.org/898853006/diff/1/examples/android/App.java#ne...
examples/android/App.java:33: private static String sTAG = "AndroidTestApp";
On 2015/02/05 16:50:07, qsr wrote:
> We usually name this just TAG.

Done. We should remove the presubmit error that requires sTAG, then.

https://codereview.chromium.org/898853006/diff/1/examples/android/App.java#ne...
examples/android/App.java:54: mProviders.add(serviceProvider);
On 2015/02/05 16:50:07, qsr wrote:
> If you are not using exposedServices, you should close it yourself.

Done.

https://codereview.chromium.org/898853006/diff/1/examples/android/App.java#ne...
examples/android/App.java:58: public void requestQuit() {}
On 2015/02/05 16:50:07, qsr wrote:
> This should stop the runloop.

Done.

https://codereview.chromium.org/898853006/diff/1/examples/android/App.java#ne...
examples/android/App.java:61: public void close() {}
On 2015/02/05 16:50:07, qsr wrote:
> This should probably close mShell.

Done.

https://codereview.chromium.org/898853006/diff/1/examples/android/App.java#ne...
examples/android/App.java:98:
mManager.bind(mServiceClass.getConstructor().newInstance(), pipe);
On 2015/02/05 16:50:07, qsr wrote:
> Can you not to use reflection? You can define an  ServiceFactoryBinder as an
> interface and pass an instance of this to the addService method.

Done.

https://codereview.chromium.org/898853006/diff/1/examples/android/App.java#ne...
examples/android/App.java:115: private InterfaceRequest<ServiceProvider>
mProvider;
On 2015/02/05 16:50:07, qsr wrote:
> You do not need to keep this object alive.

Done.

https://codereview.chromium.org/898853006/diff/1/examples/android/App.java#ne...
examples/android/App.java:136: }
On 2015/02/05 16:50:07, qsr wrote:
> You and to close the pipe if you do not bind it.

Done.

https://codereview.chromium.org/898853006/diff/1/examples/android/BUILD.gn
File examples/android/BUILD.gn (right):

https://codereview.chromium.org/898853006/diff/1/examples/android/BUILD.gn#ne...
examples/android/BUILD.gn:21: "//mojo/public/java:bindings",
On 2015/02/05 16:50:07, qsr wrote:
> Maybe those last 2 (at least the first one) should be added by the
> mojo_java_android_application template?

Done.

https://codereview.chromium.org/898853006/diff/1/mojo/android/BUILD.gn
File mojo/android/BUILD.gn (right):

https://codereview.chromium.org/898853006/diff/1/mojo/android/BUILD.gn#newcode37
mojo/android/BUILD.gn:37: source_set("libsystem_java_src") {
On 2015/02/05 16:50:07, qsr wrote:
> You can let this with its previous name.

Done.

https://codereview.chromium.org/898853006/diff/1/mojo/android/system/base_run...
File mojo/android/system/base_run_loop.cc (right):

https://codereview.chromium.org/898853006/diff/1/mojo/android/system/base_run...
mojo/android/system/base_run_loop.cc:22: return (uintptr_t) message_loop;
On 2015/02/05 16:50:07, qsr wrote:
> Do not use C-cast. This should be a reinterpret_cast

Done.

https://codereview.chromium.org/898853006/diff/1/mojo/android/system/base_run...
mojo/android/system/base_run_loop.cc:48:
base::TimeDelta::FromMicroseconds(delay));*/
On 2015/02/05 16:50:07, qsr wrote:
> Either you do not need this method at all, or you probably want it to not be
> completely commented.

Sorry. Done.

https://codereview.chromium.org/898853006/diff/1/mojo/android/system/src/org/...
File mojo/android/system/src/org/chromium/mojo/system/impl/BaseRunLoop.java
(right):

https://codereview.chromium.org/898853006/diff/1/mojo/android/system/src/org/...
mojo/android/system/src/org/chromium/mojo/system/impl/BaseRunLoop.java:41:
nativeDeleteMessageLoop(mRunLoopID);
On 2015/02/05 16:50:07, qsr wrote:
> Hum... I need to think about this. This seems wrong, as we might want to run
the
> same runloop multiple time. You might want to make RunLoop closeable and
delete
> on close.

Done.

https://codereview.chromium.org/898853006/diff/1/mojo/public/java/system/src/...
File mojo/public/java/system/src/org/chromium/mojo/system/Core.java (right):

https://codereview.chromium.org/898853006/diff/1/mojo/public/java/system/src/...
mojo/public/java/system/src/org/chromium/mojo/system/Core.java:320: public void
setCurrentRunLoop(RunLoop runLoop);
On 2015/02/05 16:50:07, qsr wrote:
> Hum... What is the use case for this method? I think you can remove this one.

Removed. Agreed we don't need it for now, but we will need something similar
when we will allow people to inject their own run loops.

https://codereview.chromium.org/898853006/diff/1/mojo/public/java/system/src/...
File mojo/public/java/system/src/org/chromium/mojo/system/RunLoop.java (right):

https://codereview.chromium.org/898853006/diff/1/mojo/public/java/system/src/...
mojo/public/java/system/src/org/chromium/mojo/system/RunLoop.java:30: * be
executed. May be 0 for immediate execution.
On 2015/02/05 16:50:07, qsr wrote:
> immediate is ambiguous in this context. 0 still means it will be enqueued, not
> run synchronously.

Done.

https://codereview.chromium.org/898853006/diff/1/mojo/public/mojo_application...
File mojo/public/mojo_application.gni (left):

https://codereview.chromium.org/898853006/diff/1/mojo/public/mojo_application...
mojo/public/mojo_application.gni:164: assert(defined(invoker.input_so))
On 2015/02/05 16:50:07, qsr wrote:
> What is the use case where you do not have an input_so ?

None.

Actually, I merged the two new templates as we don't really need them both.

https://codereview.chromium.org/898853006/diff/1/services/android/BUILD.gn
File services/android/BUILD.gn (right):

https://codereview.chromium.org/898853006/diff/1/services/android/BUILD.gn#ne...
services/android/BUILD.gn:9: output_name = "java_handler"
On 2015/02/05 16:50:07, qsr wrote:
> Any reason you need an output_name?

Done.

https://codereview.chromium.org/898853006/diff/1/services/android/java_handle...
File services/android/java_handler.cc (right):

https://codereview.chromium.org/898853006/diff/1/services/android/java_handle...
services/android/java_handler.cc:90: // application context.
On 2015/02/05 16:50:08, qsr wrote:
> s/only if/because/

Done.

[qsr, 2015-02-06 17:16:14]

https://codereview.chromium.org/898853006/diff/40001/build/android/gyp/javac.py
File build/android/gyp/javac.py (right):

https://codereview.chromium.org/898853006/diff/40001/build/android/gyp/javac....
build/android/gyp/javac.py:120: main_class: If present, the class containing the
main() function.
Add the comment for manifest_entries

https://codereview.chromium.org/898853006/diff/40001/build/config/android/int...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/898853006/diff/40001/build/config/android/int...
build/config/android/internal_rules.gni:731: invoker.main_class,
Why using --manifest-entry=foo for 1 and --main-class bar for the other?
Shouldn't you use the first form for both.

Also, do we really want to support main-class here while we do not use it?

https://codereview.chromium.org/898853006/diff/40001/examples/android/App.java
File examples/android/App.java (right):

https://codereview.chromium.org/898853006/diff/40001/examples/android/App.jav...
examples/android/App.java:32: private static String TAG = "AndroidTestApp";
Android doesn't really tell us anything when looking at the log. What about
ExampleServiceApp ?

https://codereview.chromium.org/898853006/diff/40001/examples/android/App.jav...
examples/android/App.java:37: private List<ServiceProvider> mProviders = new
ArrayList<ServiceProvider>();
final?

Also, I think you do not need those. Each provider will be kept alive by the
pipe they are bound to.

https://codereview.chromium.org/898853006/diff/40001/examples/android/App.jav...
examples/android/App.java:98: public void
bindNewInstanceToMessagePipe(MessagePipeHandle pipe) throws Exception {
Why do you need to throws Exception here? If that's because of call() you should
have your own Interface that doesn't throw.

https://codereview.chromium.org/898853006/diff/40001/examples/android/App.jav...
examples/android/App.java:108: private Map<String, ServiceFactoryBinder>
mNameToServiceMap =
I'm surprise you do not have a warning. Please use ServiceFactoryBinder<?
extends Interface>

https://codereview.chromium.org/898853006/diff/40001/mojo/android/system/base...
File mojo/android/system/base_run_loop.cc (right):

https://codereview.chromium.org/898853006/diff/40001/mojo/android/system/base...
mojo/android/system/base_run_loop.cc:44: FROM_HERE,
base::Bind(&Java_BaseRunLoop_runRunnable, env, runnable),
Do not keep env. You should post to a local method and attach to thread.

Also, unless I'm missing something, your runnable is not kept by anything, and
can be gc'd or moved anytime. You need a global ref on it.

https://codereview.chromium.org/898853006/diff/40001/mojo/android/system/src/...
File mojo/android/system/src/org/chromium/mojo/system/impl/BaseRunLoop.java
(right):

https://codereview.chromium.org/898853006/diff/40001/mojo/android/system/src/...
mojo/android/system/src/org/chromium/mojo/system/impl/BaseRunLoop.java:53:
nativeDeleteMessageLoop(mRunLoopID);
You need the close to notify CoreImpl so that here is no current runloop
anymore.

https://codereview.chromium.org/898853006/diff/40001/mojo/public/mojo_applica...
File mojo/public/mojo_application.gni (right):

https://codereview.chromium.org/898853006/diff/40001/mojo/public/mojo_applica...
mojo/public/mojo_application.gni:164: assert(defined(invoker.input_dex_jar))
Can you revert the changes on this file?

https://codereview.chromium.org/898853006/diff/40001/services/android/BUILD.gn
File services/android/BUILD.gn (right):

https://codereview.chromium.org/898853006/diff/40001/services/android/BUILD.g...
services/android/BUILD.gn:39: dex_output_path =
"$target_out_dir/java_handler_java.dex.jar"
Maybe move this before the 2 usages, and let a blank line.

https://codereview.chromium.org/898853006/diff/40001/services/android/java/sr...
File services/android/java/src/org/chromium/services/android/JavaHandler.java
(right):

https://codereview.chromium.org/898853006/diff/40001/services/android/java/sr...
services/android/java/src/org/chromium/services/android/JavaHandler.java:28:
private static final String TAG = "AndroidHandler";
Should that be JavaHandler?

https://codereview.chromium.org/898853006/diff/40001/services/android/java/sr...
services/android/java/src/org/chromium/services/android/JavaHandler.java:39: //
called.
The last word can probably go to the previous line.

https://codereview.chromium.org/898853006/diff/40001/services/android/java/sr...
services/android/java/src/org/chromium/services/android/JavaHandler.java:106:
clearTemporaryFiles(context, application_java_library);
Why not having a single finally to clear this?

https://codereview.chromium.org/898853006/diff/40001/services/android/java/sr...
services/android/java/src/org/chromium/services/android/JavaHandler.java:120:
loadedClass.getMethod("mojoMain", Context.class, Core.class, int.class);
Instead of sending an int there, you should transform this into a handle here
and pass the handle directly.

https://codereview.chromium.org/898853006/diff/40001/services/android/rules.gni
File services/android/rules.gni (right):

https://codereview.chromium.org/898853006/diff/40001/services/android/rules.g...
services/android/rules.gni:13: android_lib_name = "${target_name}_lib"
Maybe prepend those names with __

I can imagine someone creating an application that would define myapp_lib
himself.

[etiennej, 2015-02-09 14:09:52]

https://codereview.chromium.org/898853006/diff/40001/build/android/gyp/javac.py
File build/android/gyp/javac.py (right):

https://codereview.chromium.org/898853006/diff/40001/build/android/gyp/javac....
build/android/gyp/javac.py:120: main_class: If present, the class containing the
main() function.
On 2015/02/06 17:16:13, qsr wrote:
> Add the comment for manifest_entries

Done.

https://codereview.chromium.org/898853006/diff/40001/build/config/android/int...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/898853006/diff/40001/build/config/android/int...
build/config/android/internal_rules.gni:731: invoker.main_class,
On 2015/02/06 17:16:14, qsr wrote:
> Why using --manifest-entry=foo for 1 and --main-class bar for the other?
> Shouldn't you use the first form for both.
> 
> Also, do we really want to support main-class here while we do not use it?

Done.

https://codereview.chromium.org/898853006/diff/40001/examples/android/App.java
File examples/android/App.java (right):

https://codereview.chromium.org/898853006/diff/40001/examples/android/App.jav...
examples/android/App.java:32: private static String TAG = "AndroidTestApp";
On 2015/02/06 17:16:14, qsr wrote:
> Android doesn't really tell us anything when looking at the log. What about
> ExampleServiceApp ?

Done.

https://codereview.chromium.org/898853006/diff/40001/examples/android/App.jav...
examples/android/App.java:37: private List<ServiceProvider> mProviders = new
ArrayList<ServiceProvider>();
On 2015/02/06 17:16:14, qsr wrote:
> final?
> 
> Also, I think you do not need those. Each provider will be kept alive by the
> pipe they are bound to.

Done.

https://codereview.chromium.org/898853006/diff/40001/examples/android/App.jav...
examples/android/App.java:98: public void
bindNewInstanceToMessagePipe(MessagePipeHandle pipe) throws Exception {
On 2015/02/06 17:16:14, qsr wrote:
> Why do you need to throws Exception here? If that's because of call() you
should
> have your own Interface that doesn't throw.

Done.

https://codereview.chromium.org/898853006/diff/40001/examples/android/App.jav...
examples/android/App.java:108: private Map<String, ServiceFactoryBinder>
mNameToServiceMap =
On 2015/02/06 17:16:14, qsr wrote:
> I'm surprise you do not have a warning. Please use ServiceFactoryBinder<?
> extends Interface>

Done.

https://codereview.chromium.org/898853006/diff/40001/mojo/android/system/base...
File mojo/android/system/base_run_loop.cc (right):

https://codereview.chromium.org/898853006/diff/40001/mojo/android/system/base...
mojo/android/system/base_run_loop.cc:44: FROM_HERE,
base::Bind(&Java_BaseRunLoop_runRunnable, env, runnable),
On 2015/02/06 17:16:14, qsr wrote:
> Do not keep env. You should post to a local method and attach to thread.
> 
> Also, unless I'm missing something, your runnable is not kept by anything, and
> can be gc'd or moved anytime. You need a global ref on it.

Done.

https://codereview.chromium.org/898853006/diff/40001/mojo/android/system/src/...
File mojo/android/system/src/org/chromium/mojo/system/impl/BaseRunLoop.java
(right):

https://codereview.chromium.org/898853006/diff/40001/mojo/android/system/src/...
mojo/android/system/src/org/chromium/mojo/system/impl/BaseRunLoop.java:53:
nativeDeleteMessageLoop(mRunLoopID);
On 2015/02/06 17:16:14, qsr wrote:
> You need the close to notify CoreImpl so that here is no current runloop
> anymore.

Added a method in Core to close the run loop.

https://codereview.chromium.org/898853006/diff/40001/mojo/public/mojo_applica...
File mojo/public/mojo_application.gni (right):

https://codereview.chromium.org/898853006/diff/40001/mojo/public/mojo_applica...
mojo/public/mojo_application.gni:164: assert(defined(invoker.input_dex_jar))
On 2015/02/06 17:16:14, qsr wrote:
> Can you revert the changes on this file?

Done.

https://codereview.chromium.org/898853006/diff/40001/services/android/BUILD.gn
File services/android/BUILD.gn (right):

https://codereview.chromium.org/898853006/diff/40001/services/android/BUILD.g...
services/android/BUILD.gn:39: dex_output_path =
"$target_out_dir/java_handler_java.dex.jar"
On 2015/02/06 17:16:14, qsr wrote:
> Maybe move this before the 2 usages, and let a blank line.

Done.

https://codereview.chromium.org/898853006/diff/40001/services/android/java/sr...
File services/android/java/src/org/chromium/services/android/JavaHandler.java
(right):

https://codereview.chromium.org/898853006/diff/40001/services/android/java/sr...
services/android/java/src/org/chromium/services/android/JavaHandler.java:28:
private static final String TAG = "AndroidHandler";
On 2015/02/06 17:16:14, qsr wrote:
> Should that be JavaHandler?

Done.

https://codereview.chromium.org/898853006/diff/40001/services/android/java/sr...
services/android/java/src/org/chromium/services/android/JavaHandler.java:39: //
called.
On 2015/02/06 17:16:14, qsr wrote:
> The last word can probably go to the previous line.

Done.

https://codereview.chromium.org/898853006/diff/40001/services/android/java/sr...
services/android/java/src/org/chromium/services/android/JavaHandler.java:106:
clearTemporaryFiles(context, application_java_library);
On 2015/02/06 17:16:14, qsr wrote:
> Why not having a single finally to clear this?

Done.

https://codereview.chromium.org/898853006/diff/40001/services/android/java/sr...
services/android/java/src/org/chromium/services/android/JavaHandler.java:120:
loadedClass.getMethod("mojoMain", Context.class, Core.class, int.class);
On 2015/02/06 17:16:14, qsr wrote:
> Instead of sending an int there, you should transform this into a handle here
> and pass the handle directly.

Done.

https://codereview.chromium.org/898853006/diff/40001/services/android/rules.gni
File services/android/rules.gni (right):

https://codereview.chromium.org/898853006/diff/40001/services/android/rules.g...
services/android/rules.gni:13: android_lib_name = "${target_name}_lib"
On 2015/02/06 17:16:14, qsr wrote:
> Maybe prepend those names with __
> 
> I can imagine someone creating an application that would define myapp_lib
> himself.

Done.

[qsr, 2015-02-09 15:54:53]

Can you re-upload with the changes we talked about offline?

https://codereview.chromium.org/898853006/diff/60001/mojo/android/system/base...
File mojo/android/system/base_run_loop.cc (right):

https://codereview.chromium.org/898853006/diff/60001/mojo/android/system/base...
mojo/android/system/base_run_loop.cc:51: new
base::android::ScopedJavaGlobalRef<jobject>();
Look if you can either directly pass a ScopedJavaGlobalRef (I think it should
work), or pass a scoped_ptr<base::android::ScopedJavaGlobalRef> instead of using
bare pointers.

https://codereview.chromium.org/898853006/diff/60001/services/android/java/sr...
File services/android/java/src/org/chromium/services/android/JavaHandler.java
(right):

https://codereview.chromium.org/898853006/diff/60001/services/android/java/sr...
services/android/java/src/org/chromium/services/android/JavaHandler.java:108:
clearTemporaryFiles(context, application_java_library);
This is not needed.

https://codereview.chromium.org/898853006/diff/60001/services/android/java/sr...
services/android/java/src/org/chromium/services/android/JavaHandler.java:114:
clearTemporaryFiles(context, application_java_library);
Neither is this.

[etiennej, 2015-02-09 16:41:30]

https://codereview.chromium.org/898853006/diff/60001/mojo/android/system/base...
File mojo/android/system/base_run_loop.cc (right):

https://codereview.chromium.org/898853006/diff/60001/mojo/android/system/base...
mojo/android/system/base_run_loop.cc:51: new
base::android::ScopedJavaGlobalRef<jobject>();
On 2015/02/09 15:54:52, qsr wrote:
> Look if you can either directly pass a ScopedJavaGlobalRef (I think it should
> work), or pass a scoped_ptr<base::android::ScopedJavaGlobalRef> instead of
using
> bare pointers.

Done.

https://codereview.chromium.org/898853006/diff/60001/services/android/java/sr...
File services/android/java/src/org/chromium/services/android/JavaHandler.java
(right):

https://codereview.chromium.org/898853006/diff/60001/services/android/java/sr...
services/android/java/src/org/chromium/services/android/JavaHandler.java:108:
clearTemporaryFiles(context, application_java_library);
On 2015/02/09 15:54:53, qsr wrote:
> This is not needed.

Done.

https://codereview.chromium.org/898853006/diff/60001/services/android/java/sr...
services/android/java/src/org/chromium/services/android/JavaHandler.java:114:
clearTemporaryFiles(context, application_java_library);
On 2015/02/09 15:54:53, qsr wrote:
> Neither is this.

Done.

[qsr, 2015-02-10 11:58:19]

https://codereview.chromium.org/898853006/diff/70023/base/macros.h
File base/macros.h (right):

https://codereview.chromium.org/898853006/diff/70023/base/macros.h#newcode66
base/macros.h:66: #define arraysize(array) (sizeof(::ArraySizeHelper(array)))
This is now fixed upstream.

https://codereview.chromium.org/898853006/diff/70023/build/android/gyp/javac.py
File build/android/gyp/javac.py (right):

https://codereview.chromium.org/898853006/diff/70023/build/android/gyp/javac....
build/android/gyp/javac.py:110: manifest_entries=None):
This should be done upstream. Anything in /build/ lives in Chromium.

https://codereview.chromium.org/898853006/diff/70023/build/config/android/int...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/898853006/diff/70023/build/config/android/int...
build/config/android/internal_rules.gni:875: main_class = invoker.main_class
Is main_class is used? If not, you can remove it.

https://codereview.chromium.org/898853006/diff/70023/examples/android/App.java
File examples/android/App.java (right):

https://codereview.chromium.org/898853006/diff/70023/examples/android/App.jav...
examples/android/App.java:34: public ApplicationImpl(MessagePipeHandle
applicationRequest, Core core) {
Maybe invert the 2 parameters. Core is some kind of constant, while the request
is the thing your are working on.

https://codereview.chromium.org/898853006/diff/70023/examples/android/App.jav...
examples/android/App.java:109: } catch (Exception e) {
Why do you need to catch something here? Should not be necessary.

https://codereview.chromium.org/898853006/diff/70023/mojo/android/system/src/...
File mojo/android/system/src/org/chromium/mojo/system/impl/BaseRunLoop.java
(right):

https://codereview.chromium.org/898853006/diff/70023/mojo/android/system/src/...
mojo/android/system/src/org/chromium/mojo/system/impl/BaseRunLoop.java:50:
public void close() {
close should be callable multiple time, so you should set mRunLoopID to 0 and
check this first.

https://codereview.chromium.org/898853006/diff/70023/mojo/android/system/src/...
File mojo/android/system/src/org/chromium/mojo/system/impl/CoreImpl.java
(right):

https://codereview.chromium.org/898853006/diff/70023/mojo/android/system/src/...
mojo/android/system/src/org/chromium/mojo/system/impl/CoreImpl.java:235: void
releaseRunLoop() {
Can you add some comment here. Thanks.

https://codereview.chromium.org/898853006/diff/70023/mojo/android/system/src/...
mojo/android/system/src/org/chromium/mojo/system/impl/CoreImpl.java:235: void
releaseRunLoop() {
I would rename this: clearCurrentRunLoop

https://codereview.chromium.org/898853006/diff/70023/mojo/public/java/system/...
File mojo/public/java/system/src/org/chromium/mojo/system/RunLoop.java (right):

https://codereview.chromium.org/898853006/diff/70023/mojo/public/java/system/...
mojo/public/java/system/src/org/chromium/mojo/system/RunLoop.java:10: public
interface RunLoop extends AutoCloseable {
You should use Closeable instead of AutoCloseable, it is easier to use (you can
call close multiple time with a guarantee that closing a closed resource is a
no-op.

https://codereview.chromium.org/898853006/diff/70023/mojo/public/java/system/...
mojo/public/java/system/src/org/chromium/mojo/system/RunLoop.java:35: * Closes
the run loop and deregister it from Core.
I would use destroy or delete instead of close in the comment.

https://codereview.chromium.org/898853006/diff/70023/services/android/BUILD.gn
File services/android/BUILD.gn (right):

https://codereview.chromium.org/898853006/diff/70023/services/android/BUILD.g...
services/android/BUILD.gn:22: "//mojo/public/platform/native:system",
This seems to be already included in mojo/public/c/system:for_shared_library

https://codereview.chromium.org/898853006/diff/70023/services/android/java/sr...
File services/android/java/src/org/chromium/services/android/JavaHandler.java
(right):

https://codereview.chromium.org/898853006/diff/70023/services/android/java/sr...
services/android/java/src/org/chromium/services/android/JavaHandler.java:47: *
to clean up after the previous run.
Comment is not adapted to this class usage.

[etiennej, 2015-02-10 14:32:04]

https://codereview.chromium.org/898853006/diff/70023/base/macros.h
File base/macros.h (right):

https://codereview.chromium.org/898853006/diff/70023/base/macros.h#newcode66
base/macros.h:66: #define arraysize(array) (sizeof(::ArraySizeHelper(array)))
On 2015/02/10 11:58:18, qsr wrote:
> This is now fixed upstream.

Done.

https://codereview.chromium.org/898853006/diff/70023/build/android/gyp/javac.py
File build/android/gyp/javac.py (right):

https://codereview.chromium.org/898853006/diff/70023/build/android/gyp/javac....
build/android/gyp/javac.py:110: manifest_entries=None):
On 2015/02/10 11:58:18, qsr wrote:
> This should be done upstream. Anything in /build/ lives in Chromium.

CL in review.

https://codereview.chromium.org/898853006/diff/70023/build/config/android/int...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/898853006/diff/70023/build/config/android/int...
build/config/android/internal_rules.gni:875: main_class = invoker.main_class
On 2015/02/10 11:58:18, qsr wrote:
> Is main_class is used? If not, you can remove it.

Done.

https://codereview.chromium.org/898853006/diff/70023/examples/android/App.java
File examples/android/App.java (right):

https://codereview.chromium.org/898853006/diff/70023/examples/android/App.jav...
examples/android/App.java:34: public ApplicationImpl(MessagePipeHandle
applicationRequest, Core core) {
On 2015/02/10 11:58:18, qsr wrote:
> Maybe invert the 2 parameters. Core is some kind of constant, while the
request
> is the thing your are working on.

Done.

https://codereview.chromium.org/898853006/diff/70023/examples/android/App.jav...
examples/android/App.java:109: } catch (Exception e) {
On 2015/02/10 11:58:18, qsr wrote:
> Why do you need to catch something here? Should not be necessary.

Done.

https://codereview.chromium.org/898853006/diff/70023/mojo/android/system/src/...
File mojo/android/system/src/org/chromium/mojo/system/impl/BaseRunLoop.java
(right):

https://codereview.chromium.org/898853006/diff/70023/mojo/android/system/src/...
mojo/android/system/src/org/chromium/mojo/system/impl/BaseRunLoop.java:50:
public void close() {
On 2015/02/10 11:58:18, qsr wrote:
> close should be callable multiple time, so you should set mRunLoopID to 0 and
> check this first.

Done.

https://codereview.chromium.org/898853006/diff/70023/mojo/android/system/src/...
File mojo/android/system/src/org/chromium/mojo/system/impl/CoreImpl.java
(right):

https://codereview.chromium.org/898853006/diff/70023/mojo/android/system/src/...
mojo/android/system/src/org/chromium/mojo/system/impl/CoreImpl.java:235: void
releaseRunLoop() {
On 2015/02/10 11:58:18, qsr wrote:
> Can you add some comment here. Thanks.

Done.

https://codereview.chromium.org/898853006/diff/70023/mojo/android/system/src/...
mojo/android/system/src/org/chromium/mojo/system/impl/CoreImpl.java:235: void
releaseRunLoop() {
On 2015/02/10 11:58:18, qsr wrote:
> I would rename this: clearCurrentRunLoop

Done.

https://codereview.chromium.org/898853006/diff/70023/mojo/public/java/system/...
File mojo/public/java/system/src/org/chromium/mojo/system/RunLoop.java (right):

https://codereview.chromium.org/898853006/diff/70023/mojo/public/java/system/...
mojo/public/java/system/src/org/chromium/mojo/system/RunLoop.java:10: public
interface RunLoop extends AutoCloseable {
On 2015/02/10 11:58:18, qsr wrote:
> You should use Closeable instead of AutoCloseable, it is easier to use (you
can
> call close multiple time with a guarantee that closing a closed resource is a
> no-op.

Done.

https://codereview.chromium.org/898853006/diff/70023/mojo/public/java/system/...
mojo/public/java/system/src/org/chromium/mojo/system/RunLoop.java:35: * Closes
the run loop and deregister it from Core.
On 2015/02/10 11:58:18, qsr wrote:
> I would use destroy or delete instead of close in the comment.

Done.

https://codereview.chromium.org/898853006/diff/70023/services/android/BUILD.gn
File services/android/BUILD.gn (right):

https://codereview.chromium.org/898853006/diff/70023/services/android/BUILD.g...
services/android/BUILD.gn:22: "//mojo/public/platform/native:system",
On 2015/02/10 11:58:18, qsr wrote:
> This seems to be already included in mojo/public/c/system:for_shared_library

Done.

https://codereview.chromium.org/898853006/diff/70023/services/android/java/sr...
File services/android/java/src/org/chromium/services/android/JavaHandler.java
(right):

https://codereview.chromium.org/898853006/diff/70023/services/android/java/sr...
services/android/java/src/org/chromium/services/android/JavaHandler.java:47: *
to clean up after the previous run.
On 2015/02/10 11:58:18, qsr wrote:
> Comment is not adapted to this class usage.

Done.

[qsr, 2015-02-10 14:48:22]

LGTM with nits.

https://codereview.chromium.org/898853006/diff/130001/services/android/java/s...
File services/android/java/src/org/chromium/services/android/JavaHandler.java
(right):

https://codereview.chromium.org/898853006/diff/130001/services/android/java/s...
services/android/java/src/org/chromium/services/android/JavaHandler.java:48:
static void clearTemporaryFiles(Context context, File archive) {
Any reason this and the next method is package protected? It seems to me it
should be private.

[etiennej, 2015-02-20 10:53:30]

New patchsets have been uploaded after l-g-t-m from qsr@chromium.org

[etiennej, 2015-02-20 11:39:57]

Committed patchset #9 (id:150001) manually as
f2b541cc026bf5d63126bec23da6fd5f167db9a5 (presubmit successful).