Start implementing new navigator.connect API

Start implementing new navigator.connect API.

As it turns out, we changed our mind about the desired API yet again, and this
time rather completely. This is the start of implementing the currently specced
API, which hopefully will actually stick.

This CL adds stubs for the new ServicePortCollection API.

BUG=426458
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197697

[Marijn Kruisselbrink, 2015-06-22 17:40:12]

Patchset #1 (id:1) has been deleted

[Marijn Kruisselbrink, 2015-06-22 17:40:21]

Patchset #1 (id:20001) has been deleted

[Marijn Kruisselbrink, 2015-06-22 17:47:09]

mek@chromium.org changed reviewers:
+ scheib@chromium.org

[scheib, 2015-06-23 03:57:21]

https://codereview.chromium.org/1192913003/diff/40001/Source/modules/navigato...
File Source/modules/navigatorconnect/NavigatorServices.cpp (right):

https://codereview.chromium.org/1192913003/diff/40001/Source/modules/navigato...
Source/modules/navigatorconnect/NavigatorServices.cpp:13:
NavigatorServices::NavigatorServices()
Method definitions in the corresponding .cc file should be the same as the
declaration order, as much as possible.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

https://codereview.chromium.org/1192913003/diff/40001/Source/modules/navigato...
File Source/modules/navigatorconnect/ServicePortCollection.cpp (right):

https://codereview.chromium.org/1192913003/diff/40001/Source/modules/navigato...
Source/modules/navigatorconnect/ServicePortCollection.cpp:19:
ServicePortCollection::ServicePortCollection(ExecutionContext* context)
Method definitions in the corresponding .cc file should be the same as the
declaration order, as much as possible.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

https://codereview.chromium.org/1192913003/diff/40001/Source/modules/navigato...
Source/modules/navigatorconnect/ServicePortCollection.cpp:45: return
EventTargetNames::ServicePortCollection;
Doesn't seem to be defined.

https://codereview.chromium.org/1192913003/diff/40001/Source/modules/navigato...
File Source/modules/navigatorconnect/ServicePortCollection.idl (right):

https://codereview.chromium.org/1192913003/diff/40001/Source/modules/navigato...
Source/modules/navigatorconnect/ServicePortCollection.idl:13: attribute
EventHandler onconnect;
Spec has a "Exposed=ServiceWorker" before the EventHandlers (with indentation I
don't understand. Just checking that nothing needs to be entered here? Perhaps a
comment for why they differ.

https://codereview.chromium.org/1192913003/diff/40001/Source/modules/navigato...
File Source/modules/navigatorconnect/WorkerNavigatorServices.cpp (right):

https://codereview.chromium.org/1192913003/diff/40001/Source/modules/navigato...
Source/modules/navigatorconnect/WorkerNavigatorServices.cpp:13:
WorkerNavigatorServices::WorkerNavigatorServices()
Method definitions in the corresponding .cc file should be the same as the
declaration order, as much as possible.

[Marijn Kruisselbrink, 2015-06-23 04:11:05]

https://codereview.chromium.org/1192913003/diff/40001/Source/modules/navigato...
File Source/modules/navigatorconnect/NavigatorServices.cpp (right):

https://codereview.chromium.org/1192913003/diff/40001/Source/modules/navigato...
Source/modules/navigatorconnect/NavigatorServices.cpp:13:
NavigatorServices::NavigatorServices()
On 2015/06/23 at 03:57:21, scheib wrote:
> Method definitions in the corresponding .cc file should be the same as the
declaration order, as much as possible.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

Done

https://codereview.chromium.org/1192913003/diff/40001/Source/modules/navigato...
File Source/modules/navigatorconnect/ServicePortCollection.cpp (right):

https://codereview.chromium.org/1192913003/diff/40001/Source/modules/navigato...
Source/modules/navigatorconnect/ServicePortCollection.cpp:19:
ServicePortCollection::ServicePortCollection(ExecutionContext* context)
On 2015/06/23 at 03:57:21, scheib wrote:
> Method definitions in the corresponding .cc file should be the same as the
declaration order, as much as possible.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

Done

https://codereview.chromium.org/1192913003/diff/40001/Source/modules/navigato...
Source/modules/navigatorconnect/ServicePortCollection.cpp:45: return
EventTargetNames::ServicePortCollection;
On 2015/06/23 at 03:57:21, scheib wrote:
> Doesn't seem to be defined.

It compiles (see trybots), so obviously it is defined (it's the change to
Source/modules/EventTargetModulesFactory.in
 that defines this).

https://codereview.chromium.org/1192913003/diff/40001/Source/modules/navigato...
File Source/modules/navigatorconnect/ServicePortCollection.idl (right):

https://codereview.chromium.org/1192913003/diff/40001/Source/modules/navigato...
Source/modules/navigatorconnect/ServicePortCollection.idl:13: attribute
EventHandler onconnect;
On 2015/06/23 03:57:21, scheib wrote:
> Spec has a "Exposed=ServiceWorker" before the EventHandlers (with indentation
I
> don't understand. Just checking that nothing needs to be entered here? Perhaps
a
> comment for why they differ. 

Oh yes, will definitely add a comment. I don't understand the indendation in the
spec either, but it's currently missing here because the blink bindings removed
support for per-attribute Exposed extended attributes a couple of days ago.

https://codereview.chromium.org/1192913003/diff/40001/Source/modules/navigato...
File Source/modules/navigatorconnect/WorkerNavigatorServices.cpp (right):

https://codereview.chromium.org/1192913003/diff/40001/Source/modules/navigato...
Source/modules/navigatorconnect/WorkerNavigatorServices.cpp:13:
WorkerNavigatorServices::WorkerNavigatorServices()
On 2015/06/23 at 03:57:21, scheib wrote:
> Method definitions in the corresponding .cc file should be the same as the
declaration order, as much as possible.

Done

[scheib, 2015-06-23 04:57:02]

LGTM

[Marijn Kruisselbrink, 2015-06-23 16:45:52]

mek@chromium.org changed reviewers:
+ chrishtr@chromium.org

[Marijn Kruisselbrink, 2015-06-23 16:45:52]

+chrishtr for Source/modules/EventTargetModulesFactory.in OWNERS (although I'm
not entirely sure why that file requires top-level blink owners approval...)

[chrishtr, 2015-06-23 16:54:53]

chrishtr@chromium.org changed reviewers:
+ haraken@chromium.org, tkent@chromium.org

[chrishtr, 2015-06-23 16:54:54]

+tkent, haraken to check that this is using best practices for modules, I'm not
an expert.

[tkent, 2015-06-23 23:54:49]

lgtm

https://codereview.chromium.org/1192913003/diff/60001/Source/modules/navigato...
File Source/modules/navigatorconnect/ServicePortCollection.h (right):

https://codereview.chromium.org/1192913003/diff/60001/Source/modules/navigato...
Source/modules/navigatorconnect/ServicePortCollection.h:29: virtual
~ServicePortCollection();
nit: remove |virtual|, and add |override|.

[tkent, 2015-06-23 23:56:00]

On 2015/06/23 16:45:52, Marijn Kruisselbrink wrote:
> +chrishtr for Source/modules/EventTargetModulesFactory.in OWNERS (although I'm
> not entirely sure why that file requires top-level blink owners approval...)

Public API changes should be reviewed by an API owner.

[Marijn Kruisselbrink, 2015-06-24 00:26:52]

On 2015/06/23 at 23:56:00, tkent wrote:
> On 2015/06/23 16:45:52, Marijn Kruisselbrink wrote:
> > +chrishtr for Source/modules/EventTargetModulesFactory.in OWNERS (although
I'm
> > not entirely sure why that file requires top-level blink owners approval...)
> 
> Public API changes should be reviewed by an API owner.

Yeah, I wasn't trying to say that I didn't think this CL should be reviewed by
an API owner. I just don't quite understand the difference at an OWNERS file
level between changes to Source/modules/modules.gypi (anybody can
change/approve) and Source/modules/EventTargetModulesFactory.in (needs API
owners approval). Both files just seem to be more or less lists of files/classes
that make up modules (one just being the subset of files that happen to inherit
from EventTarget). And there are many other possible web exposed API changes
that don't require (at OWNERS level) API owners to review the change.

https://codereview.chromium.org/1192913003/diff/60001/Source/modules/navigato...
File Source/modules/navigatorconnect/ServicePortCollection.h (right):

https://codereview.chromium.org/1192913003/diff/60001/Source/modules/navigato...
Source/modules/navigatorconnect/ServicePortCollection.h:29: virtual
~ServicePortCollection();
On 2015/06/23 at 23:54:48, tkent wrote:
> nit: remove |virtual|, and add |override|.

done

[Marijn Kruisselbrink, 2015-06-24 00:30:49]

The CQ bit was checked by mek@chromium.org

[Marijn Kruisselbrink, 2015-06-24 00:30:49]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
scheib@chromium.org
Link to the patchset: https://codereview.chromium.org/1192913003/#ps80001
(title: "override instead of virtual")

[commit-bot: I haz the power, 2015-06-24 00:31:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1192913003/80001

[haraken, 2015-06-24 01:18:46]

LGTM

https://codereview.chromium.org/1192913003/diff/80001/Source/modules/navigato...
File Source/modules/navigatorconnect/ServicePortCollection.h (right):

https://codereview.chromium.org/1192913003/diff/80001/Source/modules/navigato...
Source/modules/navigatorconnect/ServicePortCollection.h:19: class MODULES_EXPORT
ServicePortCollection final

Nit: We can move this to Oilpan's heap. The oilpan team will take care of it.

https://codereview.chromium.org/1192913003/diff/80001/Source/modules/navigato...
File Source/modules/navigatorconnect/ServicePortCollection.idl (right):

https://codereview.chromium.org/1192913003/diff/80001/Source/modules/navigato...
Source/modules/navigatorconnect/ServicePortCollection.idl:14: // supported by
blink bindings, so figure out if this is needed.

This is now supported :)

https://codereview.chromium.org/1202683002/

[commit-bot: I haz the power, 2015-06-24 04:11:56]

Committed patchset #3 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197697