Added Web Share (navigator.share) experimental web API (stub).

Added Web Share (navigator.share) experimental web API (stub).

Requires the enable-experimental-web-platform-features flag. Currently
does nothing; future CLs will implement this on Android as an
experiment.

Re-land of r390004.

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

[Matt Giuca, 2016-03-17 07:40:54]

Description was changed from

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

Adds navigator.actions.share function. On Android, this will send a
system intent with the given data. 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.

Adds navigator.actions.share function. On Android, this will send a
system intent with the given data. 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-03-17 07:43:07]

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

[Matt Giuca, 2016-03-17 07:43:08]

Hi Sam. I implemented, very hackily, the Blink side of this API. I know you were
going to do it, but I needed it in order to test the other side (which I haven't
done yet).

I assume you won't get a chance to work on this properly for awhile so I'm just
going to keep hacking away at it. Sharing with you so we don't duplicate work.

[Matt Giuca, 2016-03-18 04:01:35]

Description was changed from

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

Adds navigator.actions.share function. On Android, this will send a
system intent with the given data. 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 navigator.actions.share experimental web API (stub).

Adds navigator.actions.share function. Requires the
enable-experimental-web-platform-features flag. Currently does nothing;
future CLs will implement this on Android as an experiment.

WORK IN PROGRESS. DO NOT SUBMIT.

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

[Matt Giuca, 2016-03-18 04:03:24]

Description was changed from

==========
Added navigator.actions.share experimental web API (stub).

Adds navigator.actions.share function. Requires the
enable-experimental-web-platform-features flag. Currently does nothing;
future CLs will implement this on Android as an experiment.

WORK IN PROGRESS. DO NOT SUBMIT.

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

to

==========
Added navigator.actions.share experimental web API (stub).

Adds navigator.actions.share function. Requires the
enable-experimental-web-platform-features flag. Currently does nothing;
future CLs will implement this on Android as an experiment.

WORK IN PROGRESS. DO NOT SUBMIT.

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

[Matt Giuca, 2016-03-18 04:03:53]

Description was changed from

==========
Added navigator.actions.share experimental web API (stub).

Adds navigator.actions.share function. Requires the
enable-experimental-web-platform-features flag. Currently does nothing;
future CLs will implement this on Android as an experiment.

WORK IN PROGRESS. DO NOT SUBMIT.

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

to

==========
Added navigator.actions.share experimental web API (stub).

Requires the enable-experimental-web-platform-features flag. Currently
does nothing; future CLs will implement this on Android as an
experiment.

WORK IN PROGRESS. DO NOT SUBMIT.

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

[Sam McNally, 2016-03-31 00:33:28]

Description was changed from

==========
Added navigator.actions.share experimental web API (stub).

Requires the enable-experimental-web-platform-features flag. Currently
does nothing; future CLs will implement this on Android as an
experiment.

WORK IN PROGRESS. DO NOT SUBMIT.

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

to

==========
Added navigator.actions.share experimental web API (stub).

Requires the enable-experimental-web-platform-features flag. Currently
does nothing; future CLs will implement this on Android as an
experiment.

WORK IN PROGRESS. DO NOT SUBMIT.

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

[Sam McNally, 2016-03-31 00:33:28]

sammc@chromium.org changed reviewers:
+ sammc@chromium.org
- sammc@google.com

[Sam McNally, 2016-04-01 02:51:25]

https://codereview.chromium.org/1806253002/diff/1/content/browser/ballista/ba...
File content/browser/ballista/ballista_service_impl.h (right):

https://codereview.chromium.org/1806253002/diff/1/content/browser/ballista/ba...
content/browser/ballista/ballista_service_impl.h:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
I would leave the browser side out of this CL.

https://codereview.chromium.org/1806253002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1806253002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1886: // TODO(mgiuca): This
doesn't seem to be used (instead, the one in
https://codereview.chromium.org/1830883002/ will let you do this properly.

https://codereview.chromium.org/1806253002/diff/1/content/common/ballista_ser...
File content/common/ballista_service.mojom (right):

https://codereview.chromium.org/1806253002/diff/1/content/common/ballista_ser...
content/common/ballista_service.mojom:1: // Copyright 2016 The Chromium Authors.
All rights reserved.
This should be in third_party/WebKit/public/platform/modules/ballista. Also, no
"_service".

https://codereview.chromium.org/1806253002/diff/1/content/common/ballista_ser...
content/common/ballista_service.mojom:5: module content;
module blink.mojom;

https://codereview.chromium.org/1806253002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/ballista/Actions.cpp (right):

https://codereview.chromium.org/1806253002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/ballista/Actions.cpp:41:
m_service->Share(titleMojo, textMojo, [this, client](const mojo::String& error)
{
Use sameThreadBindForMojo() to bind.

https://codereview.chromium.org/1806253002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/ballista/Actions.h (right):

https://codereview.chromium.org/1806253002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/ballista/Actions.h:11: #include
"content/common/ballista_service.mojom.h"
You'll want to use the blink variant. Currently, this will end up in a
"ballista.mojom-wtf.h"; I'm going to try to fix it to be
"ballista.mojom-blink.h". It also ends up with an extra nested "wtf" namespace
(also probably will be "blink"): so blink::mojom::wtf.

https://codereview.chromium.org/1806253002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/ballista/NavigatorActions.idl (right):

https://codereview.chromium.org/1806253002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/ballista/NavigatorActions.idl:10: readonly
attribute Actions actions;
I'm not a fan of calling it "actions" yet.

[Matt Giuca, 2016-04-11 05:43:28]

Patchset #3 (id:40001) has been deleted

[Matt Giuca, 2016-04-12 06:12:21]

https://codereview.chromium.org/1806253002/diff/1/content/browser/ballista/ba...
File content/browser/ballista/ballista_service_impl.h (right):

https://codereview.chromium.org/1806253002/diff/1/content/browser/ballista/ba...
content/browser/ballista/ballista_service_impl.h:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
On 2016/04/01 02:51:25, Sam McNally wrote:
> I would leave the browser side out of this CL.

It makes sense to me to have one CL that pipes this all the way from JavaScript
through to the browser; all the non-platform-specific code for share. Otherwise,
how do I know that any of the Mojo stuff is working?

For example, I am getting problems #including ballista.mojom.h from this file
(now that I have moved it into third_party). That needs to be considered before
landing the other part.

Are you suggesting I put this in its own CL or in the Android-specific CL?

https://codereview.chromium.org/1806253002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1806253002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1886: // TODO(mgiuca): This
doesn't seem to be used (instead, the one in
On 2016/04/01 02:51:25, Sam McNally wrote:
> https://codereview.chromium.org/1830883002/ will let you do this properly.

Done.

https://codereview.chromium.org/1806253002/diff/1/content/common/ballista_ser...
File content/common/ballista_service.mojom (right):

https://codereview.chromium.org/1806253002/diff/1/content/common/ballista_ser...
content/common/ballista_service.mojom:1: // Copyright 2016 The Chromium Authors.
All rights reserved.
On 2016/04/01 02:51:25, Sam McNally wrote:
> This should be in third_party/WebKit/public/platform/modules/ballista. Also,
no
> "_service".

Done.

https://codereview.chromium.org/1806253002/diff/1/content/common/ballista_ser...
content/common/ballista_service.mojom:5: module content;
On 2016/04/01 02:51:25, Sam McNally wrote:
> module blink.mojom;

Done.

https://codereview.chromium.org/1806253002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/ballista/Actions.cpp (right):

https://codereview.chromium.org/1806253002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/ballista/Actions.cpp:41:
m_service->Share(titleMojo, textMojo, [this, client](const mojo::String& error)
{
On 2016/04/01 02:51:25, Sam McNally wrote:
> Use sameThreadBindForMojo() to bind.

Done.

https://codereview.chromium.org/1806253002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/ballista/Actions.h (right):

https://codereview.chromium.org/1806253002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/ballista/Actions.h:11: #include
"content/common/ballista_service.mojom.h"
On 2016/04/01 02:51:25, Sam McNally wrote:
> You'll want to use the blink variant. Currently, this will end up in a
> "ballista.mojom-wtf.h"; I'm going to try to fix it to be
> "ballista.mojom-blink.h". It also ends up with an extra nested "wtf" namespace
> (also probably will be "blink"): so blink::mojom::wtf.

Done.

https://codereview.chromium.org/1806253002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/ballista/NavigatorActions.idl (right):

https://codereview.chromium.org/1806253002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/ballista/NavigatorActions.idl:10: readonly
attribute Actions actions;
On 2016/04/01 02:51:25, Sam McNally wrote:
> I'm not a fan of calling it "actions" yet.

What do you want to call it?

[Matt Giuca, 2016-04-12 07:16:06]

Description was changed from

==========
Added navigator.actions.share experimental web API (stub).

Requires the enable-experimental-web-platform-features flag. Currently
does nothing; future CLs will implement this on Android as an
experiment.

WORK IN PROGRESS. DO NOT SUBMIT.

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

to

==========
Added navigator.actions.share experimental web API (stub).

Requires the enable-experimental-web-platform-features flag. Currently
does nothing; future CLs will implement this on Android as an
experiment.

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

[Matt Giuca, 2016-04-12 07:21:37]

Patchset #6 (id:120001) has been deleted

[Matt Giuca, 2016-04-12 07:41:12]

https://codereview.chromium.org/1806253002/diff/1/content/browser/ballista/ba...
File content/browser/ballista/ballista_service_impl.h (right):

https://codereview.chromium.org/1806253002/diff/1/content/browser/ballista/ba...
content/browser/ballista/ballista_service_impl.h:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
On 2016/04/12 06:12:21, Matt Giuca wrote:
> On 2016/04/01 02:51:25, Sam McNally wrote:
> > I would leave the browser side out of this CL.
> 
> It makes sense to me to have one CL that pipes this all the way from
JavaScript
> through to the browser; all the non-platform-specific code for share.
Otherwise,
> how do I know that any of the Mojo stuff is working?
> 
> For example, I am getting problems #including ballista.mojom.h from this file
> (now that I have moved it into third_party). That needs to be considered
before
> landing the other part.
> 
> Are you suggesting I put this in its own CL or in the Android-specific CL?

OK Done (separated into https://codereview.chromium.org/1880003002 -- maybe will
talk directly to Java but I'm not sure that makes sense given that we eventually
want this to be cross-platform, and also because I don't actually want to write
a BallistaService in Java, just call the send intent methods which are exposed
to C++. But we can discuss that in the next CL.)

[Matt Giuca, 2016-04-13 01:51:06]

Patchset #7 (id:160001) has been deleted

[Matt Giuca, 2016-04-13 03:29:55]

sammc: PTAL. Tests using the old framework. Let me know what you think (whether
I should change to the new framework for USB, split into multiple html files,
etc).

[Matt Giuca, 2016-04-13 03:47:28]

OK split tests into four separate HTML files. PTAL.

[Sam McNally, 2016-04-13 04:32:35]

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/ballista/resources/mock-ballista-service.js
(right):

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/ballista/resources/mock-ballista-service.js:6:
var expectTitle;
expectedTitle

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/ballista/resources/mock-ballista-service.js:7:
var expectText;
expectedText

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/ballista/resources/mock-ballista-service.js:11:
['third_party/WebKit/public/platform/modules/ballista/ballista.mojom',
Sort these.

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/ballista/resources/mock-ballista-service.js:35:
let result = new Promise(resolve => this.pendingRequests_.push([title, text,
resolve]));
Wrap to 80 chars.

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/ballista/resources/mock-ballista-service.js:68:
debug('share completed successfully');
Be consistent with indentation.

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/ballista/share-arity.html (right):

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/ballista/share-arity.html:18: debug('page is
visible');
Remove. Fix everywhere.

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/ballista/share-arity.html:21:
navigator.actions.share().then(shareSuccess, shareFailure);
Test 1 and 3 args.

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/ballista/Actions.cpp (right):

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/ballista/Actions.cpp:26: if (m_actions &&
m_actions->m_clients.contains(this))
When would m_clients not contain this?

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/ballista/Actions.h (right):

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/ballista/Actions.h:44: class
BallistaClientImpl : public GarbageCollectedFinalized<BallistaClientImpl> {
Does this need to be defined inline?

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/ballista/Actions.idl (right):

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/ballista/Actions.idl:5: // TODO(mgiuca): Add
spec URL here.
How about a link to the github repo for now?

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/ballista/NavigatorActions.idl (right):

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/ballista/NavigatorActions.idl:5: //
TODO(mgiuca): Add spec URL here.
Here too.

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/pub...
File third_party/WebKit/public/BUILD.gn (right):

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/BUILD.gn:180:
"platform/modules/ballista/ballista.mojom",
Update the GYP version.

[Matt Giuca, 2016-04-13 04:44:22]

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

[commit-bot: I haz the power, 2016-04-13 04:44:30]

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

[commit-bot: I haz the power, 2016-04-13 04:46:19]

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

[commit-bot: I haz the power, 2016-04-13 04:46:20]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)

[Matt Giuca, 2016-04-13 05:43:02]

https://codereview.chromium.org/1806253002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/ballista/NavigatorActions.idl (right):

https://codereview.chromium.org/1806253002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/ballista/NavigatorActions.idl:10: readonly
attribute Actions actions;
On 2016/04/12 06:12:21, Matt Giuca wrote:
> On 2016/04/01 02:51:25, Sam McNally wrote:
> > I'm not a fan of calling it "actions" yet.
> 
> What do you want to call it?

Ping.

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/ballista/resources/mock-ballista-service.js
(right):

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/ballista/resources/mock-ballista-service.js:6:
var expectTitle;
On 2016/04/13 04:32:35, Sam McNally wrote:
> expectedTitle

Done.

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/ballista/resources/mock-ballista-service.js:7:
var expectText;
On 2016/04/13 04:32:35, Sam McNally wrote:
> expectedText

Done.

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/ballista/resources/mock-ballista-service.js:11:
['third_party/WebKit/public/platform/modules/ballista/ballista.mojom',
On 2016/04/13 04:32:35, Sam McNally wrote:
> Sort these.

Done.

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/ballista/resources/mock-ballista-service.js:35:
let result = new Promise(resolve => this.pendingRequests_.push([title, text,
resolve]));
On 2016/04/13 04:32:35, Sam McNally wrote:
> Wrap to 80 chars.

Done.

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/ballista/resources/mock-ballista-service.js:68:
debug('share completed successfully');
On 2016/04/13 04:32:35, Sam McNally wrote:
> Be consistent with indentation.

Done.

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/ballista/share-arity.html (right):

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/ballista/share-arity.html:18: debug('page is
visible');
On 2016/04/13 04:32:35, Sam McNally wrote:
> Remove. Fix everywhere.

Done.

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/ballista/share-arity.html:21:
navigator.actions.share().then(shareSuccess, shareFailure);
On 2016/04/13 04:32:35, Sam McNally wrote:
> Test 1 and 3 args.

Done.

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/ballista/Actions.cpp (right):

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/ballista/Actions.cpp:26: if (m_actions &&
m_actions->m_clients.contains(this))
On 2016/04/13 04:32:35, Sam McNally wrote:
> When would m_clients not contain this?

Done.

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/ballista/Actions.h (right):

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/ballista/Actions.h:44: class
BallistaClientImpl : public GarbageCollectedFinalized<BallistaClientImpl> {
On 2016/04/13 04:32:35, Sam McNally wrote:
> Does this need to be defined inline?

Done.

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/ballista/Actions.idl (right):

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/ballista/Actions.idl:5: // TODO(mgiuca): Add
spec URL here.
On 2016/04/13 04:32:35, Sam McNally wrote:
> How about a link to the github repo for now?

Done.

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/ballista/NavigatorActions.idl (right):

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/ballista/NavigatorActions.idl:5: //
TODO(mgiuca): Add spec URL here.
On 2016/04/13 04:32:35, Sam McNally wrote:
> Here too.

Done.

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/pub...
File third_party/WebKit/public/BUILD.gn (right):

https://codereview.chromium.org/1806253002/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/BUILD.gn:180:
"platform/modules/ballista/ballista.mojom",
On 2016/04/13 04:32:35, Sam McNally wrote:
> Update the GYP version.

Done.

https://codereview.chromium.org/1806253002/diff/240001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/ballista/share-arity-expected.txt (right):

https://codereview.chromium.org/1806253002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/ballista/share-arity-expected.txt:7: share
failed: TypeError: Failed to execute 'share' on 'Actions': 2 arguments required,
but only 1 present.
Do you think this could be flaky (order problems)? This test failed once and I
couldn't see the output, then I accidentally overwrote it and now I can't make
it fail again :(. It would suck to have to split this up into two separate tests
and I can't see why it wouldn't report the errors in the order they occurred.

[Sam McNally, 2016-04-15 06:41:40]

https://codereview.chromium.org/1806253002/diff/240001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/ballista/share-arity-expected.txt (right):

https://codereview.chromium.org/1806253002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/ballista/share-arity-expected.txt:7: share
failed: TypeError: Failed to execute 'share' on 'Actions': 2 arguments required,
but only 1 present.
On 2016/04/13 05:43:02, Matt Giuca wrote:
> Do you think this could be flaky (order problems)? This test failed once and I
> couldn't see the output, then I accidentally overwrote it and now I can't make
> it fail again :(. It would suck to have to split this up into two separate
tests
> and I can't see why it wouldn't report the errors in the order they occurred.

This should be fine. The invalid cases should run in order and the success case
will never be ready early enough to matter.

[Matt Giuca, 2016-04-15 06:46:32]

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

[Matt Giuca, 2016-04-15 06:46:33]

mkwst@: Please have a look or suggest alternative reviewers.

Context: Initial experimental version of https://github.com/chromium/ballista
for Android. See bug description. We intend to take this to the experimental
framework next, but as a first step just putting it behind the
enable-experimental-web-platform-features flag.

Please ignore the huge amount of test failures; that's caused by an unrelated
bug with a fix in the pipeline (https://crbug.com/603782).

[Matt Giuca, 2016-04-18 01:14:05]

Patchset #12 (id:280001) has been deleted

[Mike West, 2016-04-18 11:16:19]

I added some comments. Please also add an OWNERS file for the new module. :)

https://codereview.chromium.org/1806253002/diff/260001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/ballista/share-arity.html (right):

https://codereview.chromium.org/1806253002/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/ballista/share-arity.html:4: <script
src="../resources/js-test.js"></script>
Please use `/resources/testharness.js`, not `js-test.js`. The former provides a
clean path to eventually getting tests into the shared "web-platform-tests"
repository to ensure interop with other browsers, the latter is/should be
deprecated.

https://codereview.chromium.org/1806253002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/ballista/Actions.cpp (right):

https://codereview.chromium.org/1806253002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/ballista/Actions.cpp:57:
frame(scriptState)->serviceRegistry()->connectToRemoteService(mojo::GetProxy(&m_service));
Nit: Null-check `frame()`? Or DCHECK?

https://codereview.chromium.org/1806253002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/ballista/Actions.cpp:58: 
Nit: DCHECK(m_service)

https://codereview.chromium.org/1806253002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/ballista/Actions.cpp:78: Document*
Actions::document(ScriptState* scriptState) const
Nit: Why do you need this? Why not just cast inside `frame()` (which is the only
callsite)?

Nit: For that matter, why do you need `frame()`? You only call it once at the
top of `share()`?

https://codereview.chromium.org/1806253002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/ballista/Actions.cpp:86: return doc ?
doc->frame() : nullptr;
Is there any case in which you need this check? It looks like you're only
exposing the API to documents (or do you intend to eventually expose it to SW as
well?)

https://codereview.chromium.org/1806253002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/ballista/Actions.idl (right):

https://codereview.chromium.org/1806253002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/ballista/Actions.idl:10: NoInterfaceObject,
Nit: Why `NoInterfaceObject`?

https://codereview.chromium.org/1806253002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/ballista/Actions.idl:13:
[CallWith=ScriptState] Promise<undefined> share (
`undefined` seems strange. It would be nice to have a spec to point to here. :)

[Mike West, 2016-04-18 11:17:20]

Also: Have you sent an "Intent to Implement" to blink-dev? If so, please add a
link to the thread to the CL description. If not, please do so. :)

[Matt Giuca, 2016-04-19 01:35:35]

Patchset #12 (id:300001) has been deleted

[Matt Giuca, 2016-04-22 06:14:02]

Hi Mike, thanks for the review.

Added OWNERS.

No Intent to Implement has been sent, but I will send one today or early next
week (I have it drafted and owencm@ is just looking over it).

https://codereview.chromium.org/1806253002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/ballista/Actions.cpp (right):

https://codereview.chromium.org/1806253002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/ballista/Actions.cpp:57:
frame(scriptState)->serviceRegistry()->connectToRemoteService(mojo::GetProxy(&m_service));
On 2016/04/18 11:16:18, Mike West (slow until 25th) wrote:
> Nit: Null-check `frame()`? Or DCHECK?

Done.

https://codereview.chromium.org/1806253002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/ballista/Actions.cpp:58: 
On 2016/04/18 11:16:18, Mike West (slow until 25th) wrote:
> Nit: DCHECK(m_service)

Done.

https://codereview.chromium.org/1806253002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/ballista/Actions.cpp:78: Document*
Actions::document(ScriptState* scriptState) const
On 2016/04/18 11:16:18, Mike West (slow until 25th) wrote:
> Nit: Why do you need this? Why not just cast inside `frame()` (which is the
only
> callsite)?
> 
> Nit: For that matter, why do you need `frame()`? You only call it once at the
> top of `share()`?

I was just following the pattern from Geolocation.cpp. Happy to fold it in.

https://codereview.chromium.org/1806253002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/ballista/Actions.cpp:86: return doc ?
doc->frame() : nullptr;
On 2016/04/18 11:16:18, Mike West (slow until 25th) wrote:
> Is there any case in which you need this check? It looks like you're only
> exposing the API to documents (or do you intend to eventually expose it to SW
as
> well?)

Again I was just following Geolocation.cpp. I'm not sure under what
circumstances ScriptState::getExecutionContext() or Document::frame() is null
(the documentation says nothing for the former but some virtual overrides can
return null; the latter the docs just say "can be null").

Is it to do with whether it's in a service worker? I do eventually plan to
expose this to SW.

Geolocation diligently null-checks all of these things but maybe it is better to
just crash if they are null. Do you know what the null cases are? I'm just
putting in DCHECKs now.

https://codereview.chromium.org/1806253002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/ballista/Actions.idl (right):

https://codereview.chromium.org/1806253002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/ballista/Actions.idl:10: NoInterfaceObject,
On 2016/04/18 11:16:18, Mike West (slow until 25th) wrote:
> Nit: Why `NoInterfaceObject`?

Done.

https://codereview.chromium.org/1806253002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/ballista/Actions.idl:13:
[CallWith=ScriptState] Promise<undefined> share (
On 2016/04/18 11:16:18, Mike West (slow until 25th) wrote:
> `undefined` seems strange. It would be nice to have a spec to point to here.
:)

Ah I see, it's supposed to be Promise<void>.

There is no spec because we're just trying to experiment without trying to set
anything in concrete just yet (in fact this API will probably change to a more
complex one soon after landing this). The web platform team have told us that
rapid experimentation is prefered to spec writing so we're deliberately avoiding
writing a formal spec for now.

[Matt Giuca, 2016-04-22 07:50:12]

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

[commit-bot: I haz the power, 2016-04-22 07:50:24]

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

[commit-bot: I haz the power, 2016-04-22 07:52:34]

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

[commit-bot: I haz the power, 2016-04-22 07:52:35]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Matt Giuca, 2016-04-23 01:19:32]

https://codereview.chromium.org/1806253002/diff/380001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/ballista/resources/mock-ballista-service.js
(right):

https://codereview.chromium.org/1806253002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/ballista/resources/mock-ballista-service.js:45:
while (this.pendingRequests_.length) {
I forgot to simplify this logic: the while loop is no longer necessary; this
code should just be folded in to share() and then the body of the loop only
needs to run once.

I'll do that next time I'm at work.

[Mike West, 2016-04-26 08:09:23]

Looks like you need to rebase. Otherwise LGTM. Please send out the intent to
implement in the near future. :)

On the spec vs. experimentation point, I agree that it's fine to experiment
without a formalized document. On the other hand, it's really hard to review
things if I don't know what the thing is supposed to look like and do. Idunno.
There's a balance to be found there somewhere, and I'm pretty sure this is on
the right side of it. *shrug*

[Sam McNally, 2016-04-26 08:31:22]

lgtm

https://codereview.chromium.org/1806253002/diff/380001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/ballista/resources/mock-ballista-service.js
(right):

https://codereview.chromium.org/1806253002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/ballista/resources/mock-ballista-service.js:72:
function pushMockBallistaServiceResult(expectedTitle, expectedText,
Remove this.

https://codereview.chromium.org/1806253002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/ballista/resources/mock-ballista-service.js:80:
function shareSuccess() {
And this.

https://codereview.chromium.org/1806253002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/ballista/resources/mock-ballista-service.js:85:
function shareFailure(error) {
And this.

https://codereview.chromium.org/1806253002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/ballista/resources/mock-ballista-service.js:90:
var mockBallistaServiceReady = mockBallistaService.then();
And this.

https://codereview.chromium.org/1806253002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/ballista/Actions.cpp (right):

https://codereview.chromium.org/1806253002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/ballista/Actions.cpp:49:
m_resolver->reject(DOMException::create(AbortError, error));
Add a TODO to figure out which error to use.

https://codereview.chromium.org/1806253002/diff/380001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/ballista/OWNERS (right):

https://codereview.chromium.org/1806253002/diff/380001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/ballista/OWNERS:1:
mgiuca@chromium.org
Add:

# Changes to Mojo interfaces require a security review to avoid
# introducing new sandbox escapes.
per-file *.mojom=set noparent
per-file *.mojom=dcheng@chromium.org
per-file *.mojom=inferno@chromium.org
per-file *.mojom=jln@chromium.org
per-file *.mojom=jschuh@chromium.org
per-file *.mojom=kenrb@chromium.org
per-file *.mojom=mkwst@chromium.org
per-file *.mojom=nasko@chromium.org
per-file *.mojom=palmer@chromium.org
per-file *.mojom=tsepez@chromium.org
per-file *.mojom=wfh@chromium.org

[Matt Giuca, 2016-04-26 09:14:07]

https://codereview.chromium.org/1806253002/diff/380001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/ballista/resources/mock-ballista-service.js
(right):

https://codereview.chromium.org/1806253002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/ballista/resources/mock-ballista-service.js:45:
while (this.pendingRequests_.length) {
On 2016/04/23 01:19:32, Matt Giuca wrote:
> I forgot to simplify this logic: the while loop is no longer necessary; this
> code should just be folded in to share() and then the body of the loop only
> needs to run once.
> 
> I'll do that next time I'm at work.

Done.

https://codereview.chromium.org/1806253002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/ballista/resources/mock-ballista-service.js:72:
function pushMockBallistaServiceResult(expectedTitle, expectedText,
On 2016/04/26 08:31:22, Sam McNally wrote:
> Remove this.

Done.

https://codereview.chromium.org/1806253002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/ballista/resources/mock-ballista-service.js:80:
function shareSuccess() {
On 2016/04/26 08:31:21, Sam McNally wrote:
> And this.

Done.

https://codereview.chromium.org/1806253002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/ballista/resources/mock-ballista-service.js:85:
function shareFailure(error) {
On 2016/04/26 08:31:22, Sam McNally wrote:
> And this.

Done.

https://codereview.chromium.org/1806253002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/ballista/resources/mock-ballista-service.js:90:
var mockBallistaServiceReady = mockBallistaService.then();
On 2016/04/26 08:31:22, Sam McNally wrote:
> And this.

Done.

https://codereview.chromium.org/1806253002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/ballista/Actions.cpp (right):

https://codereview.chromium.org/1806253002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/ballista/Actions.cpp:49:
m_resolver->reject(DOMException::create(AbortError, error));
On 2016/04/26 08:31:22, Sam McNally wrote:
> Add a TODO to figure out which error to use.

Done.

https://codereview.chromium.org/1806253002/diff/380001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/ballista/OWNERS (right):

https://codereview.chromium.org/1806253002/diff/380001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/ballista/OWNERS:1:
mgiuca@chromium.org
On 2016/04/26 08:31:22, Sam McNally wrote:
> Add:
> 
> # Changes to Mojo interfaces require a security review to avoid
> # introducing new sandbox escapes.
> per-file *.mojom=set noparent
> per-file mailto:*.mojom=dcheng@chromium.org
> per-file mailto:*.mojom=inferno@chromium.org
> per-file mailto:*.mojom=jln@chromium.org
> per-file mailto:*.mojom=jschuh@chromium.org
> per-file mailto:*.mojom=kenrb@chromium.org
> per-file mailto:*.mojom=mkwst@chromium.org
> per-file mailto:*.mojom=nasko@chromium.org
> per-file mailto:*.mojom=palmer@chromium.org
> per-file mailto:*.mojom=tsepez@chromium.org
> per-file mailto:*.mojom=wfh@chromium.org

Done.

That is kind of ridiculous duplication. What happens if someone joins or leaves
the team?

Shouldn't this be using an OWNERS import or something?

[Matt Giuca, 2016-04-26 09:14:19]

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

[commit-bot: I haz the power, 2016-04-26 09:14:45]

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

[commit-bot: I haz the power, 2016-04-26 10:28:39]

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

[commit-bot: I haz the power, 2016-04-26 10:28:41]

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

[Matt Giuca, 2016-04-27 02:43:28]

The CQ bit was checked by mgiuca@chromium.org

[Matt Giuca, 2016-04-27 02:43:28]

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/1806253002/#ps460001
(title: "Update global-interface-listing expectation.
")

[commit-bot: I haz the power, 2016-04-27 02:43:44]

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

[commit-bot: I haz the power, 2016-04-27 02:51:57]

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

[commit-bot: I haz the power, 2016-04-27 02:52:00]

Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[Matt Giuca, 2016-04-27 03:41:32]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2016-04-27 03:41:39]

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

[commit-bot: I haz the power, 2016-04-27 03:59:49]

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

[commit-bot: I haz the power, 2016-04-27 03:59:52]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[Matt Giuca, 2016-04-27 05:02:19]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2016-04-27 05:02:26]

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

[commit-bot: I haz the power, 2016-04-27 05:12:06]

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

[commit-bot: I haz the power, 2016-04-27 05:12:07]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[Matt Giuca, 2016-04-27 05:56:28]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2016-04-27 05:56:42]

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

[commit-bot: I haz the power, 2016-04-27 06:25:50]

Description was changed from

==========
Added navigator.actions.share experimental web API (stub).

Requires the enable-experimental-web-platform-features flag. Currently
does nothing; future CLs will implement this on Android as an
experiment.

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

to

==========
Added navigator.actions.share experimental web API (stub).

Requires the enable-experimental-web-platform-features flag. Currently
does nothing; future CLs will implement this on Android as an
experiment.

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

[commit-bot: I haz the power, 2016-04-27 06:25:52]

Committed patchset #19 (id:460001)

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

Description was changed from

==========
Added navigator.actions.share experimental web API (stub).

Requires the enable-experimental-web-platform-features flag. Currently
does nothing; future CLs will implement this on Android as an
experiment.

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

to

==========
Added navigator.actions.share experimental web API (stub).

Requires the enable-experimental-web-platform-features flag. Currently
does nothing; future CLs will implement this on Android as an
experiment.

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

Committed: https://crrev.com/4eeea499401e03bcf098437d92bf35cf10ba4675
Cr-Commit-Position: refs/heads/master@{#390004}
==========

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

Patchset 19 (id:??) landed as
https://crrev.com/4eeea499401e03bcf098437d92bf35cf10ba4675
Cr-Commit-Position: refs/heads/master@{#390004}

[Matt Giuca, 2016-04-27 07:15:39]

A revert of this CL (patchset #19 id:460001) has been created in
https://codereview.chromium.org/1924753002/ by mgiuca@chromium.org.

The reason for reverting is: The LayoutTests added by this CL are failing on Mac
bots.

BUG=607086.

[Matt Giuca, 2016-04-27 07:30:04]

Description was changed from

==========
Added navigator.actions.share experimental web API (stub).

Requires the enable-experimental-web-platform-features flag. Currently
does nothing; future CLs will implement this on Android as an
experiment.

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

Committed: https://crrev.com/4eeea499401e03bcf098437d92bf35cf10ba4675
Cr-Commit-Position: refs/heads/master@{#390004}
==========

to

==========
Added navigator.actions.share experimental web API (stub).

Requires the enable-experimental-web-platform-features flag. Currently
does nothing; future CLs will implement this on Android as an
experiment.

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

Committed: https://crrev.com/4eeea499401e03bcf098437d92bf35cf10ba4675
Cr-Commit-Position: refs/heads/master@{#390004}
==========

[esprehn, 2016-04-27 08:12:22]

Please don't commit code before sending the intent to implement, that's really
backwards. The point of the intent to implement is that people get to discuss
what you're doing in advance.

[esprehn, 2016-04-27 08:18:55]

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

[esprehn, 2016-04-27 08:18:57]

https://codereview.chromium.org/1806253002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/ballista/Actions.cpp (right):

https://codereview.chromium.org/1806253002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/ballista/Actions.cpp:59: Document* doc =
toDocument(scriptState->getExecutionContext());
don't abbreviate in blink.

https://codereview.chromium.org/1806253002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/ballista/Actions.h (right):

https://codereview.chromium.org/1806253002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/ballista/Actions.h:12: #include
"core/events/EventTarget.h"
do you need this?

https://codereview.chromium.org/1806253002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/ballista/Actions.h:26: class Actions final :
public GarbageCollectedFinalized<Actions>, public ScriptWrappable {
this is too generic, you're taking the word Actions in the whole Blink
namespace. This would need to be DOMActions I suspect, but also Actions seems
too generic to ship to the web

https://codereview.chromium.org/1806253002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/ballista/Actions.idl (right):

https://codereview.chromium.org/1806253002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/ballista/Actions.idl:11: ] interface Actions {
This is too generic to ship, you're adding Actions to the global. I think you
need to come up with a less generic name.

https://codereview.chromium.org/1806253002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/ballista/OWNERS (right):

https://codereview.chromium.org/1806253002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/ballista/OWNERS:2: sammc@chromium.org
I don't think we should add you two as owners yet, you haven't written much code
in blink:

https://blink.lc/chromium/log/third_party/WebKit?qt=author&q=mgiuca

https://codereview.chromium.org/1806253002/diff/460001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/ballista/ballista.mojom (right):

https://codereview.chromium.org/1806253002/diff/460001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/ballista/ballista.mojom:7: interface
BallistaService {
Don't add code names to blink.

[Matt Giuca, 2016-04-27 09:08:18]

https://codereview.chromium.org/1806253002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/ballista/NavigatorActions.cpp (right):

https://codereview.chromium.org/1806253002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/ballista/NavigatorActions.cpp:7: #include
"config.h"
mostynb: Remove this stray include.

[dglazkov, 2016-04-27 17:10:59]

On 2016/04/27 at 08:12:22, esprehn wrote:
> Please don't commit code before sending the intent to implement, that's really
backwards. The point of the intent to implement is that people get to discuss
what you're doing in advance.

+1.

[Matt Giuca, 2016-06-22 03:27:18]

Patchset #24 (id:550001) has been deleted

[Matt Giuca, 2016-06-22 03:27:32]

Patchset #24 (id:570001) has been deleted

[Matt Giuca, 2016-06-22 03:52:48]

Update: There was a long offline discussion about the naming of this and
strategy. We decided to go with "Web Share" and not brand this as Ballista or a
generic "actions" name. Therefore, everything had to get renamed.

This proposal now has a public WICG repo:
https://github.com/WICG/web-share
WICG discourse post:
https://discourse.wicg.io/t/web-share-api-for-sharing-content-to-arbitrary-de...
Blink intent to implement:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/1BOhy5av8MQ/discus...

This is gonna make code review a bit tricky but here are the main points since
the last round of review (PS 19):
- PS 22, 23, 25 and 26 are some small readable changes.
- PS 27 is the giant rename; since all the files were renamed you can't tell the
diffs, but it was basically just renaming the files and all the internal
classes. Nothing else changed.

mkwst: Since it's changed a lot since you last looked, I'll let you have another
look if you want.

esprehn: Are you OK with this now? The other concern you raised was OWNERS file.
Can you suggest someone to be owners there if you don't think Sam and myself
should be? Thanks.

I also still need to investigate the Mac failure that caused this CL to be
reverted in the first place but I'll do that in parallel with the above
questions.

[Matt Giuca, 2016-06-22 03:53:00]

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

[commit-bot: I haz the power, 2016-06-22 03:53:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1806253002/650001

[commit-bot: I haz the power, 2016-06-22 04:00:21]

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

[commit-bot: I haz the power, 2016-06-22 04:00:23]

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

[Matt Giuca, 2016-06-22 04:20:07]

Description was changed from

==========
Added navigator.actions.share experimental web API (stub).

Requires the enable-experimental-web-platform-features flag. Currently
does nothing; future CLs will implement this on Android as an
experiment.

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

Committed: https://crrev.com/4eeea499401e03bcf098437d92bf35cf10ba4675
Cr-Commit-Position: refs/heads/master@{#390004}
==========

to

==========
Added Web Share (navigator.share) experimental web API (stub).

Requires the enable-experimental-web-platform-features flag. Currently
does nothing; future CLs will implement this on Android as an
experiment.

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

Committed: https://crrev.com/4eeea499401e03bcf098437d92bf35cf10ba4675
Cr-Commit-Position: refs/heads/master@{#390004}
==========

[Matt Giuca, 2016-06-22 04:20:47]

Description was changed from

==========
Added Web Share (navigator.share) experimental web API (stub).

Requires the enable-experimental-web-platform-features flag. Currently
does nothing; future CLs will implement this on Android as an
experiment.

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

Committed: https://crrev.com/4eeea499401e03bcf098437d92bf35cf10ba4675
Cr-Commit-Position: refs/heads/master@{#390004}
==========

to

==========
Added Web Share (navigator.share) experimental web API (stub).

Requires the enable-experimental-web-platform-features flag. Currently
does nothing; future CLs will implement this on Android as an
experiment.

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

[Matt Giuca, 2016-06-22 04:21:34]

Description was changed from

==========
Added Web Share (navigator.share) experimental web API (stub).

Requires the enable-experimental-web-platform-features flag. Currently
does nothing; future CLs will implement this on Android as an
experiment.

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

to

==========
Added Web Share (navigator.share) experimental web API (stub).

Requires the enable-experimental-web-platform-features flag. Currently
does nothing; future CLs will implement this on Android as an
experiment.

Re-land of r390004.

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

[Matt Giuca, 2016-06-22 06:36:30]

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

[commit-bot: I haz the power, 2016-06-22 06:36:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1806253002/670001

[commit-bot: I haz the power, 2016-06-22 06:44:01]

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

[commit-bot: I haz the power, 2016-06-22 06:44:02]

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

[esprehn, 2016-06-22 08:53:03]

I would suggest you don't need an OWNERS file until you've both landed a bunch
of patches in there. :)

[Matt Giuca, 2016-06-23 01:35:43]

On 2016/06/22 08:53:03, esprehn wrote:
> I would suggest you don't need an OWNERS file until you've both landed a bunch
> of patches in there. :)

Oh, OK. I thought every Blink module was supposed to have an OWNERS file

Mike specifically asked for it in his review:
https://codereview.chromium.org/1806253002/#msg28

webshare would be the only directory in public/platform/modules without one.

[Matt Giuca, 2016-07-07 04:26:54]

Mike: Would you be able to look over this CL as it has changed a lot since PS
19.

[Mike West, 2016-07-08 07:17:49]

Still LGTM % nits.

https://codereview.chromium.org/1806253002/diff/710001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt
(right):

https://codereview.chromium.org/1806253002/diff/710001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt:3931:
method share
Nit: Consider adding an `idlharness.js` test as well to verify the bits and
pieces of this  (http://testthewebforward.org/docs/testharness-idlharness.html:
see http/tests/credentialmanager/idl.html for a concrete example). It's probably
not critical given that you only have one method, but it would take care of a
few of the edge cases you're poking at in tests below.

https://codereview.chromium.org/1806253002/diff/710001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webshare/NavigatorShare.h (right):

https://codereview.chromium.org/1806253002/diff/710001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webshare/NavigatorShare.h:33: static
ScriptPromise share(ScriptState*, Navigator&, const String&, const String&);
Nit: Can you name the various `String` parameters here? `ScriptState` and
`Navigator` are clear, `String` less so.

[Matt Giuca, 2016-07-11 06:52:53]

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

[commit-bot: I haz the power, 2016-07-11 06:53:08]

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

[Matt Giuca, 2016-07-11 07:40:38]

https://codereview.chromium.org/1806253002/diff/710001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt
(right):

https://codereview.chromium.org/1806253002/diff/710001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt:3931:
method share
On 2016/07/08 07:17:49, Mike West wrote:
> Nit: Consider adding an `idlharness.js` test as well to verify the bits and
> pieces of this 
(http://testthewebforward.org/docs/testharness-idlharness.html:
> see http/tests/credentialmanager/idl.html for a concrete example). It's
probably
> not critical given that you only have one method, but it would take care of a
> few of the edge cases you're poking at in tests below.

Thanks for the details. I have written up the IDL test but I think landing that
is outside the scope of this CL (since as you say it isn't critical since I've
already covered it with other tests).

Along the way I found a bug (which I will write up tomorrow) in Chrome's
bindings which make it impossible for idlharness to actually pass. (idlharness
expects navigator.share.apply(null) to reject the promise, but it throws a
TypeError instead). This appears to be a problem with Chrome's IDL bindings
since it happens for all methods and disagrees with Firefox. I will do a proper
write-up tomorrow but I don't want to block this CL on sorting that mess out.

(Since very few things are using idlharness I'd guess this hasn't been tested
very thoroughly...)

https://codereview.chromium.org/1806253002/diff/710001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webshare/NavigatorShare.h (right):

https://codereview.chromium.org/1806253002/diff/710001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webshare/NavigatorShare.h:33: static
ScriptPromise share(ScriptState*, Navigator&, const String&, const String&);
On 2016/07/08 07:17:49, Mike West wrote:
> Nit: Can you name the various `String` parameters here? `ScriptState` and
> `Navigator` are clear, `String` less so.

Done.

[Mike West, 2016-07-11 08:09:37]

On 2016/07/11 at 07:40:38, mgiuca wrote:
>
https://codereview.chromium.org/1806253002/diff/710001/third_party/WebKit/Lay...
> File
third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt
(right):
> 
>
https://codereview.chromium.org/1806253002/diff/710001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt:3931:
method share
> On 2016/07/08 07:17:49, Mike West wrote:
> > Nit: Consider adding an `idlharness.js` test as well to verify the bits and
> > pieces of this 
(http://testthewebforward.org/docs/testharness-idlharness.html:
> > see http/tests/credentialmanager/idl.html for a concrete example). It's
probably
> > not critical given that you only have one method, but it would take care of
a
> > few of the edge cases you're poking at in tests below.
> 
> Thanks for the details. I have written up the IDL test but I think landing
that is outside the scope of this CL (since as you say it isn't critical since
I've already covered it with other tests).
> 
> Along the way I found a bug (which I will write up tomorrow) in Chrome's
bindings which make it impossible for idlharness to actually pass. (idlharness
expects navigator.share.apply(null) to reject the promise, but it throws a
TypeError instead). This appears to be a problem with Chrome's IDL bindings
since it happens for all methods and disagrees with Firefox. I will do a proper
write-up tomorrow but I don't want to block this CL on sorting that mess out.
> 
> (Since very few things are using idlharness I'd guess this hasn't been tested
very thoroughly...)

SGTM, and yes. We're not very good about using idlharness yet. As we get better,
I'm sure we'll come across bugs. :)

[commit-bot: I haz the power, 2016-07-11 08:09:48]

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

[commit-bot: I haz the power, 2016-07-11 08:09:50]

Dry run: This issue passed the CQ dry run.

[Matt Giuca, 2016-07-12 00:49:05]

The CQ bit was checked by mgiuca@chromium.org

[Matt Giuca, 2016-07-12 00:49:06]

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/1806253002/#ps770001
(title: "Add argument names.")

[commit-bot: I haz the power, 2016-07-12 00:49:49]

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-07-12 00:59:23]

Description was changed from

==========
Added Web Share (navigator.share) experimental web API (stub).

Requires the enable-experimental-web-platform-features flag. Currently
does nothing; future CLs will implement this on Android as an
experiment.

Re-land of r390004.

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

to

==========
Added Web Share (navigator.share) experimental web API (stub).

Requires the enable-experimental-web-platform-features flag. Currently
does nothing; future CLs will implement this on Android as an
experiment.

Re-land of r390004.

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

[commit-bot: I haz the power, 2016-07-12 00:59:25]

Committed patchset #33 (id:770001)

[commit-bot: I haz the power, 2016-07-12 00:59:30]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-12 01:00:40]

Description was changed from

==========
Added Web Share (navigator.share) experimental web API (stub).

Requires the enable-experimental-web-platform-features flag. Currently
does nothing; future CLs will implement this on Android as an
experiment.

Re-land of r390004.

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

to

==========
Added Web Share (navigator.share) experimental web API (stub).

Requires the enable-experimental-web-platform-features flag. Currently
does nothing; future CLs will implement this on Android as an
experiment.

Re-land of r390004.

BUG=595608

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

[commit-bot: I haz the power, 2016-07-12 01:00:42]

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

[Matt Giuca, 2016-07-12 01:31:27]

On 2016/07/12 01:00:42, commit-bot: I haz the power wrote:
> Patchset 33 (id:??) landed as
> https://crrev.com/d123f9a4d82543ae3769094df5c4cac24cd2d693
> Cr-Commit-Position: refs/heads/master@{#404759}

I filed the IDL bindings bug: https://crbug.com/627309.