Make the DOM Nodes viewer currentIndex-clean and do more intelligent rebuilding

RESOLVED FIXED

Status

Other Applications
DOM Inspector
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: crussell, Assigned: crussell)

Tracking

(Blocks: 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

See bug 533124.  Pull out the appropriate pieces of attachment 561734 [details] from bug 310370.
Blocks: 310370
Created attachment 468925 [details] [diff] [review]
Fix up various places dealing with tree selection.
Assignee: nobody → Sevenspade
Status: NEW → ASSIGNED
Attachment #468925 - Flags: review?(neil)

Comment 2

7 years ago
Comment on attachment 468925 [details] [diff] [review]
Fix up various places dealing with tree selection.

>+      currentNode = new XPCNativeWrapper(viewer.currentNode);
>+      parentNode = currentNode && currentNode.parentNode &&
>+                   new XPCNativeWrapper(currentNode.parentNode);
Generic XPCNativeWrappers are deep, so you don't need to rewrap the parent.

>+    this.mDOMTree.view.selection.currentIndex =
>+      this.getRowIndexFromNode(currentNode);
I couldn't work out whether you could be sure whether the current node was guaranteed to be visible by this point. If not, perhaps showNodeInTree should return the index of the node, thus allowing us to write
this.mDOMTree.view.selection.currentIndex =
  this.showNodeInTree(currentNode, false, false, true, true);
(In reply to comment #2)
> Comment on attachment 468925 [details] [diff] [review]
...
> >+    this.mDOMTree.view.selection.currentIndex =
> >+      this.getRowIndexFromNode(currentNode);
> I couldn't work out whether you could be sure whether the current node was
> guaranteed to be visible by this point.

I was prepared to answer that yes, it would have a corresponding row in the tree, but in certain circumstances where you're going from not showing anons to showing anons, and the insertion point for what was the currentNode is somewhere in the subtree of an anon, then no, it won't have a row, because that anonymous ancestor won't be open.

There's another issue with calling changeSelection on the old viewer selection without checking that it hasn't also gotten filtered out.  In that case, you'd get the kind of weird result of the object pane reflecting a node that's not actually shown in the tree.
Attachment #468925 - Flags: review?(neil)
Created attachment 480435 [details] [diff] [review]
Same idea as before, but more intelligent reselection during rebuilding (phase 1)

A side effect of this is that the bug 481349 error in isCommandEnabled gets fixed. XPCNativeWrapper doesn't like null arguments.
Attachment #468925 - Attachment is obsolete: true
Attachment #480435 - Flags: review?(neil)
Blocks: 481349
Attachment #480435 - Attachment description: Same idea as before, but more intelligent reselection during rebuilding → Same idea as before, but more intelligent reselection during rebuilding (phase 1)
Created attachment 480436 [details] [diff] [review]
Handle cases where viewerSelection and currentNode might be ignorable (phase 2)
Attachment #480436 - Flags: review?(neil)
Oh, another thing.  The next-best chain thing is kind of nice, because it's an algorithm which a lot of pieces have an interest in.  For example, for bug 310370, I was using this algorithm to determine which nodes should be selected after the selected nodes got deleted.  Having the concept of a next-best chain that can be probed is nice, because when bug 310370 gets fixed, getNextInNextBestChain can be trivially extended to take an optional function parameter that would be used in conjunction with isIgnorableNode to further rule out the nodes it considers to be in the next-best chain.  In the case of bug 310370, it would work like

viewer.getNextInNextBestChain(currentNode, function DVr_WillDelete(aNode)
  {
     return deletableNodes.indexOf(aNode) >= 0;
  });

Comment 7

7 years ago
Comment on attachment 480435 [details] [diff] [review]
Same idea as before, but more intelligent reselection during rebuilding (phase 1)

>+   * @param aPosition [optional]
>+   *        aPosition.value will be set to the position of aIndex in
>+   *        aIndexList, or -1 if it cannot be found.
Not sure why you need to return this, as any callers can emulate it.

Comment 8

7 years ago
Comment on attachment 480436 [details] [diff] [review]
Handle cases where viewerSelection and currentNode might be ignorable (phase 2)

>+    var includeAnons = aHint == "inspector.dom.showAnon" ?
>+                         !PrefUtils.getPref("inspector.dom.showAnon") :
>+                         this.mDOMView.showAnonymousContent;
When aHint is "inspector.dom.showAnon" then presumably the caller has already updated this.mDOMView.showAnonymousContent to the current pref value. So you only need to toggle that value, rather than fetching the pref again.

Actually thinking about it maybe it would be easier for onPrefChanged to save the previous value of showAnonymousContent and pass that to rebuild instead.

>+        debug("new viewer selection expected to have row in tree but " +
>+              "doesn't");
[I don't see the point of wrapping the debug message.]

>+    var includeAnons =
>+      aIncludeAnons === undefined ? showAnons : aIncludeAnons;
[Test for undefined is for future expansion?]

Comment 9

7 years ago
Comment on attachment 480435 [details] [diff] [review]
Same idea as before, but more intelligent reselection during rebuilding (phase 1)

>-    if (!aEl) {
>-      bx.view.selection.select(null);
>-      return false;
>+    if (!aNode) {
>+      if (!aAugment && !aNoSelect) {
>+        bx.view.selection.select(null);
>+      }
>+      return;
>     }
[select(null) makes no sense because select takes an integer argument.]

>+    var viewerSelection = this.selection;
>+    if (this.isIgnorableNode(viewerSelection)) {
>+      let idx = this.getRowIndexFromNode(viewerSelection);
>+      let nearestNonIgnorableSelectedIndex =
>+        this.getNearestIndex(idx, nonIgnorableSelectedIndexes);
>+      viewerSelection =
>+        this.getNodeFromRowIndex(nearestNonIgnorableSelectedIndex);
>+    }
>+    // XXX What happens if viewerSelection (and currentNode, for that matter)
>+    // is ignorable?
Didn't you just check for that? ;-)
What if there are no nonIgnorableSelectedIndexes?
Or does the second phase fix these?

>+      while (current && current != this.mDOMView.rootNode &&
>+             current.ownerDocument) {
>+        if (current.ownerDocument.getBindingParent(current)) {
>+          return true;
>+        }
>+        current = this.mDOMUtils.getParentForNode(current.ownerDocument, 
>+                                                  true);
>+      }
current is unlikely to be rootNode since that's normally a document.

>+function cmdEditCopy() {
>+  this.mNode = viewer.selectedNode;
Shouldn't you clone it?
(In reply to comment #7)
> Comment on attachment 480435 [details] [diff] [review]
> Same idea as before, but more intelligent reselection during rebuilding (phase
> 1)
> 
> >+   * @param aPosition [optional]
> >+   *        aPosition.value will be set to the position of aIndex in
> >+   *        aIndexList, or -1 if it cannot be found.
> Not sure why you need to return this, as any callers can emulate it.

Well it's unused right now, but how so?

(In reply to comment #8)
> Comment on attachment 480436 [details] [diff] [review]
...
> >+        debug("new viewer selection expected to have row in tree but " +
> >+              "doesn't");
> [I don't see the point of wrapping the debug message.]

I've been wrapping for 78 columns.

> >+    var includeAnons =
> >+      aIncludeAnons === undefined ? showAnons : aIncludeAnons;
> [Test for undefined is for future expansion?]

Yes.  See comment 6.  Although the signature in that case would be

DVr_GetNextInNextBestChain(aNode, aFilterFn, aIncludeAnons)

> >+    var viewerSelection = this.selection;
> >+    if (this.isIgnorableNode(viewerSelection)) {
> >+      let idx = this.getRowIndexFromNode(viewerSelection);
> >+      let nearestNonIgnorableSelectedIndex =
> >+        this.getNearestIndex(idx, nonIgnorableSelectedIndexes);
> >+      viewerSelection =
> >+        this.getNodeFromRowIndex(nearestNonIgnorableSelectedIndex);
> >+    }
> >+    // XXX What happens if viewerSelection (and currentNode, for that matter)
> >+    // is ignorable?
> Didn't you just check for that? ;-)
> What if there are no nonIgnorableSelectedIndexes?

Yeah, that's what I was referring to.  If nonIgnorableSelectedIndexes is empty, getNearestIndex will return -1, and getNodeFromRowIndex on that will return null, which qualifies as ignorable.  currentNode might be non-null and still ignorable, though.

> Or does the second phase fix these?

Yeah.  The second phase is just making the necessary changes to address these.
 
> >+      while (current && current != this.mDOMView.rootNode &&
> >+             current.ownerDocument) {
> >+        if (current.ownerDocument.getBindingParent(current)) {
> >+          return true;
> >+        }
> >+        current = this.mDOMUtils.getParentForNode(current.ownerDocument, 
> >+                                                  true);
> >+      }
> current is unlikely to be rootNode since that's normally a document.

That's true, but eventually I don't want to keep making those assumptions, since eventually I want to be able to bring up the DOM nodes viewer on arbitrary nodes, e.g., to have a look at subtrees extending from "unrooted" nodes from Venkman.  On that note, I don't really see why we shouldn't be able to inspect those subtrees when we do "Inspect in New Window", either.

> >+function cmdEditCopy() {
> >+  this.mNode = viewer.selectedNode;
> Shouldn't you clone it?

Hmm, maybe.  It gets cloned in the doTransaction method.

Cloning it here works for how we're currently using it with the panelset's command handling, but I'm imagining some hypothetical case where you'd want to be able to instantiate the Copy transaction, then carry out the actual copying at some later time, at which point you'd push it onto the transaction manager's stack or call doTransaction() yourself.  If we clone it in the constructor, you'll lose any mutations that have occurred since instantiation.
(In reply to comment #10)
>(In reply to comment #7)
>>(From update of attachment 480435 [details] [diff] [review])
>>>+   * @param aPosition [optional]
>>>+   *        aPosition.value will be set to the position of aIndex in
>>>+   *        aIndexList, or -1 if it cannot be found.
>>Not sure why you need to return this, as any callers can emulate it.
>Well it's unused right now, but how so?
var nearest = this.getNearestIndex(index, array);
var position = nearest == -1 || array[nearest] != index ? -1 : nearest;

>(In reply to comment #8)
>>(From update of attachment 480436 [details] [diff] [review])
>>>+        debug("new viewer selection expected to have row in tree but " +
>>>+              "doesn't");
>>[I don't see the point of wrapping the debug message.]
>I've been wrapping for 78 columns.
I just didn't think it was worth wrapping a debugging literal string.

>>current is unlikely to be rootNode since that's normally a document.
>That's true, but eventually I don't want to keep making those assumptions,
>since eventually I want to be able to bring up the DOM nodes viewer on
>arbitrary nodes
I should have complained the other way around i.e. current is normally a frame-type element of some sort and therefore unlikely to be equal to something else. But what would work is to bail out if current is a real node in rootNode's document.

>>>+function cmdEditCopy() {
>>>+  this.mNode = viewer.selectedNode;
>>Shouldn't you clone it?
>Hmm, maybe.  It gets cloned in the doTransaction method.
Actually it's not as bad as I first thought, although there is a very small difference between the two implementations, in that redoing the copy clones the node in its current state instead of the state it was in originally.
(In reply to comment #11)
> (In reply to comment #10)
> >(In reply to comment #7)
> >>(From update of attachment 480435 [details] [diff] [review])
> >>>+   * @param aPosition [optional]
> >>>+   *        aPosition.value will be set to the position of aIndex in
> >>>+   *        aIndexList, or -1 if it cannot be found.
> >>Not sure why you need to return this, as any callers can emulate it.
> >Well it's unused right now, but how so?
> var nearest = this.getNearestIndex(index, array);
> var position = nearest == -1 || array[nearest] != index ? -1 : nearest;

array is an array of indexes here, none of which have anything to do with array itself.  Suppose array is [ 21, 22, 24, 25, 26, 29 ].  (Given the callers, these would be the indexes of selected rows in the DOM tree.)  sayingGetNearestIndex(25, array, pos) is going to return 26, which is obviously an invalid index for array itself. pos.value would be 3, because array[3] is 25.

> I should have complained the other way around i.e. current is normally a
> frame-type element of some sort and therefore unlikely to be equal to something
> else. But what would work is to bail out if current is a real node in
> rootNode's document.

I don't understand this.  In particular, it's the "therefore unlikely to be equal to something else" part, and I'm not seeing the implication of/what implication leads to "bail[ing] out if current is a real node in rootNode's document".
(In reply to comment #12)
> sayingGetNearestIndex(25, array, pos) is going to return 26

s/sayingGetNearestIndex/saying getNearestIndex/
(In reply to comment #12)
> Suppose array is [ 21, 22, 24, 25, 26, 29 ].  (Given the callers,
> these would be the indexes of selected rows in the DOM tree.) 
> sayingGetNearestIndex(25, array, pos) is going to return 26

Gah, bad example!  This isn't true.  As the comment states, this will actually return 25, since 25 is in array.  So consider array is [ 21, 22, 24, 26, 29 ].  Then getNearestIndex(25, array, pos) will return 26 and pos.value will be -1, since 25 doesn't appear in the list.  In the flawed example from comment 12, though, pos.value would still be 3.

The whole aPosition thing is really just something I put in to allow getNearestIndex to function as a general purpose indexOf-like method for arrays of ordered, non-negative integers.
(In reply to comment #12)
> > I should have complained the other way around i.e. current is normally a
> > frame-type element of some sort and therefore unlikely to be equal to something
> > else. But what would work is to bail out if current is a real node in
> > rootNode's document.
>I don't understand this.  In particular, it's the "therefore unlikely to be
>equal to something else" part, and I'm not seeing the implication of/what
>implication leads to "bail[ing] out if current is a real node in rootNode's
>document".
I'm trying to say that the test isn't useful, since it's so unlikely to happen in practice. On the other hand, if you reach rootNode's document, and you're not anonymous, then you presumably must be a visible child of rootNode. In fact, you want to avoid checking rootNode's document's frame element, in case that's anonymous! (Aside: I now notice that the showSubDocuments test expects rootNode to be a document...)

(In reply to comment #14)
> So consider array is [ 21, 22, 24, 26, 29 ].  Then
> getNearestIndex(25, array, pos) will return 26 and pos.value will be -1,
> since 25 doesn't appear in the list.
> 
> The whole aPosition thing is really just something I put in to allow
> getNearestIndex to function as a general purpose indexOf-like method for arrays
> of ordered, non-negative integers.
Ah, so it returns the nearest value in the array. I'm not sure it's worth returning both the value and position, but I won't argue the point now.
Created attachment 482111 [details] [diff] [review]
phase 1 - address remarks in comment 7, comment 9, and comment 15
Attachment #480435 - Attachment is obsolete: true
Attachment #482111 - Flags: review?(neil)
Attachment #480435 - Flags: review?(neil)
Created attachment 482112 [details] [diff] [review]
phase 2 - address remarks in comment 8 and fix IsIgnorableNode issue where it would give incorrect values for rootNode
Attachment #480436 - Attachment is obsolete: true
Attachment #482112 - Flags: review?(neil)
Attachment #480436 - Flags: review?(neil)

Updated

7 years ago
Attachment #482111 - Flags: review?(neil) → review+
Comment on attachment 482112 [details] [diff] [review]
phase 2 - address remarks in comment 8 and fix IsIgnorableNode issue where it would give incorrect values for rootNode

>         this.setAnonContent(value);
>+        includeAnons = !value;
[Is it possible to get a "bogus" notification when our old value of
 this.mDOMView.showAnonymousContent is already correct? That's why I suggested
 just using var includeAnons = this.mDOMView.showAnonymousContent;]
Attachment #482112 - Flags: review?(neil) → review+
(In reply to comment #18)
> [Is it possible to get a "bogus" notification when our old value of
>  this.mDOMView.showAnonymousContent is already correct? That's why I suggested
>  just using var includeAnons = this.mDOMView.showAnonymousContent;]

Pushed with that change as well as these two changes to comments:

> @@ -164,18 +164,18 @@
>   
>       return false;
>     },
>   
>  -  createDOMWalker: function DVr_CreateDOMWalker(aRoot)
>  +  /**
>  +   * The next-best chain extending from a given node is defined as the next
>  +   * non-ignorable siblings in document order, followed the preceding non-
> -+   * ignorable siblings in reverse document order, followed by the nearest
> -+   * non-ignorable ancestor and its non-ignorable siblings ordered similarly,
> ++   * ignorable siblings in reverse document order, followed by the parent node
> ++   * (if it's non-ignorable) and its non-ignorable siblings ordered similarly,
>  +   * continuing all the way up the ancestor chain.
>  +   * @param aNode
>  +   *        The node for which we'll traverse its next-best chain.
>  +   * @param aIncludeAnons [optional]
>  +   *        Whether to consider the effects of anonymous content on the chain
>  +   *        structure.  Note that this is not necessarily the same as the DOM
>  +   *        view's showAnonymousContent attribute, and even if true, won't
>  +   *        affect whether anonymous nodes can be returned.  However, the
> @@ -242,17 +242,17 @@
>  +    // and we're not showing anons), so we'll need to check for it later.
>  +    ancestor = this.mDOMUtils.getParentForNode(topmost, includeAnons);
>  +    // As a side-effect of special-casing the rootNode-as-topmost check
>  +    // earlier, we're guaranteed ancestor is non-null here.
>  +    var walker = this.createDOMWalker(ancestor, includeAnons);
>  +    var current = walker.firstChild();
>  +
>  +    // We have to relocate the walker over topmost by iterating over nodes
> -+    // until we get there, we'll save the ones we pass up along the way so we
> ++    // until we get there.  We'll save the ones we pass up along the way so we
>  +    // can immediately begin looking at those preceding nodes in reverse order
>  +    // if we find that we've exhausted all of the nodes that follow.
>  +    var preceding = [];
>  +    while (current != topmost) {
>  +      preceding.push(current);
>  +      current = walker.nextSibling();
>  +      if (!current) {
>  +        debug("unexpected end of nodes");


http://hg.mozilla.org/dom-inspector/rev/7306e47879d5
http://hg.mozilla.org/dom-inspector/rev/6bc77fa3ce34
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Blocks: 592530
Summary: Make the DOM Nodes viewer currentIndex-clean. → Make the DOM Nodes viewer currentIndex-clean and do more intelligent rebuilding
You need to log in before you can comment on or make changes to this bug.