Implement BroadcastChannel

Implement BroadcastChannel as specified in https://html.spec.whatwg.org/multipage/comms.html#broadcastchannel

BUG=161070
Committed: https://crrev.com/e69cdae3ae32d3a52e2089c95b34c45ec023a0b9
Cr-Commit-Position: refs/heads/master@{#403331}

[Marijn Kruisselbrink, 2016-05-20 23:15:32]

Patchset #1 (id:1) has been deleted

[Marijn Kruisselbrink, 2016-05-20 23:23:59]

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

[Marijn Kruisselbrink, 2016-05-20 23:24:00]

This is nowhere near ready to be submitted (so please ignore typos, naming,
missing/wrong comments, the fact that my BroadcastChannelService implementation
leaks memory, lack of tests, etc).

At this point I'm primarily trying to figure out if the way I'm using mojo in
this CL makes sense, and in the context of content modularization what the best
way is to expose a (per storage partition) broadcast channel service in the
browser process.

The mojo side here is somewhat complicated by the need for messages send across
all BroadcastChannels associated with a particular event loop to stay ordered
relative to eachother. Hence all the associated interfaces.

As far as code organization is concerned, having a "webmessaging" component or
service or something seems like it would make sense, also as the home for a
future componentized version of the existing MessagePortService. But not sure if
this should actually be a component  (and either way I should probably move the
mojom files in a separate subdirectory in that componenet).

[Ken Rockot(use gerrit already), 2016-05-21 00:01:18]

It makes sense as a component imho, primarily because it doesn't make sense as
its own standalone service process. In the short term we'll probably embed
components like this in content/browser so this all seems reasonable. In the
long term, I'd expect this to be embedded in one or more //services, but it
would still be implemented in //components.

Unfortunately I'm not sure I have a great answer regarding
component+browser+storage context, just some rambling ideas. In the limit I
think we want to isolate storage context stuff within the mojo:user app (see
//services/user), which mojo local storage is using today.

In that world we'd expose a StoragePartition-like mojo interface from that app,
and you'd access it by connecting to "mojo:user" and requesting said interface.

Note that BrowserContext::GetMojoConnectorFor today exposes a shell::Connector
which allows you to connect to apps in a way that is (opaquely) associated with
that BrowserContext. We in turn "run" an instance of "mojo:user" per
BrowserContext that anyone could connect to (registered here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...
 consumed here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/do...).

It might make sense to build a rudimentary StorageParition interface on the user
service and expose an embedder/delegate interface that content could hook to
implement it (by routing requests to a specific StoragePartitionImpl instance)
as an embedder. Ultimately this code would move out of content and into the
service and there'd be no such thing as a "user service embedder." But this
gives us a migration path we can iterate on.

https://codereview.chromium.org/2004643002/diff/20001/components/webmessaging...
File components/webmessaging/broadcast_channel.mojom (right):

https://codereview.chromium.org/2004643002/diff/20001/components/webmessaging...
components/webmessaging/broadcast_channel.mojom:20: ConnectToChannel(string
origin, string name, associated BroadcastChannelClient receiver, associated
BroadcastChannelClient& sender);
It seems like we could simplify this by avoiding associated interfaces:

  interface BroadcastClient {
    OnMessage(string message)
  };

  interface BroadcastService {
    Subscribe(string origin, string channel_name, BroadcastClient client);
    Broadcast(string origin, string channel_name, string message);
  };

Assuming any given event loop only needs one BroadcastService connection and one
BroadcastClient binding, this should accomplish the same thing?

[Marijn Kruisselbrink, 2016-05-21 00:34:21]

On 2016/05/21 at 00:01:18, rockot wrote:
> It makes sense as a component imho, primarily because it doesn't make sense as
its own standalone service process. In the short term we'll probably embed
components like this in content/browser so this all seems reasonable. In the
long term, I'd expect this to be embedded in one or more //services, but it
would still be implemented in //components.

Okay. So in the short term //compoments/webmessaging will contain things (I'd
say services, but that already has a different meaning) to support broadcast
channel and message port, with //content/browser directly including those files
in the right place (at least message ports don't need any kind of storage
partition or even browser context specific state, so that one should be even
simpler, other than the integration with all the other postMessage
methods/IPCs).

Would the mojom files for these then be in //components/webmessaging itself, or
in //components/webmessaging/public/interfaces (or something else)? It isn't
entirely clear to me what the "normal" way is to deal with this. And at least
content will consume more than just the "public" interface...

> Unfortunately I'm not sure I have a great answer regarding
component+browser+storage context, just some rambling ideas. In the limit I
think we want to isolate storage context stuff within the mojo:user app (see
//services/user), which mojo local storage is using today.
> 
> In that world we'd expose a StoragePartition-like mojo interface from that
app, and you'd access it by connecting to "mojo:user" and requesting said
interface.
> 
> Note that BrowserContext::GetMojoConnectorFor today exposes a shell::Connector
which allows you to connect to apps in a way that is (opaquely) associated with
that BrowserContext. We in turn "run" an instance of "mojo:user" per
BrowserContext that anyone could connect to (registered here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...
 consumed here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/do...).
> 
> It might make sense to build a rudimentary StorageParition interface on the
user service and expose an embedder/delegate interface that content could hook
to implement it (by routing requests to a specific StoragePartitionImpl
instance) as an embedder. Ultimately this code would move out of content and
into the service and there'd be no such thing as a "user service embedder." But
this gives us a migration path we can iterate on.
> 

Hah, that is mostly going way over my head. I don't quite understand yet what
apps, shells, services, etc are and how they would be implemented on the one
side, and used from blink on the other side. But yeah, if a "mojo:user" thing
represents a BrowserContext it does seem like it would make sense to have
something in that to then represent a StoragePartition (and presumably then some
way for blink to connect to a service where that service can be implemented
either in the storage partition, the browser context, or globally? Or would
blink code have to explicitly specify which one of those it is? Either might
make sense, since it is unlikely that blink wouldn't know at what layer a
particular service exists.

https://codereview.chromium.org/2004643002/diff/20001/components/webmessaging...
File components/webmessaging/broadcast_channel.mojom (right):

https://codereview.chromium.org/2004643002/diff/20001/components/webmessaging...
components/webmessaging/broadcast_channel.mojom:20: ConnectToChannel(string
origin, string name, associated BroadcastChannelClient receiver, associated
BroadcastChannelClient& sender);
On 2016/05/21 at 00:01:18, Ken Rockot wrote:
> It seems like we could simplify this by avoiding associated interfaces:
> 
>   interface BroadcastClient {
>     OnMessage(string message)
>   };
> 
>   interface BroadcastService {
>     Subscribe(string origin, string channel_name, BroadcastClient client);
>     Broadcast(string origin, string channel_name, string message);
>   };
> 
> Assuming any given event loop only needs one BroadcastService connection and
one BroadcastClient binding, this should accomplish the same thing?

It would be possible to have a mojo API that is more like that, yes. It would
mean I would have to write extra code on the blink side to do all the
multiplexing to get messages to from the correct BroadcastChannel instances. And
with that exact API the client passed to Subscribe would still need to be an
associated interface to get the ordering the spec currently specifies (although
maybe it would be okay to loosen the spec to not use the same "task source" for
different channels, thus allowing the reordering that can happen by having
multiple BroadcastClient pipes).

One big benefit to doing my own multiplexing in blink would be that local
messages can be delivered without an IPC round-trip in between. But I'd
essentially be duplicating all the broadcast channel service code from the
browser process into the renderer process (keeping track of all the active
channel connections, routing the correct messages to the correct channel
instances, etc). I did consider that, but this approach ultimately seemed
simpler, and I don't think the overhead of IPC for same-thread messages is
really going to be a major issue that's going to effect how this API can be
used.

[Marijn Kruisselbrink, 2016-05-21 00:36:15]

https://codereview.chromium.org/2004643002/diff/20001/components/webmessaging...
File components/webmessaging/broadcast_channel.mojom (right):

https://codereview.chromium.org/2004643002/diff/20001/components/webmessaging...
components/webmessaging/broadcast_channel.mojom:20: ConnectToChannel(string
origin, string name, associated BroadcastChannelClient receiver, associated
BroadcastChannelClient& sender);
On 2016/05/21 at 00:34:21, Marijn Kruisselbrink wrote:
> On 2016/05/21 at 00:01:18, Ken Rockot wrote:
> > It seems like we could simplify this by avoiding associated interfaces:
> > 
> >   interface BroadcastClient {
> >     OnMessage(string message)
> >   };
> > 
> >   interface BroadcastService {
> >     Subscribe(string origin, string channel_name, BroadcastClient client);
> >     Broadcast(string origin, string channel_name, string message);
> >   };
> > 
> > Assuming any given event loop only needs one BroadcastService connection and
one BroadcastClient binding, this should accomplish the same thing?
> 
> It would be possible to have a mojo API that is more like that, yes. It would
mean I would have to write extra code on the blink side to do all the
multiplexing to get messages to from the correct BroadcastChannel instances. And
with that exact API the client passed to Subscribe would still need to be an
associated interface to get the ordering the spec currently specifies (although
maybe it would be okay to loosen the spec to not use the same "task source" for
different channels, thus allowing the reordering that can happen by having
multiple BroadcastClient pipes).
> 
> One big benefit to doing my own multiplexing in blink would be that local
messages can be delivered without an IPC round-trip in between. But I'd
essentially be duplicating all the broadcast channel service code from the
browser process into the renderer process (keeping track of all the active
channel connections, routing the correct messages to the correct channel
instances, etc). I did consider that, but this approach ultimately seemed
simpler, and I don't think the overhead of IPC for same-thread messages is
really going to be a major issue that's going to effect how this API can be
used.

And maybe I should just implement it, to see just how ugly/non-ugly it might
get....

[Ken Rockot(use gerrit already), 2016-05-25 16:12:19]

On 2016/05/21 at 00:34:21, mek wrote:
> On 2016/05/21 at 00:01:18, rockot wrote:
> > It makes sense as a component imho, primarily because it doesn't make sense
as its own standalone service process. In the short term we'll probably embed
components like this in content/browser so this all seems reasonable. In the
long term, I'd expect this to be embedded in one or more //services, but it
would still be implemented in //components.
> 
> Okay. So in the short term //compoments/webmessaging will contain things (I'd
say services, but that already has a different meaning) to support broadcast
channel and message port, with //content/browser directly including those files
in the right place (at least message ports don't need any kind of storage
partition or even browser context specific state, so that one should be even
simpler, other than the integration with all the other postMessage
methods/IPCs).
> 
> Would the mojom files for these then be in //components/webmessaging itself,
or in //components/webmessaging/public/interfaces (or something else)? It isn't
entirely clear to me what the "normal" way is to deal with this. And at least
content will consume more than just the "public" interface...

I think //components/webmessaging/public/interfaces. As a general rule, if a
component exposes mojom outside if itself, the mojom should be in
public/interfaces. There are examples where this isn't done today, and I think
we should fix them.

> 
> > Unfortunately I'm not sure I have a great answer regarding
component+browser+storage context, just some rambling ideas. In the limit I
think we want to isolate storage context stuff within the mojo:user app (see
//services/user), which mojo local storage is using today.
> > 
> > In that world we'd expose a StoragePartition-like mojo interface from that
app, and you'd access it by connecting to "mojo:user" and requesting said
interface.
> > 
> > Note that BrowserContext::GetMojoConnectorFor today exposes a
shell::Connector which allows you to connect to apps in a way that is (opaquely)
associated with that BrowserContext. We in turn "run" an instance of "mojo:user"
per BrowserContext that anyone could connect to (registered here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...
 consumed here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/do...).
> > 
> > It might make sense to build a rudimentary StorageParition interface on the
user service and expose an embedder/delegate interface that content could hook
to implement it (by routing requests to a specific StoragePartitionImpl
instance) as an embedder. Ultimately this code would move out of content and
into the service and there'd be no such thing as a "user service embedder." But
this gives us a migration path we can iterate on.
> > 
> 
> Hah, that is mostly going way over my head. I don't quite understand yet what
apps, shells, services, etc are and how they would be implemented on the one
side, and used from blink on the other side. But yeah, if a "mojo:user" thing
represents a BrowserContext it does seem like it would make sense to have
something in that to then represent a StoragePartition (and presumably then some
way for blink to connect to a service where that service can be implemented
either in the storage partition, the browser context, or globally? Or would
blink code have to explicitly specify which one of those it is? Either might
make sense, since it is unlikely that blink wouldn't know at what layer a
particular service exists.

I think eventually blink will just get a shell::Connector it can use to make app
connections. This would be provided by the embedder and would be established in
such a way that its outgoing requests are associated (authoritatively by the
shell) with a specific user GUID. Apps in turn run with an authoritative GUID
association, and that should be sufficient for mojo:user et al to coordinate
user context across services.

FWIW there's a (slightly dated) rough outline of things here
https://docs.google.com/document/d/1U7mcQ2o25KgXfEuJSIXQUpEF6Y9134ysaqSNUcmR-...
which we should probably flesh out more.

> 
>
https://codereview.chromium.org/2004643002/diff/20001/components/webmessaging...
> File components/webmessaging/broadcast_channel.mojom (right):
> 
>
https://codereview.chromium.org/2004643002/diff/20001/components/webmessaging...
> components/webmessaging/broadcast_channel.mojom:20: ConnectToChannel(string
origin, string name, associated BroadcastChannelClient receiver, associated
BroadcastChannelClient& sender);
> On 2016/05/21 at 00:01:18, Ken Rockot wrote:
> > It seems like we could simplify this by avoiding associated interfaces:
> > 
> >   interface BroadcastClient {
> >     OnMessage(string message)
> >   };
> > 
> >   interface BroadcastService {
> >     Subscribe(string origin, string channel_name, BroadcastClient client);
> >     Broadcast(string origin, string channel_name, string message);
> >   };
> > 
> > Assuming any given event loop only needs one BroadcastService connection and
one BroadcastClient binding, this should accomplish the same thing?
> 
> It would be possible to have a mojo API that is more like that, yes. It would
mean I would have to write extra code on the blink side to do all the
multiplexing to get messages to from the correct BroadcastChannel instances. And
with that exact API the client passed to Subscribe would still need to be an
associated interface to get the ordering the spec currently specifies (although
maybe it would be okay to loosen the spec to not use the same "task source" for
different channels, thus allowing the reordering that can happen by having
multiple BroadcastClient pipes).
> 
> One big benefit to doing my own multiplexing in blink would be that local
messages can be delivered without an IPC round-trip in between. But I'd
essentially be duplicating all the broadcast channel service code from the
browser process into the renderer process (keeping track of all the active
channel connections, routing the correct messages to the correct channel
instances, etc). I did consider that, but this approach ultimately seemed
simpler, and I don't think the overhead of IPC for same-thread messages is
really going to be a major issue that's going to effect how this API can be
used.

[Marijn Kruisselbrink, 2016-05-31 23:40:45]

Description was changed from

==========
WIP Implement BroadcastChannel

BUG=161070
==========

to

==========
Implement BroadcastChannel as specified in
https://html.spec.whatwg.org/multipage/comms.html#broadcastchannel

BUG=161070
==========

[Marijn Kruisselbrink, 2016-05-31 23:40:54]

https://codereview.chromium.org/2004643002/diff/20001/components/webmessaging...
File components/webmessaging/broadcast_channel.mojom (right):

https://codereview.chromium.org/2004643002/diff/20001/components/webmessaging...
components/webmessaging/broadcast_channel.mojom:20: ConnectToChannel(string
origin, string name, associated BroadcastChannelClient receiver, associated
BroadcastChannelClient& sender);
On 2016/05/21 at 00:36:15, Marijn Kruisselbrink wrote:
> On 2016/05/21 at 00:34:21, Marijn Kruisselbrink wrote:
> > On 2016/05/21 at 00:01:18, Ken Rockot wrote:
> > > It seems like we could simplify this by avoiding associated interfaces:
> > > 
> > >   interface BroadcastClient {
> > >     OnMessage(string message)
> > >   };
> > > 
> > >   interface BroadcastService {
> > >     Subscribe(string origin, string channel_name, BroadcastClient client);
> > >     Broadcast(string origin, string channel_name, string message);
> > >   };
> > > 
> > > Assuming any given event loop only needs one BroadcastService connection
and one BroadcastClient binding, this should accomplish the same thing?
> > 
> > It would be possible to have a mojo API that is more like that, yes. It
would mean I would have to write extra code on the blink side to do all the
multiplexing to get messages to from the correct BroadcastChannel instances. And
with that exact API the client passed to Subscribe would still need to be an
associated interface to get the ordering the spec currently specifies (although
maybe it would be okay to loosen the spec to not use the same "task source" for
different channels, thus allowing the reordering that can happen by having
multiple BroadcastClient pipes).
> > 
> > One big benefit to doing my own multiplexing in blink would be that local
messages can be delivered without an IPC round-trip in between. But I'd
essentially be duplicating all the broadcast channel service code from the
browser process into the renderer process (keeping track of all the active
channel connections, routing the correct messages to the correct channel
instances, etc). I did consider that, but this approach ultimately seemed
simpler, and I don't think the overhead of IPC for same-thread messages is
really going to be a major issue that's going to effect how this API can be
used.
> 
> And maybe I should just implement it, to see just how ugly/non-ugly it might
get....

Okay, I ended up implementing something with a mojom interface much closer to
what you suggested. It certainly resulted in quite a bit more code/logic in the
renderer, but I think the benefits might actually be worth it. And (in the
fourth or so iteration I ended up with) it's not as ugly as I feared it might
get.

[Marijn Kruisselbrink, 2016-05-31 23:57:09]

https://codereview.chromium.org/2004643002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp
(right):

https://codereview.chromium.org/2004643002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp:93:
m_binding.set_connection_error_handler(createBaseCallback(bind(&BroadcastChannelConnection::onError,
this)));
Oh, one more mojo specific question: since my error callback is pretty much just
"delete this", would it make sense to have something like a
mojo::StrongAssociatedBinding that would just do that for me? Since regular
interfaces already have Binding and StrongBinding it might make sense to have
both for associated interfaces as well.

[Ken Rockot(use gerrit already), 2016-06-01 15:41:38]

On 2016/05/31 at 23:57:09, mek wrote:
>
https://codereview.chromium.org/2004643002/diff/140001/third_party/WebKit/Sou...
> File
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp
(right):
> 
>
https://codereview.chromium.org/2004643002/diff/140001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp:93:
m_binding.set_connection_error_handler(createBaseCallback(bind(&BroadcastChannelConnection::onError,
this)));
> Oh, one more mojo specific question: since my error callback is pretty much
just "delete this", would it make sense to have something like a
mojo::StrongAssociatedBinding that would just do that for me? Since regular
interfaces already have Binding and StrongBinding it might make sense to have
both for associated interfaces as well.

That seems like a reasonable suggestion to me.

[Ken Rockot(use gerrit already), 2016-06-01 16:07:50]

https://codereview.chromium.org/2004643002/diff/140001/components/webmessagin...
File components/webmessaging/public/interfaces/broadcast_channel.mojom (right):

https://codereview.chromium.org/2004643002/diff/140001/components/webmessagin...
components/webmessaging/public/interfaces/broadcast_channel.mojom:21:
Subscribe(url.mojom.Origin origin, string name, associated
BroadcastChannelClient client);
Sorry, I think I may have lost track of exactly what you want to accomplish by
using an associated interface here. The only possible reason I can come with is
that in the following scenario:

  s.Subscribe(a, b, client1)
  s.Subscribe(a, b, client2)
  s.Subscribe(a, b, client3)
  s.Broadcast(a, b, foo)
  s.Broadcast(a, b, bar)

you want to guarantee that no client dispatches OnMessage(bar) until all three
have dispatched OnMessage(foo). Is that correct?

[Marijn Kruisselbrink, 2016-06-01 21:37:12]

Okay, added (and used) a new mojo::StrongAssociatedBinding class.

https://codereview.chromium.org/2004643002/diff/140001/components/webmessagin...
File components/webmessaging/public/interfaces/broadcast_channel.mojom (right):

https://codereview.chromium.org/2004643002/diff/140001/components/webmessagin...
components/webmessaging/public/interfaces/broadcast_channel.mojom:21:
Subscribe(url.mojom.Origin origin, string name, associated
BroadcastChannelClient client);
On 2016/06/01 at 16:07:50, Ken Rockot wrote:
> Sorry, I think I may have lost track of exactly what you want to accomplish by
using an associated interface here. The only possible reason I can come with is
that in the following scenario:
> 
>   s.Subscribe(a, b, client1)
>   s.Subscribe(a, b, client2)
>   s.Subscribe(a, b, client3)
>   s.Broadcast(a, b, foo)
>   s.Broadcast(a, b, bar)
> 
> you want to guarantee that no client dispatches OnMessage(bar) until all three
have dispatched OnMessage(foo). Is that correct?

No, that's not the case I'm concerned about. What I'm trying to guarantee is
that in the following scenario:

s1.Subscribe(a, b, client1)
s1.Subscribe(a, c, client2)

s2.Broadcast(a, b, foo)
s2.Broadcast(a, c, bar)

I want to guarantee that client1.OnMessage(foo) is always called before
client2.OnMessage(bar). In the spec all the BroadcastChannel instances in the
same context use the same single task source for the tasks that dispatch their
events, which pretty much means messages have to remain ordered like this.

As a side note, each MessagePort instance has its own task source in the spec,
so it seems like for MessagePort it should at least be spec compliant to not
give any of these guarantees. If such a change is going to web compatible is of
course a different issue, as currently I don't think any implementation actually
makes use of the fact that each port has its own task source.

[Marijn Kruisselbrink, 2016-06-01 21:37:34]

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

[Marijn Kruisselbrink, 2016-06-01 21:37:35]

+kinuko: would you mind reviewing this?

[Ken Rockot(use gerrit already), 2016-06-01 21:47:08]

https://codereview.chromium.org/2004643002/diff/140001/components/webmessagin...
File components/webmessaging/public/interfaces/broadcast_channel.mojom (right):

https://codereview.chromium.org/2004643002/diff/140001/components/webmessagin...
components/webmessaging/public/interfaces/broadcast_channel.mojom:21:
Subscribe(url.mojom.Origin origin, string name, associated
BroadcastChannelClient client);
On 2016/06/01 at 21:37:12, Marijn Kruisselbrink wrote:
> On 2016/06/01 at 16:07:50, Ken Rockot wrote:
> > Sorry, I think I may have lost track of exactly what you want to accomplish
by using an associated interface here. The only possible reason I can come with
is that in the following scenario:
> > 
> >   s.Subscribe(a, b, client1)
> >   s.Subscribe(a, b, client2)
> >   s.Subscribe(a, b, client3)
> >   s.Broadcast(a, b, foo)
> >   s.Broadcast(a, b, bar)
> > 
> > you want to guarantee that no client dispatches OnMessage(bar) until all
three have dispatched OnMessage(foo). Is that correct?
> 
> No, that's not the case I'm concerned about. What I'm trying to guarantee is
that in the following scenario:
> 
> s1.Subscribe(a, b, client1)
> s1.Subscribe(a, c, client2)
> 
> s2.Broadcast(a, b, foo)
> s2.Broadcast(a, c, bar)
> 
> I want to guarantee that client1.OnMessage(foo) is always called before
client2.OnMessage(bar). In the spec all the BroadcastChannel instances in the
same context use the same single task source for the tasks that dispatch their
events, which pretty much means messages have to remain ordered like this.
> 
> As a side note, each MessagePort instance has its own task source in the spec,
so it seems like for MessagePort it should at least be spec compliant to not
give any of these guarantees. If such a change is going to web compatible is of
course a different issue, as currently I don't think any implementation actually
makes use of the fact that each port has its own task source.

Oh, right. I guess it's really the same thing, but my example was overly
specific. That makes sense then.

So what about a case like:

 s1.Subscribe(a, b, client1)
 s1.Subscribe(a, c, client2)

 s2.Broadcast(a, b, foo)

 s3.Broadcast(a, c, bar)

Is it OK that client1.OnMessage(foo) can happen before client2.OnMessage(bar) in
this case?

[Marijn Kruisselbrink, 2016-06-01 22:07:59]

https://codereview.chromium.org/2004643002/diff/140001/components/webmessagin...
File components/webmessaging/public/interfaces/broadcast_channel.mojom (right):

https://codereview.chromium.org/2004643002/diff/140001/components/webmessagin...
components/webmessaging/public/interfaces/broadcast_channel.mojom:21:
Subscribe(url.mojom.Origin origin, string name, associated
BroadcastChannelClient client);
On 2016/06/01 at 21:47:08, Ken Rockot wrote:

> So what about a case like:
> 
>  s1.Subscribe(a, b, client1)
>  s1.Subscribe(a, c, client2)
> 
>  s2.Broadcast(a, b, foo)
> 
>  s3.Broadcast(a, c, bar)
> 
> Is it OK that client1.OnMessage(foo) can happen before client2.OnMessage(bar)
in this case?

Since (with the implementation that only one BroadcastChannelService connection
is created for any thread) in that case s2 and s3 are guaranteed to be on
different threads (or processes), it is indeed okay that those message can get
delivered in either order. I haven't entirely convinced myself that there aren't
any possible scenarios where ordering/behavior would observably differ from what
the spec defines though. In particular the behavior around closing connections
and what messages "should" be delivered isn't quite possible to fully implement
(or at least very complicated). And I expect any remaining edge cases to be
minor and unlikely enough to in practice not cause any real interop problems.

[haraken, 2016-06-02 04:23:40]

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

[haraken, 2016-06-02 04:23:42]

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp
(right):

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp:23:
channel->suspendIfNeeded();

If you make BroadcastChannel a ContextLifecycleObserver, you don't need to call
channel->suspendIfNeeded().

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.h
(right):

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.h:19: ,
public ActiveDOMObject

ActiveDOMObject => ContextLifecycleObserver

If you don't need to observe suspend/resume, ContextLifecycleObserver would be
enough.

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp
(right):

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp:39:
DEFINE_THREAD_SAFE_STATIC_LOCAL(ThreadSpecific<ConnectionMap>, connectionMap,
new ThreadSpecific<ConnectionMap>);

It's a bit nasty to use thread-local storage for this. Isn't there any object
that should own the connection map?

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp:76:
delete this;

Can we use a unique_ptr to avoid calling manual delete? Blink doesn't prefer
using manual delete.

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.h
(right):

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.h:23:
class Client {

We should make this a GarbageCollectedMixin. Currently
BroadcastChannelConnection::m_clients holds raw pointers to on-heap objects,
which is potentially unsafe.

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.h:64:
ListHashSet<Client*> m_clients;

Does it need to be a ListHashSet? I guess HashSet would be enough. (Note: The
implementation of ListHashSet is complicated and heavy.)

I think you should use HeapHashSet<WeakMember>. Then you don't need to call
unregister() because the unregistration is automatically done by weak
processing.

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/broadcastchannel/DEPS (right):

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/DEPS:2:
"+components/webmessaging/public",

We're discussing this in another review
(https://codereview.chromium.org/2027023002/), but it looks better to add only
*mojom-blink.h to DEPS.

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/HashTraits.h (right):

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/HashTraits.h:248: static const bool
hasIsEmptyValueFunction = FirstTraits::hasIsEmptyValueFunction ||
SecondTraits::hasIsEmptyValueFunction;

Would you elaborate why you need this change? Is this an existing bug of
WTF::HashMap?

[Marijn Kruisselbrink, 2016-06-02 18:33:30]

Thanks for all the comments. I tried to explain a bit more how I ended up with
the current ownership/memory management model, and why relying more on garbage
collection for various parts of this is rather non-trivial. Regardless of what
design I end up with I should probably add more detailed comments about
ownership and what references what to BroadcastChannelConnection.h, because
regardless of what design this ends up having it's likely to still be very
non-obvious why things are done the way they are done...

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp
(right):

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp:23:
channel->suspendIfNeeded();
On 2016/06/02 at 04:23:41, haraken wrote:
> If you make BroadcastChannel a ContextLifecycleObserver, you don't need to
call channel->suspendIfNeeded().

Done

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.h
(right):

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.h:19: ,
public ActiveDOMObject
On 2016/06/02 at 04:23:41, haraken wrote:
> ActiveDOMObject => ContextLifecycleObserver
> 
> If you don't need to observe suspend/resume, ContextLifecycleObserver would be
enough.

Done

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp
(right):

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp:39:
DEFINE_THREAD_SAFE_STATIC_LOCAL(ThreadSpecific<ConnectionMap>, connectionMap,
new ThreadSpecific<ConnectionMap>);
On 2016/06/02 at 04:23:41, haraken wrote:
> It's a bit nasty to use thread-local storage for this. Isn't there any object
that should own the connection map?

All BroadcastChannel instances that share thread, name and origin need to
somehow coordinate with eachother before hitting IPC. In an earlier patch (up
till PS4 I believe) I did that by having a more complicated mojo API (having
multiple layers of associated interfaces to ensure all messages related to all
broadcast channels from a particular thread are send and received in the correct
order), which also had the downside that go to/from the same thread still had to
go over IPC multiple times.

So the slightly more complicated design of explicitly keeping track of all
BroadcastChannel instances on the renderer side (on a per thread, per
origin/name basis). Rather than per-thread this could be per-process, but that
would make the code even more complicated.

So while the connections themselves could be jointly "owned" by the channel
instances that use the connection, new channel instances will still need a way
to find a potentially existing connection to reuse.

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp:76:
delete this;
On 2016/06/02 at 04:23:41, haraken wrote:
> Can we use a unique_ptr to avoid calling manual delete? Blink doesn't prefer
using manual delete.

The lifetime of this class is mostly managed by the
mojo::StrongAssociatedBinding (which tied the lifetime of an implementation of a
mojo interface to the lifetime of the underlying mojo connection). But
additionally we also want to close the connection explicitly when all clients
for a particular connection go away (to not leak memory/connections). Hence the
delete this.

I could change this ownership by making the ConnectionMap store unique_ptr<>. I
would then have to go back to using mojo::AssociatedBinding (which doesn't
manage lifetime), and add my own connection error handler that removes the
conenction from the map (thereby destroying the connection instance). And the
delete this here would then also change to remove the connection from the map.

A downside of that would be that at least with "delete this" it is obvious that
this is being deleted. If the code here just removes this from a map it isn't
clear at all that the result of that is |this| being invalid afterwards. Also
it's more code since I'll need my own mojo error handlers. So it's unclear to me
what the benefit would be.

What maybe might work is to make BroadcastChannelConnection garbage collected
(or refcounted), and have the BroadcastChannel instances that together want to
keep it alive make sure of that by having a (strong) reference to the
connection. As mentioned in one of my other comments I would still need to
somehow have an ordered list of weak references to the BroadcastChannel
instances from the Connection though. And on top of that if
BroadcastChannelConnection would be garbage collected, the ConnectionMap becomes
another problem of wanting to store weak references to garbage collected objects
in a container that doesn't currently support weak references (as new
BroadcastChannel instances would still need to be able to somehow find the
correct Connection to use, thus the need for some kind of (origin, name) =>
connection map).

So it seems that doing that would maybe make some things work more
automagically, but would also require more hacks in other places. So with that
in mind it seemed much preferable to have simpler code that is easier to reason
about (since all the memory management logic is fairly self contained), rather
than having code that still requires some manual bookkeeping but is much harder
to reason about since now the bookkeeping interacts with garbage collection in
interesting ways...

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.h
(right):

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.h:23:
class Client {
On 2016/06/02 at 04:23:41, haraken wrote:
> We should make this a GarbageCollectedMixin. Currently
BroadcastChannelConnection::m_clients holds raw pointers to on-heap objects,
which is potentially unsafe.

Unfortunately I couldn't figure out a way to do what I needed to do without
holding raw pointers to on-heap objects... So the whole point of having this
Client interface rather than just storing BroadcastChannel references directly
was to get around the compiler complaints about storing pointers to on-heap
objects. See my other answers for more explanation of the what and why.

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.h:64:
ListHashSet<Client*> m_clients;
On 2016/06/02 at 04:23:41, haraken wrote:
> Does it need to be a ListHashSet? I guess HashSet would be enough. (Note: The
implementation of ListHashSet is complicated and heavy.)

The List parts of ListHashSet (specifically the ordered by insertion order part)
is in fact a hard requirement. The spec requires dispatching events to
BroadcastChannel instances (when those instances live on the same event loop) to
be done in creation order of the BroadcastChannel instances. So I need some kind
of container that maintains insertion order to store this list. I don't care
particularly about the HashSet part, since it's unlikely that there will be
enough BroadcastChannel instances for any particular channel in any particular
thread for the performance difference when removing channels to be relevant, but
since it exists I might as well use it.


> I think you should use HeapHashSet<WeakMember>. Then you don't need to call
unregister() because the unregistration is automatically done by weak
processing.

Even with some hypothetical OrderedContainer<WeakMember<...>>, if there was any
kind of container that maintained ordering and would support weak members, I'd
still need some kind of unregister() (or a way to tell the container to call
some method of me when the container empties itself), as I want to be able to
close the mojo connection when no more BroadcastChannel instances remain for
this particular channel.

Maybe that could be made to work differently by changing the way ownership of
BroadcastChannelConnection works (see my replyto your questions about the
connection map stuff), but I'm not sure it would be any clearer, and certainly
non-obvious that it would be correct.

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/broadcastchannel/DEPS (right):

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/DEPS:2:
"+components/webmessaging/public",
On 2016/06/02 at 04:23:41, haraken wrote:
> We're discussing this in another review
(https://codereview.chromium.org/2027023002/), but it looks better to add only
*mojom-blink.h to DEPS.

That makes a lot of sense, done.

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/HashTraits.h (right):

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/HashTraits.h:248: static const bool
hasIsEmptyValueFunction = FirstTraits::hasIsEmptyValueFunction ||
SecondTraits::hasIsEmptyValueFunction;
On 2016/06/02 at 04:23:41, haraken wrote:
> Would you elaborate why you need this change? Is this an existing bug of
WTF::HashMap?

Yes, I'd say this is an existing bug of the std::pair HashTraits. It was
possible to have a HashMap<RefPtr<SecurityOrigin>, T>, and it was possible to
have a HashMap<std::pair<T, U>, V>, but the last one only if both T and U don't
use a custom isEmptyValue function. With this change it becomes possible to use
types with isEmptyValue functions inside std::pair in HashMap and friends as
well.

[haraken, 2016-06-03 05:33:03]

I don't yet understood all of your requirements, but let me post some ideas. I
hope it helps.

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp
(right):

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp:76:
delete this;
On 2016/06/02 18:33:29, Marijn Kruisselbrink wrote:
> On 2016/06/02 at 04:23:41, haraken wrote:
> > Can we use a unique_ptr to avoid calling manual delete? Blink doesn't prefer
> using manual delete.
> 
> The lifetime of this class is mostly managed by the
> mojo::StrongAssociatedBinding (which tied the lifetime of an implementation of
a
> mojo interface to the lifetime of the underlying mojo connection). But
> additionally we also want to close the connection explicitly when all clients
> for a particular connection go away (to not leak memory/connections). Hence
the
> delete this.
> 
> I could change this ownership by making the ConnectionMap store unique_ptr<>.
I
> would then have to go back to using mojo::AssociatedBinding (which doesn't
> manage lifetime), and add my own connection error handler that removes the
> conenction from the map (thereby destroying the connection instance). And the
> delete this here would then also change to remove the connection from the map.
> 
> A downside of that would be that at least with "delete this" it is obvious
that
> this is being deleted. If the code here just removes this from a map it isn't
> clear at all that the result of that is |this| being invalid afterwards. Also
> it's more code since I'll need my own mojo error handlers. So it's unclear to
me
> what the benefit would be.

We normally use a dispose() method to promptly dispose something (before waiting
for a destructor). You can explicitly call dispose() here and let a GC destruct
the object later.

> 
> What maybe might work is to make BroadcastChannelConnection garbage collected
> (or refcounted), and have the BroadcastChannel instances that together want to
> keep it alive make sure of that by having a (strong) reference to the
> connection. As mentioned in one of my other comments I would still need to
> somehow have an ordered list of weak references to the BroadcastChannel
> instances from the Connection though. And on top of that if
> BroadcastChannelConnection would be garbage collected, the ConnectionMap
becomes
> another problem of wanting to store weak references to garbage collected
objects
> in a container that doesn't currently support weak references (as new
> BroadcastChannel instances would still need to be able to somehow find the
> correct Connection to use, thus the need for some kind of (origin, name) =>
> connection map).
> 
> So it seems that doing that would maybe make some things work more
> automagically, but would also require more hacks in other places. So with that
> in mind it seemed much preferable to have simpler code that is easier to
reason
> about (since all the memory management logic is fairly self contained), rather
> than having code that still requires some manual bookkeeping but is much
harder
> to reason about since now the bookkeeping interacts with garbage collection in
> interesting ways...

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.h
(right):

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.h:23:
class Client {
On 2016/06/02 18:33:30, Marijn Kruisselbrink wrote:
> On 2016/06/02 at 04:23:41, haraken wrote:
> > We should make this a GarbageCollectedMixin. Currently
> BroadcastChannelConnection::m_clients holds raw pointers to on-heap objects,
> which is potentially unsafe.
> 
> Unfortunately I couldn't figure out a way to do what I needed to do without
> holding raw pointers to on-heap objects... So the whole point of having this
> Client interface rather than just storing BroadcastChannel references directly
> was to get around the compiler complaints about storing pointers to on-heap
> objects. See my other answers for more explanation of the what and why.

This Client must be GarbageCollectedMixin, because it is inherited by a
GarbageCollected object.

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.h:64:
ListHashSet<Client*> m_clients;
On 2016/06/02 18:33:29, Marijn Kruisselbrink wrote:
> On 2016/06/02 at 04:23:41, haraken wrote:
> > Does it need to be a ListHashSet? I guess HashSet would be enough. (Note:
The
> implementation of ListHashSet is complicated and heavy.)
> 
> The List parts of ListHashSet (specifically the ordered by insertion order
part)
> is in fact a hard requirement. The spec requires dispatching events to
> BroadcastChannel instances (when those instances live on the same event loop)
to
> be done in creation order of the BroadcastChannel instances. So I need some
kind
> of container that maintains insertion order to store this list. I don't care
> particularly about the HashSet part, since it's unlikely that there will be
> enough BroadcastChannel instances for any particular channel in any particular
> thread for the performance difference when removing channels to be relevant,
but
> since it exists I might as well use it.
> 
> 
> > I think you should use HeapHashSet<WeakMember>. Then you don't need to call
> unregister() because the unregistration is automatically done by weak
> processing.
> 
> Even with some hypothetical OrderedContainer<WeakMember<...>>, if there was
any
> kind of container that maintained ordering and would support weak members, I'd
> still need some kind of unregister() (or a way to tell the container to call
> some method of me when the container empties itself), as I want to be able to
> close the mojo connection when no more BroadcastChannel instances remain for
> this particular channel.
> 
> Maybe that could be made to work differently by changing the way ownership of
> BroadcastChannelConnection works (see my replyto your questions about the
> connection map stuff), but I'm not sure it would be any clearer, and certainly
> non-obvious that it would be correct.

You can use HeapListHashSet<WeakMember<>>.

Also you can keep the unregister method. My point is that you must not call the
unregister method in the destructor of Client. A destructor is not allowed to
touch another on-heap object (because the destruction order is undterministic).
That's why you need to use weak members and ask the Oilpan to unregister the
Client from the hash set.

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/HashTraits.h (right):

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/HashTraits.h:248: static const bool
hasIsEmptyValueFunction = FirstTraits::hasIsEmptyValueFunction ||
SecondTraits::hasIsEmptyValueFunction;
On 2016/06/02 18:33:30, Marijn Kruisselbrink wrote:
> On 2016/06/02 at 04:23:41, haraken wrote:
> > Would you elaborate why you need this change? Is this an existing bug of
> WTF::HashMap?
> 
> Yes, I'd say this is an existing bug of the std::pair HashTraits. It was
> possible to have a HashMap<RefPtr<SecurityOrigin>, T>, and it was possible to
> have a HashMap<std::pair<T, U>, V>, but the last one only if both T and U
don't
> use a custom isEmptyValue function. With this change it becomes possible to
use
> types with isEmptyValue functions inside std::pair in HashMap and friends as
> well.

Would you make the change in a separate CL?

[Marijn Kruisselbrink, 2016-06-03 20:37:04]

Okay, I think I managed to move all of BroadcastChannelConnection to be properly
garbage collected. There is no such thing as a HeapListHashSet<WeakMember<>>,
but HeapLinkedHashSet<WeakMember<>> seems to do the same thing/the thing I want
as well. Not sure what the difference between ListHashSet and LinkedHashSet is.
The only documented difference seems to be in iteration behavior...

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/HashTraits.h (right):

https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/HashTraits.h:248: static const bool
hasIsEmptyValueFunction = FirstTraits::hasIsEmptyValueFunction ||
SecondTraits::hasIsEmptyValueFunction;
On 2016/06/03 at 05:33:03, haraken wrote:
> On 2016/06/02 18:33:30, Marijn Kruisselbrink wrote:
> > On 2016/06/02 at 04:23:41, haraken wrote:
> > > Would you elaborate why you need this change? Is this an existing bug of
> > WTF::HashMap?
> > 
> > Yes, I'd say this is an existing bug of the std::pair HashTraits. It was
> > possible to have a HashMap<RefPtr<SecurityOrigin>, T>, and it was possible
to
> > have a HashMap<std::pair<T, U>, V>, but the last one only if both T and U
don't
> > use a custom isEmptyValue function. With this change it becomes possible to
use
> > types with isEmptyValue functions inside std::pair in HashMap and friends as
> > well.
> 
> Would you make the change in a separate CL?

Split off into https://codereview.chromium.org/2042453002

[haraken, 2016-06-06 02:05:45]

haraken@chromium.org changed reviewers:
+ hiroshige@chromium.org, sigbjornf@opera.com

[haraken, 2016-06-06 02:05:48]

+sigbjornf, +hiroshige (See my comments below.)

> Okay, I think I managed to move all of BroadcastChannelConnection to be
properly
> garbage collected. There is no such thing as a HeapListHashSet<WeakMember<>>,
> but HeapLinkedHashSet<WeakMember<>> seems to do the same thing/the thing I
want
> as well. Not sure what the difference between ListHashSet and LinkedHashSet
is.
> The only documented difference seems to be in iteration behavior...

Sorry about the confusion... We're working on unifying ListHashSet and
LinkedHashSet (https://bugs.chromium.org/p/chromium/issues/detail?id=614112).

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp
(right):

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp:58:
ALLOW_UNUSED_LOCAL(success);

Nit: Do we need this?

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp:63:
m_connection = nullptr;

Do you really want to clear m_connection before calling close()?

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp:86: ,
ContextLifecycleObserver(executionContext)

The fact that you're observing ContextLifecycleObserver means that you want to
observe when the ExecutionContext stops (and take some action). If not, you
should instead just add WeakMember<ExecutionContext> or Member<ExecutionContext>
to BroadcastChannel.

I'm guessing that you want to close the connection when the ExecutionContext
stops (i.e., Document::detach() is called). If that is the case, you should
override ContextLifecycleObserver::contextDestroyed() and call close().

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp
(right):

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp:42:
DEFINE_THREAD_SAFE_STATIC_LOCAL(ThreadSpecific<Persistent<ConnectionMap>>,
connectionMap, new ThreadSpecific<Persistent<ConnectionMap>>);

+sigbjornf@: Please check if this usage is correct :)

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp:100:
m_binding.set_connection_error_handler(createBaseCallback(bind(&BroadcastChannelConnection::onError,
WeakPersistentThisPointer<BroadcastChannelConnection>(this))));

+hiroshige@: Is this a correct usage to bind a Blink function with mojo?

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp:103:
ALLOW_UNUSED_LOCAL(result);

Nit: Is this needed?

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.h
(right):

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.h:23:
, public webmessaging::mojom::blink::BroadcastChannelClient {

Just to confirm: No one is keeping a reference to
webmessaging::mojom::blink::BroadcastChannelClient using a raw pointer, right?
If that happens, there is a risk that the raw pointer becomes stale when the
BroadcastChannelConnection gets garbage-collected.

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.h:32:
// messages send by this client to the channel.

sent by

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/broadcastchannel/DEPS (right):

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/DEPS:3: "+mojo/public",

Can you be make this more specific? I think "+mojo/public/cpp/bindings" would be
enough.

[esprehn, 2016-06-06 02:29:41]

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

[esprehn, 2016-06-06 02:29:42]

Hmm I'm not a big fan of the number of statics here. We should be able to just
open a connection to the browser process and send messages and it should route
to the right location. This is a lot of book keeping in the render process.

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp
(right):

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp:26:
DEFINE_THREAD_SAFE_STATIC_LOCAL(ThreadSpecific<webmessaging::mojom::blink::BroadcastChannelServicePtr>,
service, new
ThreadSpecific<webmessaging::mojom::blink::BroadcastChannelServicePtr>);
Hmm why do we need statics for this? I don't think we want to be adding statics
for services. Why can't frames or pages keep the connection?

[haraken, 2016-06-06 02:44:59]

On 2016/06/06 02:29:42, esprehn wrote:
> Hmm I'm not a big fan of the number of statics here. We should be able to just
> open a connection to the browser process and send messages and it should route
> to the right location. This is a lot of book keeping in the render process.
> 
>
https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
> File
>
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp
> (right):
> 
>
https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp:26:
>
DEFINE_THREAD_SAFE_STATIC_LOCAL(ThreadSpecific<webmessaging::mojom::blink::BroadcastChannelServicePtr>,
> service, new
> ThreadSpecific<webmessaging::mojom::blink::BroadcastChannelServicePtr>);
> Hmm why do we need statics for this? I don't think we want to be adding
statics
> for services. Why can't frames or pages keep the connection?

Agreed.
https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
has some context, but I'm not convinced why we can't use frames or pages to keep
the connection.

[sof, 2016-06-06 06:35:45]

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp
(right):

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp:42:
DEFINE_THREAD_SAFE_STATIC_LOCAL(ThreadSpecific<Persistent<ConnectionMap>>,
connectionMap, new ThreadSpecific<Persistent<ConnectionMap>>);
On 2016/06/06 02:05:47, haraken wrote:
> 
> +sigbjornf@: Please check if this usage is correct :)

Usage is exemplary; we don't try to hide very many details surrounding
ThreadSpecific<> persistent singleton initialization yet.

[Marijn Kruisselbrink, 2016-06-06 20:05:30]

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp
(right):

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp:58:
ALLOW_UNUSED_LOCAL(success);
On 2016/06/06 at 02:05:47, haraken wrote:
> Nit: Do we need this?

Not sure. Assertions.h seems to suggest so though by suggesting that
DCHECK+ALLOW_UNUSED_LOCAL is the correct replacement for ASSERT_UNUSED.

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp:63:
m_connection = nullptr;
On 2016/06/06 at 02:05:47, haraken wrote:
> Do you really want to clear m_connection before calling close()?

Good question. That pretty much turns close() into a no-op in this case. I do
want to avoid the call to unregisterClient though, as that just makes the code
in the BCConnection implementation a bit simpler (hence the unclear "After
onError is called a client should not try to do anything with the connection
anymore" comment, which I now made a bit more explicit about this specific
case).

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp
(right):

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp:26:
DEFINE_THREAD_SAFE_STATIC_LOCAL(ThreadSpecific<webmessaging::mojom::blink::BroadcastChannelServicePtr>,
service, new
ThreadSpecific<webmessaging::mojom::blink::BroadcastChannelServicePtr>);
On 2016/06/06 at 02:29:42, esprehn wrote:
> Hmm why do we need statics for this? I don't think we want to be adding
statics for services. Why can't frames or pages keep the connection?

On 2016/06/06 at 02:44:59, haraken wrote:
> Agreed.
https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
has some context, but I'm not convinced why we can't use frames or pages to keep
the connection.

Because in mojo every interface is its own pipe without any ordering guarantees
I do need to use the same mojo connection for all channels that share the same
"responsible event loop" (step 8 in
https://html.spec.whatwg.org/multipage/comms.html#dom-broadcastchannel-postme...
requires me to sort all BCChannel instances with the same event loop in creation
order). For workers that's no problem, and it would be possible to somehow
attach the mojo connection to the worker, but for frames/pages there can be at
most one event loop for any "unit of related browsing contexts". I suppose that
I could use the "top-most" context as representative of this unit of related
browsing contexts, but it seemed simpler to just do this as a thread-local thing
instead. In particular as windows can disown themselves (by writing to
self.opener), getting that right becomes very tricky.

I could go back to something more like PS4, which doesn't have most of the
bookkeeping in the renderer (but still has this particular threadspecific static
service connection). Downside of that is that the mojo API is (in my opinion)
more confusing, and it's of course also less efficient when a thread has more
than one connection to the same channel (as all messages to/from that thread
will then end up being send multiple times over IPC). I still do like the
simplicity of PS4 though, but I also like the benefits of having this extra
bookkeeping in the renderer (even though the performance is probably not going
to matter much for what I imagine this would be used for).

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp:100:
m_binding.set_connection_error_handler(createBaseCallback(bind(&BroadcastChannelConnection::onError,
WeakPersistentThisPointer<BroadcastChannelConnection>(this))));
On 2016/06/06 at 02:05:47, haraken wrote:
> +hiroshige@: Is this a correct usage to bind a Blink function with mojo?

fwiw this is how at least PaymentRequest does it
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/paymen...)

It does seem somewhat fragile though that the WeakPersistentThisPointer is
necessary to avoid having a memory leak because oilpan can't detect the
BroadcastChannelConnection->m_binding->error
callback->BroadcastChannelConnection cycle...

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp:103:
ALLOW_UNUSED_LOCAL(result);
On 2016/06/06 at 02:05:47, haraken wrote:
> Nit: Is this needed?

Not sure. Assertions.h seems to suggest so though by suggesting that
DCHECK+ALLOW_UNUSED_LOCAL is the correct replacement for ASSERT_UNUSED.

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.h
(right):

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.h:23:
, public webmessaging::mojom::blink::BroadcastChannelClient {
On 2016/06/06 at 02:05:48, haraken wrote:
> Just to confirm: No one is keeping a reference to
webmessaging::mojom::blink::BroadcastChannelClient using a raw pointer, right?
If that happens, there is a risk that the raw pointer becomes stale when the
BroadcastChannelConnection gets garbage-collected.

m_binding obviously is, to be able to call methods when mojo call come in.
PaymentRequest, and probably other places where blink classes implement mojo
interfaces all use the same pattern though, so hopefully this is not a
problematic usage?

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.h:32:
// messages send by this client to the channel.
On 2016/06/06 at 02:05:47, haraken wrote:
> sent by

fixed

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/broadcastchannel/DEPS (right):

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/DEPS:3: "+mojo/public",
On 2016/06/06 at 02:05:48, haraken wrote:
> Can you be make this more specific? I think "+mojo/public/cpp/bindings" would
be enough.

Done

[haraken, 2016-06-07 00:53:26]

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp
(right):

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp:86: ,
ContextLifecycleObserver(executionContext)
On 2016/06/06 02:05:47, haraken wrote:
> 
> The fact that you're observing ContextLifecycleObserver means that you want to
> observe when the ExecutionContext stops (and take some action). If not, you
> should instead just add WeakMember<ExecutionContext> or
Member<ExecutionContext>
> to BroadcastChannel.
> 
> I'm guessing that you want to close the connection when the ExecutionContext
> stops (i.e., Document::detach() is called). If that is the case, you should
> override ContextLifecycleObserver::contextDestroyed() and call close().
> 

Any idea on this?

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp
(right):

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp:26:
DEFINE_THREAD_SAFE_STATIC_LOCAL(ThreadSpecific<webmessaging::mojom::blink::BroadcastChannelServicePtr>,
service, new
ThreadSpecific<webmessaging::mojom::blink::BroadcastChannelServicePtr>);
On 2016/06/06 20:05:29, Marijn Kruisselbrink wrote:
> On 2016/06/06 at 02:29:42, esprehn wrote:
> > Hmm why do we need statics for this? I don't think we want to be adding
> statics for services. Why can't frames or pages keep the connection?
> 
> On 2016/06/06 at 02:44:59, haraken wrote:
> > Agreed.
>
https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
> has some context, but I'm not convinced why we can't use frames or pages to
keep
> the connection.
> 
> Because in mojo every interface is its own pipe without any ordering
guarantees
> I do need to use the same mojo connection for all channels that share the same
> "responsible event loop" (step 8 in
>
https://html.spec.whatwg.org/multipage/comms.html#dom-broadcastchannel-postme...
> requires me to sort all BCChannel instances with the same event loop in
creation
> order). For workers that's no problem, and it would be possible to somehow
> attach the mojo connection to the worker, but for frames/pages there can be at
> most one event loop for any "unit of related browsing contexts". I suppose
that
> I could use the "top-most" context as representative of this unit of related
> browsing contexts, but it seemed simpler to just do this as a thread-local
thing
> instead. In particular as windows can disown themselves (by writing to
> self.opener), getting that right becomes very tricky.
> 
> I could go back to something more like PS4, which doesn't have most of the
> bookkeeping in the renderer (but still has this particular threadspecific
static
> service connection). Downside of that is that the mojo API is (in my opinion)
> more confusing, and it's of course also less efficient when a thread has more
> than one connection to the same channel (as all messages to/from that thread
> will then end up being send multiple times over IPC). I still do like the
> simplicity of PS4 though, but I also like the benefits of having this extra
> bookkeeping in the renderer (even though the performance is probably not going
> to matter much for what I imagine this would be used for).

OK. Another option would be to use Page for the main thread and use
WorkerGlobalScope for workers, but it would be nastier than this CL.

The underlying issue would be that Blink doesn't have a good class that
abstracts the "top-level" browsing context.

https://codereview.chromium.org/2004643002/diff/260001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp
(right):

https://codereview.chromium.org/2004643002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp:42:
DEFINE_THREAD_SAFE_STATIC_LOCAL(ThreadSpecific<Persistent<ConnectionMap>>,
connectionMap, new ThreadSpecific<Persistent<ConnectionMap>>);

How about creating a map from ExecutionContext* to BroadcastChannelConnection?
Then you won't need a thread-specific map. (We don't want to add unnecessary
dependency on thread-local storage.)

[Marijn Kruisselbrink, 2016-06-07 01:18:40]

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp
(right):

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp:86: ,
ContextLifecycleObserver(executionContext)
On 2016/06/07 at 00:53:26, haraken wrote:
> On 2016/06/06 02:05:47, haraken wrote:
> > 
> > The fact that you're observing ContextLifecycleObserver means that you want
to
> > observe when the ExecutionContext stops (and take some action). If not, you
> > should instead just add WeakMember<ExecutionContext> or
Member<ExecutionContext>
> > to BroadcastChannel.
> > 
> > I'm guessing that you want to close the connection when the ExecutionContext
> > stops (i.e., Document::detach() is called). If that is the case, you should
> > override ContextLifecycleObserver::contextDestroyed() and call close().
> > 
> 
> Any idea on this?

Sorry about missing this one. Yes, contextDestroyed should call close() (just
curious, do you know if there is any particular reason why MessagePort overrides
ActiveDOMObject::stop for presumably the same behavior?)

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp
(right):

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp:26:
DEFINE_THREAD_SAFE_STATIC_LOCAL(ThreadSpecific<webmessaging::mojom::blink::BroadcastChannelServicePtr>,
service, new
ThreadSpecific<webmessaging::mojom::blink::BroadcastChannelServicePtr>);
On 2016/06/07 at 00:53:26, haraken wrote:
> On 2016/06/06 20:05:29, Marijn Kruisselbrink wrote:
> > On 2016/06/06 at 02:29:42, esprehn wrote:
> > > Hmm why do we need statics for this? I don't think we want to be adding
> > statics for services. Why can't frames or pages keep the connection?
> > 
> > On 2016/06/06 at 02:44:59, haraken wrote:
> > > Agreed.
> >
https://codereview.chromium.org/2004643002/diff/180001/third_party/WebKit/Sou...
> > has some context, but I'm not convinced why we can't use frames or pages to
keep
> > the connection.
> > 
> > Because in mojo every interface is its own pipe without any ordering
guarantees
> > I do need to use the same mojo connection for all channels that share the
same
> > "responsible event loop" (step 8 in
> >
https://html.spec.whatwg.org/multipage/comms.html#dom-broadcastchannel-postme...
> > requires me to sort all BCChannel instances with the same event loop in
creation
> > order). For workers that's no problem, and it would be possible to somehow
> > attach the mojo connection to the worker, but for frames/pages there can be
at
> > most one event loop for any "unit of related browsing contexts". I suppose
that
> > I could use the "top-most" context as representative of this unit of related
> > browsing contexts, but it seemed simpler to just do this as a thread-local
thing
> > instead. In particular as windows can disown themselves (by writing to
> > self.opener), getting that right becomes very tricky.
> > 
> > I could go back to something more like PS4, which doesn't have most of the
> > bookkeeping in the renderer (but still has this particular threadspecific
static
> > service connection). Downside of that is that the mojo API is (in my
opinion)
> > more confusing, and it's of course also less efficient when a thread has
more
> > than one connection to the same channel (as all messages to/from that thread
> > will then end up being send multiple times over IPC). I still do like the
> > simplicity of PS4 though, but I also like the benefits of having this extra
> > bookkeeping in the renderer (even though the performance is probably not
going
> > to matter much for what I imagine this would be used for).
> 
> OK. Another option would be to use Page for the main thread and use
WorkerGlobalScope for workers, but it would be nastier than this CL.

Yeah, anything more contained than a service connection per thread seems like it
would be more effort/complication than it's worth.

> The underlying issue would be that Blink doesn't have a good class that
abstracts the "top-level" browsing context.

And even with such a thing, code would still have to be really careful that such
a concept would be used in the expected way, especially in cases where documents
can disown themselves, thus possibly changing their top-level browsing context
(depending on how that would be defined).

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.h
(right):

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.h:23:
, public webmessaging::mojom::blink::BroadcastChannelClient {
On 2016/06/06 at 20:05:30, Marijn Kruisselbrink wrote:
> On 2016/06/06 at 02:05:48, haraken wrote:
> > Just to confirm: No one is keeping a reference to
webmessaging::mojom::blink::BroadcastChannelClient using a raw pointer, right?
If that happens, there is a risk that the raw pointer becomes stale when the
BroadcastChannelConnection gets garbage-collected.
> 
> m_binding obviously is, to be able to call methods when mojo call come in.
PaymentRequest, and probably other places where blink classes implement mojo
interfaces all use the same pattern though, so hopefully this is not a
problematic usage?

I'm still curious about this though. It sounds like the current pattern of
having a garbage collected class inherit from a mojo interface (and then passing
"this" to an m_binding mojo::Binding) might actually not be safe. Should there
be some kind of mojo::TraceableBinding that would then by traced by the
interface implementation?

https://codereview.chromium.org/2004643002/diff/260001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp
(right):

https://codereview.chromium.org/2004643002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp:42:
DEFINE_THREAD_SAFE_STATIC_LOCAL(ThreadSpecific<Persistent<ConnectionMap>>,
connectionMap, new ThreadSpecific<Persistent<ConnectionMap>>);
On 2016/06/07 at 00:53:26, haraken wrote:
> How about creating a map from ExecutionContext* to BroadcastChannelConnection?
Then you won't need a thread-specific map. (We don't want to add unnecessary
dependency on thread-local storage.)

Maybe I should just go back to not having any kind of connection map, like I had
in PS4. It really is more of a performance optimization that isn't obviously
useful (it really depends on how people might end up using BroadcastChannel),
and it does complicate things quite a bit. Not sure why in my mind having the
current .mojom interface was linked was also doing this local connections
optimization, but it really isn't. Having a per-thread
BroadcastChannelServicePtr (with the current interface) already will result in
the correct ordering. The only downside would be that according to a strict
reading of the spec all events for BroadcastChannel instances for the same
messages should be queued as tasks synchronously, and by having different mojo
calls for each instance it would be possible for other tasks to be queued in
between (which would be script observable). Not sure if that's worth the extra
complication of this connection map...

[haraken, 2016-06-07 01:29:56]

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp
(right):

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp:86: ,
ContextLifecycleObserver(executionContext)
On 2016/06/07 01:18:39, Marijn Kruisselbrink wrote:
> On 2016/06/07 at 00:53:26, haraken wrote:
> > On 2016/06/06 02:05:47, haraken wrote:
> > > 
> > > The fact that you're observing ContextLifecycleObserver means that you
want
> to
> > > observe when the ExecutionContext stops (and take some action). If not,
you
> > > should instead just add WeakMember<ExecutionContext> or
> Member<ExecutionContext>
> > > to BroadcastChannel.
> > > 
> > > I'm guessing that you want to close the connection when the
ExecutionContext
> > > stops (i.e., Document::detach() is called). If that is the case, you
should
> > > override ContextLifecycleObserver::contextDestroyed() and call close().
> > > 
> > 
> > Any idea on this?
> 
> Sorry about missing this one. Yes, contextDestroyed should call close() (just
> curious, do you know if there is any particular reason why MessagePort
overrides
> ActiveDOMObject::stop for presumably the same behavior?)

Don't know. But I think MessagePort should inherit from
ContextLifecycleObserver, not ActiveDOMObject. ActiveDOMObject makes sense only
when it defines behavior for suspend and resume.

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.h
(right):

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.h:23:
, public webmessaging::mojom::blink::BroadcastChannelClient {
On 2016/06/07 01:18:39, Marijn Kruisselbrink wrote:
> On 2016/06/06 at 20:05:30, Marijn Kruisselbrink wrote:
> > On 2016/06/06 at 02:05:48, haraken wrote:
> > > Just to confirm: No one is keeping a reference to
> webmessaging::mojom::blink::BroadcastChannelClient using a raw pointer, right?
> If that happens, there is a risk that the raw pointer becomes stale when the
> BroadcastChannelConnection gets garbage-collected.
> > 
> > m_binding obviously is, to be able to call methods when mojo call come in.
> PaymentRequest, and probably other places where blink classes implement mojo
> interfaces all use the same pattern though, so hopefully this is not a
> problematic usage?
> 
> I'm still curious about this though. It sounds like the current pattern of
> having a garbage collected class inherit from a mojo interface (and then
passing
> "this" to an m_binding mojo::Binding) might actually not be safe. Should there
> be some kind of mojo::TraceableBinding that would then by traced by the
> interface implementation?

Can we use the same pattern as m_paymentProvider?

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/paymen...

https://codereview.chromium.org/2004643002/diff/260001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp
(right):

https://codereview.chromium.org/2004643002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.cpp:42:
DEFINE_THREAD_SAFE_STATIC_LOCAL(ThreadSpecific<Persistent<ConnectionMap>>,
connectionMap, new ThreadSpecific<Persistent<ConnectionMap>>);
On 2016/06/07 01:18:39, Marijn Kruisselbrink wrote:
> On 2016/06/07 at 00:53:26, haraken wrote:
> > How about creating a map from ExecutionContext* to
BroadcastChannelConnection?
> Then you won't need a thread-specific map. (We don't want to add unnecessary
> dependency on thread-local storage.)
> 
> Maybe I should just go back to not having any kind of connection map, like I
had
> in PS4. It really is more of a performance optimization that isn't obviously
> useful (it really depends on how people might end up using BroadcastChannel),
> and it does complicate things quite a bit. Not sure why in my mind having the
> current .mojom interface was linked was also doing this local connections
> optimization, but it really isn't. Having a per-thread
> BroadcastChannelServicePtr (with the current interface) already will result in
> the correct ordering. The only downside would be that according to a strict
> reading of the spec all events for BroadcastChannel instances for the same
> messages should be queued as tasks synchronously, and by having different mojo
> calls for each instance it would be possible for other tasks to be queued in
> between (which would be script observable). Not sure if that's worth the extra
> complication of this connection map...

Yeah, let's exclude the optimization from this CL. We can iterate later.

[Marijn Kruisselbrink, 2016-06-07 17:41:39]

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.h
(right):

https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.h:23:
, public webmessaging::mojom::blink::BroadcastChannelClient {
On 2016/06/07 at 01:29:55, haraken wrote:
> On 2016/06/07 01:18:39, Marijn Kruisselbrink wrote:
> > On 2016/06/06 at 20:05:30, Marijn Kruisselbrink wrote:
> > > On 2016/06/06 at 02:05:48, haraken wrote:
> > > > Just to confirm: No one is keeping a reference to
> > webmessaging::mojom::blink::BroadcastChannelClient using a raw pointer,
right?
> > If that happens, there is a risk that the raw pointer becomes stale when the
> > BroadcastChannelConnection gets garbage-collected.
> > > 
> > > m_binding obviously is, to be able to call methods when mojo call come in.
> > PaymentRequest, and probably other places where blink classes implement mojo
> > interfaces all use the same pattern though, so hopefully this is not a
> > problematic usage?
> > 
> > I'm still curious about this though. It sounds like the current pattern of
> > having a garbage collected class inherit from a mojo interface (and then
passing
> > "this" to an m_binding mojo::Binding) might actually not be safe. Should
there
> > be some kind of mojo::TraceableBinding that would then by traced by the
> > interface implementation?
> 
> Can we use the same pattern as m_paymentProvider?
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/paymen...

m_clientBinding in PaymentRequest has the same issue. I'm fairly sure that
garbage collected classes having pointers to external mojo interfaces all works
fine and doesn't have any real issues (other than the minor one that if you're
not careful to use a WeakPersistentThisPointer when assigning an error callback
you're introducing a cycle/memory leak). But the reverse of garbage collected
classes implementing a mojo interface and binding that to a mojo pipe using a
mojo::Binding does seem wrong. As the Binding holds a raw pointer to the "this"
implementing the interface, and will happily keep dispatching incoming method
calls to that implementation until the binding is destroyed/explicitly closed.

[haraken, 2016-06-08 00:04:10]

On 2016/06/07 17:41:39, Marijn Kruisselbrink wrote:
>
https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
> File
>
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.h
> (right):
> 
>
https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.h:23:
> , public webmessaging::mojom::blink::BroadcastChannelClient {
> On 2016/06/07 at 01:29:55, haraken wrote:
> > On 2016/06/07 01:18:39, Marijn Kruisselbrink wrote:
> > > On 2016/06/06 at 20:05:30, Marijn Kruisselbrink wrote:
> > > > On 2016/06/06 at 02:05:48, haraken wrote:
> > > > > Just to confirm: No one is keeping a reference to
> > > webmessaging::mojom::blink::BroadcastChannelClient using a raw pointer,
> right?
> > > If that happens, there is a risk that the raw pointer becomes stale when
the
> > > BroadcastChannelConnection gets garbage-collected.
> > > > 
> > > > m_binding obviously is, to be able to call methods when mojo call come
in.
> > > PaymentRequest, and probably other places where blink classes implement
mojo
> > > interfaces all use the same pattern though, so hopefully this is not a
> > > problematic usage?
> > > 
> > > I'm still curious about this though. It sounds like the current pattern of
> > > having a garbage collected class inherit from a mojo interface (and then
> passing
> > > "this" to an m_binding mojo::Binding) might actually not be safe. Should
> there
> > > be some kind of mojo::TraceableBinding that would then by traced by the
> > > interface implementation?
> > 
> > Can we use the same pattern as m_paymentProvider?
> > 
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/paymen...
> 
> m_clientBinding in PaymentRequest has the same issue. I'm fairly sure that
> garbage collected classes having pointers to external mojo interfaces all
works
> fine and doesn't have any real issues (other than the minor one that if you're
> not careful to use a WeakPersistentThisPointer when assigning an error
callback
> you're introducing a cycle/memory leak). But the reverse of garbage collected
> classes implementing a mojo interface and binding that to a mojo pipe using a
> mojo::Binding does seem wrong. As the Binding holds a raw pointer to the
"this"
> implementing the interface, and will happily keep dispatching incoming method
> calls to that implementation until the binding is destroyed/explicitly closed.

I think that's why we use WeakPersistentThisPointer. As long as you use
WeakPersistentThisPointer, mojo will stop calling the callback when the |this|
object gets destroyed.

[Marijn Kruisselbrink, 2016-06-08 00:10:58]

On 2016/06/08 at 00:04:10, haraken wrote:
> On 2016/06/07 17:41:39, Marijn Kruisselbrink wrote:
> >
https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
> > File
> >
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.h
> > (right):
> > 
> >
https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
> >
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.h:23:
> > , public webmessaging::mojom::blink::BroadcastChannelClient {
> > On 2016/06/07 at 01:29:55, haraken wrote:
> > > On 2016/06/07 01:18:39, Marijn Kruisselbrink wrote:
> > > > On 2016/06/06 at 20:05:30, Marijn Kruisselbrink wrote:
> > > > > On 2016/06/06 at 02:05:48, haraken wrote:
> > > > > > Just to confirm: No one is keeping a reference to
> > > > webmessaging::mojom::blink::BroadcastChannelClient using a raw pointer,
> > right?
> > > > If that happens, there is a risk that the raw pointer becomes stale when
the
> > > > BroadcastChannelConnection gets garbage-collected.
> > > > > 
> > > > > m_binding obviously is, to be able to call methods when mojo call come
in.
> > > > PaymentRequest, and probably other places where blink classes implement
mojo
> > > > interfaces all use the same pattern though, so hopefully this is not a
> > > > problematic usage?
> > > > 
> > > > I'm still curious about this though. It sounds like the current pattern
of
> > > > having a garbage collected class inherit from a mojo interface (and then
> > passing
> > > > "this" to an m_binding mojo::Binding) might actually not be safe. Should
> > there
> > > > be some kind of mojo::TraceableBinding that would then by traced by the
> > > > interface implementation?
> > > 
> > > Can we use the same pattern as m_paymentProvider?
> > > 
> > >
> >
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/paymen...
> > 
> > m_clientBinding in PaymentRequest has the same issue. I'm fairly sure that
> > garbage collected classes having pointers to external mojo interfaces all
works
> > fine and doesn't have any real issues (other than the minor one that if
you're
> > not careful to use a WeakPersistentThisPointer when assigning an error
callback
> > you're introducing a cycle/memory leak). But the reverse of garbage
collected
> > classes implementing a mojo interface and binding that to a mojo pipe using
a
> > mojo::Binding does seem wrong. As the Binding holds a raw pointer to the
"this"
> > implementing the interface, and will happily keep dispatching incoming
method
> > calls to that implementation until the binding is destroyed/explicitly
closed.
> 
> I think that's why we use WeakPersistentThisPointer. As long as you use
WeakPersistentThisPointer, mojo will stop calling the callback when the |this|
object gets destroyed.

We seem to be talking past eachother. As you point out everything is working
fine (but fragile as it's easy to forget the WeakPersistentThisPointer) with
regard to error callbacks. Where things are not working fine is with the "this"
pointer that is passed to the binding constructor (m_clientBinding(this) in the
PaymentRequest case). There is nothing to somehow invalidate that pointer until
the PaymentRequest destructor is actually called, so mojo will happily keep
dispatching incoming method calls on that binding when the pointer is no longer
valid (unless I'm misunderstanding how oilpan works). mojo::Binding (and all its
variations like mojo::AssociatedBinding and mojo::StrongBinding) holds a raw
pointer to a class implementing a mojo interface. If that class implementing the
interface is garbage collected it seems things could be broken (i.e. methods on
the garbage collected class can get called after the class has been garbage
collected until the destructor actually gets called).

[haraken, 2016-06-08 00:52:48]

On 2016/06/08 00:10:58, Marijn Kruisselbrink wrote:
> On 2016/06/08 at 00:04:10, haraken wrote:
> > On 2016/06/07 17:41:39, Marijn Kruisselbrink wrote:
> > >
>
https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
> > > File
> > >
>
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.h
> > > (right):
> > > 
> > >
>
https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
> > >
>
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.h:23:
> > > , public webmessaging::mojom::blink::BroadcastChannelClient {
> > > On 2016/06/07 at 01:29:55, haraken wrote:
> > > > On 2016/06/07 01:18:39, Marijn Kruisselbrink wrote:
> > > > > On 2016/06/06 at 20:05:30, Marijn Kruisselbrink wrote:
> > > > > > On 2016/06/06 at 02:05:48, haraken wrote:
> > > > > > > Just to confirm: No one is keeping a reference to
> > > > > webmessaging::mojom::blink::BroadcastChannelClient using a raw
pointer,
> > > right?
> > > > > If that happens, there is a risk that the raw pointer becomes stale
when
> the
> > > > > BroadcastChannelConnection gets garbage-collected.
> > > > > > 
> > > > > > m_binding obviously is, to be able to call methods when mojo call
come
> in.
> > > > > PaymentRequest, and probably other places where blink classes
implement
> mojo
> > > > > interfaces all use the same pattern though, so hopefully this is not a
> > > > > problematic usage?
> > > > > 
> > > > > I'm still curious about this though. It sounds like the current
pattern
> of
> > > > > having a garbage collected class inherit from a mojo interface (and
then
> > > passing
> > > > > "this" to an m_binding mojo::Binding) might actually not be safe.
Should
> > > there
> > > > > be some kind of mojo::TraceableBinding that would then by traced by
the
> > > > > interface implementation?
> > > > 
> > > > Can we use the same pattern as m_paymentProvider?
> > > > 
> > > >
> > >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/paymen...
> > > 
> > > m_clientBinding in PaymentRequest has the same issue. I'm fairly sure that
> > > garbage collected classes having pointers to external mojo interfaces all
> works
> > > fine and doesn't have any real issues (other than the minor one that if
> you're
> > > not careful to use a WeakPersistentThisPointer when assigning an error
> callback
> > > you're introducing a cycle/memory leak). But the reverse of garbage
> collected
> > > classes implementing a mojo interface and binding that to a mojo pipe
using
> a
> > > mojo::Binding does seem wrong. As the Binding holds a raw pointer to the
> "this"
> > > implementing the interface, and will happily keep dispatching incoming
> method
> > > calls to that implementation until the binding is destroyed/explicitly
> closed.
> > 
> > I think that's why we use WeakPersistentThisPointer. As long as you use
> WeakPersistentThisPointer, mojo will stop calling the callback when the |this|
> object gets destroyed.
> 
> We seem to be talking past eachother. As you point out everything is working
> fine (but fragile as it's easy to forget the WeakPersistentThisPointer) with
> regard to error callbacks. Where things are not working fine is with the
"this"
> pointer that is passed to the binding constructor (m_clientBinding(this) in
the
> PaymentRequest case). There is nothing to somehow invalidate that pointer
until
> the PaymentRequest destructor is actually called, so mojo will happily keep
> dispatching incoming method calls on that binding when the pointer is no
longer
> valid (unless I'm misunderstanding how oilpan works). mojo::Binding (and all
its
> variations like mojo::AssociatedBinding and mojo::StrongBinding) holds a raw
> pointer to a class implementing a mojo interface. If that class implementing
the
> interface is garbage collected it seems things could be broken (i.e. methods
on
> the garbage collected class can get called after the class has been garbage
> collected until the destructor actually gets called).

PaymentRequest::contextDestroyed() calls clearResolversAndCloseMojoConnection,
and it closes the mojo connection, right? Can we do the same thing in this CL?

[Marijn Kruisselbrink, 2016-06-08 01:12:30]

On 2016/06/08 at 00:52:48, haraken wrote:
> On 2016/06/08 00:10:58, Marijn Kruisselbrink wrote:
> > On 2016/06/08 at 00:04:10, haraken wrote:
> > > On 2016/06/07 17:41:39, Marijn Kruisselbrink wrote:
> > > >
> >
https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
> > > > File
> > > >
> >
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.h
> > > > (right):
> > > > 
> > > >
> >
https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
> > > >
> >
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.h:23:
> > > > , public webmessaging::mojom::blink::BroadcastChannelClient {
> > > > On 2016/06/07 at 01:29:55, haraken wrote:
> > > > > On 2016/06/07 01:18:39, Marijn Kruisselbrink wrote:
> > > > > > On 2016/06/06 at 20:05:30, Marijn Kruisselbrink wrote:
> > > > > > > On 2016/06/06 at 02:05:48, haraken wrote:
> > > > > > > > Just to confirm: No one is keeping a reference to
> > > > > > webmessaging::mojom::blink::BroadcastChannelClient using a raw
pointer,
> > > > right?
> > > > > > If that happens, there is a risk that the raw pointer becomes stale
when
> > the
> > > > > > BroadcastChannelConnection gets garbage-collected.
> > > > > > > 
> > > > > > > m_binding obviously is, to be able to call methods when mojo call
come
> > in.
> > > > > > PaymentRequest, and probably other places where blink classes
implement
> > mojo
> > > > > > interfaces all use the same pattern though, so hopefully this is not
a
> > > > > > problematic usage?
> > > > > > 
> > > > > > I'm still curious about this though. It sounds like the current
pattern
> > of
> > > > > > having a garbage collected class inherit from a mojo interface (and
then
> > > > passing
> > > > > > "this" to an m_binding mojo::Binding) might actually not be safe.
Should
> > > > there
> > > > > > be some kind of mojo::TraceableBinding that would then by traced by
the
> > > > > > interface implementation?
> > > > > 
> > > > > Can we use the same pattern as m_paymentProvider?
> > > > > 
> > > > >
> > > >
> >
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/paymen...
> > > > 
> > > > m_clientBinding in PaymentRequest has the same issue. I'm fairly sure
that
> > > > garbage collected classes having pointers to external mojo interfaces
all
> > works
> > > > fine and doesn't have any real issues (other than the minor one that if
> > you're
> > > > not careful to use a WeakPersistentThisPointer when assigning an error
> > callback
> > > > you're introducing a cycle/memory leak). But the reverse of garbage
> > collected
> > > > classes implementing a mojo interface and binding that to a mojo pipe
using
> > a
> > > > mojo::Binding does seem wrong. As the Binding holds a raw pointer to the
> > "this"
> > > > implementing the interface, and will happily keep dispatching incoming
> > method
> > > > calls to that implementation until the binding is destroyed/explicitly
> > closed.
> > > 
> > > I think that's why we use WeakPersistentThisPointer. As long as you use
> > WeakPersistentThisPointer, mojo will stop calling the callback when the
|this|
> > object gets destroyed.
> > 
> > We seem to be talking past eachother. As you point out everything is working
> > fine (but fragile as it's easy to forget the WeakPersistentThisPointer) with
> > regard to error callbacks. Where things are not working fine is with the
"this"
> > pointer that is passed to the binding constructor (m_clientBinding(this) in
the
> > PaymentRequest case). There is nothing to somehow invalidate that pointer
until
> > the PaymentRequest destructor is actually called, so mojo will happily keep
> > dispatching incoming method calls on that binding when the pointer is no
longer
> > valid (unless I'm misunderstanding how oilpan works). mojo::Binding (and all
its
> > variations like mojo::AssociatedBinding and mojo::StrongBinding) holds a raw
> > pointer to a class implementing a mojo interface. If that class implementing
the
> > interface is garbage collected it seems things could be broken (i.e. methods
on
> > the garbage collected class can get called after the class has been garbage
> > collected until the destructor actually gets called).
> 
> PaymentRequest::contextDestroyed() calls clearResolversAndCloseMojoConnection,
and it closes the mojo connection, right? Can we do the same thing in this CL?

Not sure how contextDestroyed would help here, as the problematic case would be
when a PaymentRequest is garbage collected while the context isn't destroyed.
But it does seem like there are enough things interacting in complicated ways to
actually make it in practice safe in the PaymentRequest case (hasPendingActivity
makes sure that the PaymentRequest won't get garbage collected while it has an
outstanding promise, other code (currently) happens to ensure that a mojo
connection only exists while there is one of these outstanding promises, so
mostly by accident PaymentRequest indeed can't be garbage collected while there
is an active mojo binding. So PaymentRequest is probably not the best example of
what still seems like a very dangerous pattern to me. Anybody who writes a
garbage collected class that implements a mojo interface has to be extremely
careful to make sure that either the class won't get garbage collected while the
binding is active, or some other way make sure the binding gets disconnected
when it gets garbage collected (with a pre-finalizer presumably?) And all that
because currently a raw pointer is passed to mojo::binding. I guess reviewers
just have to be really careful to make sure that any garbage collected class
implementing a mojo interface actually has a pre finalizer for this (at least it
seems modules/webusb/USB.h, the only other current case of a blink class
implementing a mojom interface, already does this correctly). And I think even
though PaymentRequest happens to not be a problem now, it probably should have
this same pre finalizer to disconnect the binding. Not sure if there is some way
to more automatically catch this caveat of using mojo in blink, but it seems
risky to rely on reviewers catching these things.

[Marijn Kruisselbrink, 2016-06-08 01:15:04]

(and none of this matters too much for this CL, I know now what to do. I just
haven't had time to update my CL. I just worry that this is something very
non-obvious that people will keep missing in future CLs and something that
reviewers aren't going to always catch; I don't think anybody thought about this
issue in the PaymentRequest case, I'm not sure somebody would have caught it in
this CL)

[haraken, 2016-06-08 01:15:48]

On 2016/06/08 01:12:30, Marijn Kruisselbrink wrote:
> On 2016/06/08 at 00:52:48, haraken wrote:
> > On 2016/06/08 00:10:58, Marijn Kruisselbrink wrote:
> > > On 2016/06/08 at 00:04:10, haraken wrote:
> > > > On 2016/06/07 17:41:39, Marijn Kruisselbrink wrote:
> > > > >
> > >
>
https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
> > > > > File
> > > > >
> > >
>
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.h
> > > > > (right):
> > > > > 
> > > > >
> > >
>
https://codereview.chromium.org/2004643002/diff/240001/third_party/WebKit/Sou...
> > > > >
> > >
>
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannelConnection.h:23:
> > > > > , public webmessaging::mojom::blink::BroadcastChannelClient {
> > > > > On 2016/06/07 at 01:29:55, haraken wrote:
> > > > > > On 2016/06/07 01:18:39, Marijn Kruisselbrink wrote:
> > > > > > > On 2016/06/06 at 20:05:30, Marijn Kruisselbrink wrote:
> > > > > > > > On 2016/06/06 at 02:05:48, haraken wrote:
> > > > > > > > > Just to confirm: No one is keeping a reference to
> > > > > > > webmessaging::mojom::blink::BroadcastChannelClient using a raw
> pointer,
> > > > > right?
> > > > > > > If that happens, there is a risk that the raw pointer becomes
stale
> when
> > > the
> > > > > > > BroadcastChannelConnection gets garbage-collected.
> > > > > > > > 
> > > > > > > > m_binding obviously is, to be able to call methods when mojo
call
> come
> > > in.
> > > > > > > PaymentRequest, and probably other places where blink classes
> implement
> > > mojo
> > > > > > > interfaces all use the same pattern though, so hopefully this is
not
> a
> > > > > > > problematic usage?
> > > > > > > 
> > > > > > > I'm still curious about this though. It sounds like the current
> pattern
> > > of
> > > > > > > having a garbage collected class inherit from a mojo interface
(and
> then
> > > > > passing
> > > > > > > "this" to an m_binding mojo::Binding) might actually not be safe.
> Should
> > > > > there
> > > > > > > be some kind of mojo::TraceableBinding that would then by traced
by
> the
> > > > > > > interface implementation?
> > > > > > 
> > > > > > Can we use the same pattern as m_paymentProvider?
> > > > > > 
> > > > > >
> > > > >
> > >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/paymen...
> > > > > 
> > > > > m_clientBinding in PaymentRequest has the same issue. I'm fairly sure
> that
> > > > > garbage collected classes having pointers to external mojo interfaces
> all
> > > works
> > > > > fine and doesn't have any real issues (other than the minor one that
if
> > > you're
> > > > > not careful to use a WeakPersistentThisPointer when assigning an error
> > > callback
> > > > > you're introducing a cycle/memory leak). But the reverse of garbage
> > > collected
> > > > > classes implementing a mojo interface and binding that to a mojo pipe
> using
> > > a
> > > > > mojo::Binding does seem wrong. As the Binding holds a raw pointer to
the
> > > "this"
> > > > > implementing the interface, and will happily keep dispatching incoming
> > > method
> > > > > calls to that implementation until the binding is destroyed/explicitly
> > > closed.
> > > > 
> > > > I think that's why we use WeakPersistentThisPointer. As long as you use
> > > WeakPersistentThisPointer, mojo will stop calling the callback when the
> |this|
> > > object gets destroyed.
> > > 
> > > We seem to be talking past eachother. As you point out everything is
working
> > > fine (but fragile as it's easy to forget the WeakPersistentThisPointer)
with
> > > regard to error callbacks. Where things are not working fine is with the
> "this"
> > > pointer that is passed to the binding constructor (m_clientBinding(this)
in
> the
> > > PaymentRequest case). There is nothing to somehow invalidate that pointer
> until
> > > the PaymentRequest destructor is actually called, so mojo will happily
keep
> > > dispatching incoming method calls on that binding when the pointer is no
> longer
> > > valid (unless I'm misunderstanding how oilpan works). mojo::Binding (and
all
> its
> > > variations like mojo::AssociatedBinding and mojo::StrongBinding) holds a
raw
> > > pointer to a class implementing a mojo interface. If that class
implementing
> the
> > > interface is garbage collected it seems things could be broken (i.e.
methods
> on
> > > the garbage collected class can get called after the class has been
garbage
> > > collected until the destructor actually gets called).
> > 
> > PaymentRequest::contextDestroyed() calls
clearResolversAndCloseMojoConnection,
> and it closes the mojo connection, right? Can we do the same thing in this CL?
> 
> Not sure how contextDestroyed would help here, as the problematic case would
be
> when a PaymentRequest is garbage collected while the context isn't destroyed.
> But it does seem like there are enough things interacting in complicated ways
to
> actually make it in practice safe in the PaymentRequest case
(hasPendingActivity
> makes sure that the PaymentRequest won't get garbage collected while it has an
> outstanding promise, other code (currently) happens to ensure that a mojo
> connection only exists while there is one of these outstanding promises, so
> mostly by accident PaymentRequest indeed can't be garbage collected while
there
> is an active mojo binding. So PaymentRequest is probably not the best example
of
> what still seems like a very dangerous pattern to me. Anybody who writes a
> garbage collected class that implements a mojo interface has to be extremely
> careful to make sure that either the class won't get garbage collected while
the
> binding is active, or some other way make sure the binding gets disconnected
> when it gets garbage collected (with a pre-finalizer presumably?) And all that
> because currently a raw pointer is passed to mojo::binding. I guess reviewers
> just have to be really careful to make sure that any garbage collected class
> implementing a mojo interface actually has a pre finalizer for this (at least
it
> seems modules/webusb/USB.h, the only other current case of a blink class
> implementing a mojom interface, already does this correctly). And I think even
> though PaymentRequest happens to not be a problem now, it probably should have
> this same pre finalizer to disconnect the binding. Not sure if there is some
way
> to more automatically catch this caveat of using mojo in blink, but it seems
> risky to rely on reviewers catching these things.

Yeah, this is a good point.

I think you need to use a pre-finalizer
(https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...).
You can close the mojo connection in the pre-finalizer. A pre-finalizer is a
common way to clear a raw pointer from contents/ to Blink.

[Marijn Kruisselbrink, 2016-06-08 21:12:52]

Patchset #15 (id:300001) has been deleted

[Marijn Kruisselbrink, 2016-06-08 21:43:58]

Okay, I reverted back to having minimum logic in the renderer, even though that
is technically/arguably not entirely spec compliant. Also that meant I had to
revert to the mojo interface from PS4 as with the Subscribe/Broadcast API I had
the browser process would have had no way to link a particular Broadcast call to
a particular subscription in order to not echo back messages to the connection
that send the message.

[haraken, 2016-06-09 02:09:37]

In terms of implementation, LGTM.

https://codereview.chromium.org/2004643002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp
(right):

https://codereview.chromium.org/2004643002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp:28:
webmessaging::mojom::blink::BroadcastChannelServicePtr&
getThreadSpecificService()

Add a comment why you need to use a thread-local storage.

[kinuko, 2016-06-09 09:14:58]

https://codereview.chromium.org/2004643002/diff/320001/components/webmessagin...
File components/webmessaging/broadcast_channel_service.cc (right):

https://codereview.chromium.org/2004643002/diff/320001/components/webmessagin...
components/webmessaging/broadcast_channel_service.cc:28:
mojom::BroadcastChannelClient* client() const { return client_.get(); }
nit: could we avoid having const method returning non-const pointer?

https://codereview.chromium.org/2004643002/diff/320001/components/webmessagin...
components/webmessaging/broadcast_channel_service.cc:81:
connections_[c->origin()].insert(std::make_pair(c->name(), std::move(c)));
is it safe to access c on the same param list of std::move(c)?

https://codereview.chromium.org/2004643002/diff/320001/components/webmessagin...
File components/webmessaging/public/interfaces/broadcast_channel.mojom (right):

https://codereview.chromium.org/2004643002/diff/320001/components/webmessagin...
components/webmessaging/public/interfaces/broadcast_channel.mojom:21:
ConnectToChannel(url.mojom.Origin origin, string name, associated
BroadcastChannelClient receiver, associated BroadcastChannelClient& sender);
nit: can you wrap the line?

https://codereview.chromium.org/2004643002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/2004643002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8Binding.h:86: CORE_EXPORT
v8::Local<v8::Value> createMinimumArityTypeErrorForConstructor(v8::Isolate*,
const char* type, unsigned expected, unsigned provided);
is this related? (wasn't very obvious to me...)

https://codereview.chromium.org/2004643002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp
(right):

https://codereview.chromium.org/2004643002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp:24:
DCHECK(Platform::current()->mainThread()->isCurrentThread());
nit: DCHECK(isMainThread()) ?

https://codereview.chromium.org/2004643002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp:132: //
Remove BroadcastChannelClient for messages send from this channel to the
browser.
Um... I guess s/Remove/Remote/ ?

https://codereview.chromium.org/2004643002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.h
(right):

https://codereview.chromium.org/2004643002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.h:38:
ExecutionContext* getExecutionContext() const override { return
ContextLifecycleObserver::getExecutionContext(); }
do we need this override??

[Marijn Kruisselbrink, 2016-06-22 18:46:57]

https://codereview.chromium.org/2004643002/diff/320001/components/webmessagin...
File components/webmessaging/broadcast_channel_service.cc (right):

https://codereview.chromium.org/2004643002/diff/320001/components/webmessagin...
components/webmessaging/broadcast_channel_service.cc:28:
mojom::BroadcastChannelClient* client() const { return client_.get(); }
On 2016/06/09 at 09:14:58, kinuko wrote:
> nit: could we avoid having const method returning non-const pointer?

Done

https://codereview.chromium.org/2004643002/diff/320001/components/webmessagin...
components/webmessaging/broadcast_channel_service.cc:81:
connections_[c->origin()].insert(std::make_pair(c->name(), std::move(c)));
On 2016/06/09 at 09:14:58, kinuko wrote:
> is it safe to access c on the same param list of std::move(c)?

I think it is. std::move doesn't actually modify c, the actual move happens when
the result of std::move is consumed by insert. And I'm pretty sure all
parameters are evaluated before the method is called. So even if std::move is
evaluated before c->name() that is still safe, as both are done before insert()
is called.

But yeah, it's non-obvious that this is safe, and since there isn't really a
reason to get the name/origin from the connection, I changed it to just use the
name parameter rather than getting it from c->name().

https://codereview.chromium.org/2004643002/diff/320001/components/webmessagin...
File components/webmessaging/public/interfaces/broadcast_channel.mojom (right):

https://codereview.chromium.org/2004643002/diff/320001/components/webmessagin...
components/webmessaging/public/interfaces/broadcast_channel.mojom:21:
ConnectToChannel(url.mojom.Origin origin, string name, associated
BroadcastChannelClient receiver, associated BroadcastChannelClient& sender);
On 2016/06/09 at 09:14:58, kinuko wrote:
> nit: can you wrap the line?

Done (somebody really should add a mojom style guide to
https://www.chromium.org/developers/coding-style imho...)

https://codereview.chromium.org/2004643002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/2004643002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8Binding.h:86: CORE_EXPORT
v8::Local<v8::Value> createMinimumArityTypeErrorForConstructor(v8::Isolate*,
const char* type, unsigned expected, unsigned provided);
On 2016/06/09 at 09:14:58, kinuko wrote:
> is this related? (wasn't very obvious to me...)

The bindings generated code for a constructor can end up calling this method.
Which if the code is in modules/ requires the method to be exported. So yes,
this is/was related. But another CL in the mean time already added this missing
CORE_EXPORT so it's no longer part of this CL.

https://codereview.chromium.org/2004643002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp
(right):

https://codereview.chromium.org/2004643002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp:24:
DCHECK(Platform::current()->mainThread()->isCurrentThread());
On 2016/06/09 at 09:14:58, kinuko wrote:
> nit: DCHECK(isMainThread()) ?

Done

https://codereview.chromium.org/2004643002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp:28:
webmessaging::mojom::blink::BroadcastChannelServicePtr&
getThreadSpecificService()
On 2016/06/09 at 02:09:36, haraken wrote:
> Add a comment why you need to use a thread-local storage.

Done

https://codereview.chromium.org/2004643002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp:132: //
Remove BroadcastChannelClient for messages send from this channel to the
browser.
On 2016/06/09 at 09:14:58, kinuko wrote:
> Um... I guess s/Remove/Remote/ ?

Ah yes, done.

https://codereview.chromium.org/2004643002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.h
(right):

https://codereview.chromium.org/2004643002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.h:38:
ExecutionContext* getExecutionContext() const override { return
ContextLifecycleObserver::getExecutionContext(); }
On 2016/06/09 at 09:14:58, kinuko wrote:
> do we need this override??

Yes, EventTarget has a pure virtual getExecutionContext, and without explicitly
defining a getExecutionContext method the compiler doesn't automatically uses
the implementation from a different base class to implement that pure virtual.

[kinuko, 2016-06-23 15:29:57]

https://codereview.chromium.org/2004643002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp
(right):

https://codereview.chromium.org/2004643002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp:38:
threadSafeBind(&connectToService, passed(mojo::GetProxy(&*service))));
I was looking into connectToRemoteService impl and it looks we always post task
to main thread if this is called on non-main thread.  Do we still need this post
task?

[Marijn Kruisselbrink, 2016-06-23 16:32:32]

https://codereview.chromium.org/2004643002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp
(right):

https://codereview.chromium.org/2004643002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp:38:
threadSafeBind(&connectToService, passed(mojo::GetProxy(&*service))));
On 2016/06/23 at 15:29:57, kinuko wrote:
> I was looking into connectToRemoteService impl and it looks we always post
task to main thread if this is called on non-main thread.  Do we still need this
post task?

Ah yes, seems that was changed recently to work from non-main thread. Seems they
forgot to document the new thread safety requirements for blink::ServiceRegistry
implementations though, so I've added that in this CL.

[kinuko, 2016-06-24 14:28:03]

lgtm

https://codereview.chromium.org/2004643002/diff/360001/components/webmessagin...
File components/webmessaging/broadcast_channel_service.cc (right):

https://codereview.chromium.org/2004643002/diff/360001/components/webmessagin...
components/webmessaging/broadcast_channel_service.cc:112: }
nit: no {} for one-line body for consistency

[Marijn Kruisselbrink, 2016-06-24 18:12:26]

https://codereview.chromium.org/2004643002/diff/360001/components/webmessagin...
File components/webmessaging/broadcast_channel_service.cc (right):

https://codereview.chromium.org/2004643002/diff/360001/components/webmessagin...
components/webmessaging/broadcast_channel_service.cc:112: }
On 2016/06/24 at 14:28:03, kinuko wrote:
> nit: no {} for one-line body for consistency

done

[Marijn Kruisselbrink, 2016-06-24 18:13:05]

mek@chromium.org changed reviewers:
+ blundell@chromium.org, dcheng@chromium.org

[Marijn Kruisselbrink, 2016-06-24 18:13:06]

+blundell for components/ OWNERS
+dcheng for IPC OWNERS

[Ken Rockot(use gerrit already), 2016-06-24 18:14:50]

rockot@chromium.org changed reviewers:
- blundell@chromium.org, dcheng@chromium.org

[Ken Rockot(use gerrit already), 2016-06-24 18:14:51]

Generally LG minus naming issue

https://codereview.chromium.org/2004643002/diff/380001/components/webmessagin...
File components/webmessaging/public/interfaces/BUILD.gn (right):

https://codereview.chromium.org/2004643002/diff/380001/components/webmessagin...
components/webmessaging/public/interfaces/BUILD.gn:7: mojom_files = [
"broadcast_channel.mojom" ]
nit: Please just inline the sources list like you would for any other target.

https://codereview.chromium.org/2004643002/diff/380001/components/webmessagin...
File components/webmessaging/public/interfaces/broadcast_channel.mojom (right):

https://codereview.chromium.org/2004643002/diff/380001/components/webmessagin...
components/webmessaging/public/interfaces/broadcast_channel.mojom:14: interface
BroadcastChannelService {
I should have caught this earlier, but please don't add a "Service" suffix to
new mojo interfaces. In general we'd like to keep some consistent terminology
moving forward and "service" is reserved for aggregated interface providers in
//services which might make sense to physically isolate from each other at
runtime (see //services/README.md).

Maybe BroadcastChannelProvider or BroadcastChannelConnector?

[Marijn Kruisselbrink, 2016-06-24 18:37:09]

mek@chromium.org changed reviewers:
+ blundell@chromium.org, dcheng@chromium.org

[Marijn Kruisselbrink, 2016-06-24 18:37:11]

(and adding back reviewers that rockot@ I assume accidentally removed)

https://codereview.chromium.org/2004643002/diff/380001/components/webmessagin...
File components/webmessaging/public/interfaces/BUILD.gn (right):

https://codereview.chromium.org/2004643002/diff/380001/components/webmessagin...
components/webmessaging/public/interfaces/BUILD.gn:7: mojom_files = [
"broadcast_channel.mojom" ]
On 2016/06/24 at 18:14:51, Ken Rockot wrote:
> nit: Please just inline the sources list like you would for any other target.

Done

https://codereview.chromium.org/2004643002/diff/380001/components/webmessagin...
File components/webmessaging/public/interfaces/broadcast_channel.mojom (right):

https://codereview.chromium.org/2004643002/diff/380001/components/webmessagin...
components/webmessaging/public/interfaces/broadcast_channel.mojom:14: interface
BroadcastChannelService {
On 2016/06/24 at 18:14:51, Ken Rockot wrote:
> I should have caught this earlier, but please don't add a "Service" suffix to
new mojo interfaces. In general we'd like to keep some consistent terminology
moving forward and "service" is reserved for aggregated interface providers in
//services which might make sense to physically isolate from each other at
runtime (see //services/README.md).
> 
> Maybe BroadcastChannelProvider or BroadcastChannelConnector?

Done

[Ken Rockot(use gerrit already), 2016-06-24 19:08:14]

Oops, sorry about that. I assume I failed at keyboarding.

LGTM

[blundell, 2016-06-27 07:46:40]

Can you add the new component to //components/components.gyp and
//components/BUILD.gn? Also add per-file OWNERS to //components/OWNERS that
point to the new OWNERS file.

[Marijn Kruisselbrink, 2016-06-27 16:09:58]

On 2016/06/27 at 07:46:40, blundell wrote:
> Can you add the new component to //components/components.gyp and
//components/BUILD.gn? Also add per-file OWNERS to //components/OWNERS that
point to the new OWNERS file.

What do you mean? All the build rules for both gyp and gn are self-contained
within //components/webmessaging, so I'm not sure what you'd want me to add to
the toplevel //components build files? I suppose I could move the gyp rules to
//components/webmessaging.gypi instead, but not sure how that would be
beneficial.

[blundell, 2016-06-27 16:21:43]

On 2016/06/27 16:09:58, Marijn Kruisselbrink wrote:
> On 2016/06/27 at 07:46:40, blundell wrote:
> > Can you add the new component to //components/components.gyp and
> //components/BUILD.gn? Also add per-file OWNERS to //components/OWNERS that
> point to the new OWNERS file.
> 
> What do you mean? All the build rules for both gyp and gn are self-contained
> within //components/webmessaging, so I'm not sure what you'd want me to add to
> the toplevel //components build files? I suppose I could move the gyp rules to
> //components/webmessaging.gypi instead, but not sure how that would be
> beneficial.

Yeah, you're correct about GN; I had mistakenly recalled //components/BUILD.gn
as having deps on each of the components, but it doesn't.

Re: GYP, I would indeed prefer that you moved the file to
//components/webmessaging.gypi instead for consistency with the existing ~150
examples. I see that there are a handful of existing gypfiles for individual
components; I didn't review those and wouldn't have LG'd their addition. It's a
minor point, but it's also a minor change I think.

Also (and unrelated), we're allowing //third_party/WebKit to depend on
//components now?

[Marijn Kruisselbrink, 2016-06-27 16:28:12]

On 2016/06/27 at 16:21:43, blundell wrote:
> On 2016/06/27 16:09:58, Marijn Kruisselbrink wrote:
> > On 2016/06/27 at 07:46:40, blundell wrote:
> > > Can you add the new component to //components/components.gyp and
> > //components/BUILD.gn? Also add per-file OWNERS to //components/OWNERS that
> > point to the new OWNERS file.
> > 
> > What do you mean? All the build rules for both gyp and gn are self-contained
> > within //components/webmessaging, so I'm not sure what you'd want me to add
to
> > the toplevel //components build files? I suppose I could move the gyp rules
to
> > //components/webmessaging.gypi instead, but not sure how that would be
> > beneficial.
> 
> Yeah, you're correct about GN; I had mistakenly recalled //components/BUILD.gn
as having deps on each of the components, but it doesn't.
> 
> Re: GYP, I would indeed prefer that you moved the file to
//components/webmessaging.gypi instead for consistency with the existing ~150
examples. I see that there are a handful of existing gypfiles for individual
components; I didn't review those and wouldn't have LG'd their addition. It's a
minor point, but it's also a minor change I think.

Sure, I can make that change. Could you maybe explain the rationale behind it
and document that in //components/README? Naively I would assume keeping
everything of a component together would be preferred... Although hopefully soon
none of that matters anyway as we're moving more and more away from gyp...

> Also (and unrelated), we're allowing //third_party/WebKit to depend on
//components now?

Yes, code in //third_party/WebKit can depend on certain components in
//components (and can certainly depend on mojom interfaces declared in
//components).

[Marijn Kruisselbrink, 2016-06-27 16:53:11]

On 2016/06/27 at 16:28:12, Marijn Kruisselbrink wrote:
> On 2016/06/27 at 16:21:43, blundell wrote:
> > On 2016/06/27 16:09:58, Marijn Kruisselbrink wrote:
> > > On 2016/06/27 at 07:46:40, blundell wrote:
> > > > Can you add the new component to //components/components.gyp and
> > > //components/BUILD.gn? Also add per-file OWNERS to //components/OWNERS
that
> > > point to the new OWNERS file.
> > > 
> > > What do you mean? All the build rules for both gyp and gn are
self-contained
> > > within //components/webmessaging, so I'm not sure what you'd want me to
add to
> > > the toplevel //components build files? I suppose I could move the gyp
rules to
> > > //components/webmessaging.gypi instead, but not sure how that would be
> > > beneficial.
> > 
> > Yeah, you're correct about GN; I had mistakenly recalled
//components/BUILD.gn as having deps on each of the components, but it doesn't.
> > 
> > Re: GYP, I would indeed prefer that you moved the file to
//components/webmessaging.gypi instead for consistency with the existing ~150
examples. I see that there are a handful of existing gypfiles for individual
components; I didn't review those and wouldn't have LG'd their addition. It's a
minor point, but it's also a minor change I think.
> 
> Sure, I can make that change. Could you maybe explain the rationale behind it
and document that in //components/README? Naively I would assume keeping
everything of a component together would be preferred... Although hopefully soon
none of that matters anyway as we're moving more and more away from gyp...

Done

[blundell, 2016-06-27 17:05:04]

//components lgtm, thanks

On 2016/06/27 16:53:11, Marijn Kruisselbrink wrote:
> On 2016/06/27 at 16:28:12, Marijn Kruisselbrink wrote:
> > On 2016/06/27 at 16:21:43, blundell wrote:
> > > On 2016/06/27 16:09:58, Marijn Kruisselbrink wrote:
> > > > On 2016/06/27 at 07:46:40, blundell wrote:
> > > > > Can you add the new component to //components/components.gyp and
> > > > //components/BUILD.gn? Also add per-file OWNERS to //components/OWNERS
> that
> > > > point to the new OWNERS file.
> > > > 
> > > > What do you mean? All the build rules for both gyp and gn are
> self-contained
> > > > within //components/webmessaging, so I'm not sure what you'd want me to
> add to
> > > > the toplevel //components build files? I suppose I could move the gyp
> rules to
> > > > //components/webmessaging.gypi instead, but not sure how that would be
> > > > beneficial.
> > > 
> > > Yeah, you're correct about GN; I had mistakenly recalled
> //components/BUILD.gn as having deps on each of the components, but it
doesn't.
> > > 
> > > Re: GYP, I would indeed prefer that you moved the file to
> //components/webmessaging.gypi instead for consistency with the existing ~150
> examples. I see that there are a handful of existing gypfiles for individual
> components; I didn't review those and wouldn't have LG'd their addition. It's
a
> minor point, but it's also a minor change I think.
> > 
> > Sure, I can make that change. Could you maybe explain the rationale behind
it
> and document that in //components/README? Naively I would assume keeping
> everything of a component together would be preferred... Although hopefully
soon
> none of that matters anyway as we're moving more and more away from gyp...

This was the structure before I became a //components OWNER. I think that joi@
(no longer with the project) explained the original motivation to me once, but I
don't recall it any longer. My motivation at this point is simply consistency:
if *most* components are found via components/components.gyp:<foo>, it's extra
cognitive load on the project to have some that are instead found via
components/foo/foo.gyp:<foo>.

I also can't wait for the time when gyp is gone from Chromium.

> 
> Done

[Marijn Kruisselbrink, 2016-06-27 21:15:49]

dcheng: ping?

[dcheng, 2016-06-30 08:23:12]

mojom lgtm with a question and two nits

https://codereview.chromium.org/2004643002/diff/440001/components/webmessagin...
File components/webmessaging/public/interfaces/broadcast_channel.mojom (right):

https://codereview.chromium.org/2004643002/diff/440001/components/webmessagin...
components/webmessaging/public/interfaces/broadcast_channel.mojom:9: interface
BroadcastChannelClient {
Nit: can we add some sort of interface-level comment, e.g. the purpose of this
interface? Some sort of documentation about where the service is hosted would be
nice too (since this interface is implemented in both the trusted browser
process and the untrusted renderers)

https://codereview.chromium.org/2004643002/diff/440001/components/webmessagin...
components/webmessaging/public/interfaces/broadcast_channel.mojom:14: interface
BroadcastChannelProvider {
Is there expected to be one of these in the browser per renderer? Or some other
model? I guess it's exposed by StoragePartition, but I don't know how that
concretely maps to the process model. Sorry if this is a dumb question =)

https://codereview.chromium.org/2004643002/diff/440001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager_unittest.cc
(right):

https://codereview.chromium.org/2004643002/diff/440001/content/browser/backgr...
content/browser/background_sync/background_sync_manager_unittest.cc:141:
nullptr, nullptr, nullptr));
Unrelated, but this could probably use benefit greatly from a Params
constructor. That should not block this CL though.

https://codereview.chromium.org/2004643002/diff/440001/content/browser/storag...
File content/browser/storage_partition_impl.h (right):

https://codereview.chromium.org/2004643002/diff/440001/content/browser/storag...
content/browser/storage_partition_impl.h:179:
webmessaging::BroadcastChannelProvider* broadcast_channel_provider);
Nit: please make this a scoped_refptr, since this does take ownership. That's
not the current pattern but we have to start somewhere =)

[Marijn Kruisselbrink, 2016-06-30 17:21:57]

https://codereview.chromium.org/2004643002/diff/440001/components/webmessagin...
File components/webmessaging/public/interfaces/broadcast_channel.mojom (right):

https://codereview.chromium.org/2004643002/diff/440001/components/webmessagin...
components/webmessaging/public/interfaces/broadcast_channel.mojom:9: interface
BroadcastChannelClient {
On 2016/06/30 at 08:23:11, dcheng wrote:
> Nit: can we add some sort of interface-level comment, e.g. the purpose of this
interface? Some sort of documentation about where the service is hosted would be
nice too (since this interface is implemented in both the trusted browser
process and the untrusted renderers)

Added some explicit comments to this interface.

https://codereview.chromium.org/2004643002/diff/440001/components/webmessagin...
components/webmessaging/public/interfaces/broadcast_channel.mojom:14: interface
BroadcastChannelProvider {
On 2016/06/30 at 08:23:12, dcheng wrote:
> Is there expected to be one of these in the browser per renderer? Or some
other model? I guess it's exposed by StoragePartition, but I don't know how that
concretely maps to the process model. Sorry if this is a dumb question =)

A content::BrowserContext (chrome::Profile) consists of multiple
StoragePartition, and each renderer process lives inside a particular storage
partition (this is for example how chrome apps have separate cookie jars/storage
from regular websites).

So the way that's currently implemented is that there is one
BroadcastChannelProvider instance in the browser process for each
StoragePartition, and all connections from all renderers in that storage
partition end up binding to the same Provider implementation.

Added some more comments to explain this a bit more.

https://codereview.chromium.org/2004643002/diff/440001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager_unittest.cc
(right):

https://codereview.chromium.org/2004643002/diff/440001/content/browser/backgr...
content/browser/background_sync/background_sync_manager_unittest.cc:141:
nullptr, nullptr, nullptr));
On 2016/06/30 at 08:23:12, dcheng wrote:
> Unrelated, but this could probably use benefit greatly from a Params
constructor. That should not block this CL though.

Yeah, the way StoragePartitions are currently created is rather annoying... I've
also wondered if StoragePartition could maybe be a bit more flexible then it
currently is with something like SupportsUserData, but haven't really thought it
through...

https://codereview.chromium.org/2004643002/diff/440001/content/browser/storag...
File content/browser/storage_partition_impl.h (right):

https://codereview.chromium.org/2004643002/diff/440001/content/browser/storag...
content/browser/storage_partition_impl.h:179:
webmessaging::BroadcastChannelProvider* broadcast_channel_provider);
On 2016/06/30 at 08:23:12, dcheng wrote:
> Nit: please make this a scoped_refptr, since this does take ownership. That's
not the current pattern but we have to start somewhere =)

Done

[Marijn Kruisselbrink, 2016-06-30 17:50:54]

The CQ bit was checked by mek@chromium.org

[Marijn Kruisselbrink, 2016-06-30 17:50:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, kinuko@chromium.org, rockot@chromium.org,
blundell@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2004643002/#ps460001
(title: "address dcheng's comments")

[commit-bot: I haz the power, 2016-06-30 17:51:17]

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-06-30 19:04:03]

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

[commit-bot: I haz the power, 2016-06-30 19:04:07]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Marijn Kruisselbrink, 2016-06-30 19:05:18]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2016-06-30 19:06:33]

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-06-30 21:29:40]

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

[commit-bot: I haz the power, 2016-06-30 21:29:43]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Marijn Kruisselbrink, 2016-06-30 21:30:15]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2016-06-30 21:30:58]

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-06-30 23:19:40]

Committed patchset #22 (id:460001)

[commit-bot: I haz the power, 2016-06-30 23:19:49]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-30 23:23:23]

Description was changed from

==========
Implement BroadcastChannel as specified in
https://html.spec.whatwg.org/multipage/comms.html#broadcastchannel

BUG=161070
==========

to

==========
Implement BroadcastChannel as specified in
https://html.spec.whatwg.org/multipage/comms.html#broadcastchannel

BUG=161070

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

[commit-bot: I haz the power, 2016-06-30 23:23:26]

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

[jam, 2016-07-19 17:12:55]

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

[jam, 2016-07-19 17:12:57]

Hi I just saw this.

I don't think this should go into a component yet since it's not a generic
service, but one that's only used by the web platform. We're trying to avoid
creating new components for stuff that's only used by content/browser and blink.
components/ or services/ should be generic code that'll have multiple users.

In this specific case, the mojom should be in
third_party/WebKit/public/platform/modules/broadcastchannel. content/browser
would include the generated mojo header and
components/webmessaging/broadcast_channel_provider.* move to
content/browser/webmessaging/ (with its own owners file).

In the future, as we decompose content/browser, we can depend on shared generic
services such as  a pub/sub component to implement broadcast channel. That way
the code that's in content/browser now can move completely into blink. See more
information about this plan at
https://docs.google.com/document/d/15I7sQyQo6zsqXVNAlVd520tdGaS8FCicZHrN0yRu-...
and
https://docs.google.com/spreadsheets/d/15wDDlteNYDIv0QlG2OKofAOssBtwb225076ad...
(last row). Since we're not yet at this stage, that's why I suggest moving the
browser side code to content/browser like other web platform features, until
we're ready to decompose that directory using generic services.

[blundell, 2016-07-20 06:59:46]

On 2016/07/19 17:12:57, jam wrote:
> Hi I just saw this.
> 
> I don't think this should go into a component yet since it's not a generic
> service, but one that's only used by the web platform. We're trying to avoid
> creating new components for stuff that's only used by content/browser and
blink.
> components/ or services/ should be generic code that'll have multiple users.
> 
> In this specific case, the mojom should be in
> third_party/WebKit/public/platform/modules/broadcastchannel. content/browser
> would include the generated mojo header and
> components/webmessaging/broadcast_channel_provider.* move to
> content/browser/webmessaging/ (with its own owners file).
> 
> In the future, as we decompose content/browser, we can depend on shared
generic
> services such as  a pub/sub component to implement broadcast channel. That way
> the code that's in content/browser now can move completely into blink. See
more
> information about this plan at
>
https://docs.google.com/document/d/15I7sQyQo6zsqXVNAlVd520tdGaS8FCicZHrN0yRu-...
> and
>
https://docs.google.com/spreadsheets/d/15wDDlteNYDIv0QlG2OKofAOssBtwb225076ad...
> (last row). Since we're not yet at this stage, that's why I suggest moving the
> browser side code to content/browser like other web platform features, until
> we're ready to decompose that directory using generic services.

That SGTM. That way we would avoid adding a dependency from either Blink or
//content to //components.

[Marijn Kruisselbrink, 2016-07-20 21:09:59]

On 2016/07/20 at 06:59:46, blundell wrote:
> On 2016/07/19 17:12:57, jam wrote:
> > Hi I just saw this.
> > 
> > I don't think this should go into a component yet since it's not a generic
> > service, but one that's only used by the web platform. We're trying to avoid
> > creating new components for stuff that's only used by content/browser and
blink.
> > components/ or services/ should be generic code that'll have multiple users.
> > 
> > In this specific case, the mojom should be in
> > third_party/WebKit/public/platform/modules/broadcastchannel. content/browser
> > would include the generated mojo header and
> > components/webmessaging/broadcast_channel_provider.* move to
> > content/browser/webmessaging/ (with its own owners file).
> > 
> > In the future, as we decompose content/browser, we can depend on shared
generic
> > services such as  a pub/sub component to implement broadcast channel. That
way
> > the code that's in content/browser now can move completely into blink. See
more
> > information about this plan at
> >
https://docs.google.com/document/d/15I7sQyQo6zsqXVNAlVd520tdGaS8FCicZHrN0yRu-...
> > and
> >
https://docs.google.com/spreadsheets/d/15wDDlteNYDIv0QlG2OKofAOssBtwb225076ad...
> > (last row). Since we're not yet at this stage, that's why I suggest moving
the
> > browser side code to content/browser like other web platform features, until
> > we're ready to decompose that directory using generic services.
> 
> That SGTM. That way we would avoid adding a dependency from either Blink or
//content to //components.

Sure, I've given up on trying to understand what the content modularization is
trying to do exactly in practice, so feel free to move code around to wherever
it makes the most sense in whatever the current plan is.

[jam, 2016-07-21 00:44:07]

On 2016/07/20 21:09:59, Marijn Kruisselbrink wrote:
> On 2016/07/20 at 06:59:46, blundell wrote:
> > On 2016/07/19 17:12:57, jam wrote:
> > > Hi I just saw this.
> > > 
> > > I don't think this should go into a component yet since it's not a generic
> > > service, but one that's only used by the web platform. We're trying to
avoid
> > > creating new components for stuff that's only used by content/browser and
> blink.
> > > components/ or services/ should be generic code that'll have multiple
users.
> > > 
> > > In this specific case, the mojom should be in
> > > third_party/WebKit/public/platform/modules/broadcastchannel.
content/browser
> > > would include the generated mojo header and
> > > components/webmessaging/broadcast_channel_provider.* move to
> > > content/browser/webmessaging/ (with its own owners file).
> > > 
> > > In the future, as we decompose content/browser, we can depend on shared
> generic
> > > services such as  a pub/sub component to implement broadcast channel. That
> way
> > > the code that's in content/browser now can move completely into blink. See
> more
> > > information about this plan at
> > >
>
https://docs.google.com/document/d/15I7sQyQo6zsqXVNAlVd520tdGaS8FCicZHrN0yRu-...
> > > and
> > >
>
https://docs.google.com/spreadsheets/d/15wDDlteNYDIv0QlG2OKofAOssBtwb225076ad...
> > > (last row). Since we're not yet at this stage, that's why I suggest moving
> the
> > > browser side code to content/browser like other web platform features,
until
> > > we're ready to decompose that directory using generic services.
> > 
> > That SGTM. That way we would avoid adding a dependency from either Blink or
> //content to //components.
> 
> Sure, I've given up on trying to understand what the content modularization is
> trying to do exactly in practice

Sorry to hear that, we're going to give a presentation that should hopefully
make things clearer.

In the meantime, until we have these generic services, the general rule is that
the renderer-side code should be in blink, and the browser side code in
content/browser.

> so feel free to move code around to wherever
> it makes the most sense in whatever the current plan is.

I've sent out https://codereview.chromium.org/2158913006/