DevTools: refactor and fix bugs for DOMBreakpointsSidebarPane

DevTools: refactor and fix bugs for DOMBreakpointsSidebarPane

I'm splitting up my original monolithic CL into multiple small CLs.
For reference:
https://codereview.chromium.org/2191183003 (closed, not landed)

Plan:
Cl #1 (current) - Extract DOMBreakpointsManager from DOMBreakpointsSidebarPane within the same file
CL #2 - Move DOMBreakpointsManager into another file and remove DOM Breakpoints Proxy
CL #3 - Extract populateNodeContextMenu into ElementsPanel and
createBreakpointHitStatusMessage into CallStackSidebarPane

BUG=627296, 634513, 634510

[chenwilliam, 2016-08-17 22:32:11]

Description was changed from

==========
DevTools: refactor and fix bugs for DOMBreakpointsSidebarPane

I'm splitting up my original monolithic CL into multiple small CLs.
For reference:
https://codereview.chromium.org/2191183003 (closed, not landed)

Plan:
Cl #1 (current) - Extract DOMBreakpointsManager from DOMBreakpointsSidebarPane
within the same file
CL #2 - Move DOMBreakpointsManager into another file and remove DOM Breakpoints
Proxy
CL #3 - Extract populateNodeContextMenu into ElementsPanel and
createBreakpointHitStatusMessage into CallStackSidebarPane

BUG=627296, 634513, 634510
==========

to

==========
DevTools: refactor and fix bugs for DOMBreakpointsSidebarPane

I'm splitting up my original monolithic CL into multiple small CLs.
For reference:
https://codereview.chromium.org/2191183003 (closed, not landed)

Plan:
Cl #1 (current) - Extract DOMBreakpointsManager from DOMBreakpointsSidebarPane
within the same file
CL #2 - Move DOMBreakpointsManager into another file and remove DOM Breakpoints
Proxy
CL #3 - Extract populateNodeContextMenu into ElementsPanel and
createBreakpointHitStatusMessage into CallStackSidebarPane

BUG=627296, 634513, 634510
==========

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

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

[chenwilliam, 2016-08-17 23:02:50]

I thought it would be cleaner to just start from a fresh CL and close the old
one: 
https://codereview.chromium.org/2191183003

Let me know what you think of my plan for the series of CLs in the issue
description.

https://codereview.chromium.org/2255983002/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js
(right):

https://codereview.chromium.org/2255983002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:429:
if (!inspectedRootDocument)
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?

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

can we split this patch even further to reduce it's size?

https://codereview.chromium.org/2255983002/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js
(right):

https://codereview.chromium.org/2255983002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:254:
WebInspector.DOMBreakpointsSidebarPane.Proxy = function()
let's move this below the WI.DOMBreakpointManager so that diff is smaller

[chenwilliam, 2016-08-18 22:26:51]

PTAL. I've reduced some of the changes (keeping more of the methods in
sidebarPane where feasible) and made the diff smaller & easier to read.

Any advice on my comment/question in the original patchset? Thanks.

https://codereview.chromium.org/2255983002/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js
(right):

https://codereview.chromium.org/2255983002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:254:
WebInspector.DOMBreakpointsSidebarPane.Proxy = function()
On 2016/08/18 21:16:34, lushnikov wrote:
> let's move this below the WI.DOMBreakpointManager so that diff is smaller

Done.

[lushnikov, 2016-08-22 18:44:25]

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

https://codereview.chromium.org/2255983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:61:
this._inspectedURL = WebInspector.targetManager.inspectedURL();
so now instead of relying on inspectedURL to save DOMBreakpoints you rely on
node.ownerDocument url?
Let's make this change separately!

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

https://codereview.chromium.org/2255983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:359:
this._innerSetBreakpoint(node, type, enabled);
consider the following scenario:
1. You have a page with multiple iframes of the same website
2. You select "modification subtree" DOMBreakpoint in one of them
Q: Does it appear in another one?

From one point of view, it should not. However, you wouldn't be able to restore
breakpoint correctly (there's no way for you to differentiate between these
iframes). 
So I would say that setting breakpoint in one iframe should set it in others as
well (this is consistent with js breakpoints)

https://codereview.chromium.org/2255983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:427:
this.dispatchEventToListeners(WebInspector.DOMBreakpointManager.Events.BreakpointsChanged);
let's not send this event twice

https://codereview.chromium.org/2255983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:447:
if (breakpoint.url !== WebInspector.targetManager.inspectedURL())
who gives you guarantee that inspectedURL is already set? This is something that
happens quite late, in the end of ResourceTreeModel cached resource processing
(see WI.ResourceTreeModel._processCachedResources)

[chenwilliam, 2016-08-22 22:01:46]

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

https://codereview.chromium.org/2255983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:61:
this._inspectedURL = WebInspector.targetManager.inspectedURL();
On 2016/08/22 18:44:25, lushnikov wrote:
> so now instead of relying on inspectedURL to save DOMBreakpoints you rely on
> node.ownerDocument url?
> Let's make this change separately!

Done.

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

https://codereview.chromium.org/2255983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:359:
this._innerSetBreakpoint(node, type, enabled);
On 2016/08/22 18:44:25, lushnikov wrote:
> consider the following scenario:
> 1. You have a page with multiple iframes of the same website
> 2. You select "modification subtree" DOMBreakpoint in one of them
> Q: Does it appear in another one?
> 
> From one point of view, it should not. However, you wouldn't be able to
restore
> breakpoint correctly (there's no way for you to differentiate between these
> iframes). 
> So I would say that setting breakpoint in one iframe should set it in others
as
> well (this is consistent with js breakpoints)

I reproduced this scenario and it treats the different iframes separately
because they have different path (e.g.
"0,HTML,1,BODY,4,DIV,1,IFRAME,0,#document,0,HTML"). There's an existing bug
where on refresh, the stored breakpoints for the iframe disappear. After calling
pushNodeByPathToFrontend with the iframe node's path, the nodeId gets returned
as null. Not sure why it doesn't work, but this issue occurs in current tip of
tree. Could we tackle this later as this CL doesn't worsen this issue?

https://codereview.chromium.org/2255983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:427:
this.dispatchEventToListeners(WebInspector.DOMBreakpointManager.Events.BreakpointsChanged);
On 2016/08/22 18:44:25, lushnikov wrote:
> let's not send this event twice

I included this because if there's no DOM breakpoints on the inspected page then
the dispatchEvent inside didPushNodeByPathToFrontend never gets called.

https://codereview.chromium.org/2255983002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:447:
if (breakpoint.url !== WebInspector.targetManager.inspectedURL())
On 2016/08/22 18:44:25, lushnikov wrote:
> who gives you guarantee that inspectedURL is already set? This is something
that
> happens quite late, in the end of ResourceTreeModel cached resource processing
> (see WI.ResourceTreeModel._processCachedResources)

Done.