Bug 6590 - Web Inspector shows stale DOM tree if the DOM changes after the inspector has loaded
Summary: Web Inspector shows stale DOM tree if the DOM changes after the inspector has...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 420+
Hardware: All All
: P2 Enhancement
Assignee: Timothy Hatcher
URL:
Keywords: InRadar
: 14356 (view as bug list)
Depends on: 8191 18041
Blocks:
  Show dependency treegraph
 
Reported: 2006-01-16 10:43 PST by Timothy Hatcher
Modified: 2008-07-24 11:10 PDT (History)
4 users (show)

See Also:


Attachments
Fixes a regression where TreeOutline.findTreeElement (2.33 KB, patch)
2008-07-21 09:50 PDT, Timothy Hatcher
aroben: review+
Details | Formatted Diff | Diff
Added TreeOutline.removeChildAtIndex and TreeElement.removeChildAtIndex (4.43 KB, patch)
2008-07-21 09:50 PDT, Timothy Hatcher
aroben: review+
Details | Formatted Diff | Diff
Added InspectorController.isWindowVisible to the JavaScript class (3.18 KB, patch)
2008-07-21 09:51 PDT, Timothy Hatcher
aroben: review+
Details | Formatted Diff | Diff
Updates the elements DOM tree when nodes are added or removed (12.43 KB, patch)
2008-07-21 09:52 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Updates the elements DOM tree when nodes are added or removed (27.08 KB, patch)
2008-07-22 18:52 PDT, Timothy Hatcher
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2006-01-16 10:43:25 PST
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.
Comment 1 Alexey Proskuryakov 2007-06-24 00:55:08 PDT
*** Bug 14356 has been marked as a duplicate of this bug. ***
Comment 2 Adam Roben (:aroben) 2008-01-29 11:13:17 PST
<rdar://problem/5712921>
Comment 3 Kevin McCullough 2008-03-11 10:42:47 PDT
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.
Comment 4 Anthony Ricaud 2008-05-18 18:33:14 PDT
Firebug has also options "Highlight changes" and/or "Scroll changes into view". These are useful options when working on a big document.
Comment 5 Timothy Hatcher 2008-07-17 22:50:29 PDT
I think we will always highlight changes. Not sure about scroll...
Comment 6 Anthony Ricaud 2008-07-18 06:41:21 PDT
Highlighting all the time is not a problem.

Scrolling on user demand (with a button) is really useful.
Comment 7 Timothy Hatcher 2008-07-21 09:43:07 PDT
I have a fix for this.
Comment 8 Timothy Hatcher 2008-07-21 09:50:09 PDT
Created attachment 22407 [details]
Fixes a regression where TreeOutline.findTreeElement
Comment 9 Timothy Hatcher 2008-07-21 09:50:39 PDT
Created attachment 22408 [details]
Added TreeOutline.removeChildAtIndex and TreeElement.removeChildAtIndex
Comment 10 Timothy Hatcher 2008-07-21 09:51:13 PDT
Created attachment 22409 [details]
Added InspectorController.isWindowVisible to the JavaScript class
Comment 11 Timothy Hatcher 2008-07-21 09:52:07 PDT
Created attachment 22410 [details]
Updates the elements DOM tree when nodes are added or removed
Comment 12 Adam Roben (:aroben) 2008-07-21 10:06:37 PDT
Comment on attachment 22408 [details]
Added TreeOutline.removeChildAtIndex and TreeElement.removeChildAtIndex

+        will be used in an upcomming change.

Typo: upcomming -> upcoming

r=me
Comment 13 Adam Roben (:aroben) 2008-07-21 10:07:34 PDT
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
Comment 14 Adam Roben (:aroben) 2008-07-21 10:07:45 PDT
Comment on attachment 22407 [details]
Fixes a regression where TreeOutline.findTreeElement

r=me
Comment 15 Timothy Hatcher 2008-07-22 18:52:23 PDT
Created attachment 22443 [details]
Updates the elements DOM tree when nodes are added or removed

This version works with subframes.
Comment 16 Adam Roben (:aroben) 2008-07-23 13:53:17 PDT
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)!
Comment 17 Timothy Hatcher 2008-07-23 14:05:41 PDT
(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.
Comment 18 Adam Roben (:aroben) 2008-07-23 14:09:27 PDT
(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
Comment 19 Timothy Hatcher 2008-07-24 08:24:47 PDT
Landed in r35312, r35313, r35314 and r35317.
Comment 20 Timothy Hatcher 2008-07-24 11:07:47 PDT
Updating when attributes change has ben filed as bug 20162.
Comment 21 Timothy Hatcher 2008-07-24 11:10:23 PDT
Highlighting nodes as they change is filed as bug 20163.