[webkit-reviews] review granted: [Bug 227774] Web Inspector: Elements Tab DOM Tree does not update when a top-level item in the DOM tree is added/removed : [Attachment 433085] Patch v1.0

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Patrick Angle <pangle at apple.com>'s
request for review:
Bug 227774: Web Inspector: Elements Tab DOM Tree does not update when a
top-level item in the DOM tree is added/removed
https://bugs.webkit.org/show_bug.cgi?id=227774

Attachment 433085: Patch v1.0

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




--- Comment #2 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 433085
  --> https://bugs.webkit.org/attachment.cgi?id=433085
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 ��