DevTools: extract model from DOMBreakpointsSidebarPane

DevTools: extract model from DOMBreakpointsSidebarPane

- Extracts the domain logic from DOMBreakpointsSidebarPane into DOMBreakpointsModel
- Fixes a bug and added a test for toggling “on” a DOM breakpoint checkbox
- Fixes a minor bug where a DOM breakpoint on a leaf node (with no text) would not be 
deleted when the DOM node is deleted. DOM breakpoints should be permanently deleted 
when the DOM Node is deleted.
- Fixes missing DOM breakpoints bug when refreshing on sources panel:
https://bugs.chromium.org/p/chromium/issues/detail?id=634513
- Fixes duplicate DOM breakpoints bug when refreshing page:
https://bugs.chromium.org/p/chromium/issues/detail?id=634510

BUG=627296

[chenwilliam, 2016-07-29 21:11:43]

Description was changed from

==========
Extract model from DOMBreakpointsSidebarPane

Extract model from DOMBreakpointsSidebarPane

BUG=627296
==========

to

==========
Extract model from DOMBreakpointsSidebarPane

Extract model from DOMBreakpointsSidebarPane

- Created DOMBreakpointsModel.js which wraps the domdebuggerAgent
- Improved a test to ensure "unchecking" a DOM breakpoint works properly

BUG=627296
==========

[chenwilliam, 2016-07-29 21:41:46]

chenwilliam@chromium.org changed reviewers:
+ lushnikov@chromium.org

[chenwilliam, 2016-07-30 00:10:31]

Description was changed from

==========
Extract model from DOMBreakpointsSidebarPane

Extract model from DOMBreakpointsSidebarPane

- Created DOMBreakpointsModel.js which wraps the domdebuggerAgent
- Improved a test to ensure "unchecking" a DOM breakpoint works properly

BUG=627296
==========

to

==========
DevTools: extract model from DOMBreakpointsSidebarPane

- Created DOMBreakpointsModel.js which wraps the domdebuggerAgent
- Improved a test to ensure "unchecking" a DOM breakpoint works properly

BUG=627296
==========

[lushnikov, 2016-07-30 01:24:42]

https://codereview.chromium.org/2191183003/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js
(right):

https://codereview.chromium.org/2191183003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:60:
};
nit: we don't put semicolons after function declaration

https://codereview.chromium.org/2191183003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:206:
var breakpointId = WebInspector.DOMBreakpointsModel.createBreakpointId(node.id,
type);
breakpoint id serves only for the needs of DOMBreakpointsSidebarPane - let's
keep it in here

https://codereview.chromium.org/2191183003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:321:
this._saveBreakpoints();
i believe this logic should go to your model as well

https://codereview.chromium.org/2191183003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/DOMBreakpointsModel.js
(right):

https://codereview.chromium.org/2191183003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMBreakpointsModel.js:19: /**
you might also want to add domBreakpoints() getter here, and rely on it from
DOMBreakpointsSidebarPane

https://codereview.chromium.org/2191183003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMBreakpointsModel.js:43:
node.setMarker(WebInspector.DOMBreakpointsModel.Marker, value);
We shouldn't expose this method. We should call node.setMarker inside
setDOMBreakpoint/removeDOMBreakpoint

https://codereview.chromium.org/2191183003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMBreakpointsModel.js:54:
WebInspector.DOMBreakpointsModel.createBreakpointId = function(nodeId, type)
this should be a part of DOMBreakpointsSidebarPane

https://codereview.chromium.org/2191183003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMBreakpointsModel.js:63:
WebInspector.DOMBreakpointsModel.fromTarget = function(target)
let's inline this unless it is needed

https://codereview.chromium.org/2191183003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/module.json (right):

https://codereview.chromium.org/2191183003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/module.json:95:
"DOMBreakpointsModel.js",
DOMDebuggerModel.js

you should also add it into devtools.gypi

[chenwilliam, 2016-08-06 00:15:57]

Description was changed from

==========
DevTools: extract model from DOMBreakpointsSidebarPane

- Created DOMBreakpointsModel.js which wraps the domdebuggerAgent
- Improved a test to ensure "unchecking" a DOM breakpoint works properly

BUG=627296
==========

to

==========
DevTools: extract model from DOMBreakpointsSidebarPane

- Extracted the domain logic from DOMBreakpointsSidebarPane into
DOMBreakpointsModel
- Fixed a bug and added a test for toggling “on” a DOM breakpoint checkbox
- Fixed a minor bug where a DOM breakpoint on a leaf node (with no text) would
not be 
deleted when the DOM node is deleted. DOM breakpoints should be permanently
deleted 
when the DOM Node is deleted.
- Identified two other bugs that are more substantive and haven't been able to
fix yet:
  - https://bugs.chromium.org/p/chromium/issues/detail?id=634513
  - https://bugs.chromium.org/p/chromium/issues/detail?id=634510

BUG=627296
==========

[chenwilliam, 2016-08-06 00:28:22]

I've updated the CL description with info on related bugs.

https://codereview.chromium.org/2191183003/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js
(right):

https://codereview.chromium.org/2191183003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:60:
};
On 2016/07/30 01:24:42, lushnikov wrote:
> nit: we don't put semicolons after function declaration

Done.

https://codereview.chromium.org/2191183003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:206:
var breakpointId = WebInspector.DOMBreakpointsModel.createBreakpointId(node.id,
type);
On 2016/07/30 01:24:42, lushnikov wrote:
> breakpoint id serves only for the needs of DOMBreakpointsSidebarPane - let's
> keep it in here

I've changed it so the use of createBreakpointId is almost all in the model.

https://codereview.chromium.org/2191183003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:321:
this._saveBreakpoints();
On 2016/07/30 01:24:42, lushnikov wrote:
> i believe this logic should go to your model as well

Done.

https://codereview.chromium.org/2191183003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/DOMBreakpointsModel.js
(right):

https://codereview.chromium.org/2191183003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMBreakpointsModel.js:19: /**
On 2016/07/30 01:24:42, lushnikov wrote:
> you might also want to add domBreakpoints() getter here, and rely on it from
> DOMBreakpointsSidebarPane

Done.

https://codereview.chromium.org/2191183003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMBreakpointsModel.js:43:
node.setMarker(WebInspector.DOMBreakpointsModel.Marker, value);
On 2016/07/30 01:24:42, lushnikov wrote:
> We shouldn't expose this method. We should call node.setMarker inside
> setDOMBreakpoint/removeDOMBreakpoint

Done.

https://codereview.chromium.org/2191183003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMBreakpointsModel.js:54:
WebInspector.DOMBreakpointsModel.createBreakpointId = function(nodeId, type)
On 2016/07/30 01:24:42, lushnikov wrote:
> this should be a part of DOMBreakpointsSidebarPane

I've changed it so the use of createBreakpointId is almost all in the model.

https://codereview.chromium.org/2191183003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMBreakpointsModel.js:63:
WebInspector.DOMBreakpointsModel.fromTarget = function(target)
On 2016/07/30 01:24:42, lushnikov wrote:
> let's inline this unless it is needed

Done.

https://codereview.chromium.org/2191183003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/module.json (right):

https://codereview.chromium.org/2191183003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/module.json:95:
"DOMBreakpointsModel.js",
On 2016/07/30 01:24:42, lushnikov wrote:
> DOMDebuggerModel.js
> 
> you should also add it into devtools.gypi

Done.

https://codereview.chromium.org/2191183003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js
(right):

https://codereview.chromium.org/2191183003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:67:
return domDebuggerModels[0];
This feels a bit hacky, so perhaps there's a better way. From some
experimentation, it seems that the iframe is in the same target as the primary
document. In addition, if there were more than one domDebuggerModel instance
they are all backed by the same _domBreakpointsSetting instance.

https://codereview.chromium.org/2191183003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:142:
this.reset();
This could be optimized but I did a quick check of the performance for
_reloadBreakpointElements() and it's only ~10ms with 10 DOM breakpoints. My
assumption is that most users won't have that many DOM breakpoints for a given
page.

[lushnikov, 2016-08-08 19:53:08]

https://codereview.chromium.org/2191183003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js
(right):

https://codereview.chromium.org/2191183003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:63:
domDebuggerModel: function()
there should be no method domDebuggerModel(): DOMBreakpointsSidebarPane should
aggregate breakpoints across
all targets

https://codereview.chromium.org/2191183003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:67:
return domDebuggerModels[0];
On 2016/08/06 00:28:22, Will_Chen wrote:
> This feels a bit hacky, so perhaps there's a better way. From some
> experimentation, it seems that the iframe is in the same target as the primary
> document.

In future, they'll be in separate targets.

> In addition, if there were more than one domDebuggerModel instance
> they are all backed by the same _domBreakpointsSetting instance.

Indeed! Which means, that instead of introducing DOMDebuggerModel which operates
1-to-1 with agents, we should introduce DOMBreakpointManager (consistent here
with Breakpointmanager), which juggles multiple domdebuggerAgents inside.

https://codereview.chromium.org/2191183003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/DOMDebuggerModel.js
(right):

https://codereview.chromium.org/2191183003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMDebuggerModel.js:10:
WebInspector.DOMDebuggerModel = function(target)
the class has many similar API methods. Let's reduce the interface down to the
essential methods only

https://codereview.chromium.org/2191183003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMDebuggerModel.js:15:
this._breakpoints = {};
/** @type {!Map<.., ...>} */
this._breakpoints = new Map();

https://codereview.chromium.org/2191183003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMDebuggerModel.js:28: *
@return {!Object}
please annotate with proper type

https://codereview.chromium.org/2191183003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMDebuggerModel.js:51:
_setBreakpointWithoutSave: function(node, type, enabled)
_innerSetBreakpoint

https://codereview.chromium.org/2191183003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMDebuggerModel.js:55: node:
node,
let's create a type for this

https://codereview.chromium.org/2191183003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMDebuggerModel.js:94:
checkboxClicked: function(node, type, event)
the model should not talk in UI terms; this method should be a part of
DOMBreakpointsSIdebarPane

https://codereview.chromium.org/2191183003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMDebuggerModel.js:210:
_setDOMBreakpoint: function(node, type)
let's inline

https://codereview.chromium.org/2191183003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMDebuggerModel.js:220:
_removeDOMBreakpoint: function(node, type)
let's inline

https://codereview.chromium.org/2191183003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMDebuggerModel.js:244:
_nodeRemoved: function(event)
this method is not used?

https://codereview.chromium.org/2191183003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMDebuggerModel.js:274:
createBreakpointId: function(nodeId, type)
this is used only by the DOMBreakpointsSidebarPane and only to support it's
rendering -> there's no good reason to add this into model interface

[chenwilliam, 2016-08-10 19:48:36]

It's hard to tell from the diffs because of the file renaming, but since your
last review it's fairly similar with adjustments per your feedback.

https://codereview.chromium.org/2191183003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js
(right):

https://codereview.chromium.org/2191183003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:63:
domDebuggerModel: function()
On 2016/08/08 19:53:07, lushnikov wrote:
> there should be no method domDebuggerModel(): DOMBreakpointsSidebarPane should
> aggregate breakpoints across
> all targets

Done.

https://codereview.chromium.org/2191183003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:67:
return domDebuggerModels[0];
On 2016/08/08 19:53:06, lushnikov wrote:
> On 2016/08/06 00:28:22, Will_Chen wrote:
> > This feels a bit hacky, so perhaps there's a better way. From some
> > experimentation, it seems that the iframe is in the same target as the
primary
> > document.
> 
> In future, they'll be in separate targets.
> 
> > In addition, if there were more than one domDebuggerModel instance
> > they are all backed by the same _domBreakpointsSetting instance.
> 
> Indeed! Which means, that instead of introducing DOMDebuggerModel which
operates
> 1-to-1 with agents, we should introduce DOMBreakpointManager (consistent here
> with Breakpointmanager), which juggles multiple domdebuggerAgents inside.

Done.

https://codereview.chromium.org/2191183003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/DOMDebuggerModel.js
(right):

https://codereview.chromium.org/2191183003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMDebuggerModel.js:10:
WebInspector.DOMDebuggerModel = function(target)
On 2016/08/08 19:53:07, lushnikov wrote:
> the class has many similar API methods. Let's reduce the interface down to the
> essential methods only

Done.

https://codereview.chromium.org/2191183003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMDebuggerModel.js:15:
this._breakpoints = {};
On 2016/08/08 19:53:07, lushnikov wrote:
> /** @type {!Map<.., ...>} */
> this._breakpoints = new Map();

Done.

https://codereview.chromium.org/2191183003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMDebuggerModel.js:28: *
@return {!Object}
On 2016/08/08 19:53:07, lushnikov wrote:
> please annotate with proper type 

Done.

https://codereview.chromium.org/2191183003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMDebuggerModel.js:51:
_setBreakpointWithoutSave: function(node, type, enabled)
On 2016/08/08 19:53:07, lushnikov wrote:
> _innerSetBreakpoint

Done.

https://codereview.chromium.org/2191183003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMDebuggerModel.js:55: node:
node,
On 2016/08/08 19:53:07, lushnikov wrote:
> let's create a type for this

Done.

https://codereview.chromium.org/2191183003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMDebuggerModel.js:94:
checkboxClicked: function(node, type, event)
On 2016/08/08 19:53:07, lushnikov wrote:
> the model should not talk in UI terms; this method should be a part of
> DOMBreakpointsSIdebarPane

Done.

https://codereview.chromium.org/2191183003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMDebuggerModel.js:210:
_setDOMBreakpoint: function(node, type)
On 2016/08/08 19:53:07, lushnikov wrote:
> let's inline

Done.

https://codereview.chromium.org/2191183003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMDebuggerModel.js:220:
_removeDOMBreakpoint: function(node, type)
On 2016/08/08 19:53:07, lushnikov wrote:
> let's inline

Done.

https://codereview.chromium.org/2191183003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMDebuggerModel.js:244:
_nodeRemoved: function(event)
On 2016/08/08 19:53:07, lushnikov wrote:
> this method is not used?

It's used as a model listener.

WebInspector.targetManager.addModelListener(WebInspector.DOMModel,
WebInspector.DOMModel.Events.NodeRemoved, this._nodeRemoved, this);

https://codereview.chromium.org/2191183003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMDebuggerModel.js:274:
createBreakpointId: function(nodeId, type)
On 2016/08/08 19:53:07, lushnikov wrote:
> this is used only by the DOMBreakpointsSidebarPane and only to support it's
> rendering -> there's no good reason to add this into model interface

OK, I have moved this method to DOMBreakpointsSidebarPane. There's a similar
method in DOMBreakpointManager called _breakpointId since we need to create a
key for our map this._breakpoints, but the two implementations are not coupled
together. I.e., pane makes no assumption on how the ids are created in manager.

[chenwilliam, 2016-08-10 20:02:24]

Description was changed from

==========
DevTools: extract model from DOMBreakpointsSidebarPane

- Extracted the domain logic from DOMBreakpointsSidebarPane into
DOMBreakpointsModel
- Fixed a bug and added a test for toggling “on” a DOM breakpoint checkbox
- Fixed a minor bug where a DOM breakpoint on a leaf node (with no text) would
not be 
deleted when the DOM node is deleted. DOM breakpoints should be permanently
deleted 
when the DOM Node is deleted.
- Identified two other bugs that are more substantive and haven't been able to
fix yet:
  - https://bugs.chromium.org/p/chromium/issues/detail?id=634513
  - https://bugs.chromium.org/p/chromium/issues/detail?id=634510

BUG=627296
==========

to

==========
DevTools: extract model from DOMBreakpointsSidebarPane

- Extracts the domain logic from DOMBreakpointsSidebarPane into
DOMBreakpointsModel
- Fixes a bug and added a test for toggling “on” a DOM breakpoint checkbox
- Fixes a minor bug where a DOM breakpoint on a leaf node (with no text) would
not be 
deleted when the DOM node is deleted. DOM breakpoints should be permanently
deleted 
when the DOM Node is deleted.
- Fixes this bug: https://bugs.chromium.org/p/chromium/issues/detail?id=634513
- Identified a bug that's more substantive and haven't been able to fix yet:
  - https://bugs.chromium.org/p/chromium/issues/detail?id=634510

BUG=627296
==========

[chenwilliam, 2016-08-10 20:03:35]

Also, I forgot to note that my latest patchsets fix this bug: 
https://bugs.chromium.org/p/chromium/issues/detail?id=634513

I've noted this is in the description.

[lushnikov, 2016-08-11 18:40:19]

https://codereview.chromium.org/2191183003/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js
(right):

https://codereview.chromium.org/2191183003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:40:
this._breakpointElements = {};
let's use Map and type-annotate it

https://codereview.chromium.org/2191183003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:67:
populateNodeContextMenu: function(node, contextMenu, createSubMenu)
let's move this to the elements's panel

https://codereview.chromium.org/2191183003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:231:
return `${nodeId}:${type}`;
nit: The former was easier to understand

https://codereview.chromium.org/2191183003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/main/Main.js (right):

https://codereview.chromium.org/2191183003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/main/Main.js:197:
WebInspector.domBreakpointsSidebarPane = new
WebInspector.DOMBreakpointsSidebarPane();
the SidebarPane shouldn't be a singleton once there's a singleton model

https://codereview.chromium.org/2191183003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/DOMBreakpointManager.js
(right):

https://codereview.chromium.org/2191183003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/DOMBreakpointManager.js:51: *
@return {!Map<string, !WebInspector.DOMBreakpointManager.Breakpoint>}
why do we return map here and not an array?

https://codereview.chromium.org/2191183003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/DOMBreakpointManager.js:76: var
breakpoint = new WebInspector.DOMBreakpointManager.Breakpoint(node, type,
this._inspectedURL, enabled);
instead of using inspectedURL, we probably should use URL of the node's document

https://codereview.chromium.org/2191183003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/DOMBreakpointManager.js:130:
removeBreakpointsForNode: function(node)
why is this part of public API? AFAIU It's not used externally

https://codereview.chromium.org/2191183003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/DOMBreakpointManager.js:148: *
@return {!Object<string, boolean>}
return Map<string, boolean>

https://codereview.chromium.org/2191183003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/DOMBreakpointManager.js:167:
this._breakpoints = new Map();
this._breakpoints.clear(); to play nice with closure types

https://codereview.chromium.org/2191183003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/DOMBreakpointManager.js:252:
var target = /** @type {!WebInspector.Target} */ (event.data);
why listen to this event altogether? it appears it should not be needed

https://codereview.chromium.org/2191183003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/DOMBreakpointManager.js:267:
return `${node.id}__${type}`;
let's be consistent and separate with colon

[chenwilliam, 2016-08-17 01:19:24]

Description was changed from

==========
DevTools: extract model from DOMBreakpointsSidebarPane

- Extracts the domain logic from DOMBreakpointsSidebarPane into
DOMBreakpointsModel
- Fixes a bug and added a test for toggling “on” a DOM breakpoint checkbox
- Fixes a minor bug where a DOM breakpoint on a leaf node (with no text) would
not be 
deleted when the DOM node is deleted. DOM breakpoints should be permanently
deleted 
when the DOM Node is deleted.
- Fixes this bug: https://bugs.chromium.org/p/chromium/issues/detail?id=634513
- Identified a bug that's more substantive and haven't been able to fix yet:
  - https://bugs.chromium.org/p/chromium/issues/detail?id=634510

BUG=627296
==========

to

==========
DevTools: extract model from DOMBreakpointsSidebarPane

- Extracts the domain logic from DOMBreakpointsSidebarPane into
DOMBreakpointsModel
- Fixes a bug and added a test for toggling “on” a DOM breakpoint checkbox
- Fixes a minor bug where a DOM breakpoint on a leaf node (with no text) would
not be 
deleted when the DOM node is deleted. DOM breakpoints should be permanently
deleted 
when the DOM Node is deleted.
- Fixes missing DOM breakpoints bug when refreshing on sources panel:
https://bugs.chromium.org/p/chromium/issues/detail?id=634513
- Fixes duplicate DOM breakpoints bug when refreshing page:
https://bugs.chromium.org/p/chromium/issues/detail?id=634510

BUG=627296
==========

[chenwilliam, 2016-08-17 01:20:11]

Addresses CL feedback. Also, fixes this bug during page reloads:
https://bugs.chromium.org/p/chromium/issues/detail?id=634510

https://codereview.chromium.org/2191183003/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js
(right):

https://codereview.chromium.org/2191183003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:40:
this._breakpointElements = {};
On 2016/08/11 18:40:18, lushnikov wrote:
> let's use Map and type-annotate it

Done.

https://codereview.chromium.org/2191183003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:67:
populateNodeContextMenu: function(node, contextMenu, createSubMenu)
On 2016/08/11 18:40:18, lushnikov wrote:
> let's move this to the elements's panel

Done.

https://codereview.chromium.org/2191183003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:231:
return `${nodeId}:${type}`;
On 2016/08/11 18:40:18, lushnikov wrote:
> nit: The former was easier to understand

Done.

https://codereview.chromium.org/2191183003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/main/Main.js (right):

https://codereview.chromium.org/2191183003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/main/Main.js:197:
WebInspector.domBreakpointsSidebarPane = new
WebInspector.DOMBreakpointsSidebarPane();
On 2016/08/11 18:40:18, lushnikov wrote:
> the SidebarPane shouldn't be a singleton once there's a singleton model

Done.

https://codereview.chromium.org/2191183003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/DOMBreakpointManager.js
(right):

https://codereview.chromium.org/2191183003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/DOMBreakpointManager.js:51: *
@return {!Map<string, !WebInspector.DOMBreakpointManager.Breakpoint>}
On 2016/08/11 18:40:18, lushnikov wrote:
> why do we return map here and not an array?

I was returning a map since that's how the class was storing it. I've changed it
to return an array, it's slightly more inefficient but hides the Map keys which
should be an implementation detail specific only to this class.

https://codereview.chromium.org/2191183003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/DOMBreakpointManager.js:76: var
breakpoint = new WebInspector.DOMBreakpointManager.Breakpoint(node, type,
this._inspectedURL, enabled);
On 2016/08/11 18:40:18, lushnikov wrote:
> instead of using inspectedURL, we probably should use URL of the node's
document

Done.

https://codereview.chromium.org/2191183003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/DOMBreakpointManager.js:130:
removeBreakpointsForNode: function(node)
On 2016/08/11 18:40:19, lushnikov wrote:
> why is this part of public API? AFAIU It's not used externally

Done.

https://codereview.chromium.org/2191183003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/DOMBreakpointManager.js:148: *
@return {!Object<string, boolean>}
On 2016/08/11 18:40:19, lushnikov wrote:
> return Map<string, boolean>

Done.

https://codereview.chromium.org/2191183003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/DOMBreakpointManager.js:167:
this._breakpoints = new Map();
On 2016/08/11 18:40:19, lushnikov wrote:
> this._breakpoints.clear(); to play nice with closure types

Done.

https://codereview.chromium.org/2191183003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/DOMBreakpointManager.js:252:
var target = /** @type {!WebInspector.Target} */ (event.data);
On 2016/08/11 18:40:18, lushnikov wrote:
> why listen to this event altogether? it appears it should not be needed

Done.

https://codereview.chromium.org/2191183003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/DOMBreakpointManager.js:267:
return `${node.id}__${type}`;
On 2016/08/11 18:40:19, lushnikov wrote:
> let's be consistent and separate with colon

Done.

https://codereview.chromium.org/2191183003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/elements/module.json (right):

https://codereview.chromium.org/2191183003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/elements/module.json:199:
"factoryName": "WebInspector.DOMBreakpointsSidebarPane"
I initially tried className for both elements and sources module.json but I
couldn't debug why one panel would corrupt the UI on the other panel. It's a bit
more inefficient to use factoryName but it works properly.