| Summary: | Web Inspector: Clean up `WI.DOMNode` to no longer require the shared `WI.DOMManager` be passed during construction | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Patrick Angle <pangle> | ||||||||||||
| Component: | Web Inspector | Assignee: | Patrick Angle <pangle> | ||||||||||||
| Status: | REOPENED --- | ||||||||||||||
| Severity: | Normal | CC: | ews-watchlist, hi, inspector-bugzilla-changes, webkit-bug-importer | ||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||
| Hardware: | All | ||||||||||||||
| OS: | All | ||||||||||||||
| Bug Depends on: | 189687 | ||||||||||||||
| Bug Blocks: | |||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Patrick Angle
2022-04-12 09:25:33 PDT
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. |