Allow native_viewport to create new native windows on demand on Android.

Allow native_viewport to create new native windows on demand on Android.

With this change, each time a new native viewport is initialized, an
intent is sent to create a new task for the shell. Then, once the new
task is ready, the requesting native viewport is notified of the
availability of the new native surface so it can attach to it.

There is still no specific support for the keyboard.

R=qsr@chromium.org

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

[etiennej, 2015-08-06 13:17:48]

Patchset #1 (id:1) has been deleted

[etiennej, 2015-08-06 13:18:48]

etiennej@chromium.org changed reviewers:
+ qsr@chromium.org

[qsr, 2015-08-06 14:13:21]

https://codereview.chromium.org/1280613003/diff/20001/mojo/services/native_vi...
File mojo/services/native_viewport/public/interfaces/native_viewport.mojom
(right):

https://codereview.chromium.org/1280613003/diff/20001/mojo/services/native_vi...
mojo/services/native_viewport/public/interfaces/native_viewport.mojom:54:
interface NativeViewportShellService {
Should this be in another place. This should not be in any public API, should
it?

https://codereview.chromium.org/1280613003/diff/20001/services/native_viewpor...
File services/native_viewport/BUILD.gn (right):

https://codereview.chromium.org/1280613003/diff/20001/services/native_viewpor...
services/native_viewport/BUILD.gn:30: "//shell:java",
As discussed offline, this layering violation is a problem because on
non-android platform this application is not bundled in the shell.

https://codereview.chromium.org/1280613003/diff/20001/shell/android/apk/src/o...
File shell/android/apk/src/org/chromium/mojo/shell/ViewportActivity.java
(right):

https://codereview.chromium.org/1280613003/diff/20001/shell/android/apk/src/o...
shell/android/apk/src/org/chromium/mojo/shell/ViewportActivity.java:54:
Log.e(TAG, "Received another intent; this activity instance should only ever
receive one.");
Just throws a runtime exception if this is not an intended behavior.

https://codereview.chromium.org/1280613003/diff/20001/shell/android/native_vi...
File shell/android/native_viewport_application_loader.cc (right):

https://codereview.chromium.org/1280613003/diff/20001/shell/android/native_vi...
shell/android/native_viewport_application_loader.cc:67: if (native_viewport &&
native_viewport->NewNativeSurfaceAvailable()) {
No {} on a single if (following what is done in this file.)

[etiennej, 2015-08-07 10:16:37]

https://codereview.chromium.org/1280613003/diff/20001/mojo/services/native_vi...
File mojo/services/native_viewport/public/interfaces/native_viewport.mojom
(right):

https://codereview.chromium.org/1280613003/diff/20001/mojo/services/native_vi...
mojo/services/native_viewport/public/interfaces/native_viewport.mojom:54:
interface NativeViewportShellService {
On 2015/08/06 14:13:20, qsr wrote:
> Should this be in another place. This should not be in any public API, should
> it?

Done.

https://codereview.chromium.org/1280613003/diff/20001/services/native_viewpor...
File services/native_viewport/BUILD.gn (right):

https://codereview.chromium.org/1280613003/diff/20001/services/native_viewpor...
services/native_viewport/BUILD.gn:30: "//shell:java",
On 2015/08/06 14:13:20, qsr wrote:
> As discussed offline, this layering violation is a problem because on
> non-android platform this application is not bundled in the shell.

Done.

https://codereview.chromium.org/1280613003/diff/20001/shell/android/apk/src/o...
File shell/android/apk/src/org/chromium/mojo/shell/ViewportActivity.java
(right):

https://codereview.chromium.org/1280613003/diff/20001/shell/android/apk/src/o...
shell/android/apk/src/org/chromium/mojo/shell/ViewportActivity.java:54:
Log.e(TAG, "Received another intent; this activity instance should only ever
receive one.");
On 2015/08/06 14:13:20, qsr wrote:
> Just throws a runtime exception if this is not an intended behavior.

Done.

https://codereview.chromium.org/1280613003/diff/20001/shell/android/native_vi...
File shell/android/native_viewport_application_loader.cc (right):

https://codereview.chromium.org/1280613003/diff/20001/shell/android/native_vi...
shell/android/native_viewport_application_loader.cc:67: if (native_viewport &&
native_viewport->NewNativeSurfaceAvailable()) {
On 2015/08/06 14:13:20, qsr wrote:
> No {} on a single if (following what is done in this file.)

No longer applicable.

[qsr, 2015-08-07 11:18:01]

https://codereview.chromium.org/1280613003/diff/40001/services/native_viewpor...
File services/native_viewport/platform_viewport_android.cc (right):

https://codereview.chromium.org/1280613003/diff/40001/services/native_viewpor...
services/native_viewport/platform_viewport_android.cc:85:
java_platform_viewport_request_ =
Is this name correct? This is not a request, but a PlatformViewportAndroid

https://codereview.chromium.org/1280613003/diff/40001/services/native_viewpor...
services/native_viewport/platform_viewport_android.cc:169:
application_->ConnectToService("mojo:task_launcher", &shell_service);
task_launcher is awfully general. What about platform_viewport_support?

https://codereview.chromium.org/1280613003/diff/40001/shell/android/apk/src/o...
File shell/android/apk/src/org/chromium/mojo/shell/ViewportActivity.java
(right):

https://codereview.chromium.org/1280613003/diff/40001/shell/android/apk/src/o...
shell/android/apk/src/org/chromium/mojo/shell/ViewportActivity.java:54:
protected void onActivityResult(int requestCode, int resultCode, Intent data) {
This seems wrong. We should receive how the intent registery is supposed to
work...

https://codereview.chromium.org/1280613003/diff/40001/shell/android/main.cc
File shell/android/main.cc (right):

https://codereview.chromium.org/1280613003/diff/40001/shell/android/main.cc#n...
shell/android/main.cc:207:
g_internal_data.Get().native_viewport->NewNativeSurfaceAvailable();
This seems outdated.

https://codereview.chromium.org/1280613003/diff/40001/shell/android/task_laun...
File shell/android/task_launcher/BUILD.gn (right):

https://codereview.chromium.org/1280613003/diff/40001/shell/android/task_laun...
shell/android/task_launcher/BUILD.gn:10:
mojo_android_application("task_launcher") {
Couldn't this be a pure java application?

Also, maybe it should be bundled in the shell, especially because the native
viewport service is.

[etiennej, 2015-08-07 12:22:11]

https://codereview.chromium.org/1280613003/diff/40001/services/native_viewpor...
File services/native_viewport/platform_viewport_android.cc (right):

https://codereview.chromium.org/1280613003/diff/40001/services/native_viewpor...
services/native_viewport/platform_viewport_android.cc:85:
java_platform_viewport_request_ =
On 2015/08/07 11:18:01, qsr wrote:
> Is this name correct? This is not a request, but a PlatformViewportAndroid

Done.

https://codereview.chromium.org/1280613003/diff/40001/services/native_viewpor...
services/native_viewport/platform_viewport_android.cc:169:
application_->ConnectToService("mojo:task_launcher", &shell_service);
On 2015/08/07 11:18:00, qsr wrote:
> task_launcher is awfully general. What about platform_viewport_support?

Done.

https://codereview.chromium.org/1280613003/diff/40001/shell/android/apk/src/o...
File shell/android/apk/src/org/chromium/mojo/shell/ViewportActivity.java
(right):

https://codereview.chromium.org/1280613003/diff/40001/shell/android/apk/src/o...
shell/android/apk/src/org/chromium/mojo/shell/ViewportActivity.java:54:
protected void onActivityResult(int requestCode, int resultCode, Intent data) {
On 2015/08/07 11:18:01, qsr wrote:
> This seems wrong. We should receive how the intent registery is supposed to
> work...

Removed for now. Let's just have MojoShellActivity interacting with the outside
world.

https://codereview.chromium.org/1280613003/diff/40001/shell/android/main.cc
File shell/android/main.cc (right):

https://codereview.chromium.org/1280613003/diff/40001/shell/android/main.cc#n...
shell/android/main.cc:207:
g_internal_data.Get().native_viewport->NewNativeSurfaceAvailable();
On 2015/08/07 11:18:01, qsr wrote:
> This seems outdated.

Done.

https://codereview.chromium.org/1280613003/diff/40001/shell/android/task_laun...
File shell/android/task_launcher/BUILD.gn (right):

https://codereview.chromium.org/1280613003/diff/40001/shell/android/task_laun...
shell/android/task_launcher/BUILD.gn:10:
mojo_android_application("task_launcher") {
On 2015/08/07 11:18:01, qsr wrote:
> Couldn't this be a pure java application?
> 
> Also, maybe it should be bundled in the shell, especially because the native
> viewport service is.

I discovered JavaApplicationRegistery, so done.

[etiennej, 2015-08-07 13:50:33]

I added the todos, btw.

[qsr, 2015-08-07 14:09:39]

LGTM with nits.

https://codereview.chromium.org/1280613003/diff/60001/services/native_viewpor...
File services/native_viewport/platform_viewport_android.cc (right):

https://codereview.chromium.org/1280613003/diff/60001/services/native_viewpor...
services/native_viewport/platform_viewport_android.cc:74:
reinterpret_cast<jlong>(this));
reinterpret_cast should be to intptr_t, here and everywhere on this file.

https://codereview.chromium.org/1280613003/diff/80001/shell/android/apk/src/o...
File
shell/android/apk/src/org/chromium/mojo/shell/NativeViewportSupportApplicationDelegate.java
(right):

https://codereview.chromium.org/1280613003/diff/80001/shell/android/apk/src/o...
shell/android/apk/src/org/chromium/mojo/shell/NativeViewportSupportApplicationDelegate.java:44:
class NativeViewportShellServiceFactoryBinder
classes should probably be private, and declared at the top of this class.

[etiennej, 2015-08-07 14:15:40]

https://codereview.chromium.org/1280613003/diff/60001/services/native_viewpor...
File services/native_viewport/platform_viewport_android.cc (right):

https://codereview.chromium.org/1280613003/diff/60001/services/native_viewpor...
services/native_viewport/platform_viewport_android.cc:74:
reinterpret_cast<jlong>(this));
On 2015/08/07 14:09:39, qsr wrote:
> reinterpret_cast should be to intptr_t, here and everywhere on this file.

Done.

https://codereview.chromium.org/1280613003/diff/80001/shell/android/apk/src/o...
File
shell/android/apk/src/org/chromium/mojo/shell/NativeViewportSupportApplicationDelegate.java
(right):

https://codereview.chromium.org/1280613003/diff/80001/shell/android/apk/src/o...
shell/android/apk/src/org/chromium/mojo/shell/NativeViewportSupportApplicationDelegate.java:44:
class NativeViewportShellServiceFactoryBinder
On 2015/08/07 14:09:39, qsr wrote:
> classes should probably be private, and declared at the top of this class.

Done.

[etiennej, 2015-08-07 14:26:18]

Committed patchset #5 (id:100001) manually as
fbd9af0eb56892790b377a2724dce586b13676b9 (presubmit successful).