[webkit-reviews] review denied: [Bug 199332] Web Inspector: DOM Debugger: descendant breakpoints should be able to be enabled/disabled/deleted from a collapsed parent : [Attachment 373161] Patch

[bugzilla-daemon at webkit.org]

Matt Baker <mattbaker at apple.com> has denied Devin Rousso <drousso at apple.com>'s
request for review:
Bug 199332: Web Inspector: DOM Debugger: descendant breakpoints should be able
to be enabled/disabled/deleted from a collapsed parent
https://bugs.webkit.org/show_bug.cgi?id=199332

Attachment 373161: Patch

https://bugs.webkit.org/attachment.cgi?id=373161&action=review




--- Comment #5 from Matt Baker <mattbaker at apple.com> ---
Comment on attachment 373161
  --> https://bugs.webkit.org/attachment.cgi?id=373161
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373161&action=review

r-, only because I'm concerned about selecting the descendant DOM nodes when
revealing breakpoints. Other than that, this is awesome!

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:408
> +localizedStrings["Enable Descendant Breakpoints"] = "Enable Descendant
Breakpoints";

IMO, having "Descendant" in the context menu items makes the menu appear very
busy/cluttered. It's much more clear though, so I think it is a worthwhile
tradeoff.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:150
> +		   resolvedBreakpoints =
resolvedBreakpoints.concat(Array.from(domBreakpointNodeIdentifierMap.values()))
;

Nice cleanup.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:179
> +	   return breakpoints ? Array.from(breakpoints) : [];

Ditto!

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:189
> +	       let children = node.children.slice();

Why not `let children = Array.from(node.children)`?

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:454
> +		       console.assert(breakpoint.domNodeIdentifier ===
nodeIdentifier);

Nice catch! Did you encounter a situation where a DOM breakpoint was resolved
more than once?

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:121
> +	      
this.treeOutline.selectTreeElements(subtreeBreakpointTreeElements);

I don't think we want to select nodes in the DOM tree for the revealed
breakpoints. It could unexpectedly change an existing selection. What about
extending the animated highlight to all the revealed tree elements?