Added experimental Share API on Android behind flag.

Added experimental Web Share API on Android behind flag.

On Android, navigator.share now sends a system intent. Requires
the enable-experimental-web-platform-features flag.

BUG=595608
Committed: https://crrev.com/cd237c67f5dd71579483a6ca33e3e0bc947ff4e5
Cr-Commit-Position: refs/heads/master@{#409121}

[Matt Giuca, 2016-07-14 08:36:56]

Description was changed from

==========
Added experimental Share API on Android behind flag.

On Android, navigator.actions.share now sends a system intent. Requires
the enable-experimental-web-platform-features flag.

WORK IN PROGRESS. DO NOT SUBMIT.

BUG=595608
==========

to

==========
Added experimental Share API on Android behind flag.

On Android, navigator.actions.share now sends a system intent. Requires
the enable-experimental-web-platform-features flag.

WORK IN PROGRESS. DO NOT SUBMIT.

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

[Matt Giuca, 2016-07-21 07:52:49]

Patchset #6 (id:100001) has been deleted

[Matt Giuca, 2016-07-21 07:56:02]

Patchset #7 (id:140001) has been deleted

[Matt Giuca, 2016-07-21 08:10:13]

Description was changed from

==========
Added experimental Share API on Android behind flag.

On Android, navigator.actions.share now sends a system intent. Requires
the enable-experimental-web-platform-features flag.

WORK IN PROGRESS. DO NOT SUBMIT.

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

to

==========
Added experimental Web Share API on Android behind flag.

On Android, navigator.share now sends a system intent. Requires
the enable-experimental-web-platform-features flag.

BUG=595608
==========

[Matt Giuca, 2016-07-21 08:10:26]

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

[Matt Giuca, 2016-07-21 08:10:26]

sammc: For initial review. Thanks!

[Sam McNally, 2016-07-26 08:21:52]

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

https://codereview.chromium.org/1814133002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/mojo/ChromeServiceRegistrar.java:20:
private static class ShareServiceImplementationFactory
Why is this here?

https://codereview.chromium.org/1814133002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java
(right):

https://codereview.chromium.org/1814133002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java:28:
// Get the Activity for the web contents, which will be used to send the intent.
Is this still accurate?

https://codereview.chromium.org/1814133002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java:29:
if (webContents == null) {
Please add helper functions to map from WebContents to WindowAndroid and from
WindowAndroid to Context, propagating nulls. This is noisier than it should be.

https://codereview.chromium.org/1814133002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java:49:
if (mContext == null) return;
Is this really necessary?

https://codereview.chromium.org/1814133002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java:68:
callback.call("Could not start intent");
I don't think we should expose platform-specific errors if we can avoid it.

https://codereview.chromium.org/1814133002/diff/200001/content/public/android...
File content/public/android/BUILD.gn (right):

https://codereview.chromium.org/1814133002/diff/200001/content/public/android...
content/public/android/BUILD.gn:52:
"//third_party/WebKit/public:android_mojo_bindings_java",
Remove.

[Matt Giuca, 2016-07-28 05:58:20]

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

[Matt Giuca, 2016-07-28 05:58:32]

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

https://codereview.chromium.org/1814133002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/mojo/ChromeServiceRegistrar.java:20:
private static class ShareServiceImplementationFactory
On 2016/07/26 08:21:51, Sam McNally wrote:
> Why is this here?

Follows the pattern here:
https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro...

If your factory is only needed in this registrar, it's a simple private class
inside the registrar. PaymentRequestFactory is a bigger deal so it has its own
file.

https://codereview.chromium.org/1814133002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java
(right):

https://codereview.chromium.org/1814133002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java:28:
// Get the Activity for the web contents, which will be used to send the intent.
On 2016/07/26 08:21:51, Sam McNally wrote:
> Is this still accurate?

Done.

https://codereview.chromium.org/1814133002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java:29:
if (webContents == null) {
On 2016/07/26 08:21:51, Sam McNally wrote:
> Please add helper functions to map from WebContents to WindowAndroid and from
> WindowAndroid to Context, propagating nulls. This is noisier than it should
be.

Done.

https://codereview.chromium.org/1814133002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java:49:
if (mContext == null) return;
On 2016/07/26 08:21:51, Sam McNally wrote:
> Is this really necessary?

No, it was a joke lol.

https://codereview.chromium.org/1814133002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java:68:
callback.call("Could not start intent");
On 2016/07/26 08:21:52, Sam McNally wrote:
> I don't think we should expose platform-specific errors if we can avoid it.

I think we should just return an enum of errors if we can help it; I'll add a
TODO but for now, making the error more vague SGTM.

https://codereview.chromium.org/1814133002/diff/200001/content/public/android...
File content/public/android/BUILD.gn (right):

https://codereview.chromium.org/1814133002/diff/200001/content/public/android...
content/public/android/BUILD.gn:52:
"//third_party/WebKit/public:android_mojo_bindings_java",
On 2016/07/26 08:21:52, Sam McNally wrote:
> Remove.

Done.

[commit-bot: I haz the power, 2016-07-28 05:58:37]

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-07-28 07:16:04]

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

https://codereview.chromium.org/1814133002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/mojo/ChromeServiceRegistrar.java:20:
private static class ShareServiceImplementationFactory
On 2016/07/28 05:58:32, Matt Giuca wrote:
> On 2016/07/26 08:21:51, Sam McNally wrote:
> > Why is this here?
> 
> Follows the pattern here:
>
https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro...
> 
> If your factory is only needed in this registrar, it's a simple private class
> inside the registrar. PaymentRequestFactory is a bigger deal so it has its own
> file.

The pattern described by
"BatteryMonitorFactory can't implement ImplementationFactory itself, as we don't
depend on /content in /device. Hence we use BatteryMonitorImplementationFactory
as a wrapper."?

https://codereview.chromium.org/1814133002/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java
(right):

https://codereview.chromium.org/1814133002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java:22:
private static final String TAG = "ShareServiceImpl";
Remove.

https://codereview.chromium.org/1814133002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java:33:
static WindowAndroid windowFromWebContents(WebContents webContents) {
private

https://codereview.chromium.org/1814133002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java:43:
static Context contextFromWindow(WindowAndroid window) {
private

[Matt Giuca, 2016-07-28 07:40:42]

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

https://codereview.chromium.org/1814133002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/mojo/ChromeServiceRegistrar.java:20:
private static class ShareServiceImplementationFactory
On 2016/07/28 07:16:04, Sam McNally wrote:
> On 2016/07/28 05:58:32, Matt Giuca wrote:
> > On 2016/07/26 08:21:51, Sam McNally wrote:
> > > Why is this here?
> > 
> > Follows the pattern here:
> >
>
https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro...
> > 
> > If your factory is only needed in this registrar, it's a simple private
class
> > inside the registrar. PaymentRequestFactory is a bigger deal so it has its
own
> > file.
> 
> The pattern described by
> "BatteryMonitorFactory can't implement ImplementationFactory itself, as we
don't
> depend on /content in /device. Hence we use
BatteryMonitorImplementationFactory
> as a wrapper."?

I was looking at the VibrationManagerImplementationFactory there, which doesn't
have a corresponding other factory.

Are you suggesting that I should move this class to its own file in the
chrome/browser/webshare directory?

https://codereview.chromium.org/1814133002/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java
(right):

https://codereview.chromium.org/1814133002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java:22:
private static final String TAG = "ShareServiceImpl";
On 2016/07/28 07:16:04, Sam McNally wrote:
> Remove.

Done.

https://codereview.chromium.org/1814133002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java:33:
static WindowAndroid windowFromWebContents(WebContents webContents) {
On 2016/07/28 07:16:04, Sam McNally wrote:
> private

Done.

https://codereview.chromium.org/1814133002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java:43:
static Context contextFromWindow(WindowAndroid window) {
On 2016/07/28 07:16:04, Sam McNally wrote:
> private

Done.

[Matt Giuca, 2016-07-28 07:56:10]

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

https://codereview.chromium.org/1814133002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/mojo/ChromeServiceRegistrar.java:20:
private static class ShareServiceImplementationFactory
On 2016/07/28 07:40:42, Matt Giuca wrote:
> On 2016/07/28 07:16:04, Sam McNally wrote:
> > On 2016/07/28 05:58:32, Matt Giuca wrote:
> > > On 2016/07/26 08:21:51, Sam McNally wrote:
> > > > Why is this here?
> > > 
> > > Follows the pattern here:
> > >
> >
>
https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro...
> > > 
> > > If your factory is only needed in this registrar, it's a simple private
> class
> > > inside the registrar. PaymentRequestFactory is a bigger deal so it has its
> own
> > > file.
> > 
> > The pattern described by
> > "BatteryMonitorFactory can't implement ImplementationFactory itself, as we
> don't
> > depend on /content in /device. Hence we use
> BatteryMonitorImplementationFactory
> > as a wrapper."?
> 
> I was looking at the VibrationManagerImplementationFactory there, which
doesn't
> have a corresponding other factory.
> 
> Are you suggesting that I should move this class to its own file in the
> chrome/browser/webshare directory?

Done.

[Sam McNally, 2016-07-28 08:02:02]

lgtm

[Matt Giuca, 2016-07-28 08:09:38]

mgiuca@chromium.org changed reviewers:
+ dfalcantara@chromium.org, mkwst@chromium.org

[Matt Giuca, 2016-07-28 08:09:38]

Adding more owners for review:
- dfalcantara: chrome/android/*
- mkwst: third_party/WebKit/*

Thanks!

[Mike West, 2016-07-28 09:02:15]

On 2016/07/28 at 08:09:38, mgiuca wrote:
> Adding more owners for review:
> - dfalcantara: chrome/android/*
> - mkwst: third_party/WebKit/*
> 
> Thanks!

//third_party/WebKit LGTM.

[Mike West, 2016-07-28 09:02:31]

On 2016/07/28 at 09:02:15, Mike West wrote:
> On 2016/07/28 at 08:09:38, mgiuca wrote:
> > Adding more owners for review:
> > - dfalcantara: chrome/android/*
> > - mkwst: third_party/WebKit/*
> > 
> > Thanks!
> 
> //third_party/WebKit LGTM.

(You already sent an Intent to Implement for this, right?)

[Matt Giuca, 2016-07-28 09:07:18]

On 2016/07/28 09:02:31, Mike West wrote:
> On 2016/07/28 at 09:02:15, Mike West wrote:
> > On 2016/07/28 at 08:09:38, mgiuca wrote:
> > > Adding more owners for review:
> > > - dfalcantara: chrome/android/*
> > > - mkwst: third_party/WebKit/*
> > > 
> > > Thanks!
> > 
> > //third_party/WebKit LGTM.
> 
> (You already sent an Intent to Implement for this, right?)

Yep:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/web$20sha...

(Also: note that I'm assuming your LG applies to the mojom as well as the gn,
correct?)

[Mike West, 2016-07-28 09:08:26]

On 2016/07/28 at 09:07:18, mgiuca wrote:
> On 2016/07/28 09:02:31, Mike West wrote:
> > On 2016/07/28 at 09:02:15, Mike West wrote:
> > > On 2016/07/28 at 08:09:38, mgiuca wrote:
> > > > Adding more owners for review:
> > > > - dfalcantara: chrome/android/*
> > > > - mkwst: third_party/WebKit/*
> > > > 
> > > > Thanks!
> > > 
> > > //third_party/WebKit LGTM.
> > 
> > (You already sent an Intent to Implement for this, right?)
> 
> Yep:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/web$20sha...
> 
> (Also: note that I'm assuming your LG applies to the mojom as well as the gn,
correct?)

Yeah. I mean, those are the files you asked me to review, right? :)

[gone, 2016-07-28 16:03:20]

https://codereview.chromium.org/1814133002/diff/320001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java
(right):

https://codereview.chromium.org/1814133002/diff/320001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java:46:
send.putExtra(Intent.EXTRA_SUBJECT, text);
Shouldn't this be title?

https://codereview.chromium.org/1814133002/diff/320001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java:61:
// |webContents| can be null. Can return null.
Use real Javadoc-style comments.  Optionally use the @Nullable annotation on the
webContents parameter:
https://developer.android.com/reference/android/support/annotation/Nullable.html

[Matt Giuca, 2016-07-29 01:16:52]

On 2016/07/28 09:08:26, Mike West wrote:
> On 2016/07/28 at 09:07:18, mgiuca wrote:
> > On 2016/07/28 09:02:31, Mike West wrote:
> > > On 2016/07/28 at 09:02:15, Mike West wrote:
> > > > On 2016/07/28 at 08:09:38, mgiuca wrote:
> > > > > Adding more owners for review:
> > > > > - dfalcantara: chrome/android/*
> > > > > - mkwst: third_party/WebKit/*
> > > > > 
> > > > > Thanks!
> > > > 
> > > > //third_party/WebKit LGTM.
> > > 
> > > (You already sent an Intent to Implement for this, right?)
> > 
> > Yep:
>
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/web$20sha...
> > 
> > (Also: note that I'm assuming your LG applies to the mojom as well as the
gn,
> correct?)
> 
> Yeah. I mean, those are the files you asked me to review, right? :)

Yeah sorry. Just that the .mojom files have separate OWNERS rules so I thought
it might cause confusion.

[Matt Giuca, 2016-07-29 01:43:04]

https://codereview.chromium.org/1814133002/diff/320001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java
(right):

https://codereview.chromium.org/1814133002/diff/320001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java:46:
send.putExtra(Intent.EXTRA_SUBJECT, text);
On 2016/07/28 16:03:20, dfalcantara wrote:
> Shouldn't this be title?

Done. #oops

https://codereview.chromium.org/1814133002/diff/320001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java:61:
// |webContents| can be null. Can return null.
On 2016/07/28 16:03:20, dfalcantara wrote:
> Use real Javadoc-style comments.  Optionally use the @Nullable annotation on
the
> webContents parameter:
>
https://developer.android.com/reference/android/support/annotation/Nullable.html

Done.

[Matt Giuca, 2016-08-01 07:05:05]

Patchset #17 (id:360001) has been deleted

[gone, 2016-08-01 17:21:47]

lgtm

[Matt Giuca, 2016-08-02 00:21:41]

The CQ bit was checked by mgiuca@chromium.org

[Matt Giuca, 2016-08-02 00:21:42]

The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org,
mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1814133002/#ps380001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-08-02 00:22:09]

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-02 02:12:04]

Description was changed from

==========
Added experimental Web Share API on Android behind flag.

On Android, navigator.share now sends a system intent. Requires
the enable-experimental-web-platform-features flag.

BUG=595608
==========

to

==========
Added experimental Web Share API on Android behind flag.

On Android, navigator.share now sends a system intent. Requires
the enable-experimental-web-platform-features flag.

BUG=595608
==========

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

Committed patchset #17 (id:380001)

[commit-bot: I haz the power, 2016-08-02 02:13:17]

Description was changed from

==========
Added experimental Web Share API on Android behind flag.

On Android, navigator.share now sends a system intent. Requires
the enable-experimental-web-platform-features flag.

BUG=595608
==========

to

==========
Added experimental Web Share API on Android behind flag.

On Android, navigator.share now sends a system intent. Requires
the enable-experimental-web-platform-features flag.

BUG=595608

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

[commit-bot: I haz the power, 2016-08-02 02:13:19]

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