Add global and per-WebContents java InterfaceRegistries.

Add global and per-WebContents java InterfaceRegistries.

BUG=634568, 637174
Committed: https://crrev.com/f5f1b0fdf3a73883a2462ad7d2ed7ee5ec70533b
Cr-Commit-Position: refs/heads/master@{#419886}

[Sam McNally, 2016-08-05 02:21:58]

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

[commit-bot: I haz the power, 2016-08-05 02:22:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-05 03:35:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-05 03:35:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sam McNally, 2016-08-05 04:21:31]

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

[commit-bot: I haz the power, 2016-08-05 04:21:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2016-08-05 04:46:44]

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

[commit-bot: I haz the power, 2016-08-05 04:47:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2016-08-05 05:01:26]

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

[commit-bot: I haz the power, 2016-08-05 05:02:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2016-08-05 05:04:14]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2016-08-05 05:04:36]

Patchset #1 (id:20001) has been deleted

[Sam McNally, 2016-08-05 05:04:44]

Patchset #1 (id:40001) has been deleted

[Sam McNally, 2016-08-05 05:12:16]

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

[commit-bot: I haz the power, 2016-08-05 05:12:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2016-08-05 05:14:34]

Patchset #1 (id:60001) has been deleted

[Sam McNally, 2016-08-05 06:20:42]

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

[commit-bot: I haz the power, 2016-08-05 06:21:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-05 07:21:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-05 07:21:24]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2016-08-10 05:50:13]

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

[commit-bot: I haz the power, 2016-08-10 05:50:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 07:13:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 07:13:42]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2016-08-12 03:26:15]

Patchset #1 (id:80001) has been deleted

[Sam McNally, 2016-08-12 03:26:25]

Patchset #1 (id:100001) has been deleted

[Sam McNally, 2016-08-12 03:32:10]

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

[commit-bot: I haz the power, 2016-08-12 03:32:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2016-08-12 04:01:03]

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

[commit-bot: I haz the power, 2016-08-12 04:01:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2016-08-12 04:12:21]

Patchset #1 (id:120001) has been deleted

[Sam McNally, 2016-08-12 04:12:30]

Patchset #1 (id:140001) has been deleted

[Sam McNally, 2016-08-12 04:40:09]

Description was changed from

==========
Add global and per-WebContents java InterfaceRegistries.

BUG=634568
==========

to

==========
Add global and per-WebContents java InterfaceRegistries.

BUG=634568, 637174
==========

[commit-bot: I haz the power, 2016-08-12 04:45:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-12 04:45:43]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2016-08-12 04:49:55]

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

[Ben Goodger (Google), 2016-08-12 05:15:21]

What's the purpose of this vs. the frame-specific registries?

[Sam McNally, 2016-08-12 05:52:46]

On 2016/08/12 05:15:21, Ben Goodger (Google) wrote:
> What's the purpose of this vs. the frame-specific registries?

Note that the current Java interface implementations are parametrised only by
either the global Context or a WebContents; the Java code doesn't know about
frames. My plan is for each Java interface to be registered on the global or
per-WebContents InterfaceRegistry, via an InterfaceRegistrar, depending on
whether they need a WebContents (so far this has meant whether they display any
UI). For renderer-exposed interfaces, the C++ code is responsible for forwarding
interface requests to the appropriate InterfaceProvider, possibly filtering or
adapting if needed. In effect, the Java parts of the browser act a separate mojo
entity that exposes interfaces to the C++ parts (and probably vice-versa at some
point).

https://codereview.chromium.org/2214383002/ is how I'm planning to use this for
the existing Java-implemented interfaces. For ShareService, I expect the C++
code will soon need to start checking
content::OriginTrialPolicy::IsFeatureDisabled() before forwarding the request
onto the Java implementation.

[Ben Goodger (Google), 2016-08-12 22:04:37]

Sorry - I'm really not that familiar with the java side of things. Can you show
me an example of a java interface impl & how it's parameterized by webcontents?

It's not clear to me we want to introduce a IR at the WC level, especially not
in an OOPIF world. It may make sense for the java code to know about frames.

[Sam McNally, 2016-08-15 08:06:28]

On 2016/08/12 22:04:37, Ben Goodger (Google) wrote:
> Sorry - I'm really not that familiar with the java side of things. Can you
show
> me an example of a java interface impl & how it's parameterized by
webcontents?

The WebContents-comsuming interface impls are the ones registered in
https://chromium.googlesource.com/chromium/src/+/master/chrome/android/java/s...:
ShareService and PaymentRequest.

ShareService
(https://chromium.googlesource.com/chromium/src/+/master/chrome/android/java/s...)
requires a WebContents to look up an Activity (a subclass of Context), which it
uses to send an intent. Note that the global Context passed to other interface
impls isn't an Activity.

PaymentRequest
(https://chromium.googlesource.com/chromium/src/+/master/chrome/android/java/s...)
uses its WebContents for several purposes:
1. getting its title and URL
2. (eventually) passing it to PersonalDataManager.getFullCard(), which passes it
back to C++ code, which uses its main frame
3. looking up an Activity

1 could be replaced by either a factory or client mojo interface. 2 could be
replaced by a mojo interface. 3 is a common requirement with ShareService.

It's possible that we could parametrise interfaces by Activity (or something
else that's convertible to an Activity) instead of by WebContents if
PaymentRequest was refactored.

> 
> It's not clear to me we want to introduce a IR at the WC level, especially not
> in an OOPIF world. It may make sense for the java code to know about frames.

I'm not convinced that Java code necessarily needs to know about frames. It
seems simpler to keep the code that deals with frames in C++ that can be (at
least mostly) shared across platforms. Even if this turns out to be wrong,
migrating from a per-WC IR to a per-frame IR feels like a manageable change.

[Ben Goodger (Google), 2016-08-15 18:04:27]

+creis for an opinion about how features should be scoped within content.

On 2016/08/15 08:06:28, Sam McNally wrote:
> On 2016/08/12 22:04:37, Ben Goodger (Google) wrote:
> > Sorry - I'm really not that familiar with the java side of things. Can you
> show
> > me an example of a java interface impl & how it's parameterized by
> webcontents?
> 
> The WebContents-comsuming interface impls are the ones registered in
>
https://chromium.googlesource.com/chromium/src/+/master/chrome/android/java/s...:
> ShareService and PaymentRequest.
> 
> ShareService
>
(https://chromium.googlesource.com/chromium/src/+/master/chrome/android/java/s...)
> requires a WebContents to look up an Activity (a subclass of Context), which
it
> uses to send an intent. Note that the global Context passed to other interface
> impls isn't an Activity.
> 
> PaymentRequest
>
(https://chromium.googlesource.com/chromium/src/+/master/chrome/android/java/s...)
> uses its WebContents for several purposes:
> 1. getting its title and URL
> 2. (eventually) passing it to PersonalDataManager.getFullCard(), which passes
it
> back to C++ code, which uses its main frame
> 3. looking up an Activity
> 
> 1 could be replaced by either a factory or client mojo interface. 2 could be
> replaced by a mojo interface. 3 is a common requirement with ShareService.
> 
> It's possible that we could parametrise interfaces by Activity (or something
> else that's convertible to an Activity) instead of by WebContents if
> PaymentRequest was refactored.

I'm not an OOPIF expert or an expert on these Android features. But it seems to
me it should be possible to locate some of this from the main frame of the WC.

> I'm not convinced that Java code necessarily needs to know about frames. It
> seems simpler to keep the code that deals with frames in C++ that can be (at
> least mostly) shared across platforms. Even if this turns out to be wrong,
> migrating from a per-WC IR to a per-frame IR feels like a manageable change.

From the perspective of the current calling code perhaps. From the perspective
of having to support another (and in this case platform-specific) scope for
exposing interfaces... seems more complex :-)

-Ben

[Sam McNally, 2016-08-17 07:13:12]

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

[Sam McNally, 2016-08-17 07:13:13]

+creis for real

[Ben Goodger (Google), 2016-08-18 22:35:03]

On 2016/08/17 07:13:13, Sam McNally wrote:
> +creis for real

OK, I finally realized (heh) that you're adding this for C++ code /in the
browser process/ to talk to java code /in the browser process/. So creis
probably doesn't care after all. oops.

I'm a bit uneasy with adding this platform/language-specific thing to WC (I know
there's probably lots of that already but would like to find ways to make it
smaller). I want to understand how this will be used... do you have a specific
use case in mind you can show me?

[Sam McNally, 2016-08-22 21:52:50]

On 2016/08/18 22:35:03, Ben Goodger (Google) wrote:
> On 2016/08/17 07:13:13, Sam McNally wrote:
> > +creis for real
> 
> OK, I finally realized (heh) that you're adding this for C++ code /in the
> browser process/ to talk to java code /in the browser process/. So creis
> probably doesn't care after all. oops.
> 
> I'm a bit uneasy with adding this platform/language-specific thing to WC (I
know
> there's probably lots of that already but would like to find ways to make it
> smaller). I want to understand how this will be used... do you have a specific
> use case in mind you can show me?

The use cases for this aren't strongly coupled to per-WC IRs:
1. Some C++ code wants to make an async call to some Java code, both running in
the browser. https://codereview.chromium.org/2206953002/ was an example of this;
it now has a sync callback as well so is no longer an obvious change to make,
but it should provide a reasonable sense of what the change would look like.

2. A Java-implemented mojo interface needs some parametrisation from C++. This
would range from simply filtering access to providing a delegate interface
implemented in C++. https://codereview.chromium.org/2263213003/ is an example of
the former; I don't have anything to show for the latter, but there is interest
for it.

The per-WC IRs are to support the parts of the Java side already parametrised by
a WC:
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr....
They don't necessarily need to be parametrised by a WC as I mentioned
previously, but that's how they work now.

[Sam McNally, 2016-09-12 08:47:15]

ping

The other use case for this, that I've since learned was public all along, is
gating vibration (in //device) on site engagement (in //chrome). The mojo-based
approach that I've suggested for this is for Java to expose a factory to C++;
the factory would take a delegate that it would query to decide whether to allow
each vibration call. e.g.

interface VibrationManagerFactory {
  Create(
      VibrationManager& vibration_manager,
      VibrationDelegate? delegate);
}

interface VibrationDelegate {
  IsVibrationAllowed() => (bool allowed);
}

VibrationManager is the existing mojo interface currently exposed from Java to
the renderer. The renderer would continue requesting a VibrationManager; the C++
code would get a VibrationManagerFactory from Java and call Create(), passing in
a delegate connection that queries site engagement using the URL of the
RenderFrameHost.

The alternatives to this that we've found are:
1. Add a JNI function to create VibrationManagerImpls from C++ that takes a
delegate and use either JNI or mojo for the delegate (bypassing all of the
existing Java mojo InterfaceRegistry infrastructure).
2. Pass a RenderFrameHost as a long to ExposeInterfacesToFrame() and from there
use JNI to get back into C++, which would turn it back into a RenderFrameHost to
use for constructing the delegate (merely making the existing Java mojo
infrastructure messier).

I can split out the WebContents parts if you'd prefer. That would allow the
vibration changes to progress, at least.

[Charlie Reis, 2016-09-12 20:18:45]

creis@chromium.org changed reviewers:
- creis@chromium.org

[Charlie Reis, 2016-09-12 20:18:46]

[Sorry for missing this earlier; I think had been on vacation.  Anyway, moving
myself to CC, since it sounds like my review isn't needed and the ping is
probably meant for Ben.]

[Ben Goodger (Google), 2016-09-13 21:40:02]

ben@chromium.org changed reviewers:
+ rockot@chromium.org
- ben@chromium.org

[Ben Goodger (Google), 2016-09-13 21:40:25]

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

[Ben Goodger (Google), 2016-09-13 21:40:26]

I'm sorry my calendar is crazy. replacing self with ken.

[Ken Rockot(use gerrit already), 2016-09-14 14:31:54]

Scoping a Java registry to WebContents feels pretty misleading to me. It does
seem like Activity is the right context in these cases, and other data can be
moved around via additional interfaces in either direction. WDYT?

[Sam McNally, 2016-09-15 00:30:02]

On 2016/09/14 14:31:54, Ken Rockot wrote:
> Scoping a Java registry to WebContents feels pretty misleading to me. It does
> seem like Activity is the right context in these cases, and other data can be
> moved around via additional interfaces in either direction. WDYT?

It's a tempting idea, but it has some problems:
1. There isn't a C++ analogue for Activity; it's just passed around as a
jobject. Some nearby classes (ContentViewCore and WindowAndroid) might be more
usable, but the C++ ContentViewCore says to use WebContents instead and
WindowAndroid also feels like a weird place to hang this.
2. A WebContents can move to a different WindowAndroid, which might have a
different Activity; some impls care about this so something needs to track this
and take appropriate action (e.g.
https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro...).
C++ doesn't seem to be notified about this so that would need to be added; on
top of that, each interface where this mattered would need some C++ glue to
proxy to the right Java impl as well as any necessary hand-off when switching.

[Ken Rockot(use gerrit already), 2016-09-15 02:07:16]

OK I'm convinced. Please at least clarify the intent of the Java registries both
on the global function and the WebContents interface. Lgtm

[Sam McNally, 2016-09-19 02:27:14]

On 2016/09/15 02:07:16, Ken Rockot wrote:
> OK I'm convinced. Please at least clarify the intent of the Java registries
both
> on the global function and the WebContents interface. Lgtm

I'm not sure what you mean.

[Sam McNally, 2016-09-19 05:24:22]

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

[commit-bot: I haz the power, 2016-09-19 05:24:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-19 06:36:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-19 06:36:51]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2016-09-19 06:47:53]

sammc@chromium.org changed reviewers:
- ben@chromium.org

[Sam McNally, 2016-09-20 00:32:30]

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

[Sam McNally, 2016-09-20 00:32:31]

+jam for OWNERS

On 2016/09/15 02:07:16, Ken Rockot wrote:
> OK I'm convinced. Please at least clarify the intent of the Java registries
both
> on the global function and the WebContents interface. Lgtm

Done.

[jam, 2016-09-20 16:46:47]

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

[jam, 2016-09-20 16:46:48]

redirecting to Ben who's back now

[jam, 2016-09-20 16:50:37]

Ken mentioned you just a rubberstamp since he's already reviewed it, lgtm. this
is pretty cool

[Sam McNally, 2016-09-20 22:01:27]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2016-09-20 22:01:28]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2217813003/#ps200001
(title: " ")

[commit-bot: I haz the power, 2016-09-20 22:02:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 23:05:29]

Description was changed from

==========
Add global and per-WebContents java InterfaceRegistries.

BUG=634568, 637174
==========

to

==========
Add global and per-WebContents java InterfaceRegistries.

BUG=634568, 637174
==========

[commit-bot: I haz the power, 2016-09-20 23:05:30]

Committed patchset #3 (id:200001)

[commit-bot: I haz the power, 2016-09-20 23:06:58]

Description was changed from

==========
Add global and per-WebContents java InterfaceRegistries.

BUG=634568, 637174
==========

to

==========
Add global and per-WebContents java InterfaceRegistries.

BUG=634568, 637174

Committed: https://crrev.com/f5f1b0fdf3a73883a2462ad7d2ed7ee5ec70533b
Cr-Commit-Position: refs/heads/master@{#419886}
==========

[commit-bot: I haz the power, 2016-09-20 23:06:59]

Patchset 3 (id:??) landed as
https://crrev.com/f5f1b0fdf3a73883a2462ad7d2ed7ee5ec70533b
Cr-Commit-Position: refs/heads/master@{#419886}