DevTools: refactor and fix bugs for DOMBreakpointsSidebarPane

third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js

Index: third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js
diff --git a/third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js b/third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js
index fe4246383828943b93ef1a2347834d3ed0f61e9d..e1f4da90d2f611504a330709e44fc336ba09cbce 100644
--- a/third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js
+++ b/third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js
@@ -36,10 +36,9 @@
 WebInspector.DOMBreakpointsSidebarPane = function()
 {
     WebInspector.BreakpointsSidebarPaneBase.call(this);
-    this._domBreakpointsSetting = WebInspector.settings.createLocalSetting("domBreakpoints", []);
     this.listElement.classList.add("dom-breakpoints-list");
 
-    this._breakpointElements = {};
+    this._breakpointElements = new Map();
 
     this._breakpointTypes = {
         SubtreeModified: "subtree-modified",
@@ -56,20 +55,22 @@ WebInspector.DOMBreakpointsSidebarPane = function()
     this._contextMenuLabels[this._breakpointTypes.AttributeModified] = WebInspector.UIString.capitalize("Attributes ^modifications");
     this._contextMenuLabels[this._breakpointTypes.NodeRemoved] = WebInspector.UIString.capitalize("Node ^removal");
 
-    WebInspector.targetManager.addModelListener(WebInspector.DOMModel, WebInspector.DOMModel.Events.NodeRemoved, this._nodeRemoved, this);
-    WebInspector.targetManager.addEventListener(WebInspector.TargetManager.Events.InspectedURLChanged, this._inspectedURLChanged, this);
-    this._inspectedURL = WebInspector.targetManager.inspectedURL();
+    WebInspector.domBreakpointManager.addEventListener(WebInspector.DOMBreakpointManager.Events.BreakpointsChanged, this._reloadBreakpointElements, this);
     this._update();
 }
 
 WebInspector.DOMBreakpointsSidebarPane.Marker = "breakpoint-marker";
 
 WebInspector.DOMBreakpointsSidebarPane.prototype = {
-    _inspectedURLChanged: function()
+    _reloadBreakpointElements: function()
     {
-        this._breakpointElements = {};
+        this._breakpointElements.clear();
         this.reset();
-        this._inspectedURL = WebInspector.targetManager.inspectedURL();
+        var breakpoints = WebInspector.domBreakpointManager.domBreakpoints();
+        for (var breakpoint of breakpoints) {
+            var id = this._createBreakpointId(breakpoint.node.id, breakpoint.type);
+            this._breakpointElements.set(id, this._createBreakpointElement(breakpoint.node, breakpoint.type, breakpoint.enabled));
+        }
     },
 
     /**
@@ -82,56 +83,22 @@ WebInspector.DOMBreakpointsSidebarPane.prototype = {
         if (node.pseudoType())
             return;
 
-        var nodeBreakpoints = this._nodeBreakpoints(node);
+        var nodeBreakpoints = WebInspector.domBreakpointManager.nodeBreakpoints(node);
 
         /**
          * @param {!DOMDebuggerAgent.DOMBreakpointType} type
-         * @this {WebInspector.DOMBreakpointsSidebarPane}
          */
         function toggleBreakpoint(type)
         {
-            if (!nodeBreakpoints[type])
-                this._setBreakpoint(node, type, true);
-            else
-                this._removeBreakpoint(node, type);
-            this._saveBreakpoints();
+            WebInspector.domBreakpointManager.toggleBreakpoint(node, type);
         }
 
         var breakpointsMenu = createSubMenu ? contextMenu.appendSubMenuItem(WebInspector.UIString("Break on...")) : contextMenu;
         for (var key in this._breakpointTypes) {
             var type = this._breakpointTypes[key];
             var label = this._contextMenuLabels[type];
-            breakpointsMenu.appendCheckboxItem(label, toggleBreakpoint.bind(this, type), nodeBreakpoints[type]);
-        }
-    },
-
-    /**
-     * @param {!WebInspector.DOMNode} node
-     * @return {!Object<string, boolean>}
-     */
-    _nodeBreakpoints: function(node)
-    {
-        var nodeBreakpoints = {};
-        for (var id in this._breakpointElements) {
-            var element = this._breakpointElements[id];
-            if (element._node === node && element._checkboxElement.checked)
-                nodeBreakpoints[element._type] = true;
-        }
-        return nodeBreakpoints;
-    },
-
-    /**
-     * @param {!WebInspector.DOMNode} node
-     * @return {boolean}
-     */
-    hasBreakpoints: function(node)
-    {
-        for (var id in this._breakpointElements) {
-            var element = this._breakpointElements[id];
-            if (element._node === node && element._checkboxElement.checked)
-                return true;
+            breakpointsMenu.appendCheckboxItem(label, toggleBreakpoint.bind(null, type), nodeBreakpoints.get(type));
         }
-        return false;
     },
 
     /**
@@ -174,50 +141,6 @@ WebInspector.DOMBreakpointsSidebarPane.prototype = {
         return WebInspector.formatLocalized(message, substitutions);
     },
 
-    _nodeRemoved: function(event)
-    {
-        var node = event.data.node;
-        this._removeBreakpointsForNode(event.data.node);
-        var children = node.children();
-        if (!children)
-            return;
-        for (var i = 0; i < children.length; ++i)
-            this._removeBreakpointsForNode(children[i]);
-        this._saveBreakpoints();
-    },
-
-    /**
-     * @param {!WebInspector.DOMNode} node
-     */
-    _removeBreakpointsForNode: function(node)
-    {
-        for (var id in this._breakpointElements) {
-            var element = this._breakpointElements[id];
-            if (element._node === node)
-                this._removeBreakpoint(element._node, element._type);
-        }
-    },
-
-    /**
-     * @param {!WebInspector.DOMNode} node
-     * @param {!DOMDebuggerAgent.DOMBreakpointType} type
-     * @param {boolean} enabled
-     */
-    _setBreakpoint: function(node, type, enabled)
-    {
-        var breakpointId = this._createBreakpointId(node.id, type);
-        var breakpointElement = this._breakpointElements[breakpointId];
-        if (!breakpointElement) {
-            breakpointElement = this._createBreakpointElement(node, type, enabled);
-            this._breakpointElements[breakpointId] = breakpointElement;
-        } else {
-            breakpointElement._checkboxElement.checked = enabled;
-        }
-        if (enabled)
-            node.target().domdebuggerAgent().setDOMBreakpoint(node.id, type);
-        node.setMarker(WebInspector.DOMBreakpointsSidebarPane.Marker, true);
-    },
-
     /**
      * @param {!WebInspector.DOMNode} node
      * @param {!DOMDebuggerAgent.DOMBreakpointType} type
@@ -257,33 +180,6 @@ WebInspector.DOMBreakpointsSidebarPane.prototype = {
         return element;
     },
 
-    _removeAllBreakpoints: function()
-    {
-        for (var id in this._breakpointElements) {
-            var element = this._breakpointElements[id];
-            this._removeBreakpoint(element._node, element._type);
-        }
-        this._saveBreakpoints();
-    },
-
-    /**
-     * @param {!WebInspector.DOMNode} node
-     * @param {!DOMDebuggerAgent.DOMBreakpointType} type
-     */
-    _removeBreakpoint: function(node, type)
-    {
-        var breakpointId = this._createBreakpointId(node.id, type);
-        var element = this._breakpointElements[breakpointId];
-        if (!element)
-            return;
-
-        this.removeListElement(element);
-        delete this._breakpointElements[breakpointId];
-        if (element._checkboxElement.checked)
-            node.target().domdebuggerAgent().removeDOMBreakpoint(node.id, type);
-        node.setMarker(WebInspector.DOMBreakpointsSidebarPane.Marker, this.hasBreakpoints(node) ? true : null);
-    },
-
     /**
      * @param {!WebInspector.DOMNode} node
      * @param {!DOMDebuggerAgent.DOMBreakpointType} type
@@ -292,17 +188,10 @@ WebInspector.DOMBreakpointsSidebarPane.prototype = {
     _contextMenu: function(node, type, event)
     {
         var contextMenu = new WebInspector.ContextMenu(event);
-
-        /**
-         * @this {WebInspector.DOMBreakpointsSidebarPane}
-         */
-        function removeBreakpoint()
-        {
-            this._removeBreakpoint(node, type);
-            this._saveBreakpoints();
-        }
-        contextMenu.appendItem(WebInspector.UIString.capitalize("Remove ^breakpoint"), removeBreakpoint.bind(this));
-        contextMenu.appendItem(WebInspector.UIString.capitalize("Remove ^all DOM breakpoints"), this._removeAllBreakpoints.bind(this));
+        var removeBreakpoint = () => WebInspector.domBreakpointManager.removeBreakpoint(node, type);
+        contextMenu.appendItem(WebInspector.UIString.capitalize("Remove ^breakpoint"), removeBreakpoint);
+        var removeAllBreakpoints = () => WebInspector.domBreakpointManager.removeAllBreakpoints();
+        contextMenu.appendItem(WebInspector.UIString.capitalize("Remove ^all DOM breakpoints"), removeAllBreakpoints);
         contextMenu.show();
     },
 
@@ -313,11 +202,7 @@ WebInspector.DOMBreakpointsSidebarPane.prototype = {
      */
     _checkboxClicked: function(node, type, event)
     {
-        if (event.target.checked)
-            node.target().domdebuggerAgent().setDOMBreakpoint(node.id, type);
-        else
-            node.target().domdebuggerAgent().removeDOMBreakpoint(node.id, type);
-        this._saveBreakpoints();
+        WebInspector.domBreakpointManager.setBreakpoint(node, type, event.target["checkboxElement"].checked);
     },
 
     /**
@@ -341,7 +226,7 @@ WebInspector.DOMBreakpointsSidebarPane.prototype = {
         }
         var auxData = details.auxData;
         var breakpointId = this._createBreakpointId(auxData.nodeId, auxData.type);
-        var element = this._breakpointElements[breakpointId];
+        var element = this._breakpointElements.get(breakpointId);
         if (!element)
             return;
         WebInspector.viewManager.showView("sources.domBreakpoints");
@@ -352,39 +237,206 @@ WebInspector.DOMBreakpointsSidebarPane.prototype = {
     /**
      * @param {number} nodeId
      * @param {!DOMDebuggerAgent.DOMBreakpointType} type
+     * @return {string}
      */
     _createBreakpointId: function(nodeId, type)
     {
         return nodeId + ":" + type;
     },
 
-    _saveBreakpoints: function()
+    __proto__: WebInspector.BreakpointsSidebarPaneBase.prototype
+}
+
+/**
+ * @constructor
+ * @extends {WebInspector.VBox}
+ */
+WebInspector.DOMBreakpointsSidebarPane.Proxy = function()

[lushnikov, 2016/08/18 21:16:34]

let's move this below the WI.DOMBreakpointManager so that diff is smaller

[chenwilliam, 2016/08/18 22:26:50]

Done.

+{
+    WebInspector.VBox.call(this);
+    this.registerRequiredCSS("components/breakpointsList.css");
+}
+
+WebInspector.DOMBreakpointsSidebarPane.Proxy.prototype = {
+    wasShown: function()
+    {
+        WebInspector.SimpleView.prototype.wasShown.call(this);
+        var pane = WebInspector.domBreakpointsSidebarPane;
+        if (pane.element.parentNode !== this.element)
+            pane.show(this.element);
+    },
+
+    __proto__: WebInspector.VBox.prototype
+}
+
+/**
+ * @type {!WebInspector.DOMBreakpointsSidebarPane}
+ */
+WebInspector.domBreakpointsSidebarPane;
+
+/**
+ * @constructor
+ * @extends {WebInspector.Object}
+ * @implements {WebInspector.TargetManager.Observer}
+ */
+WebInspector.DOMBreakpointManager = function()
+{
+    /** @type {!Map<!WebInspector.Target, !Protocol.DOMDebuggerAgent>} */
+    this._agents = new Map();
+    this._domBreakpointsSetting = WebInspector.settings.createLocalSetting("domBreakpoints", []);
+    /** @type {!Map<string, !WebInspector.DOMBreakpointManager.Breakpoint>} */
+    this._breakpoints = new Map();
+    WebInspector.targetManager.addModelListener(WebInspector.DOMModel, WebInspector.DOMModel.Events.DocumentUpdated, this._restoreBreakpoints, this);
+    WebInspector.targetManager.addModelListener(WebInspector.DOMModel, WebInspector.DOMModel.Events.NodeRemoved, this._nodeRemoved, this);
+    WebInspector.targetManager.observeTargets(this);
+}
+
+WebInspector.DOMBreakpointManager.Marker = "breakpoint-marker";
+
+WebInspector.DOMBreakpointManager.Events = {
+    BreakpointsChanged: "BreakpointsChanged"
+}
+
+WebInspector.DOMBreakpointManager.prototype = {
+    /**
+     * @override
+     * @param {!WebInspector.Target} target
+     */
+    targetAdded: function(target)
+    {
+        if (target.hasDOMCapability())
+            this._agents.set(target, target.domdebuggerAgent());
+    },
+
+    /**
+     * @override
+     * @param {!WebInspector.Target} target
+     */
+    targetRemoved: function(target)
+    {
+        if (target.hasDOMCapability())
+            this._agents.delete(target);
+    },
+
+    /**
+     * @return {!Array<!WebInspector.DOMBreakpointManager.Breakpoint>}
+     */
+    domBreakpoints: function()
     {
         var breakpoints = [];
-        var storedBreakpoints = this._domBreakpointsSetting.get();
-        for (var i = 0; i < storedBreakpoints.length; ++i) {
-            var breakpoint = storedBreakpoints[i];
-            if (breakpoint.url !== this._inspectedURL)
-                breakpoints.push(breakpoint);
+        for (var breakpoint of this._breakpoints.values())
+            breakpoints.push(breakpoint);
+        return breakpoints;
+    },
+
+    /**
+     * @param {!WebInspector.DOMNode} node
+     * @param {!DOMDebuggerAgent.DOMBreakpointType} type
+     * @param {boolean} enabled
+     */
+    setBreakpoint: function(node, type, enabled)
+    {
+        this._innerSetBreakpoint(node, type, enabled);
+        this._saveBreakpoints();
+    },
+
+    /**
+     * @param {!WebInspector.DOMNode} node
+     * @param {!DOMDebuggerAgent.DOMBreakpointType} type
+     * @param {boolean} enabled
+     */
+    _innerSetBreakpoint: function(node, type, enabled)
+    {
+        var url = node.ownerDocument.documentURL.removeURLFragment();
+        var breakpoint = new WebInspector.DOMBreakpointManager.Breakpoint(node, type, url, enabled);
+        this._breakpoints.set(this._createBreakpointId(node, type), breakpoint);
+        var agent = this._agents.get(node.target());
+        if (enabled) {
+            agent.setDOMBreakpoint(node.id, type);
+            node.setMarker(WebInspector.DOMBreakpointManager.Marker, true);
+        } else {
+            agent.removeDOMBreakpoint(node.id, type);
+            node.setMarker(WebInspector.DOMBreakpointManager.Marker, this._hasOtherBreakpoints(node, type) ? true : null);
         }
-        for (var id in this._breakpointElements) {
-            var element = this._breakpointElements[id];
-            breakpoints.push({ url: this._inspectedURL, path: element._node.path(), type: element._type, enabled: element._checkboxElement.checked });
+    },
+
+    /**
+     * @param {!WebInspector.DOMNode} node
+     * @param {!DOMDebuggerAgent.DOMBreakpointType} type
+     */
+    removeBreakpoint: function(node, type)
+    {
+        this._innerRemoveBreakpoint(node, type);
+        this._saveBreakpoints();
+    },
+
+    /**
+     * @param {!WebInspector.DOMNode} node
+     * @param {!DOMDebuggerAgent.DOMBreakpointType} type
+     */
+    _innerRemoveBreakpoint: function(node, type)
+    {
+        var breakpointId = this._createBreakpointId(node, type);
+        var breakpoint = this._breakpoints.get(breakpointId);
+        if (breakpoint.enabled) {
+            var agent = this._agents.get(node.target());
+            agent.removeDOMBreakpoint(node.id, type);
+            node.setMarker(WebInspector.DOMBreakpointManager.Marker, this._hasOtherBreakpoints(node, type) ? true : null);
         }
-        this._domBreakpointsSetting.set(breakpoints);
+        this._breakpoints.remove(breakpointId);
     },
 
     /**
-     * @param {!WebInspector.DOMModel} domModel
+     * @param {!WebInspector.DOMNode} node
+     * @param {!DOMDebuggerAgent.DOMBreakpointType} type
      */
-    restoreBreakpoints: function(domModel)
+    toggleBreakpoint: function(node, type)
+    {
+        var nodeBreakpoints = this.nodeBreakpoints(node);
+        if (!nodeBreakpoints.get(type))
+            this.setBreakpoint(node, type, true);
+        else
+            this.removeBreakpoint(node, type);
+    },
+
+    removeAllBreakpoints: function()
     {
+        for (var breakpoint of this._breakpoints.values())
+            this._innerRemoveBreakpoint(breakpoint.node, breakpoint.type);
+        this._saveBreakpoints();
+    },
+
+    /**
+     * @param {!WebInspector.DOMNode} node
+     * @return {!Map<string, boolean>}
+     */
+    nodeBreakpoints: function(node)
+    {
+        var nodeBreakpoints = new Map();
+        for (var breakpoint of this._breakpoints.values()) {
+            if (breakpoint.node === node && breakpoint.enabled)
+                nodeBreakpoints.set(breakpoint.type, true);
+        }
+        return nodeBreakpoints;
+    },
+
+    /**
+     * @param {!WebInspector.Event} event
+     */
+    _restoreBreakpoints: function(event)
+    {
+        var inspectedRootDocument = /** @type {!WebInspector.DOMDocument} */ (event.data);
+        if (!inspectedRootDocument)

[chenwilliam, 2016/08/17 23:02:49]

I saw Elements panel doing something similar in its _documentUpdated method: "if
(!inspectedRootDocument) return"

Without this check, _restoreBreakpoints gets called 3 times (the first two times
it's called without an event.data set).

I noticed that when the Sources panel is selected, the event.data would always
be null (i.e. no DOMDocument), so the main body of restoreBreakpoints never gets
executed. Is there a different mechanism to know when the page changes and then
call restoreBreakpoints?

+            return;
+        var domModel = /** @type {!WebInspector.DOMModel} */ (event.target);
         var pathToBreakpoints = {};
+        this._breakpoints.clear();
+        this.dispatchEventToListeners(WebInspector.DOMBreakpointManager.Events.BreakpointsChanged);
 
         /**
          * @param {string} path
          * @param {?DOMAgent.NodeId} nodeId
-         * @this {WebInspector.DOMBreakpointsSidebarPane}
+         * @this {WebInspector.DOMBreakpointManager}
          */
         function didPushNodeByPathToFrontend(path, nodeId)
         {
@@ -392,15 +444,14 @@ WebInspector.DOMBreakpointsSidebarPane.prototype = {
             if (!node)
                 return;
 
-            var breakpoints = pathToBreakpoints[path];
-            for (var i = 0; i < breakpoints.length; ++i)
-                this._setBreakpoint(node, breakpoints[i].type, breakpoints[i].enabled);
+            for (var breakpoint of pathToBreakpoints[path])
+                this._innerSetBreakpoint(node, breakpoint.type, breakpoint.enabled);
+            this.dispatchEventToListeners(WebInspector.DOMBreakpointManager.Events.BreakpointsChanged);
         }
 
         var breakpoints = this._domBreakpointsSetting.get();
-        for (var i = 0; i < breakpoints.length; ++i) {
-            var breakpoint = breakpoints[i];
-            if (breakpoint.url !== this._inspectedURL)
+        for (var breakpoint of breakpoints) {
+            if (breakpoint.url !== WebInspector.targetManager.inspectedURL())
                 continue;
             var path = breakpoint.path;
             if (!pathToBreakpoints[path]) {
@@ -411,32 +462,96 @@ WebInspector.DOMBreakpointsSidebarPane.prototype = {
         }
     },
 
-    __proto__: WebInspector.BreakpointsSidebarPaneBase.prototype
-}
+    _saveBreakpoints: function()
+    {
+        var breakpoints = [];
+        var storedBreakpoints = this._domBreakpointsSetting.get();
+        for (var breakpoint of storedBreakpoints) {
+            if (breakpoint.url !== WebInspector.targetManager.inspectedURL())
+                breakpoints.push(breakpoint);
+        }
+        for (var breakpoint of this._breakpoints.values()) {
+            breakpoints.push({
+                url: breakpoint.url,
+                path: breakpoint.node.path(),
+                type: breakpoint.type,
+                enabled: breakpoint.enabled
+            });
+        }
+        this._domBreakpointsSetting.set(breakpoints);
+        this.dispatchEventToListeners(WebInspector.DOMBreakpointManager.Events.BreakpointsChanged);
+    },
 
-/**
- * @constructor
- * @extends {WebInspector.VBox}
- */
-WebInspector.DOMBreakpointsSidebarPane.Proxy = function()
-{
-    WebInspector.VBox.call(this);
-    this.registerRequiredCSS("components/breakpointsList.css");
-}
+    /**
+     * @param {!WebInspector.DOMNode} node
+     * @param {!DOMDebuggerAgent.DOMBreakpointType} type
+     * @return {boolean}
+     */
+    _hasOtherBreakpoints: function(node, type)
+    {
+        for (var breakpoint of this._breakpoints.values()) {
+            if (breakpoint.node === node && breakpoint.type !== type && breakpoint.enabled)
+                return true;
+        }
+        return false;
+    },
 
-WebInspector.DOMBreakpointsSidebarPane.Proxy.prototype = {
-    wasShown: function()
+    /**
+     * @param {!WebInspector.Event} event
+     */
+    _nodeRemoved: function(event)
     {
-        WebInspector.SimpleView.prototype.wasShown.call(this);
-        var pane = WebInspector.domBreakpointsSidebarPane;
-        if (pane.element.parentNode !== this.element)
-            pane.show(this.element);
+        var node = event.data.node;
+        this._removeBreakpointsForNode(node);
+        var children = node.children();
+        if (!children)
+            return;
+        for (var child of children)
+            this._removeBreakpointsForNode(child);
     },
 
-    __proto__: WebInspector.VBox.prototype
+    /**
+     * @param {!WebInspector.DOMNode} node
+     */
+    _removeBreakpointsForNode: function(node)
+    {
+        for (var breakpoint of this._breakpoints.values()) {
+            if (breakpoint.node === node)
+                this._innerRemoveBreakpoint(breakpoint.node, breakpoint.type);
+        }
+        this._saveBreakpoints();
+    },
+
+    /**
+     * @param {!WebInspector.DOMNode} node
+     * @param {!DOMDebuggerAgent.DOMBreakpointType} type
+     * @return {string}
+     */
+    _createBreakpointId: function(node, type)
+    {
+        return node.id + ":" + type;
+    },
+
+    __proto__: WebInspector.Object.prototype
+};
+
+/**
+ * @constructor
+ * @param {!WebInspector.DOMNode} node
+ * @param {!DOMDebuggerAgent.DOMBreakpointType} type
+ * @param {string} url
+ * @param {boolean} enabled
+ */
+WebInspector.DOMBreakpointManager.Breakpoint = function(node, type, url, enabled)
+{
+    this.node = node;
+    this.path = node.path();
+    this.type = type;
+    this.url = url;
+    this.enabled = enabled;
 }
 
 /**
- * @type {!WebInspector.DOMBreakpointsSidebarPane}
+ * @type {!WebInspector.DOMBreakpointManager}
  */
-WebInspector.domBreakpointsSidebarPane;
+WebInspector.domBreakpointManager;