[webkit-reviews] review granted: [Bug 189687] Web Inspector: preserve DOM.NodeId if a node is removed and re-added : [Attachment 457006] Patch v1.3 - For Review

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Patrick Angle <pangle at apple.com>'s
request for review:
Bug 189687: Web Inspector: preserve DOM.NodeId if a node is removed and
re-added
https://bugs.webkit.org/show_bug.cgi?id=189687

Attachment 457006: Patch v1.3 - For Review

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




--- Comment #20 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 457006
  --> https://bugs.webkit.org/attachment.cgi?id=457006
Patch v1.3 - For Review

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

r=me, nnniiiccceee =D

> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:-984
> -	   // FIXME: <https://webkit.org/b/189687> Preserve DOM.NodeId if a
node is removed and re-added

The presence of this FIXME implies that something needed to be done here to
support that.  Is that true, or was this an erroneous FIXME?

> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:186
> +    get boundNodesDebugDescription()

NIT: If this is only for testing, I'd put this at the very bottom in a new `//
Testing` section.  Or even better, you could have a
`Object.defineProperty(WI.DOMManager.prototype, "boundNodesDebugDescription. {
get() { ... } })` inside
`Source/WebInspectorUI/UserInterface/Debug/Bootstrap.js` (or a new
`DOMManager.js` in that same folder) so that we don't ship this code to
customers :)

Also, it seems like this isn't used by this patch.  Can we split this into a
separate patch instead?

> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:514
> +    _pseudoElementAdded(parentId, pseudoElementPayload)

NIT: The name in the protocol actually is `pseudoElement`, so it would be kinda
nice to keep it that way (though I do agree that it was badly named in the
first place).

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:166
> +    static newOrExistingFromPayload(domManager, document, payload,
{isInShadowTree} = {})

NIT: While I appreciate that you're following the existing pattern of passing
in the `WI.DOMManager` instance to the `WI.DOMNode`, I kinda feel like that's
old behavior/logic and probably shouldn't be propagated.  I'd remove the
`domManager` parameter and instead just inline `WI.domManager` where needed. 
(And we should also probably have a followup to remove `_domManager` from
`WI.DOMNode`.)

>
LayoutTests/inspector/dom-debugger/dom-breakpoint-node-removed-ancestor-expecte
d.txt:20
> +Breakpoint "domNode" set to "null".
> +Breakpoint "domNode" set to "div#node-removed-ancestor-test".

Hmm the fact that this is dispatching an event about the `domNode` being set to
`null` makes me think that it was set to something before, and then is reset? 
That kinda seems odd.  Especially since this seems to happen for _every_ event
dispatch.  Most of the tests involving `WI.DOMBreakpoint` seem to provide a
`WI.DOMNode` up front, so not really sure why it'd ever be set to `null`.  Can
you figure out what's going on here?

> LayoutTests/inspector/dom-debugger/resources/dom-breakpoint-utilities.js:62
> +    InspectorTest.DOMBreakpoint._handleDOMNodeDidChange = function(event) {

NIT: If this isn't used outside of this file, I'd just make it `function
handleDOMNodeDidChange` (at the top of the file) instead of attaching it to
`InspectorTest.DOMBreakpoint`.