Bug 6590

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

<rdar://problem/5712921>

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.

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

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

Highlighting all the time is not a problem. Scrolling on user demand (with a button) is really useful.

I have a fix for this.

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

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

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

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

Comment on attachment 22408 [details] Added TreeOutline.removeChildAtIndex and TreeElement.removeChildAtIndex + will be used in an upcomming change. Typo: upcomming -> upcoming r=me

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 on attachment 22407 [details] Fixes a regression where TreeOutline.findTreeElement r=me

Created attachment 22443 [details] Updates the elements DOM tree when nodes are added or removed This version works with subframes.

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)!

(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.

(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

Landed in r35312, r35313, r35314 and r35317.

Updating when attributes change has ben filed as bug 20162.

Highlighting nodes as they change is filed as bug 20163.