Bug 168984 - Web Inspector: DOMTreeManager dispatches DocumentUpdated twice when the document is cleared
Summary: Web Inspector: DOMTreeManager dispatches DocumentUpdated twice when the docum...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Matt Baker
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-02-28 12:52 PST by Matt Baker
Modified: 2017-03-09 09:40 PST (History)
2 users (show)

See Also:


Attachments
Patch (2.03 KB, patch)
2017-02-28 13:38 PST, Matt Baker
no flags Details | Formatted Diff | Diff
[Test] LayoutTests/inspector/dom/dom-document-updated-event.html (1.11 KB, text/html)
2017-02-28 13:46 PST, Matt Baker
no flags Details
[Test] LayoutTests/inspector/dom/dom-document-updated-event.html (1.10 KB, text/html)
2017-02-28 13:47 PST, Matt Baker
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Baker 2017-02-28 12:52:30 PST
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.
Comment 1 Matt Baker 2017-02-28 13:38:25 PST
Created attachment 302974 [details]
Patch
Comment 2 Matt Baker 2017-02-28 13:46:33 PST
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.
Comment 3 Devin Rousso 2017-02-28 13:47:20 PST
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
Comment 4 Matt Baker 2017-02-28 13:47:54 PST
Created attachment 302978 [details]
[Test] LayoutTests/inspector/dom/dom-document-updated-event.html

Fix typo in test.
Comment 5 Matt Baker 2017-02-28 13:49:11 PST
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.
Comment 6 Matt Baker 2017-03-06 18:53:57 PST
(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.
Comment 7 Timothy Hatcher 2017-03-09 09:12:43 PST
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.
Comment 8 WebKit Commit Bot 2017-03-09 09:40:09 PST
Comment on attachment 302974 [details]
Patch

Clearing flags on attachment: 302974

Committed r213644: <http://trac.webkit.org/changeset/213644>
Comment 9 WebKit Commit Bot 2017-03-09 09:40:13 PST
All reviewed patches have been landed.  Closing bug.