Followup from review on bug 189687.
<rdar://problem/91630867>
Created attachment 457339 [details] Patch v1.0
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 :)
Created attachment 457411 [details] Patch v1.1
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 :)
Created attachment 457443 [details] Patch v1.2 - Review nit
Created attachment 457484 [details] Patch v1.2 - Correct typos in for loops
Created attachment 457551 [details] [fast-cq] Patch v1.3 - Add reviewer to changelog
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].
Reverted in https://commits.webkit.org/250847@main due to issues between frontend and backend representation of DOM tree.