Scaffolding for Android implementation of PaymentRequest.

Scaffolding for Android implementation of PaymentRequest.

This patch places ServiceRegistryAndroid in content/public/, so Chrome
can register Android-specific services for Mojo. ServiceRegistryAndroid
is a pure virtual interface, implemented by ServiceRegistryAndroidImpl
in content/browser/android/.

The first user of the Android-specific features of Mojo in Chrome is the
PaymentRequest service. This patch adds a skeleton implementation of
this service.

BUG=587995
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/2f6c373bd44bfbaad1468d86d13293b4a8640c58
Cr-Commit-Position: refs/heads/master@{#385647}

[please use gerrit instead, 2016-03-25 19:46:18]

rouslan@chromium.org changed reviewers:
+ tedchoc@chromium.org

[please use gerrit instead, 2016-03-25 19:46:19]

tedchoc@, ptal patch 5.

Ignore the trybot failures. I automatically typed in "git cl try", even though
that fails when your patch depends on some other patch
(https://codereview.chromium.org/1753543002#ps780001).

[please use gerrit instead, 2016-03-25 21:00:35]

Description was changed from

==========
Scaffolding for Android implementation of PaymentRequest.

BUG=587995
==========

to

==========
Scaffolding for Android implementation of PaymentRequest.

BUG=587995
==========

[Ted C, 2016-03-28 22:46:01]

https://codereview.chromium.org/1831173002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestActivity.java
(right):

https://codereview.chromium.org/1831173002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestActivity.java:19:
public class PaymentRequestActivity implements PaymentRequest {
Activity?  Or Dialog?

Although I was not 100% anti-activity in the design doc, I realized that android
is more likely to kill the background page if you use an activity, so I think
you should avoid that at all possible.

https://codereview.chromium.org/1831173002/diff/80001/chrome/browser/android/...
File chrome/browser/android/DEPS (right):

https://codereview.chromium.org/1831173002/diff/80001/chrome/browser/android/...
chrome/browser/android/DEPS:10:
"+content/browser/mojo/service_registry_android.h",
can't do this...content/public is the only thing we can depend on.

https://codereview.chromium.org/1831173002/diff/80001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/1831173002/diff/80001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:293: void
RegisterRenderProcessAndroidMojoServices(
this is an odd api.  We should really try to use one of the existing
Register<...> calls and figure out a way to make that work.

I wonder if we should be exposing the java methods on the base registry
(ifdef'd) and then be able to use that anywhere.  That would solve the
dependency issue as well, but I'm honestly not the best person to talk about
mojo things (I've done none myself, so I think you'd want to rope in someone
else for that discussion).

[please use gerrit instead, 2016-03-29 22:49:08]

rouslan@chromium.org changed reviewers:
+ nick@chromium.org

[please use gerrit instead, 2016-03-29 22:49:09]

+nick@ as a //src/content/ owner.

nick@: what do you think about tedchoc@'s suggestion to expose Java-specific
methods in //src/content/public/common/service_registry.h? This would allow
Chrome to register Java services for mojo.

[ncarter (slow), 2016-03-29 23:07:18]

nick@chromium.org changed reviewers:
+ rockot@chromium.org

[ncarter (slow), 2016-03-29 23:07:19]

+rockot, could you provide some advice on how best to to expose
ServiceRegistryAndroid to the content embedder?

[ncarter (slow), 2016-03-29 23:08:02]

nick@chromium.org changed reviewers:
+ jam@chromium.org

[ncarter (slow), 2016-03-29 23:08:03]

rocket looks ooo, so adding jam instead

[please use gerrit instead, 2016-03-30 13:43:34]

jam@: what do you think about tedchoc@'s suggestion to expose Java-specific
methods in //src/content/public/common/service_registry.h? This would allow
Chrome to register Java services for mojo.

[jam, 2016-03-30 15:58:34]

jam@chromium.org changed reviewers:
+ sammc@chromium.org

[jam, 2016-03-30 15:58:59]

On 2016/03/30 13:43:34, Rouslan wrote:
> jam@: what do you think about tedchoc@'s suggestion to expose Java-specific
> methods in //src/content/public/common/service_registry.h? This would allow
> Chrome to register Java services for mojo.

That sounds reasonable.

+sammc who added ServiceRegistry.

[please use gerrit instead, 2016-03-30 19:45:54]

Description was changed from

==========
Scaffolding for Android implementation of PaymentRequest.

BUG=587995
==========

to

==========
Scaffolding for Android implementation of PaymentRequest.

BUG=587995
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[please use gerrit instead, 2016-03-30 21:16:26]

Description was changed from

==========
Scaffolding for Android implementation of PaymentRequest.

BUG=587995
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Scaffolding for Android implementation of PaymentRequest.

This patch enables Chrome to register Java implementations of Mojo
services by moving the Java-specific methods from ServiceRegistryAndroid
to ServiceRegistry. ServiceRegistry is the public API of content and is
already piped into Chrome prior to this patch.

The move of Java-specific methods into ServiceRegistry is accomplished
via composition of ServiceRegistryAndroid inside of ServiceRegistryImpl,
which is hidden from non-Android operating systems behind "#if
defined(OS_ANDROID)".

The first user of the Android-specific features of Mojo in Chrome is the
PaymentRequest service. This patch adds a skeleton implementation of
this service.

BUG=587995
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[please use gerrit instead, 2016-03-30 21:16:58]

Description was changed from

==========
Scaffolding for Android implementation of PaymentRequest.

This patch enables Chrome to register Java implementations of Mojo
services by moving the Java-specific methods from ServiceRegistryAndroid
to ServiceRegistry. ServiceRegistry is the public API of content and is
already piped into Chrome prior to this patch.

The move of Java-specific methods into ServiceRegistry is accomplished
via composition of ServiceRegistryAndroid inside of ServiceRegistryImpl,
which is hidden from non-Android operating systems behind "#if
defined(OS_ANDROID)".

The first user of the Android-specific features of Mojo in Chrome is the
PaymentRequest service. This patch adds a skeleton implementation of
this service.

BUG=587995
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Scaffolding for Android implementation of PaymentRequest.

This patch enables Chrome to register Java implementations of Mojo
services by exposing the Java-specific methods from
ServiceRegistryAndroid to ServiceRegistry. ServiceRegistry is the public
API of content and is already piped into Chrome prior to this patch.

The move of Java-specific methods into ServiceRegistry is accomplished
via composition of ServiceRegistryAndroid inside of ServiceRegistryImpl,
which is hidden from non-Android operating systems behind "#if
defined(OS_ANDROID)".

The first user of the Android-specific features of Mojo in Chrome is the
PaymentRequest service. This patch adds a skeleton implementation of
this service.

BUG=587995
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[please use gerrit instead, 2016-03-30 21:17:17]

Description was changed from

==========
Scaffolding for Android implementation of PaymentRequest.

This patch enables Chrome to register Java implementations of Mojo
services by exposing the Java-specific methods from
ServiceRegistryAndroid to ServiceRegistry. ServiceRegistry is the public
API of content and is already piped into Chrome prior to this patch.

The move of Java-specific methods into ServiceRegistry is accomplished
via composition of ServiceRegistryAndroid inside of ServiceRegistryImpl,
which is hidden from non-Android operating systems behind "#if
defined(OS_ANDROID)".

The first user of the Android-specific features of Mojo in Chrome is the
PaymentRequest service. This patch adds a skeleton implementation of
this service.

BUG=587995
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Scaffolding for Android implementation of PaymentRequest.

This patch enables Chrome to register Java implementations of Mojo
services by exposing the Java-specific methods of ServiceRegistryAndroid
in ServiceRegistry. ServiceRegistry is the public API of content and is
already piped into Chrome prior to this patch.

The move of Java-specific methods into ServiceRegistry is accomplished
via composition of ServiceRegistryAndroid inside of ServiceRegistryImpl,
which is hidden from non-Android operating systems behind "#if
defined(OS_ANDROID)".

The first user of the Android-specific features of Mojo in Chrome is the
PaymentRequest service. This patch adds a skeleton implementation of
this service.

BUG=587995
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[please use gerrit instead, 2016-03-30 21:18:12]

Description was changed from

==========
Scaffolding for Android implementation of PaymentRequest.

This patch enables Chrome to register Java implementations of Mojo
services by exposing the Java-specific methods of ServiceRegistryAndroid
in ServiceRegistry. ServiceRegistry is the public API of content and is
already piped into Chrome prior to this patch.

The move of Java-specific methods into ServiceRegistry is accomplished
via composition of ServiceRegistryAndroid inside of ServiceRegistryImpl,
which is hidden from non-Android operating systems behind "#if
defined(OS_ANDROID)".

The first user of the Android-specific features of Mojo in Chrome is the
PaymentRequest service. This patch adds a skeleton implementation of
this service.

BUG=587995
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Scaffolding for Android implementation of PaymentRequest.

This patch enables Chrome to register Java implementations of Mojo
services by exposing in ServiceRegistry some of the methods of
ServiceRegistryAndroid. ServiceRegistry is the public API of content and
is already piped into Chrome prior to this patch.

The move of Java-specific methods into ServiceRegistry is accomplished
via composition of ServiceRegistryAndroid inside of ServiceRegistryImpl,
which is hidden from non-Android operating systems behind "#if
defined(OS_ANDROID)".

The first user of the Android-specific features of Mojo in Chrome is the
PaymentRequest service. This patch adds a skeleton implementation of
this service.

BUG=587995
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[please use gerrit instead, 2016-03-30 21:19:21]

Description was changed from

==========
Scaffolding for Android implementation of PaymentRequest.

This patch enables Chrome to register Java implementations of Mojo
services by exposing in ServiceRegistry some of the methods of
ServiceRegistryAndroid. ServiceRegistry is the public API of content and
is already piped into Chrome prior to this patch.

The move of Java-specific methods into ServiceRegistry is accomplished
via composition of ServiceRegistryAndroid inside of ServiceRegistryImpl,
which is hidden from non-Android operating systems behind "#if
defined(OS_ANDROID)".

The first user of the Android-specific features of Mojo in Chrome is the
PaymentRequest service. This patch adds a skeleton implementation of
this service.

BUG=587995
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Scaffolding for Android implementation of PaymentRequest.

This patch enables Chrome to register Java implementations of Mojo
services by exposing in ServiceRegistry some of the methods of
ServiceRegistryAndroid. ServiceRegistry is the public API of content and
is already piped into Chrome prior to this patch.

ServiceRegistryImpl now owns an instance of ServiceRegistryAndroid,
which is hidden from non-Android operating systems behind "#if
defined(OS_ANDROID)".

The first user of the Android-specific features of Mojo in Chrome is the
PaymentRequest service. This patch adds a skeleton implementation of
this service.

BUG=587995
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[please use gerrit instead, 2016-03-30 21:22:01]

Description was changed from

==========
Scaffolding for Android implementation of PaymentRequest.

This patch enables Chrome to register Java implementations of Mojo
services by exposing in ServiceRegistry some of the methods of
ServiceRegistryAndroid. ServiceRegistry is the public API of content and
is already piped into Chrome prior to this patch.

ServiceRegistryImpl now owns an instance of ServiceRegistryAndroid,
which is hidden from non-Android operating systems behind "#if
defined(OS_ANDROID)".

The first user of the Android-specific features of Mojo in Chrome is the
PaymentRequest service. This patch adds a skeleton implementation of
this service.

BUG=587995
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Scaffolding for Android implementation of PaymentRequest.

This patch enables Chrome to register Java implementations of Mojo
services by exposing in ServiceRegistry some of the methods of
ServiceRegistryAndroid. ServiceRegistry is the public API of content and
is already piped into Chrome prior to this patch.

ServiceRegistryImpl now owns an instance of ServiceRegistryAndroid,
which is hidden from non-Android operating systems behind "#if
defined(OS_ANDROID)". Only ServiceRegistryImpl should reference
ServiceRegistryAndroid directly from now on.

The first user of the Android-specific features of Mojo in Chrome is the
PaymentRequest service. This patch adds a skeleton implementation of
this service.

BUG=587995
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[please use gerrit instead, 2016-03-30 22:04:49]

rouslan@chromium.org changed reviewers:
- rockot@chromium.org

[please use gerrit instead, 2016-03-30 22:04:49]

jam@, sammc@: ptal patch 9.

I've exposed GetJavaObj() inside of ServiceRegistry. Chrome sends this object to
ChromeServiceRegistrar to add its own services to the list.

[Sam McNally, 2016-03-31 23:40:26]

It feels a bit weird to have Java-specific methods in something that's shared
with non-browser processes, but otherwise this looks OK.

https://codereview.chromium.org/1831173002/diff/140002/content/common/mojo/se...
File content/common/mojo/service_registry_impl.cc (right):

https://codereview.chromium.org/1831173002/diff/140002/content/common/mojo/se...
content/common/mojo/service_registry_impl.cc:20: android_impl_(new
ServiceRegistryAndroid(this)),
Can you delay creating this until the first call to GetJavaObj()?

[please use gerrit instead, 2016-04-01 01:23:27]

sammc@, ptal patch 12.

On 2016/03/31 23:40:26, Sam McNally wrote:
> It feels a bit weird to have Java-specific methods in something that's shared
> with non-browser processes, but otherwise this looks OK.

I'm open to your ideas and suggestions. My initial gut reaction was to add a new
method to content_browser_client.h:

#if defined(OS_ANDROID)
  virtual void RegisterRenderProcessAndroidMojoServices(
      ServiceRegistryAndroid* registry) {}
#endif

tedchoc@ suggested instead that I place the Java-specific methods in
ServiceRegistry. This lets Chrome re-use the existing Register* methods without
#ifdef in the API. After making the necessary changes, I think this is a good
direction, because it lets us delete ServiceRegistryAndroid from such places as
render_frame_host_impl.h and mojo_application_host.h. This makes the code
cleaner, IMHO.

https://codereview.chromium.org/1831173002/diff/140002/content/common/mojo/se...
File content/common/mojo/service_registry_impl.cc (right):

https://codereview.chromium.org/1831173002/diff/140002/content/common/mojo/se...
content/common/mojo/service_registry_impl.cc:20: android_impl_(new
ServiceRegistryAndroid(this)),
On 2016/03/31 23:40:26, Sam McNally wrote:
> Can you delay creating this until the first call to GetJavaObj()?

Done.

[ncarter (slow), 2016-04-04 21:32:19]

Overall, I like the amount of simplification here.

Help me understand one thing: Because this code is in content/common,
ServiceRegistryImpl::GetJavaObj will be callable from content/renderer -- will
that be a problem? My understanding was that we don't use Java from renderer
processes, but I assume we do from other child process types (the gpu process,
presumably?).

[please use gerrit instead, 2016-04-04 21:57:07]

On 2016/04/04 21:32:19, ncarter wrote:
> Because this code is in content/common,
> ServiceRegistryImpl::GetJavaObj will be callable from content/renderer

That was not my intention. I can resolve it this way:

1) Move service_registry_android to content/public/browser/mojo/.
2) Remove GetJavaObj() from service_registry.
3) Add "static ServiceRegistryAndroid* FromServiceRegistry(ServiceRegistry*)"
method to service_registry_android.
4) ServiceRegistry will maintain a private pointer to ServiceRegistryAndroid.
5) ServiceRegistryAndroid will be marked a "friend" of ServiceRegistry so that
FromServiceRegistry() method can return the Android-specific private member.

Does this sound sensible to you?

[ncarter (slow), 2016-04-04 23:28:09]

On 2016/04/04 21:57:07, Rouslan wrote:
> On 2016/04/04 21:32:19, ncarter wrote:
> > Because this code is in content/common,
> > ServiceRegistryImpl::GetJavaObj will be callable from content/renderer
> 
> That was not my intention. I can resolve it this way:
> 
> 1) Move service_registry_android to content/public/browser/mojo/.
> 2) Remove GetJavaObj() from service_registry.
> 3) Add "static ServiceRegistryAndroid* FromServiceRegistry(ServiceRegistry*)"
> method to service_registry_android.
> 4) ServiceRegistry will maintain a private pointer to ServiceRegistryAndroid.
> 5) ServiceRegistryAndroid will be marked a "friend" of ServiceRegistry so that
> FromServiceRegistry() method can return the Android-specific private member.
> 
> Does this sound sensible to you?

What if we did something like this --
https://codereview.chromium.org/1858103002/? The idea is to have a
specialization of ServiceRegistry called BrowserServiceRegistry, that is defined
in content/browser, and behaves just like a normal ServiceRegistry, but it also
knows how to register Java methods.

[please use gerrit instead, 2016-04-05 00:45:38]

Description was changed from

==========
Scaffolding for Android implementation of PaymentRequest.

This patch enables Chrome to register Java implementations of Mojo
services by exposing in ServiceRegistry some of the methods of
ServiceRegistryAndroid. ServiceRegistry is the public API of content and
is already piped into Chrome prior to this patch.

ServiceRegistryImpl now owns an instance of ServiceRegistryAndroid,
which is hidden from non-Android operating systems behind "#if
defined(OS_ANDROID)". Only ServiceRegistryImpl should reference
ServiceRegistryAndroid directly from now on.

The first user of the Android-specific features of Mojo in Chrome is the
PaymentRequest service. This patch adds a skeleton implementation of
this service.

BUG=587995
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Scaffolding for Android implementation of PaymentRequest.

// TODO(rouslan): Write a description when the dust has settled.

The first user of the Android-specific features of Mojo in Chrome is the
PaymentRequest service. This patch adds a skeleton implementation of
this service.

BUG=587995
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[please use gerrit instead, 2016-04-05 00:53:04]

On 2016/04/04 23:28:09, ncarter wrote:
> What if we did something like this --
> https://codereview.chromium.org/1858103002/ The idea is to have a
> specialization of ServiceRegistry called BrowserServiceRegistry, that is
defined
> in content/browser, and behaves just like a normal ServiceRegistry, but it
also
> knows how to register Java methods.

I'm checking out your proposal. Meanwhile checkout the implementation of
ServiceRegistryAndroid::From(ServiceRegistry) in patch 16.

[ncarter (slow), 2016-04-05 18:22:40]

Using SupportsUserData is a solution that works as well. I am happy with this
approach.

https://codereview.chromium.org/1831173002/diff/290001/content/public/browser...
File content/public/browser/android/service_registry_android.h (right):

https://codereview.chromium.org/1831173002/diff/290001/content/public/browser...
content/public/browser/android/service_registry_android.h:19: class
CONTENT_EXPORT ServiceRegistryAndroid : private ServiceRegistry::Data {
content/public contains only interfaces & structs, and static functions. (
https://www.chromium.org/developers/content-module/content-api )

So you'll need to split this up into a ServiceRegistryAndroid (a pure virtual
interface class with no data members) and ServiceRegistryAndroidImpl (somewhere
in content/browser/.., not content/public) which implements
ServiceRegistryAndroid in terms of ServiceRegistryImpl::Data).

There's no need for a service_registry_android.cc, methods like
~ServiceRegistryAndroid() and ServiceRegistryAndroid::From can be declared in
service_registry_android.h and defined in the service_registry_android_impl.cc
file.

https://codereview.chromium.org/1831173002/diff/290001/content/public/common/...
File content/public/common/service_registry.h (right):

https://codereview.chromium.org/1831173002/diff/290001/content/public/common/...
content/public/common/service_registry.h:26: class CONTENT_EXPORT
ServiceRegistry : private base::SupportsUserData {
For SupportsUserData, instead of private inheritance on the content/public
interface, let's just use public inheritance on the implementation class,
ServiceRegistryImpl.

[please use gerrit instead, 2016-04-06 01:05:47]

Description was changed from

==========
Scaffolding for Android implementation of PaymentRequest.

// TODO(rouslan): Write a description when the dust has settled.

The first user of the Android-specific features of Mojo in Chrome is the
PaymentRequest service. This patch adds a skeleton implementation of
this service.

BUG=587995
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Scaffolding for Android implementation of PaymentRequest.

This patch places ServiceRegistryAndroid in content/public/, so Chrome
can register Android-specific services for Mojo. ServiceRegistryAndroid
is a pure virtual interface, implemented by ServiceRegistryAndroidImpl
in content/browser/android/.

The first user of the Android-specific features of Mojo in Chrome is the
PaymentRequest service. This patch adds a skeleton implementation of
this service.

BUG=587995
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[please use gerrit instead, 2016-04-06 03:04:57]

ncarter@: ptal patch 20, which contains the implementation upon which we've
settled over VC.

[please use gerrit instead, 2016-04-06 17:53:28]

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

[commit-bot: I haz the power, 2016-04-06 17:53:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1831173002/370001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1831173002/370001

[ncarter (slow), 2016-04-06 18:17:37]

lgtm with nits

Thanks for all the iterations you did to get this implemented so cleanly!

https://codereview.chromium.org/1831173002/diff/370001/content/browser/androi...
File content/browser/android/service_registry_android_impl.cc (right):

https://codereview.chromium.org/1831173002/diff/370001/content/browser/androi...
content/browser/android/service_registry_android_impl.cc:40: // statc
typo

https://codereview.chromium.org/1831173002/diff/370001/content/browser/androi...
File content/browser/android/service_registry_android_impl.h (right):

https://codereview.chromium.org/1831173002/diff/370001/content/browser/androi...
content/browser/android/service_registry_android_impl.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
2016, since it's a new file.

https://codereview.chromium.org/1831173002/diff/370001/content/browser/androi...
content/browser/android/service_registry_android_impl.h:5: #ifndef
CONTENT_BROWSER_ANDROID_SERVICE_REGISTRY_ANDROID_H_
Update these to _IMPL_H_

[please use gerrit instead, 2016-04-06 18:30:13]

tedchoc@, ptal in patch 21:

chrome/browser/android/*
chrome/android/*

https://codereview.chromium.org/1831173002/diff/370001/content/browser/androi...
File content/browser/android/service_registry_android_impl.cc (right):

https://codereview.chromium.org/1831173002/diff/370001/content/browser/androi...
content/browser/android/service_registry_android_impl.cc:40: // statc
On 2016/04/06 18:17:37, ncarter wrote:
> typo

Done.

https://codereview.chromium.org/1831173002/diff/370001/content/browser/androi...
File content/browser/android/service_registry_android_impl.h (right):

https://codereview.chromium.org/1831173002/diff/370001/content/browser/androi...
content/browser/android/service_registry_android_impl.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2016/04/06 18:17:37, ncarter wrote:
> 2016, since it's a new file.

Done.

https://codereview.chromium.org/1831173002/diff/370001/content/browser/androi...
content/browser/android/service_registry_android_impl.h:5: #ifndef
CONTENT_BROWSER_ANDROID_SERVICE_REGISTRY_ANDROID_H_
On 2016/04/06 18:17:37, ncarter wrote:
> Update these to _IMPL_H_

Done.

[please use gerrit instead, 2016-04-06 18:34:44]

https://codereview.chromium.org/1831173002/diff/390001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ServiceRegistry.java
(right):

https://codereview.chromium.org/1831173002/diff/390001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ServiceRegistry.java:5:
package org.chromium.content.browser;

tedchoc@: Should this package change from org.chromium.content.browser to
org.chromium.content_public.browser?

[Ted C, 2016-04-06 19:46:57]

lgtm w/ a nits

https://codereview.chromium.org/1831173002/diff/390001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/mojo/ChromeServiceRegistrar.java
(right):

https://codereview.chromium.org/1831173002/diff/390001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/mojo/ChromeServiceRegistrar.java:14:
class ChromeServiceRegistrar {
quick javadoc

https://codereview.chromium.org/1831173002/diff/390001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/mojo/ChromeServiceRegistrar.java:16:
static void registerRenderFrameMojoServices(
make this private

https://codereview.chromium.org/1831173002/diff/390001/chrome/browser/android...
File chrome/browser/android/chrome_jni_registrar.cc (right):

https://codereview.chromium.org/1831173002/diff/390001/chrome/browser/android...
chrome/browser/android/chrome_jni_registrar.cc:249: {"ChromeServiceRegistrar",
ChromeServiceRegistrarAndroid::RegisterJni},
Simply Register or RegisterChromeServiceRegistrar are the norms in this file.

https://codereview.chromium.org/1831173002/diff/390001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ServiceRegistry.java
(right):

https://codereview.chromium.org/1831173002/diff/390001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ServiceRegistry.java:5:
package org.chromium.content.browser;
On 2016/04/06 18:34:44, Rouslan wrote:
> 
> tedchoc@: Should this package change from org.chromium.content.browser to
> org.chromium.content_public.browser?

It probably should go into content_public, but as an interface, and the we'd
need ServiceRegistryImpl in content.  Happy to have that be done in a follow up
instead of this change getting too large.

[please use gerrit instead, 2016-04-06 20:07:53]

rouslan@chromium.org changed reviewers:
+ esprehn@chromium.org, thestig@chromium.org

[please use gerrit instead, 2016-04-06 20:07:55]

esprehn@ ptal third_party/WebKit/public/blink.gyp -- generating java bindings
for blink's mojo.

thestig@ ptal chrome/chrome.gyp -- making chrome_java depend blink's java mojo
bindings.

https://codereview.chromium.org/1831173002/diff/390001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/mojo/ChromeServiceRegistrar.java
(right):

https://codereview.chromium.org/1831173002/diff/390001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/mojo/ChromeServiceRegistrar.java:14:
class ChromeServiceRegistrar {
On 2016/04/06 19:46:56, Ted C (OOO 4.7-4.13) wrote:
> quick javadoc

Done.

https://codereview.chromium.org/1831173002/diff/390001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/mojo/ChromeServiceRegistrar.java:16:
static void registerRenderFrameMojoServices(
On 2016/04/06 19:46:56, Ted C (OOO 4.7-4.13) wrote:
> make this private

Done.

https://codereview.chromium.org/1831173002/diff/390001/chrome/browser/android...
File chrome/browser/android/chrome_jni_registrar.cc (right):

https://codereview.chromium.org/1831173002/diff/390001/chrome/browser/android...
chrome/browser/android/chrome_jni_registrar.cc:249: {"ChromeServiceRegistrar",
ChromeServiceRegistrarAndroid::RegisterJni},
On 2016/04/06 19:46:56, Ted C (OOO 4.7-4.13) wrote:
> Simply Register or RegisterChromeServiceRegistrar are the norms in this file.

Done.

https://codereview.chromium.org/1831173002/diff/390001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ServiceRegistry.java
(right):

https://codereview.chromium.org/1831173002/diff/390001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ServiceRegistry.java:5:
package org.chromium.content.browser;
On 2016/04/06 19:46:56, Ted C (OOO 4.7-4.13) wrote:
> On 2016/04/06 18:34:44, Rouslan wrote:
> > 
> > tedchoc@: Should this package change from org.chromium.content.browser to
> > org.chromium.content_public.browser?
> 
> It probably should go into content_public, but as an interface, and the we'd
> need ServiceRegistryImpl in content.  Happy to have that be done in a follow
up
> instead of this change getting too large.

Writing a follow up.

[please use gerrit instead, 2016-04-06 20:31:55]

On 2016/04/06 20:07:55, Rouslan wrote:
> esprehn@ ptal third_party/WebKit/public/blink.gyp -- generating java bindings
> for blink's mojo.
> 
> thestig@ ptal chrome/chrome.gyp -- making chrome_java depend blink's java mojo
> bindings.

.. in patch 22.

[Lei Zhang, 2016-04-06 20:42:21]

https://codereview.chromium.org/1831173002/diff/410001/chrome/android/BUILD.gn
File chrome/android/BUILD.gn (right):

https://codereview.chromium.org/1831173002/diff/410001/chrome/android/BUILD.g...
chrome/android/BUILD.gn:130: "//mojo/public/java:bindings",
Should chrome/chrome.gyp have these deps as well to match?

https://codereview.chromium.org/1831173002/diff/410001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/1831173002/diff/410001/chrome/chrome.gyp#newc...
chrome/chrome.gyp:603:
'../third_party/WebKit/public/blink.gyp:mojo_bindings_java',
'W' before 'a' in ASCII order, just like in chrome/android/BUILD.gn.

[esprehn, 2016-04-06 21:10:35]

lgtm

https://codereview.chromium.org/1831173002/diff/410001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestDialog.java
(right):

https://codereview.chromium.org/1831173002/diff/410001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestDialog.java:45:
public void abort() {}
Do these need TODO or something? I assume we'll implement all these empty
methods?

https://codereview.chromium.org/1831173002/diff/410001/third_party/WebKit/pub...
File third_party/WebKit/public/blink.gyp (right):

https://codereview.chromium.org/1831173002/diff/410001/third_party/WebKit/pub...
third_party/WebKit/public/blink.gyp:134: 'target_name': 'mojo_bindings_java',
how slow is it going to be generating java output for every mojom in blink when
we don't really use them?

[please use gerrit instead, 2016-04-06 23:07:24]

thestig@ ptal patch 23.

https://codereview.chromium.org/1831173002/diff/410001/chrome/android/BUILD.gn
File chrome/android/BUILD.gn (right):

https://codereview.chromium.org/1831173002/diff/410001/chrome/android/BUILD.g...
chrome/android/BUILD.gn:130: "//mojo/public/java:bindings",
On 2016/04/06 20:42:20, Lei Zhang wrote:
> Should chrome/chrome.gyp have these deps as well to match?

Done.

https://codereview.chromium.org/1831173002/diff/410001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestDialog.java
(right):

https://codereview.chromium.org/1831173002/diff/410001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestDialog.java:45:
public void abort() {}
On 2016/04/06 21:10:35, esprehn wrote:
> Do these need TODO or something? I assume we'll implement all these empty
> methods?

tedchoc@ and I are implementing these empty methods in
https://codereview.chromium.org/1845663003/.

https://codereview.chromium.org/1831173002/diff/410001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/1831173002/diff/410001/chrome/chrome.gyp#newc...
chrome/chrome.gyp:603:
'../third_party/WebKit/public/blink.gyp:mojo_bindings_java',
On 2016/04/06 20:42:20, Lei Zhang wrote:
> 'W' before 'a' in ASCII order, just like in chrome/android/BUILD.gn.

Done.

https://codereview.chromium.org/1831173002/diff/410001/third_party/WebKit/pub...
File third_party/WebKit/public/blink.gyp (right):

https://codereview.chromium.org/1831173002/diff/410001/third_party/WebKit/pub...
third_party/WebKit/public/blink.gyp:134: 'target_name': 'mojo_bindings_java',
On 2016/04/06 21:10:35, esprehn wrote:
> how slow is it going to be generating java output for every mojom in blink
when
> we don't really use them?

Changed to generate java bindings only for PaymentRequest.

[Lei Zhang, 2016-04-06 23:15:52]

chrome/ build files LGTM

[please use gerrit instead, 2016-04-06 23:26:28]

The CQ bit was checked by rouslan@chromium.org

[please use gerrit instead, 2016-04-06 23:26:29]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org,
tedchoc@chromium.org, esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1831173002/#ps430001
(title: "thestig@ & esprehn@ comments")

[commit-bot: I haz the power, 2016-04-06 23:27:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1831173002/430001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1831173002/430001

[please use gerrit instead, 2016-04-07 02:22:56]

The CQ bit was unchecked by rouslan@chromium.org

[please use gerrit instead, 2016-04-07 03:41:00]

The CQ bit was checked by rouslan@chromium.org

[commit-bot: I haz the power, 2016-04-07 03:41:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1831173002/430001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1831173002/430001

[commit-bot: I haz the power, 2016-04-07 03:52:20]

Committed patchset #23 (id:430001)

[commit-bot: I haz the power, 2016-04-07 03:54:39]

Description was changed from

==========
Scaffolding for Android implementation of PaymentRequest.

This patch places ServiceRegistryAndroid in content/public/, so Chrome
can register Android-specific services for Mojo. ServiceRegistryAndroid
is a pure virtual interface, implemented by ServiceRegistryAndroidImpl
in content/browser/android/.

The first user of the Android-specific features of Mojo in Chrome is the
PaymentRequest service. This patch adds a skeleton implementation of
this service.

BUG=587995
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Scaffolding for Android implementation of PaymentRequest.

This patch places ServiceRegistryAndroid in content/public/, so Chrome
can register Android-specific services for Mojo. ServiceRegistryAndroid
is a pure virtual interface, implemented by ServiceRegistryAndroidImpl
in content/browser/android/.

The first user of the Android-specific features of Mojo in Chrome is the
PaymentRequest service. This patch adds a skeleton implementation of
this service.

BUG=587995
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/2f6c373bd44bfbaad1468d86d13293b4a8640c58
Cr-Commit-Position: refs/heads/master@{#385647}
==========

[commit-bot: I haz the power, 2016-04-07 03:54:41]

Patchset 23 (id:??) landed as
https://crrev.com/2f6c373bd44bfbaad1468d86d13293b4a8640c58
Cr-Commit-Position: refs/heads/master@{#385647}