Initial implementation of stashed message ports, blink side

Initial implementation of stashed message ports, blink side

This is part of a series of patches to implement an
initial version of a mechanism for a MessagePort to
outlive a service worker as described in
https://github.com/mkruisselbrink/navigator-connect/blob/gh-pages/docs/api-idea.md#persisted-messageports

[1/3] this patch, blink side implementation
[2/3] https://codereview.chromium.org/1110103003/, content side implementation
[3/3] https://codereview.chromium.org/1063533002/, layout tests

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

[Marijn Kruisselbrink, 2015-04-30 18:15:41]

Patchset #1 (id:1) has been deleted

[Marijn Kruisselbrink, 2015-04-30 18:15:48]

Patchset #1 (id:20001) has been deleted

[Marijn Kruisselbrink, 2015-04-30 18:15:55]

Patchset #1 (id:40001) has been deleted

[Marijn Kruisselbrink, 2015-04-30 18:16:03]

Patchset #1 (id:60001) has been deleted

[Marijn Kruisselbrink, 2015-04-30 18:16:10]

Patchset #1 (id:80001) has been deleted

[Marijn Kruisselbrink, 2015-04-30 20:21:12]

Patchset #1 (id:100001) has been deleted

[Marijn Kruisselbrink, 2015-04-30 20:21:19]

Patchset #1 (id:120001) has been deleted

[Marijn Kruisselbrink, 2015-04-30 23:05:30]

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

[Marijn Kruisselbrink, 2015-05-05 21:47:06]

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

[Marijn Kruisselbrink, 2015-05-05 21:47:07]

actually I should probably get this reviewed before bothering serviceworker
OWNERS; scheib, would you mind taking a look?

[scheib, 2015-05-06 21:44:31]

Looking generally good to me, some Qs:

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.h (right):

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.h:71: 
Remove blank line and have ports() in same group attributed to .idl.

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
File Source/modules/serviceworkers/StashedMessagePort.cpp (right):

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
Source/modules/serviceworkers/StashedMessagePort.cpp:24:
StashedMessagePort::StashedMessagePort(ExecutionContext& executionContext,
PassOwnPtr<WebMessagePortChannel> remote, const String& name)
definitions in the corresponding .cc file should be the same as the declaration
order
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_...

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
Source/modules/serviceworkers/StashedMessagePort.cpp:50:
DEFINE_TRACE(StashedMessagePort)
Maybe use DEFINE_TRACE_INLINE for this in .h?

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
Source/modules/serviceworkers/StashedMessagePort.cpp:61: void
StashedMessagePort::dispatchMessages()
There's no need to check the closed, started states as in 
MessagePort::dispatchMessages()?

This method doesn't seem to operate with any state of this object other than
executionContext(), should it be filtering based on name()? Or just send all the
events that can be sent?

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
File Source/modules/serviceworkers/StashedMessagePort.h (right):

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
Source/modules/serviceworkers/StashedMessagePort.h:17: class StashedMessagePort
final
Class comment

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
Source/modules/serviceworkers/StashedMessagePort.h:26: String name() const;
// StashedMessagePort.idl

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
Source/modules/serviceworkers/StashedMessagePort.h:36:
WeakPtrFactory<StashedMessagePort> m_weakFactory;
WeakPtrFactory best practice is as last member to ensure it is first member
destroyed, if other members hold weak references they will be invalidated before
their objects destroyed.

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
File Source/modules/serviceworkers/StashedMessagePort.idl (right):

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
Source/modules/serviceworkers/StashedMessagePort.idl:11: readonly attribute
USVString name;
The spec should explain why USVString vs String is required.

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
File Source/modules/serviceworkers/StashedPortCollection.cpp (right):

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
Source/modules/serviceworkers/StashedPortCollection.cpp:45: static AtomicString
s;
Document why empty string.
From EventTarget:
// - Override EventTarget::interfaceName() and executionContext(). The former
//   will typically return EventTargetNames::YourClassName.

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
File Source/modules/serviceworkers/StashedPortCollection.h (right):

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
Source/modules/serviceworkers/StashedPortCollection.h:29:
PassRefPtrWillBeRawPtr<StashedMessagePort> add(ScriptState*, const String& name,
MessagePort*);
// StashedPortCollection.idl, then methods implementing that.

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
Source/modules/serviceworkers/StashedPortCollection.h:33: const AtomicString&
interfaceName() const override;
What interface is this implementing?

[Marijn Kruisselbrink, 2015-05-12 06:55:06]

Patchset #2 (id:160001) has been deleted

[Marijn Kruisselbrink, 2015-05-12 06:56:41]

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.h (right):

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.h:71: 
On 2015/05/06 21:44:30, scheib wrote:
> Remove blank line and have ports() in same group attributed to .idl.

Done, although all the methods upto the next comment all are attributable to the
.idl. But it probably still makes sense to group this one together with the
other two getters.

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
File Source/modules/serviceworkers/StashedMessagePort.cpp (right):

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
Source/modules/serviceworkers/StashedMessagePort.cpp:24:
StashedMessagePort::StashedMessagePort(ExecutionContext& executionContext,
PassOwnPtr<WebMessagePortChannel> remote, const String& name)
On 2015/05/06 21:44:30, scheib wrote:
> definitions in the corresponding .cc file should be the same as the
declaration
> order
>
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_...

yes, while that's true it's pretty common in at least blink code to have the
definition of constructors immediately following the definition of ::create
methods.
Having the public ::create method and private constructor closer together in the
declaration order would definitely violate the style guide, but at least the
rule about defitition order has some wiggle room ("as much as possible.").

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
Source/modules/serviceworkers/StashedMessagePort.cpp:50:
DEFINE_TRACE(StashedMessagePort)
On 2015/05/06 21:44:30, scheib wrote:
> Maybe use DEFINE_TRACE_INLINE for this in .h?

Done.

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
Source/modules/serviceworkers/StashedMessagePort.cpp:61: void
StashedMessagePort::dispatchMessages()
On 2015/05/06 21:44:30, scheib wrote:
> There's no need to check the closed, started states as in 
> MessagePort::dispatchMessages()?
!isEntangled() implies !closed(), and for now I'm leaning towards the act of
stashing a port always starting it (the call to start() in the constructor), so
started() would always be true.

> This method doesn't seem to operate with any state of this object other than
> executionContext(), should it be filtering based on name()? Or just send all
the
> events that can be sent?

It just sends all the events. The source attribute of the events will be set to
this instance, so javascript code can do whatever filtering on name() it needs
to do.

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
File Source/modules/serviceworkers/StashedMessagePort.h (right):

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
Source/modules/serviceworkers/StashedMessagePort.h:17: class StashedMessagePort
final
On 2015/05/06 21:44:31, scheib wrote:
> Class comment

Done.

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
Source/modules/serviceworkers/StashedMessagePort.h:26: String name() const;
On 2015/05/06 21:44:31, scheib wrote:
> // StashedMessagePort.idl

Done.

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
Source/modules/serviceworkers/StashedMessagePort.h:36:
WeakPtrFactory<StashedMessagePort> m_weakFactory;
On 2015/05/06 21:44:30, scheib wrote:
> WeakPtrFactory best practice is as last member to ensure it is first member
> destroyed, if other members hold weak references they will be invalidated
before
> their objects destroyed.

Done.

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
File Source/modules/serviceworkers/StashedMessagePort.idl (right):

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
Source/modules/serviceworkers/StashedMessagePort.idl:11: readonly attribute
USVString name;
On 2015/05/06 21:44:31, scheib wrote:
> The spec should explain why USVString vs String is required. 

Hmm, from the comments on
https://github.com/slightlyoff/BackgroundSync/issues/51 it seems this should
probably just be a DOMString, so changed it.

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
File Source/modules/serviceworkers/StashedPortCollection.cpp (right):

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
Source/modules/serviceworkers/StashedPortCollection.cpp:45: static AtomicString
s;
On 2015/05/06 21:44:31, scheib wrote:
> Document why empty string.
> From EventTarget:
> // - Override EventTarget::interfaceName() and executionContext(). The former
> //   will typically return EventTargetNames::YourClassName.

My mistake, shouldn't be empty string of course.

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
File Source/modules/serviceworkers/StashedPortCollection.h (right):

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
Source/modules/serviceworkers/StashedPortCollection.h:29:
PassRefPtrWillBeRawPtr<StashedMessagePort> add(ScriptState*, const String& name,
MessagePort*);
On 2015/05/06 21:44:31, scheib wrote:
> // StashedPortCollection.idl, then methods implementing that.

Done.

https://codereview.chromium.org/1008533005/diff/140001/Source/modules/service...
Source/modules/serviceworkers/StashedPortCollection.h:33: const AtomicString&
interfaceName() const override;
On 2015/05/06 21:44:31, scheib wrote:
> What interface is this implementing?

Done.

[scheib, 2015-05-12 18:12:17]

LGTM

[Marijn Kruisselbrink, 2015-05-12 23:47:56]

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

[Marijn Kruisselbrink, 2015-05-12 23:47:57]

+falken for serviceworker OWNERS

[nhiroki, 2015-05-13 01:09:37]

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

[nhiroki, 2015-05-13 01:09:38]

Let me make comments from the SW's point of view (falken@ might be busy due to
the blinkon)

https://codereview.chromium.org/1008533005/diff/180001/Source/modules/service...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.idl (right):

https://codereview.chromium.org/1008533005/diff/180001/Source/modules/service...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.idl:42:
[RuntimeEnabled=NavigatorConnect] readonly attribute StashedPortCollection
ports;
For improving modularity, can we move this to
ServiceWorkerGlobalScopeNavigatorConnect.idl and make
ServiceWorkerGlobalScopeNavigatorConnect.{cpp,h}?

Seemingly other partial interfaces (eg. NavigatorStorageQuota.idl,
WorkerGlobalScopeIndexedDatabase.idl) are doing such a thing.

https://codereview.chromium.org/1008533005/diff/180001/Source/modules/service...
File Source/modules/serviceworkers/StashedMessagePort.cpp (right):

https://codereview.chromium.org/1008533005/diff/180001/Source/modules/service...
Source/modules/serviceworkers/StashedMessagePort.cpp:74: 
nit: an extra blank line

https://codereview.chromium.org/1008533005/diff/180001/Source/modules/service...
File Source/modules/serviceworkers/StashedMessagePort.h (right):

https://codereview.chromium.org/1008533005/diff/180001/Source/modules/service...
Source/modules/serviceworkers/StashedMessagePort.h:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
You might want to move StashedMessagePort.* and StashedPortCollection.* to
mosuldes/navigatorconnect/ because those interfaces are defined in the
NavigatorConnect spec and seem independent of the ServiceWorker.

https://codereview.chromium.org/1008533005/diff/180001/Source/modules/service...
Source/modules/serviceworkers/StashedMessagePort.h:15: typedef
WillBeHeapVector<RefPtrWillBeMember<StashedMessagePort>, 1>
StashedMessagePortArray;
nit: typedef -> using

[Marijn Kruisselbrink, 2015-05-13 01:46:53]

https://codereview.chromium.org/1008533005/diff/180001/Source/modules/service...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.idl (right):

https://codereview.chromium.org/1008533005/diff/180001/Source/modules/service...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.idl:42:
[RuntimeEnabled=NavigatorConnect] readonly attribute StashedPortCollection
ports;
On 2015/05/13 01:09:37, nhiroki wrote:
> For improving modularity, can we move this to
> ServiceWorkerGlobalScopeNavigatorConnect.idl and make
> ServiceWorkerGlobalScopeNavigatorConnect.{cpp,h}?
> 
> Seemingly other partial interfaces (eg. NavigatorStorageQuota.idl,
> WorkerGlobalScopeIndexedDatabase.idl) are doing such a thing.

Certainly possible, although I think conceptually this is much more a core
service worker functionality than either of those others (and even more a core
service worker functionality than for example fetch, which is defined in this
file).
(see also my other reply to your other comment)

But if you disagree I'll happily move this code somewhere else (maybe I should
also find some of the SW people here at BlinkOn to discuss some of this in
person...)

https://codereview.chromium.org/1008533005/diff/180001/Source/modules/service...
File Source/modules/serviceworkers/StashedMessagePort.cpp (right):

https://codereview.chromium.org/1008533005/diff/180001/Source/modules/service...
Source/modules/serviceworkers/StashedMessagePort.cpp:74: 
On 2015/05/13 01:09:38, nhiroki wrote:
> nit: an extra blank line

Done.

https://codereview.chromium.org/1008533005/diff/180001/Source/modules/service...
File Source/modules/serviceworkers/StashedMessagePort.h (right):

https://codereview.chromium.org/1008533005/diff/180001/Source/modules/service...
Source/modules/serviceworkers/StashedMessagePort.h:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
On 2015/05/13 01:09:38, nhiroki wrote:
> You might want to move StashedMessagePort.* and StashedPortCollection.* to
> mosuldes/navigatorconnect/ because those interfaces are defined in the
> NavigatorConnect spec and seem independent of the ServiceWorker.

I'm not sure about this. It's certainly possible to move this to
modules/navigatorconnect, but I'm not sure if that really makes sense. The whole
stashed port thing is conceptually completely independent of navigator.connect.
It purely is a way to make message ports useful in service workers. So given
that it seemed to me that grouping this together with navigator.connect would
make a lot less sense than grouping it together with other basic service worker
functionality.

https://codereview.chromium.org/1008533005/diff/180001/Source/modules/service...
Source/modules/serviceworkers/StashedMessagePort.h:15: typedef
WillBeHeapVector<RefPtrWillBeMember<StashedMessagePort>, 1>
StashedMessagePortArray;
On 2015/05/13 01:09:38, nhiroki wrote:
> nit: typedef -> using

Done.

[kinuko, 2015-05-13 04:30:43]

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

[kinuko, 2015-05-13 04:30:44]

https://codereview.chromium.org/1008533005/diff/180001/Source/modules/service...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.idl (right):

https://codereview.chromium.org/1008533005/diff/180001/Source/modules/service...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.idl:42:
[RuntimeEnabled=NavigatorConnect] readonly attribute StashedPortCollection
ports;
On 2015/05/13 01:46:53, Marijn Kruisselbrink wrote:
> On 2015/05/13 01:09:37, nhiroki wrote:
> > For improving modularity, can we move this to
> > ServiceWorkerGlobalScopeNavigatorConnect.idl and make
> > ServiceWorkerGlobalScopeNavigatorConnect.{cpp,h}?
> > 
> > Seemingly other partial interfaces (eg. NavigatorStorageQuota.idl,
> > WorkerGlobalScopeIndexedDatabase.idl) are doing such a thing.
> 
> Certainly possible, although I think conceptually this is much more a core
> service worker functionality than either of those others (and even more a core
> service worker functionality than for example fetch, which is defined in this
> file).
> (see also my other reply to your other comment)
> 
> But if you disagree I'll happily move this code somewhere else (maybe I should
> also find some of the SW people here at BlinkOn to discuss some of this in
> person...)

Was taking a look around spec/github/models, it looks this one's actually
orthogonal to n.c. It's not really a part of SW either, but given that this
one's somewhat independent (meaning that it should be relatively easy to
refactor later when necessary) I'm ok having this here for now.

Can you add a link to the github doc, and also have a TODO comment to update
this when spec status becomes clearer and we find the right home for this?

(falken@ and other SW owners: please feel free to chime in)

[nhiroki, 2015-05-13 07:12:50]

Thank you for your replies. I understand the ground of this change and I'm OK to
keep these components as is.

LGTM after kinuko@'s comment is addressed.

https://codereview.chromium.org/1008533005/diff/180001/Source/modules/service...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.idl (right):

https://codereview.chromium.org/1008533005/diff/180001/Source/modules/service...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.idl:42:
[RuntimeEnabled=NavigatorConnect] readonly attribute StashedPortCollection
ports;
On 2015/05/13 04:30:44, kinuko wrote:
> On 2015/05/13 01:46:53, Marijn Kruisselbrink wrote:
> > On 2015/05/13 01:09:37, nhiroki wrote:
> > > For improving modularity, can we move this to
> > > ServiceWorkerGlobalScopeNavigatorConnect.idl and make
> > > ServiceWorkerGlobalScopeNavigatorConnect.{cpp,h}?
> > > 
> > > Seemingly other partial interfaces (eg. NavigatorStorageQuota.idl,
> > > WorkerGlobalScopeIndexedDatabase.idl) are doing such a thing.
> > 
> > Certainly possible, although I think conceptually this is much more a core
> > service worker functionality than either of those others (and even more a
core
> > service worker functionality than for example fetch, which is defined in
this
> > file).
> > (see also my other reply to your other comment)

I see... at first, I thought this should be under navigatorconnect/ based on the
place of the definition, but as you mentioned, this wouldn't directly relate to
the navigatorconnect, too.

> > But if you disagree I'll happily move this code somewhere else (maybe I
should
> > also find some of the SW people here at BlinkOn to discuss some of this in
> > person...)
> 
> Was taking a look around spec/github/models, it looks this one's actually
> orthogonal to n.c. It's not really a part of SW either, but given that this
> one's somewhat independent (meaning that it should be relatively easy to
> refactor later when necessary) I'm ok having this here for now.
> 
> Can you add a link to the github doc, and also have a TODO comment to update
> this when spec status becomes clearer and we find the right home for this?
> 
> (falken@ and other SW owners: please feel free to chime in)

Sounds reasonable.

[Marijn Kruisselbrink, 2015-05-15 04:55:40]

On 2015/05/13 07:12:50, nhiroki wrote:
> On 2015/05/13 04:30:44, kinuko wrote:
> > On 2015/05/13 01:46:53, Marijn Kruisselbrink wrote:
> > > But if you disagree I'll happily move this code somewhere else (maybe I
> should
> > > also find some of the SW people here at BlinkOn to discuss some of this in
> > > person...)
> > 
> > Was taking a look around spec/github/models, it looks this one's actually
> > orthogonal to n.c. It's not really a part of SW either, but given that this
> > one's somewhat independent (meaning that it should be relatively easy to
> > refactor later when necessary) I'm ok having this here for now.
> > 
> > Can you add a link to the github doc, and also have a TODO comment to update
> > this when spec status becomes clearer and we find the right home for this?
> > 
> > (falken@ and other SW owners: please feel free to chime in)
> 
> Sounds reasonable.

Done

[Marijn Kruisselbrink, 2015-05-15 04:59:11]

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

[Marijn Kruisselbrink, 2015-05-15 04:59:12]

+jochen for Source/core, Source/modules and public/web OWNERS

[jochen (gone - plz use gerrit), 2015-05-15 15:25:03]

some initial comments

https://codereview.chromium.org/1008533005/diff/220001/Source/modules/service...
File Source/modules/serviceworkers/StashedMessagePort.h (right):

https://codereview.chromium.org/1008533005/diff/220001/Source/modules/service...
Source/modules/serviceworkers/StashedMessagePort.h:15: using
StashedMessagePortArray =
WillBeHeapVector<RefPtrWillBeMember<StashedMessagePort>, 1>;
aren't modules already on oilpan, so you don't need WillBe types anymore?

https://codereview.chromium.org/1008533005/diff/220001/Source/modules/service...
Source/modules/serviceworkers/StashedMessagePort.h:20: : public MessagePort {
nit. on previous line

https://codereview.chromium.org/1008533005/diff/220001/Source/modules/service...
Source/modules/serviceworkers/StashedMessagePort.h:21: DEFINE_WRAPPERTYPEINFO();
WTF_MAKE_NONCOPYABLE?

https://codereview.chromium.org/1008533005/diff/220001/Source/modules/service...
Source/modules/serviceworkers/StashedMessagePort.h:37: explicit
StashedMessagePort(ExecutionContext&, PassOwnPtr<WebMessagePortChannel>, const
String& name);
no explicit, should also come before other private methods

https://codereview.chromium.org/1008533005/diff/220001/Source/modules/service...
File Source/modules/serviceworkers/StashedPortCollection.h (right):

https://codereview.chromium.org/1008533005/diff/220001/Source/modules/service...
Source/modules/serviceworkers/StashedPortCollection.h:24:
WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(StashedPortCollection);
also non-copyable

[Marijn Kruisselbrink, 2015-05-18 18:29:35]

https://codereview.chromium.org/1008533005/diff/220001/Source/modules/service...
File Source/modules/serviceworkers/StashedMessagePort.h (right):

https://codereview.chromium.org/1008533005/diff/220001/Source/modules/service...
Source/modules/serviceworkers/StashedMessagePort.h:15: using
StashedMessagePortArray =
WillBeHeapVector<RefPtrWillBeMember<StashedMessagePort>, 1>;
On 2015/05/15 at 15:25:03, jochen (traveling) wrote:
> aren't modules already on oilpan, so you don't need WillBe types anymore?

But StashedMessagePort inherits from MessagePort, which is still refcounted. So
unfortunately still need WillBe types.

https://codereview.chromium.org/1008533005/diff/220001/Source/modules/service...
Source/modules/serviceworkers/StashedMessagePort.h:20: : public MessagePort {
On 2015/05/15 at 15:25:03, jochen (traveling) wrote:
> nit. on previous line

Done

https://codereview.chromium.org/1008533005/diff/220001/Source/modules/service...
Source/modules/serviceworkers/StashedMessagePort.h:21: DEFINE_WRAPPERTYPEINFO();
On 2015/05/15 at 15:25:03, jochen (traveling) wrote:
> WTF_MAKE_NONCOPYABLE?

Done

https://codereview.chromium.org/1008533005/diff/220001/Source/modules/service...
Source/modules/serviceworkers/StashedMessagePort.h:37: explicit
StashedMessagePort(ExecutionContext&, PassOwnPtr<WebMessagePortChannel>, const
String& name);
On 2015/05/15 at 15:25:03, jochen (traveling) wrote:
> no explicit, should also come before other private methods

Done

https://codereview.chromium.org/1008533005/diff/220001/Source/modules/service...
File Source/modules/serviceworkers/StashedPortCollection.h (right):

https://codereview.chromium.org/1008533005/diff/220001/Source/modules/service...
Source/modules/serviceworkers/StashedPortCollection.h:24:
WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(StashedPortCollection);
On 2015/05/15 at 15:25:03, jochen (traveling) wrote:
> also non-copyable

Done

[jochen (gone - plz use gerrit), 2015-05-19 11:10:59]

hum, so basically, messages over a message port can randomly disappear then,
right?

eg. when a message is already in the message loop of a renderer which is then
killed? and when it's later brought back up, other messages get delivered again?

[Marijn Kruisselbrink, 2015-05-19 16:36:02]

On 2015/05/19 at 11:10:59, jochen wrote:
> hum, so basically, messages over a message port can randomly disappear then,
right?
> 
> eg. when a message is already in the message loop of a renderer which is then
killed? and when it's later brought back up, other messages get delivered again?

Yes, it's true that in the current implementation there are situations where it
is possible for messages send to stashed ports to disappear. That is almost
entirely in the content side of this patch though. The content side message port
implementation holds a queue of messages to be delivered in a particular
renderer, and that in this initial implementation is indeed missing logic for
dealing with situations where renderers get killed while there are still
messages queued. I think I have a TODO somewhere in that patch for that
(hopefully rare) particular case.

[jochen (gone - plz use gerrit), 2015-05-19 16:40:25]

mhm

lgtm

[Marijn Kruisselbrink, 2015-05-19 17:39:51]

The CQ bit was checked by mek@chromium.org

[Marijn Kruisselbrink, 2015-05-19 17:39:52]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
nhiroki@chromium.org
Link to the patchset: https://codereview.chromium.org/1008533005/#ps260001
(title: "rebase")

[commit-bot: I haz the power, 2015-05-19 17:40:18]

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

[commit-bot: I haz the power, 2015-05-19 17:45:19]

Committed patchset #6 (id:260001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195535