[DevTools]: Require explicit connection

third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js

Index: third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js
diff --git a/third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js b/third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js
index 01e473d910df32197230e1dd63bd464670543630..faa260e985a782398f686c519bd34fb9f91b1437 100644
--- a/third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js
+++ b/third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js
@@ -18,6 +18,8 @@ WebInspector.SubTargetsManager = function(target)
     this._attachedTargets = new Map();
     /** @type {!Map<string, !WebInspector.SubTargetConnection>} */
     this._connections = new Map();
+    /** @type {!Map<string, !WebInspector.PendingConnection>} */
+    this._pendingConnections = new Map();
 
     this._agent.setAutoAttach(true /* autoAttach */, true /* waitForDebuggerOnStart */);
     if (Runtime.experiments.isEnabled("autoAttachToCrossProcessSubframes"))
@@ -33,8 +35,8 @@ WebInspector.SubTargetsManager = function(target)
 
 /** @enum {symbol} */
 WebInspector.SubTargetsManager.Events = {
-    SubTargetAdded: Symbol("SubTargetAdded"),
-    SubTargetRemoved: Symbol("SubTargetRemoved"),
+    SubTargetConnectionCreated: Symbol("SubTargetCreated"),

[dgozman, 2016/10/28 18:31:35]

PendingConnectionCreated

[eostroukhov, 2016/11/01 20:28:14]

I removed the word "connection" - it is too ambiguous in our context.

+    SubTargetConnectionDestroyed: Symbol("SubTargetDestroyed"),
 };
 
 WebInspector.SubTargetsManager._InfoSymbol = Symbol("SubTargetInfo");
@@ -73,6 +75,8 @@ WebInspector.SubTargetsManager.prototype = {
             this._agent.detachFromTarget(connection._targetId);
             connection._onDisconnect.call(null, "disposed");
         }
+        for (var pending of this._pendingConnections.values())
+            pending.disconnect();
         this._connections.clear();
         this._attachedTargets.clear();
     },
@@ -160,17 +164,18 @@ WebInspector.SubTargetsManager.prototype = {
             var parsedURL = targetInfo.url.asParsedURL();
             targetName = parsedURL ? parsedURL.lastPathComponentWithFragment() : "#" + (++this._lastAnonymousTargetId);
         }
-        var target = WebInspector.targetManager.createTarget(targetName, this._capabilitiesForType(targetInfo.type), this._createConnection.bind(this, targetInfo.id), this.target());
+        var target = WebInspector.targetManager.createTarget(targetName, this._capabilitiesForType(targetInfo.type), this._createConnection.bind(this, targetInfo.id), this.target(), true);
         target[WebInspector.SubTargetsManager._InfoSymbol] = targetInfo;
         this._attachedTargets.set(targetInfo.id, target);
-
+        WebInspector.targetManager.addTarget(target);
         // Only pause new worker if debugging SW - we are going through the pause on start checkbox.
         var mainIsServiceWorker = !this.target().parentTarget() && this.target().hasTargetCapability() && !this.target().hasBrowserCapability();
         if (mainIsServiceWorker && waitingForDebugger)
             target.debuggerAgent().pause();
         target.runtimeAgent().runIfWaitingForDebugger();
-
-        this.dispatchEventToListeners(WebInspector.SubTargetsManager.Events.SubTargetAdded, target);
+        var connection = this._pendingConnections.get(targetInfo.id);
+        if (connection)
+            connection.notifyAttached(target);
     },
 
     /**
@@ -192,7 +197,6 @@ WebInspector.SubTargetsManager.prototype = {
     {
         var target = this._attachedTargets.get(targetId);
         this._attachedTargets.delete(targetId);
-        this.dispatchEventToListeners(WebInspector.SubTargetsManager.Events.SubTargetRemoved, target);
         var connection = this._connections.get(targetId);
         if (connection)
             connection._onDisconnect.call(null, "target terminated");
@@ -212,12 +216,19 @@ WebInspector.SubTargetsManager.prototype = {
 
     /**
      * @param {!WebInspector.TargetInfo} targetInfo
+     * @return {?WebInspector.PendingConnection}
      */
     _targetCreated: function(targetInfo)
     {
         if (targetInfo.type !== "node")
-            return;
-        this._agent.attachToTarget(targetInfo.id);
+            return null;
+        var connection = this._pendingConnections.get(targetInfo.id);

[dgozman, 2016/10/28 18:31:35]

console.assert(!this._pendingConnections.has(targetInfo.id))

[eostroukhov, 2016/11/01 20:28:14]

Why not "console.assert(!connection)"?

[eostroukhov, 2016/11/02 21:55:52]

I had to rollback the change. As we discussed before, in some cases attached
arrives before the create - what I do is init a subtarget in that case. Then
create will be called with the subtarget already in the map.

+        if (connection)
+            return connection;
+        connection = new WebInspector.PendingConnection(targetInfo, this);
+        this._pendingConnections.set(targetInfo.id, connection);
+        this.dispatchEventToListeners(WebInspector.SubTargetsManager.Events.SubTargetConnectionCreated, connection);
+        return connection;

[dgozman, 2016/10/28 18:31:35]

You never use return value, which is good :-)

[eostroukhov, 2016/11/01 20:28:14]

Fixed

     },
 
     /**
@@ -225,7 +236,19 @@ WebInspector.SubTargetsManager.prototype = {
      */
     _targetDestroyed: function(targetId)
     {
-        // All the work is done in _detachedFromTarget.
+        var connection = this._pendingConnections.get(targetId);
+        if (!connection)
+            return;
+        this._pendingConnections.delete(targetId);
+        this.dispatchEventToListeners(WebInspector.SubTargetsManager.Events.SubTargetConnectionDestroyed, connection);
+    },
+
+    /**
+     * @return {!Array<!WebInspector.PendingConnection>}
+     */
+    targetConnections: function()

[dgozman, 2016/10/28 18:31:35]

pendingConnections

[eostroukhov, 2016/11/01 20:28:14]

subTargets?

+    {
+        return Array.from(this._pendingConnections.values());

[dgozman, 2016/10/28 18:31:35]

.valuesArray()

[eostroukhov, 2016/11/01 20:28:14]

Done.

     },
 
     /**
@@ -250,6 +273,16 @@ WebInspector.SubTargetsManager.prototype = {
 };
 
 /**
+ * @param {!WebInspector.Target} target
+ * @return {?WebInspector.SubTargetsManager}
+ */
+WebInspector.SubTargetsManager.fromTarget = function(target)
+{
+    return /** @type {?WebInspector.SubTargetsManager} */ (target.model(WebInspector.SubTargetsManager));
+};
+
+
+/**
  * @constructor
  * @implements {TargetAgent.Dispatcher}
  * @param {!WebInspector.SubTargetsManager} manager
@@ -341,6 +374,14 @@ WebInspector.SubTargetConnection.prototype = {
     {
         throw new Error("Not implemented");
     },
+
+    /**
+     * @return {string}
+     */
+    targetId: function()
+    {
+        return this._targetId;

[dgozman, 2016/10/28 18:31:35]

Just use ._targetId directly in this file.

[eostroukhov, 2016/11/01 20:28:14]

Done.

+    },
 };
 
 /**
@@ -352,7 +393,7 @@ WebInspector.TargetInfo = function(payload)
     this.id = payload.targetId;
     this.url = payload.url;
     this.type = payload.type;
-    if (this.type !== "page" && this.type !== "iframe") {
+    if (this.type !== "page" && this.type !== "iframe" && this.type !== "node") {

[dgozman, 2016/10/28 18:31:35]

This has changed recently, please rebase.

[eostroukhov, 2016/11/01 20:28:15]

Done.

         this.title = WebInspector.UIString("Worker: %s", this.url);
         this.canActivate = false;
     } else {
@@ -360,3 +401,74 @@ WebInspector.TargetInfo = function(payload)
         this.canActivate = true;
     }
 };
+
+/**
+ * @constructor
+ * @param {!WebInspector.TargetInfo} info
+ * @param {!WebInspector.SubTargetsManager} manager
+ */
+WebInspector.PendingConnection = function(info, manager)
+{
+    this._info = info;
+    this._manager = manager;
+    /** @type {?Promise} */
+    this._connectPromise = null;
+    /** @type {?Function} */
+    this._attachedCallback = null;
+};
+
+WebInspector.PendingConnection.prototype = {
+    /**
+     * @return {string}
+     */
+    id: function()
+    {
+        return this._info.id;
+    },
+
+    /**
+     * @return {?WebInspector.Target}
+     */
+    target: function()
+    {
+        return this._manager.targetForId(this.id());
+    },
+
+    /**
+     * @return {string}
+     */
+    name: function()
+    {
+        return this._info.title;
+    },
+
+    /**
+     * @return {!Promise}
+     */
+    connect: function()
+    {
+        if (this._connectPromise)
+            return this._connectPromise;
+        if (this.target())
+            return Promise.resolve(this.target());
+        this._connectPromise = new Promise(resolve => this._attachedCallback = resolve)

[dgozman, 2016/10/28 18:31:35]

If you don't follow the pattern I described, then you have to do all the work in
promiseBody(resolve) function. This means removing .then here and putting
attachToTarget call inside of promise body.

[eostroukhov, 2016/11/01 20:28:14]

Not sure I understood correctly, but I restructured code a bit.

+            .then(() => {
+                this._connectPromise = null;
+                this._attachedCallback = null;
+                return this.target();
+            });
+        this._manager._agent.attachToTarget(this.id());
+        return this._connectPromise;
+    },
+
+    disconnect: function()
+    {
+        this._manager._agent.detachFromTarget(this.id());
+    },
+
+    notifyAttached: function(target)

[dgozman, 2016/10/28 18:31:35]

JSDoc please.

[eostroukhov, 2016/11/01 20:28:15]

Done.

+    {
+        if (this._attachedCallback)
+            this._attachedCallback(target);
+    },
+};