Bug 227774 – Web Inspector: Elements Tab DOM Tree does not update when a top-level item in the DOM tree is added/removed

• Component: Web Inspector
• Status: RESOLVED FIXED

[Patrick Angle]

<rdar://31137058>

[Patrick Angle]

Created attachment 433085 [details]
Patch v1.0

[Devin Rousso]

Comment on attachment 433085 [details]
Patch v1.0

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

r=me

> Source/WebInspectorUI/UserInterface/Views/DOMTreeUpdater.js:134
> +            if (parentNode.attached && !parentNode.parentNode) {

I think the only way for this to be true is if `parentNode` is the document, so it might be better to directly check for that instead (especially since we probably will eventually move to a future where nodes can be sent to the frontend without including parent info, so `attached` might be `true` even if the `parentNode` is `null`).  That way you could also rename it to `documentNeedsUpdate`, which IMO is more understandable/reasonable as a reason to update the entire `WI.DOMTreeOutline`.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeUpdater.js:151
> +            this._recentlyModifiedNodes.clear();
> +            this._recentlyModifiedAttributes.clear();

NIT: Instead of duplicating these `clear()` here, you could take the entire `for` below and put it inside an `else` for this.  I personally try to avoid early returns like this cause it means there's more state one has to keep track of when creating branches in the future, whereas if there's no `return` then everything always flows down the same path to the same end.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeUpdater.js:167
> -                return;
> +                continue;

LOL oops wonder how many other problems this caused 😅

[Patrick Angle]

Comment on attachment 433085 [details]
Patch v1.0

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

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeUpdater.js:167
>> +                continue;
> 
> LOL oops wonder how many other problems this caused 😅

Such a good bug, I later fixed it anyways apparently https://bugs.webkit.org/show_bug.cgi?id=230289

[Patrick Angle]

Pull request: https://github.com/WebKit/WebKit/pull/3914

[EWS]

Committed 254062@main (f6c535899310): <https://commits.webkit.org/254062@main>

Reviewed commits have been landed. Closing PR #3914 and removing active labels.