Web Inspector: DOMTreeManager dispatches DocumentUpdated twice when the document is cleared

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

Summary:
DOMTreeManager dispatches DocumentUpdated twice when the document is cleared.

Note:
The backend is dispatching two DOM.documentUpdated events, which seems odd, but the frontend should handle it gracefully regardless.

[Matt Baker]

Created attachment 302974 [details]
Patch

[Matt Baker]

Created attachment 302977 [details]
[Test] LayoutTests/inspector/dom/dom-document-updated-event.html

The layout test intended for this patch times out. It looks like the DocumentUpdated event is never dispatched.

[Devin Rousso]

Comment on attachment 302974 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/DOMTreeManager.js:209
> +        if (this._document === newDocument)

I might be wrong about this, but I thought that comparing Objects doesn't work unless both variables literally hold the same object.  As such, wouldn't this if statement never trigger?

let a = {x: 1, y: 2};
let b = {x: 1, y: 2};
console.log(a === b); // false

[Matt Baker]

Created attachment 302978 [details]
[Test] LayoutTests/inspector/dom/dom-document-updated-event.html

Fix typo in test.

[Matt Baker]

Comment on attachment 302974 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/DOMTreeManager.js:209
>> +        if (this._document === newDocument)
> 
> I might be wrong about this, but I thought that comparing Objects doesn't work unless both variables literally hold the same object.  As such, wouldn't this if statement never trigger?
> 
> let a = {x: 1, y: 2};
> let b = {x: 1, y: 2};
> console.log(a === b); // false

You are correct. This addresses the case where the document is null, and being set to null again.

[Matt Baker]

(In reply to comment #2)
> Created attachment 302977 [details]
> [Test] LayoutTests/inspector/dom/dom-document-updated-event.html
> 
> The layout test intended for this patch times out. It looks like the
> DocumentUpdated event is never dispatched.

The backend only dispatches DocumentUpdated is the document has been requested. Rewriting test.

[Timothy Hatcher]

Comment on attachment 302974 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Controllers/DOMTreeManager.js:209
>>> +        if (this._document === newDocument)
>> 
>> I might be wrong about this, but I thought that comparing Objects doesn't work unless both variables literally hold the same object.  As such, wouldn't this if statement never trigger?
>> 
>> let a = {x: 1, y: 2};
>> let b = {x: 1, y: 2};
>> console.log(a === b); // false
> 
> You are correct. This addresses the case where the document is null, and being set to null again.

Matt is right. This would also just start working in the future if `new WebInspector.DOMNode` started returning the same (cached) object with the same node id.

[WebKit Commit Bot]

Comment on attachment 302974 [details]
Patch

Clearing flags on attachment: 302974

Committed r213644: <http://trac.webkit.org/changeset/213644>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.