Bug 239129 – Web Inspector: Clean up `WI.DOMNode` to no longer require the shared `WI.DOMManager` be passed during construction

• Status: REOPENED
• Component: Web Inspector

[Patrick Angle]

Followup from review on bug 189687.

[Radar WebKit Bug Importer]

<rdar://problem/91630867>

[Patrick Angle]

Created attachment 457339 [details]
Patch v1.0

[Devin Rousso]

Comment on attachment 457339 [details]
Patch v1.0

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

I actually had something similar I was working on last night :P

> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:191
> +    setNodeForId(nodeId, node)

NIT: It's probably not necessary to path both the `nodeId` and `node` given that the `node.id` should match the `nodeId`, so we could just use `node.id` instead.

> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:195
> +        this._idToDOMNode[nodeId] = node;

IMO the fact that this is only used in one spot makes me think that having a method for it is maybe overkill.  Yes it's not our usual style to allow for private members to be accessed outside of that class, but perhaps for a one-off like this that's somewhat OK (especially since we don't want this to be used by anyone else).

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:35
> +    constructor(doc, isInShadowTree, payload)

While we're at it, I think we could/should adjust this to be more in line with modern Web Inspector style:
- rename `doc` to `ownerDocument`
- have `payload` as the first parameter as that's really the only one that's required (yes technically `ownerDocument` is normally expected, but the fact that there's a codepath where it's not used (when `this._nodeType === Node.DOCUMENT_NODE`) in my mind means it's not required)
- move `ownerDocument` and `isInShadowTree` to an `options = {}` parameter
resulting in something like `constructor(payload, {ownerDocument, isInShadowTree} = {})`, which should let us get rid of all the `null` provided when creating any `WI.DOMNode`.

And I'd also make these changes to `WI.DOMNode.newOrExistingFromPayload` so that they match :)

[Patrick Angle]

Created attachment 457411 [details]
Patch v1.1

[Devin Rousso]

Comment on attachment 457411 [details]
Patch v1.1

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

r=me, nice!  Thanks for iterating :)

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:60
> +        this.ownerDocument = this._nodeType === Node.DOCUMENT_NODE ? this : ownerDocument;

NIT: probably could/should have a `console.assert(this.ownerDocument, this);` right after :)

[Patrick Angle]

Created attachment 457443 [details]
Patch v1.2 - Review nit

[Patrick Angle]

Created attachment 457484 [details]
Patch v1.2 - Correct typos in for loops

[Patrick Angle]

Created attachment 457551 [details]
[fast-cq] Patch v1.3 - Add reviewer to changelog

[EWS]

Committed r292818 (249598@main): <https://commits.webkit.org/249598@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 457551 [details].

[Patrick Angle]

Reverted in https://commits.webkit.org/250847@main due to issues between frontend and backend representation of DOM tree.