522601 - Implement inDeepTreeWalker::firstChild and ::nextSibling

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[John J. Barton]

After a lot of discussion and various implementation efforts, we concluded that JS cannot walk anonymous nodes from and XBL document outside of the DOM inspector's treeview widget.
See http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/d89ed07aea746c50#

If the following two functions where implemented, I think Chromebug could support anonymous nodes. (see http://code.google.com/p/fbug/issues/detail?id=2343&q=anonymous&sort=-id)


http://mxr.mozilla.org/comm-central/source/mozilla/layout/inspector/src/inDeepTreeWalker.cpp#196

http://mxr.mozilla.org/comm-central/source/mozilla/layout/inspector/src/inDeepTreeWalker.cpp#214

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Comment 2

[John J. Barton]

> It also ignores whatToShow and the filter.

So the walker acts as if SHOW_ALL were set, correct?

Is showAnonymousContent obeyed. (IMO it ought to default TRUE, since its why we use this, but I guess it's too late).
Is showSubDocuments obeyed. (default FALSE)

---
 : mShowAnonymousContent(PR_FALSE),  
     mShowSubDocuments(PR_FALSE),
     mWhatToShow(nsIDOMNodeFilter::SHOW_ALL)
---

"It also ignores init() argument 'whatToShow', and sets the property whatToShow==SHOW_ALL, and ignores the filter."

(perhaps you should just remove the whatToShow argument?)

Comment 3

[Boris Zbarsky [:bzbarsky]]

> So the walker acts as if SHOW_ALL were set, correct?
> Is showAnonymousContent obeyed.
> Is showSubDocuments obeyed.

Yes to all, as long as you set those booleans before calling init().

> (perhaps you should just remove the whatToShow argument?)

Tempting, yes.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: With the test actually included

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 406847 [details] [diff] [review]
With the test actually included

> inDeepTreeWalker::NextNode(nsIDOMNode **_retval)
> {
...
>+    nsCOMPtr<nsIDOMNode> parent;
>+    rv = ParentNode(getter_AddRefs(parent));
>+    if (!parent) {
>+      // Nowhere else to go; we're done.  Restore our state
>+      while (lastChildCallsToMake--) {
>+	nsCOMPtr<nsIDOMNode> dummy;
>+	LastChild(getter_AddRefs(dummy));

You have tabs in here (!?!). Also, could you assert that this 'walk back' mechanism ends up at the node we started at.

r=me with that

Comment 6

[Boris Zbarsky [:bzbarsky]]

> You have tabs in here (!?!)

I blame dastardly lack of modeline.  Which I have now fixed locally.  And added that assert.

Comment 7

[Shawn Wilsher :sdwilsh]

Comment on attachment 406847 [details] [diff] [review]
With the test actually included

Sure would be nice if we actually had a stack class like the stls...

> inDeepTreeWalker::PreviousSibling(nsIDOMNode **_retval)
>+  // Pop off the current node, and push the new one
So, I understand that this is what you are doing (the code is obvious), but the comment doesn't really tell me why.  In general, a few more comments explaining what is going on would be very useful.  I'm going to point out the few places that really aren't clear to me, but more comments would be greatly welcomed!

>+  mStack.RemoveElementAt(mStack.Length()-1);
nit: space around minus operator

> inDeepTreeWalker::NextSibling(nsIDOMNode **_retval)
>+  // Pop off the current node, and push the new one
>+  mStack.RemoveElementAt(mStack.Length()-1);
ditto on last two comments

> inDeepTreeWalker::PreviousNode(nsIDOMNode **_retval)
>+  nsCOMPtr<nsIDOMNode> node;
>+  nsresult rv = PreviousSibling(getter_AddRefs(node));
>+  NS_ENSURE_SUCCESS(rv, rv);
PreviousSibling only returns NS_OK, so it doesn't make much sense to check the return variable here.

>+  while (node) {
>+    rv = LastChild(getter_AddRefs(node));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
I'm not really sure what this loop is doing, so a comment please.  Also, LastChild only ever returns NS_OK, so no need to check.

> inDeepTreeWalker::NextNode(nsIDOMNode **_retval)
>+  // First try our kids
>+  nsresult rv = FirstChild(_retval);
>+  NS_ENSURE_SUCCESS(rv, rv);
ditto on on checking rv

r=sdwilsh

Comment 8

[Boris Zbarsky [:bzbarsky]]

> In general, a few more comments explaining what is going on would be very
> useful.

Will add.

> nit: space around minus operator

Will fix.

> PreviousSibling only returns NS_OK,

I was aiming to not make those assumptions, but I guess I can change that.  No problem.

> I'm not really sure what this loop is doing, so a comment please

Will do.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/a830ed8bf8dc with those changes.

John, please let me know (probably in bug form?) if there are any other issues with making this work for you?

Comment 10

[Shawn Wilsher :sdwilsh]

Would we want this on 1.9.2 (and maybe 1.9.1)?  I should get an updated version of DOMi out to the masses with your fix in it too...

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Yes, please!  Having Chromebug be more useful to extension devs at large is high-value.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

(By which I mean "both 1.9.1 and 1.9.2 would be ideal").

Comment 13

[Boris Zbarsky [:bzbarsky]]

I'm not quite willing to do this on 1.9.1, since it is in fact an api-breaking change.  I can see landing it on 1.9.2.

Comment 14

[John J. Barton]

(In reply to comment #9)
> John, please let me know (probably in bug form?) if there are any other issues
> with making this work for you?

Thanks!  

Firebug tracking for completing the Chromebug part is 
http://code.google.com/p/fbug/issues/detail?id=2343
(But it may be a week or so before I can get it).

Comment 15

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/70cbc9ee2134

Comment 16

[Colby Russell :crussell (no longer Mozilla-ing)]

If the end of the document is reached, the walker will loop infinitely on the last node if you try to advance it with nextNode(), instead of returning null for walker.currentNode. See bug 526939.

Comment 17

[Boris Zbarsky [:bzbarsky]]

That's the correct behavior for the walker.  The problem is in the consumer, which I apparently missed somehow...