SharedWorker: Simplify connection sequence

content/renderer/shared_worker/websharedworker_proxy.cc

Index: content/renderer/shared_worker/websharedworker_proxy.cc
diff --git a/content/renderer/shared_worker/websharedworker_proxy.cc b/content/renderer/shared_worker/websharedworker_proxy.cc
index 15d534d9deeee2941383d1881af6d82c48bd1827..338ed281726d1f7dfb1796b4b11b99426f7dbe1b 100644
--- a/content/renderer/shared_worker/websharedworker_proxy.cc
+++ b/content/renderer/shared_worker/websharedworker_proxy.cc
@@ -6,36 +6,44 @@
 
 #include <stddef.h>
 
+#include "content/child/child_thread_impl.h"
 #include "content/child/webmessageportchannel_impl.h"
 #include "content/common/view_messages.h"
 #include "content/common/worker_messages.h"
 #include "ipc/message_router.h"
+#include "third_party/WebKit/public/web/WebSharedWorkerConnectListener.h"
 
 namespace content {
 
-WebSharedWorkerProxy::WebSharedWorkerProxy(IPC::MessageRouter* router,
-                                           int route_id)
-    : route_id_(route_id),
-      router_(router),
+WebSharedWorkerProxy::WebSharedWorkerProxy(
+    std::unique_ptr<blink::WebSharedWorkerConnectListener> listener)
+    : route_id_(MSG_ROUTING_NONE),
+      router_(ChildThreadImpl::current()->GetRouter()),
       message_port_id_(MSG_ROUTING_NONE),
-      connect_listener_(nullptr) {
-  DCHECK_NE(MSG_ROUTING_NONE, route_id_);
-  router_->AddRoute(route_id_, this);
-}
+      listener_(std::move(listener)) {}
 
 WebSharedWorkerProxy::~WebSharedWorkerProxy() {
+  DCHECK_NE(MSG_ROUTING_NONE, route_id_);
   router_->RemoveRoute(route_id_);
 }
 
-void WebSharedWorkerProxy::connect(blink::WebMessagePortChannel* channel,

[nhiroki, 2017/01/13 05:28:53]

Question: Who owns this |channel|? AFAICS, this is passed from
SharedWorkerConnector and no one manages it.

[kinuko, 2017/01/13 07:31:34]

The comment says:
"The object owns itself and is signalled that its not needed anymore with the
destroy() call."

but looks like we may not be calling destroy()

[horo, 2017/01/16 07:11:03]

Humm...
It looks leaking after this change.
https://codereview.chromium.org/2043583002/diff/1/third_party/WebKit/Source/w...

[nhiroki, 2017/01/18 05:42:37]

Thank you. As horo's another comment, this seems to be destructed on
WebMessagePortChannelImpl::OnMessagesQueued().

Btw, this class is managed as a unique ptr in blink but as a ref ptr in
chromium. This seems to be confusing. It'd be nice if we could simplify such
complex memory management.

-                                   ConnectListener* listener) {
+void WebSharedWorkerProxy::connect(ViewHostMsg_CreateWorker_Params params,
+                                   blink::WebMessagePortChannel* channel) {
+  // Send synchronous IPC to get |route_id|.
+  // TODO(nhiroki): Stop using synchronous IPC (https://crbug.com/679654).
+  ViewHostMsg_CreateWorker_Reply reply;
+  router_->Send(new ViewHostMsg_CreateWorker(params, &reply));
+  route_id_ = reply.route_id;
+  router_->AddRoute(route_id_, this);
+  listener_->workerCreated(reply.error);
+
   DCHECK_EQ(MSG_ROUTING_NONE, message_port_id_);
   WebMessagePortChannelImpl* webchannel =
         static_cast<WebMessagePortChannelImpl*>(channel);
   message_port_id_ = webchannel->message_port_id();
   DCHECK_NE(MSG_ROUTING_NONE, message_port_id_);
   webchannel->QueueMessages();
-  connect_listener_ = listener;
+  listener_->connecting();

[kinuko, 2017/01/13 07:31:34]

Why do we need this method separately from workerCreated?

[nhiroki, 2017/01/18 05:42:37]

Yeah, this is not necessary. Removed.

   // An actual connection request will be issued on OnWorkerCreated().
 }
 
@@ -53,26 +61,19 @@ bool WebSharedWorkerProxy::OnMessageReceived(const IPC::Message& message) {
 }
 
 void WebSharedWorkerProxy::OnWorkerCreated() {
-  // connect() should be called before.
-  DCHECK_NE(MSG_ROUTING_NONE, message_port_id_);
-
   // The worker is created - now send off the connection request.
   router_->Send(
       new ViewHostMsg_ConnectToWorker(route_id_, message_port_id_));
 }
 
 void WebSharedWorkerProxy::OnWorkerScriptLoadFailed() {
-  if (connect_listener_) {
-    // This can result in this object being freed.
-    connect_listener_->scriptLoadFailed();
-  }
+  listener_->scriptLoadFailed();
+  delete this;

[kinuko, 2017/01/13 07:31:34]

would have been nice if we could remove this... but we're replacing this class
soon, so maybe ok

[nhiroki, 2017/01/18 05:42:37]

Acknowledged.

 }
 
 void WebSharedWorkerProxy::OnWorkerConnected() {
-  if (connect_listener_) {
-    // This can result in this object being freed.
-    connect_listener_->connected();
-  }
+  listener_->connected();
+  delete this;
 }
 
 }  // namespace content