Expose navigator.connect client side API in blink.

Source/modules/navigatorconnect/NavigatorConnect.cpp

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "config.h"
+#include "modules/navigatorconnect/NavigatorConnect.h"
+
+#include "bindings/core/v8/ScriptPromise.h"
+#include "bindings/core/v8/ScriptPromiseResolver.h"
+#include "core/dom/DOMException.h"
+#include "core/dom/ExceptionCode.h"
+#include "core/dom/MessageChannel.h"
+#include "core/dom/MessagePort.h"
+#include "public/platform/Platform.h"
+#include "public/platform/WebCallbacks.h"
+#include "public/platform/WebNavigatorConnectProvider.h"
+
+namespace blink {
+
+namespace {
+
+class ConnectCallbacks : public WebCallbacks<void, void> {
+public:
+    ConnectCallbacks(PassRefPtr<ScriptPromiseResolver> resolver, PassRefPtrWillBeRawPtr<MessagePort> port)
+        : m_resolver(resolver), m_port(port)
+    {
+        ASSERT(m_resolver);
+    }
+    ~ConnectCallbacks() override { }
+
+    void onSuccess() override
+    {
+        if (!m_resolver->executionContext() || m_resolver->executionContext()->activeDOMObjectsAreStopped()) {
+            return;
+        }
+        m_resolver->resolve(m_port);

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

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?

[Marijn Kruisselbrink, 2014/12/05 09:23:42]

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.

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

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.
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.
 
Unfortunately disentangle is undocumented and non-obvious what that means w.r.t.
object lifetimes.
Sounds fine in this write-up, but it's not clear in the code/comments.
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.

+    }
+
+    void onError() override
+    {
+        if (!m_resolver->executionContext() || m_resolver->executionContext()->activeDOMObjectsAreStopped()) {
+            return;
+        }
+        m_resolver->reject(DOMException::create(AbortError));
+    }
+
+private:
+    RefPtr<ScriptPromiseResolver> m_resolver;
+    RefPtrWillBeMember<MessagePort> m_port;
+    WTF_MAKE_NONCOPYABLE(ConnectCallbacks);
+};
+
+} // namespace
+
+ScriptPromise NavigatorConnect::connect(ScriptState* scriptState, Navigator&, const String& url)
+{
+    WebNavigatorConnectProvider* provider = Platform::current()->navigatorConnectProvider();
+    if (!provider)
+        return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(NotSupportedError));
+
+    RefPtr<ScriptPromiseResolver> resolver = ScriptPromiseResolver::create(scriptState);
+    ScriptPromise promise = resolver->promise();
+    RefPtrWillBeRawPtr<MessageChannel> channel(MessageChannel::create(scriptState->executionContext()));

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

Object lifetime is a little obscure here, some code or comments making the
channel lifetime ownership really clear would help.

[Marijn Kruisselbrink, 2014/12/05 09:23:42]

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.

[scheib, 2014/12/05 17:39:34]

Acknowledged.

+    OwnPtr<WebMessagePortChannel> webchannel = channel->port2()->disentangle();
+    provider->connect(
+        scriptState->executionContext()->completeURL(url),
+        webchannel.leakPtr(),
+        new ConnectCallbacks(resolver, channel->port1()));
+    return promise;
+}
+
+
+} // namespace blink