[webkit-reviews] review granted: [Bug 215795] Web Inspector: allow DOM breakpoints to be configured : [Attachment 407948] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has granted Devin Rousso <drousso at apple.com>'s
request for review:
Bug 215795: Web Inspector: allow DOM breakpoints to be configured
https://bugs.webkit.org/show_bug.cgi?id=215795

Attachment 407948: Patch

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




--- Comment #8 from Brian Burg <bburg at apple.com> ---
Comment on attachment 407948
  --> https://bugs.webkit.org/attachment.cgi?id=407948
Patch

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

r=me, nice work!

> Source/WebCore/inspector/agents/page/PageDOMDebuggerAgent.cpp:-121
> -    if (rootBit & inheritableDOMBreakpointTypesMask) {

Wow this is gross

> Source/WebCore/inspector/agents/page/PageDOMDebuggerAgent.cpp:171
> +static int calculateDistance(Node& child, Node& ancestor)

Ugh, -1 is back. Can we use Optional and nullopt to represent this?

> Source/WebCore/inspector/agents/page/PageDOMDebuggerAgent.cpp:-177
> -    if (hasBreakpoint(&parent, SubtreeModified)) {

Can we do an empty hashmap check here for the fast path?

> Source/WebCore/inspector/agents/page/PageDOMDebuggerAgent.cpp:196
> +    for (auto [breakpointOwner, breakpoint] :
m_domSubtreeModifiedBreakpoints) {

Can you explain with a comment or Changelog comment about what the algorithm
is? Roughly, "look for the closest DOM node that has subtreeModified
breakpoint".

> Source/WebCore/inspector/agents/page/PageDOMDebuggerAgent.cpp:224
> +    int closestDistance = unrelated;

Can we do an empty hashmap check here for the fast path?

> Source/WebCore/inspector/agents/page/PageDOMDebuggerAgent.cpp:283
> +    Vector<Node*> stack({ child });

Needs high-level description of the algorithm.

> Source/WebCore/inspector/agents/page/PageDOMDebuggerAgent.cpp:297
> +	   if (auto* nextSibling = InspectorDOMAgent::innerNextSibling(child))

Nice.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:302
> +		   // We should get the target associated with the
nodeIdentifier of this breakpoint.

Nit: "we should.." sort of implies that we don't. Do we? Should this be a
FIXME?

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:543
> +	       this._ignoreDOMBreakpointDOMNodeWillChange = true;

Nit: It would be better to name this after the state being entered/exited, such
as 'this._clearingDOMBreakpoints'.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:798
> +	   // We should get the target associated with the nodeIdentifier of
this breakpoint.

Ditto above.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:821
> +	   this._restoringBreakpoints = true;

I like this in preference to this._ignoreFooBar.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:822
> +	   // We should get the target associated with the nodeIdentifier of
this breakpoint.

Ditto above.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:994
> +	       this._ignoreDOMBreakpointDOMNodeWillChange = true;

Ditto above comment.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:1019
> +	       if (!breakpoint.domNodeIdentifier)

This could be expressed using Array.prototype.filter

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:1036
> +		       resolvableBreakpoints.splice(i, 1);

�� Mutating while iterating ��

Is there a way to avoid this? It just makes me super anxious to do this on
purpose.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:1063
> +		   this._ignoreDOMBreakpointDOMNodeWillChange = true;

Ditto above comment.

> Source/WebInspectorUI/UserInterface/Models/DOMBreakpoint.js:66
> +	   console.assert();

This should have a message to assist in grepping for error messages ;-)

> LayoutTests/inspector/dom-debugger/attribute-modified-style.html:120
> +	   testCaseNamePrefix: "Style.",

So nice.