Bug 6590 – Web Inspector shows stale DOM tree if the DOM changes after the inspector has loaded

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Timothy Hatcher]

Watch for DOM mutation events and update the tree when nodes are added or removed.

If the node is currently focused, it should stay in the tree until the user changes to another node. This will 
let you inspect nodes that are short lived.

[Alexey Proskuryakov]

*** Bug 14356 has been marked as a duplicate of this bug. ***

[Adam Roben (:aroben)]

<rdar://problem/5712921>

[Kevin McCullough]

I'm using XHR to modify my DOM and it's additional elements show in the inspector because they were added before the inspector was opened.  This issue is about DOM changes while the inspector is open.

[Anthony Ricaud]

Firebug has also options "Highlight changes" and/or "Scroll changes into view". These are useful options when working on a big document.

[Timothy Hatcher]

I think we will always highlight changes. Not sure about scroll...

[Anthony Ricaud]

Highlighting all the time is not a problem.

Scrolling on user demand (with a button) is really useful.

[Timothy Hatcher]

I have a fix for this.

[Timothy Hatcher]

Created attachment 22407 [details]
Fixes a regression where TreeOutline.findTreeElement

[Timothy Hatcher]

Created attachment 22408 [details]
Added TreeOutline.removeChildAtIndex and TreeElement.removeChildAtIndex

[Timothy Hatcher]

Created attachment 22409 [details]
Added InspectorController.isWindowVisible to the JavaScript class

[Timothy Hatcher]

Created attachment 22410 [details]
Updates the elements DOM tree when nodes are added or removed

[Adam Roben (:aroben)]

Comment on attachment 22408 [details]
Added TreeOutline.removeChildAtIndex and TreeElement.removeChildAtIndex

+        will be used in an upcomming change.

Typo: upcomming -> upcoming

r=me

[Adam Roben (:aroben)]

Comment on attachment 22409 [details]
Added InspectorController.isWindowVisible to the JavaScript class

+    InspectorController* controller = reinterpret_cast<InspectorController*>(JSObjectGetPrivate(thisObject));

This can be a static_cast

r=me

[Adam Roben (:aroben)]

Comment on attachment 22407 [details]
Fixes a regression where TreeOutline.findTreeElement

r=me

[Timothy Hatcher]

Created attachment 22443 [details]
Updates the elements DOM tree when nodes are added or removed

This version works with subframes.

[Adam Roben (:aroben)]

Comment on attachment 22443 [details]
Updates the elements DOM tree when nodes are added or removed

+    _addMutationEventListeners: function(moniteredWindow)
+    _removeMutationEventListeners: function(moniteredWindow)
+    updateMutationEventListeners: function(moniteredWindow)
+    registerMutationEventListeners: function(moniteredWindow)
+    unregisterMutationEventListeners: function(moniteredWindow)

Typo: monitered -> monitored

+                    for (var i = (treeChildIndex + 1); i < treeElement.children.length; ++i) {
+                        if (sameObjects(treeElement.children[i].representedObject, child)) {
+                            existingTreeElement = treeElement.children[i];
+                            break;
+                        }
+                    }

I wonder if this check is really worth it, performance-wise. I guess we need it to preserve the selected element.

+        // Remove any tree elements that no longer have this node (or this node's contentDocument) as their parent.
+        for (var i = (this.children.length - 1); i > 0; --i) {

Can't we skip this in the fullRefresh case?

I don't understand the "or this nodes contentDocument" part. Why do we want to leave these top-level nodes in?

+            if (currentNode.contentDocument)
+                this.treeOutline.panel.unregisterMutationEventListeners(currentNode.contentDocument.defaultView);

Why is this correct? It seems like it will cause us to stop listening for mutation events in any frame where a node is removed.

+TreeElement.prototype.hasAncestor = function(ancestor) {

Maybe isDescendantOf would be a better name? I guess either is fine.

I'd really like to see some tests landed with this patch.

I'll wait until you answer my questions to r+ this, but it looks good (and was surprisingly easy to follow)!

[Timothy Hatcher]

(In reply to comment #16)
> (From update of attachment 22443 [details] [edit])
> +                    for (var i = (treeChildIndex + 1); i <
> treeElement.children.length; ++i) {
> +                        if
> (sameObjects(treeElement.children[i].representedObject, child)) {
> +                            existingTreeElement = treeElement.children[i];
> +                            break;
> +                        }
> +                    }
> 
> I wonder if this check is really worth it, performance-wise. I guess we need it
> to preserve the selected element.

It is not just selection, but it prevents throwing away whole subtrees that did not change. Otherwise we will be recreating many more tree elements each time and unrelated node changes. This really isn't hit that often in  the common cases of appending an inserting. It might happen once, but the next iteration elements are shifted and line up again, so objectsAreSame(currentTreeElement.representedObject, child) will be true and nothing will be needed.

> +        // Remove any tree elements that no longer have this node (or this
> node's contentDocument) as their parent.
> +        for (var i = (this.children.length - 1); i > 0; --i) {
> 
> Can't we skip this in the fullRefresh case?

It is effectively skipped in the fullRefresh case since at the start of updateChildren when doing a full refresh, we do this.removeChildren(). So this.children.length will be zero by this point in the code.

> I don't understand the "or this nodes contentDocument" part. Why do we want to
> leave these top-level nodes in?

Because we append children of this node and child of this node's contentDocument. So we need to keep the child if it has either of those parents.

        if (this.representedObject.contentDocument)
            updateChildrenOfNode(this.representedObject.contentDocument);
        updateChildrenOfNode(this.representedObject);

> 
> +            if (currentNode.contentDocument)
> +               
> this.treeOutline.panel.unregisterMutationEventListeners(currentNode.contentDocument.defaultView);
> 
> Why is this correct? It seems like it will cause us to stop listening for
> mutation events in any frame where a node is removed.

No, not any node. Any time a frame or object is removed only, since only they have contentDocuments.

> I'd really like to see some tests landed with this patch.

I will clean up my test that I have locally. And land it.

[Adam Roben (:aroben)]

(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 22443 [details] [edit] [edit])
> > +                    for (var i = (treeChildIndex + 1); i <
> > treeElement.children.length; ++i) {
> > +                        if
> > (sameObjects(treeElement.children[i].representedObject, child)) {
> > +                            existingTreeElement = treeElement.children[i];
> > +                            break;
> > +                        }
> > +                    }
> > 
> > I wonder if this check is really worth it, performance-wise. I guess we need it
> > to preserve the selected element.
> 
> It is not just selection, but it prevents throwing away whole subtrees that did
> not change. Otherwise we will be recreating many more tree elements each time
> and unrelated node changes. This really isn't hit that often in  the common
> cases of appending an inserting. It might happen once, but the next iteration
> elements are shifted and line up again, so
> objectsAreSame(currentTreeElement.representedObject, child) will be true and
> nothing will be needed.

OK.

> > +        // Remove any tree elements that no longer have this node (or this
> > node's contentDocument) as their parent.
> > +        for (var i = (this.children.length - 1); i > 0; --i) {
> > 
> > Can't we skip this in the fullRefresh case?
> 
> It is effectively skipped in the fullRefresh case since at the start of
> updateChildren when doing a full refresh, we do this.removeChildren(). So
> this.children.length will be zero by this point in the code.

OK.

> > I don't understand the "or this nodes contentDocument" part. Why do we want to
> > leave these top-level nodes in?
> 
> Because we append children of this node and child of this node's
> contentDocument. So we need to keep the child if it has either of those
> parents.
> 
>         if (this.representedObject.contentDocument)
>             updateChildrenOfNode(this.representedObject.contentDocument);
>         updateChildrenOfNode(this.representedObject);
> 
> > 
> > +            if (currentNode.contentDocument)
> > +               
> > this.treeOutline.panel.unregisterMutationEventListeners(currentNode.contentDocument.defaultView);
> > 
> > Why is this correct? It seems like it will cause us to stop listening for
> > mutation events in any frame where a node is removed.
> 
> No, not any node. Any time a frame or object is removed only, since only they
> have contentDocuments.

Ah, I was confusing contentDocument with ownerDocument. Makes sense now.

> > I'd really like to see some tests landed with this patch.
> 
> I will clean up my test that I have locally. And land it.

Great!

r=me

[Timothy Hatcher]

Landed in r35312, r35313, r35314 and r35317.

[Timothy Hatcher]

Updating when attributes change has ben filed as bug 20162.

[Timothy Hatcher]

Highlighting nodes as they change is filed as bug 20163.