Expose navigator.connect client side API in blink.

Expose navigator.connect client side API in blink.

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

[Marijn Kruisselbrink, 2014-12-04 14:01:47]

Patchset #2 (id:20001) has been deleted

[Marijn Kruisselbrink, 2014-12-04 14:01:52]

Patchset #1 (id:1) has been deleted

[Marijn Kruisselbrink, 2014-12-04 14:04:04]

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

[scheib, 2014-12-05 00:16:40]

I'm a bit stumped so far on lifetime of channel:

https://codereview.chromium.org/782463002/diff/40001/Source/modules/navigator...
File Source/modules/navigatorconnect/NavigatorConnect.cpp (right):

https://codereview.chromium.org/782463002/diff/40001/Source/modules/navigator...
Source/modules/navigatorconnect/NavigatorConnect.cpp:36:
m_resolver->resolve(m_port);
The lifetime of the port, and I assume then the channel, is passed to script and
that is the only remaining reference at this point. However, the connect
provider has a reference to the channel as well. Should there be more explicit
lifetime control of the channel in the WebNavigatorConnectProvider interface?
What if the platform needs to keep the channel instance alive?

https://codereview.chromium.org/782463002/diff/40001/Source/modules/navigator...
Source/modules/navigatorconnect/NavigatorConnect.cpp:63:
RefPtrWillBeRawPtr<MessageChannel>
channel(MessageChannel::create(scriptState->executionContext()));
Object lifetime is a little obscure here, some code or comments making the
channel lifetime ownership really clear would help.

https://codereview.chromium.org/782463002/diff/40001/public/platform/WebNavig...
File public/platform/WebNavigatorConnectProvider.h (right):

https://codereview.chromium.org/782463002/diff/40001/public/platform/WebNavig...
public/platform/WebNavigatorConnectProvider.h:22: virtual void connect(const
WebURL&, WebMessagePortChannel*, WebCallbacks<void, void>*) { }
Probably leave this pure virtual. The platform default impl will return NULL for
provider until implemented, but if an implementation exists it should probably
compile fail unless it overrides connect.

[Marijn Kruisselbrink, 2014-12-05 09:23:43]

https://codereview.chromium.org/782463002/diff/40001/Source/modules/navigator...
File Source/modules/navigatorconnect/NavigatorConnect.cpp (right):

https://codereview.chromium.org/782463002/diff/40001/Source/modules/navigator...
Source/modules/navigatorconnect/NavigatorConnect.cpp:36:
m_resolver->resolve(m_port);
On 2014/12/05 00:16:40, scheib wrote:
> The lifetime of the port, and I assume then the channel, is passed to script
and
> that is the only remaining reference at this point. However, the connect
> provider has a reference to the channel as well. Should there be more explicit
> lifetime control of the channel in the WebNavigatorConnectProvider interface?
> What if the platform needs to keep the channel instance alive?
What do you mean with "channel instance"? Is it the blink::MessageChannel that
gets created in the connect implementation? That really isn't any more than a
pair of ports. There isn't really any representation of the channel itself other
than the two ports referencing eachother.
One of the two ports remains in this renderer process, with a reference in
ConnectCallbacks::m_port while the request is pending, and that reference is
then passed off to javascript code if the connections is made (and then it's up
to javascript code to keep the reference alive as long as necessary).
The other port of the pair is different, there we immediately disentangle the
blink port from the content layer port (confusingly called
WebMessagePortChannel). At that point we don't care about the blink::MessagePort
formerly associated with that port anymore, so when the channel goes out of
scope that port will also be released.
The lifetime management of the content::WebMessagePortChannelImpl then is indeed
a bit hidden. This class is refcounted, but always stored and passed around as
raw pointers. For its lifetime, the constructor increases the reference count,
and calling QueueMessages on it (in the content layer code in preparation of
sending the port to the browser process) will eventually cause the reference
count to be decreased and the port to be freed (in the renderer process).
In the browser process there then is a map of port-id to port-id to actually
keep track of the channel, but there isn't really a channel instance as such.

https://codereview.chromium.org/782463002/diff/40001/Source/modules/navigator...
Source/modules/navigatorconnect/NavigatorConnect.cpp:63:
RefPtrWillBeRawPtr<MessageChannel>
channel(MessageChannel::create(scriptState->executionContext()));
On 2014/12/05 00:16:40, scheib wrote:
> Object lifetime is a little obscure here, some code or comments making the
> channel lifetime ownership really clear would help.
MessageChannel really is just a std::pair<RefPtr<MessagePort>,
RefPtr<MessagePort>>, so its lifetime really doesn't matter so much. I tried to
add a short comment explaining some of these lifetime matters, although this is
probably not quite the right place to explain all of the complicated lifetime
management surrounding MessagePort and WebMessagePortChannelImpl.

https://codereview.chromium.org/782463002/diff/40001/public/platform/WebNavig...
File public/platform/WebNavigatorConnectProvider.h (right):

https://codereview.chromium.org/782463002/diff/40001/public/platform/WebNavig...
public/platform/WebNavigatorConnectProvider.h:22: virtual void connect(const
WebURL&, WebMessagePortChannel*, WebCallbacks<void, void>*) { }
On 2014/12/05 00:16:40, scheib wrote:
> Probably leave this pure virtual. The platform default impl will return NULL
for
> provider until implemented, but if an implementation exists it should probably
> compile fail unless it overrides connect.
Ideally, yes. But I've been told that while blink and chromium are still
separate repositories, it's better to avoid marking methods as pure virtual (as
well avoid marking implementations of these as override) to make it easier to
change things. Pure virtual methods (and marking methods as override) makes
changes to these interface even harder than they already are in the split repo
world.

[scheib, 2014-12-05 17:39:35]

lgtm with comment suggestions.

https://codereview.chromium.org/782463002/diff/40001/Source/modules/navigator...
File Source/modules/navigatorconnect/NavigatorConnect.cpp (right):

https://codereview.chromium.org/782463002/diff/40001/Source/modules/navigator...
Source/modules/navigatorconnect/NavigatorConnect.cpp:36:
m_resolver->resolve(m_port);
On 2014/12/05 09:23:42, Marijn Kruisselbrink wrote:
> On 2014/12/05 00:16:40, scheib wrote:
> > The lifetime of the port, and I assume then the channel, is passed to script
> and
> > that is the only remaining reference at this point. However, the connect
> > provider has a reference to the channel as well. Should there be more
explicit
> > lifetime control of the channel in the WebNavigatorConnectProvider
interface?
> > What if the platform needs to keep the channel instance alive?
> What do you mean with "channel instance"? Is it the blink::MessageChannel that
> gets created in the connect implementation? That really isn't any more than a
> pair of ports. There isn't really any representation of the channel itself
other
> than the two ports referencing eachother.

I'm not familiar with details of the MessagePort|Channel classes, and they're
frustratingly under-documented. Yes, I was thinking of the MessageChannel object
instance, but if it's not needed to keep the ports functioning, then OK.

> One of the two ports remains in this renderer process, with a reference in
> ConnectCallbacks::m_port while the request is pending, and that reference is
> then passed off to javascript code if the connections is made (and then it's
up
> to javascript code to keep the reference alive as long as necessary).

If the classes are designed such that JS releasing the port reference is safe
while the other port lives, then that's fine. I think the MessagePort class
comments could make that much clearer. It's not obvious.
 
> The other port of the pair is different, there we immediately disentangle

Unfortunately disentangle is undocumented and non-obvious what that means w.r.t.
object lifetimes.

> the
> blink port from the content layer port (confusingly called
> WebMessagePortChannel). At that point we don't care about the
blink::MessagePort
> formerly associated with that port anymore, so when the channel goes out of
> scope that port will also be released.

Sounds fine in this write-up, but it's not clear in the code/comments.

> The lifetime management of the content::WebMessagePortChannelImpl then is
indeed
> a bit hidden. This class is refcounted, but always stored and passed around as
> raw pointers. For its lifetime, the constructor increases the reference count,
> and calling QueueMessages on it (in the content layer code in preparation of
> sending the port to the browser process) will eventually cause the reference
> count to be decreased and the port to be freed (in the renderer process).
> In the browser process there then is a map of port-id to port-id to actually
> keep track of the channel, but there isn't really a channel instance as such.

I'm 80% there without reading through the implementation of these objects. They
clearly could use better documentation, but that's out of scope for this patch.
In this patch, in this context, I think explaining that WebMessagePortChannel
ownership is transferred away is enough.

https://codereview.chromium.org/782463002/diff/40001/Source/modules/navigator...
Source/modules/navigatorconnect/NavigatorConnect.cpp:63:
RefPtrWillBeRawPtr<MessageChannel>
channel(MessageChannel::create(scriptState->executionContext()));
On 2014/12/05 09:23:42, Marijn Kruisselbrink wrote:
> On 2014/12/05 00:16:40, scheib wrote:
> > Object lifetime is a little obscure here, some code or comments making the
> > channel lifetime ownership really clear would help.
> MessageChannel really is just a std::pair<RefPtr<MessagePort>,
> RefPtr<MessagePort>>, so its lifetime really doesn't matter so much. I tried
to
> add a short comment explaining some of these lifetime matters, although this
is
> probably not quite the right place to explain all of the complicated lifetime
> management surrounding MessagePort and WebMessagePortChannelImpl.

Acknowledged.

https://codereview.chromium.org/782463002/diff/40001/public/platform/WebNavig...
File public/platform/WebNavigatorConnectProvider.h (right):

https://codereview.chromium.org/782463002/diff/40001/public/platform/WebNavig...
public/platform/WebNavigatorConnectProvider.h:22: virtual void connect(const
WebURL&, WebMessagePortChannel*, WebCallbacks<void, void>*) { }
On 2014/12/05 09:23:43, Marijn Kruisselbrink wrote:
> On 2014/12/05 00:16:40, scheib wrote:
> > Probably leave this pure virtual. The platform default impl will return NULL
> for
> > provider until implemented, but if an implementation exists it should
probably
> > compile fail unless it overrides connect.
> Ideally, yes. But I've been told that while blink and chromium are still
> separate repositories, it's better to avoid marking methods as pure virtual
(as
> well avoid marking implementations of these as override) to make it easier to
> change things. Pure virtual methods (and marking methods as override) makes
> changes to these interface even harder than they already are in the split repo
> world.

OK with me, check with the platform OWNER I suppose. ;) I had pushback on recent
patch.

https://codereview.chromium.org/782463002/diff/60001/Source/modules/navigator...
File Source/modules/navigatorconnect/NavigatorConnect.cpp (right):

https://codereview.chromium.org/782463002/diff/60001/Source/modules/navigator...
Source/modules/navigatorconnect/NavigatorConnect.cpp:67: // method, which is
okay as it's only the ports within that matter.
It is a bit indirect that ownership of port2 is transfered out. The leakPtr is a
hint, but the ::connect signature doesn't mention ownership transfer.
WebMessagePortChannel class comments sort of deal with this as "object owns
itself", but actually I believe this code is relying on content or later owners
to call destroy manually.

This comment helps, it may be a bit more readable if it were in an enumerated
list form:
Ownership:
port1 ...
port2 ...
channel ...

https://codereview.chromium.org/782463002/diff/60001/public/platform/WebNavig...
File public/platform/WebNavigatorConnectProvider.h (right):

https://codereview.chromium.org/782463002/diff/60001/public/platform/WebNavig...
public/platform/WebNavigatorConnectProvider.h:22: virtual void connect(const
WebURL&, WebMessagePortChannel*, WebCallbacks<void, void>*) { }
Ownership of WebMessagePortChannel is also transferred, with the special case of
requiring WebMessagePortChannel::destroy instead of delete.

[Marijn Kruisselbrink, 2014-12-08 18:50:36]

https://codereview.chromium.org/782463002/diff/60001/Source/modules/navigator...
File Source/modules/navigatorconnect/NavigatorConnect.cpp (right):

https://codereview.chromium.org/782463002/diff/60001/Source/modules/navigator...
Source/modules/navigatorconnect/NavigatorConnect.cpp:67: // method, which is
okay as it's only the ports within that matter.
On 2014/12/05 17:39:35, scheib wrote:
> It is a bit indirect that ownership of port2 is transfered out. The leakPtr is
a
> hint, but the ::connect signature doesn't mention ownership transfer.
> WebMessagePortChannel class comments sort of deal with this as "object owns
> itself", but actually I believe this code is relying on content or later
owners
> to call destroy manually.
> 
> This comment helps, it may be a bit more readable if it were in an enumerated
> list form:
> Ownership:
> port1 ...
> port2 ...
> channel ...

Done.

https://codereview.chromium.org/782463002/diff/60001/public/platform/WebNavig...
File public/platform/WebNavigatorConnectProvider.h (right):

https://codereview.chromium.org/782463002/diff/60001/public/platform/WebNavig...
public/platform/WebNavigatorConnectProvider.h:22: virtual void connect(const
WebURL&, WebMessagePortChannel*, WebCallbacks<void, void>*) { }
On 2014/12/05 17:39:35, scheib wrote:
> Ownership of WebMessagePortChannel is also transferred, with the special case
of
> requiring WebMessagePortChannel::destroy instead of delete. 

Done.

[Marijn Kruisselbrink, 2014-12-08 18:51:25]

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

[Marijn Kruisselbrink, 2014-12-08 18:51:25]

+dglazkov for OWNERS review

[dglazkov, 2014-12-09 22:56:54]

lgtm

[Marijn Kruisselbrink, 2014-12-09 23:06:37]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2014-12-09 23:07:45]

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

[commit-bot: I haz the power, 2014-12-09 23:22:36]

Committed patchset #4 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186813