447917 - Implement ElementTraversal

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

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

We should implement this spec as it allows for simpler and more performant JS.

Got a patch, just need to write some tests.

Comment 1

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

Attachment: Patch to fix

Trivial patch.

I also added a mozChildElements property (as we are creating that nodelist anyway in order to not have to recompute childElementCount every time it is gotten). There is hopefully a spec being made soon for just the .childElements property, which would allow us to drop the 'moz' prefix.

Comment 2

[Peter Van der Beken [:peterv]]

Should probably add mChildElements to cycle collection.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Do all the methods work properly with XUL?
Is EnsureContentsGenerated() eventually called?

Comment 4

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

Comment on attachment 331273 [details] [diff] [review]
Patch to fix

Doh! On both accounts.

Comment 5

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

Attachment: Patch v2

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 332284 [details] [diff] [review]
Patch v2

>+class nsNSElementTearoff : public nsIDOMNSElement
...
>+  inline nsNSElementTearoff(nsGenericElement *aContent);
Why not just actually inline the code here.
Or is this here because of assigning to nsRefPtr<nsGenericElement>?
Why then move the class up before nsGenericElement definition?

>diff --git a/content/base/test/Makefile.in b/content/base/test/Makefile.in
>--- a/content/base/test/Makefile.in
>+++ b/content/base/test/Makefile.in
>@@ -181,16 +181,17 @@ _TEST_FILES = 	test_bug5141.html \
> 		test_bug425201.html \
> 		test_bug431833.html \
> 		test_bug438519.html \
> 		test_bug444722.html \
> 		test_text_replaceWholeText.html \
> 		test_text_wholeText.html \
> 		wholeTexty-helper.xml \
> 		test_bug444030.xhtml \
>+		test_elementTraversal.html \
I know, this was discussed, but IMHO the file name should still
contain the bug number too. (Especially now that we don't have anything
like bonsai, which linkifies logs and annotates properly.)

(I wonder if slots should have a strong pointer to nsNSElementTearoff, and
nsNSElementTearoff weak pointer back to element. That way the tearoff could be reused.
mChildElements could go to nsNSElementTearoff. This all would make nsNSElementTearoff sizeof(void*)
larger but could reduce the number of created tearoffs. Though, creating slots for always when
the tearoff is used is perhaps a bit heavy. Someone should test this a bit.)

But anyway r=me

Comment 7

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

> >+  inline nsNSElementTearoff(nsGenericElement *aContent);
> Why not just actually inline the code here.
> Or is this here because of assigning to nsRefPtr<nsGenericElement>?
> Why then move the class up before nsGenericElement definition?

It has to live before in order to be a friend. And I can't inline right there because of the nsRefPtr :(

> >+		test_elementTraversal.html \
> I know, this was discussed, but IMHO the file name should still
> contain the bug number too. (Especially now that we don't have anything
> like bonsai, which linkifies logs and annotates properly.)

The bug itself is fairly useless, and if you really want it you will be able to dig out from the checkin message. By not putting the bug number in there we can add future tests to this file is needed.

> (I wonder if slots should have a strong pointer to nsNSElementTearoff, and
> nsNSElementTearoff weak pointer back to element. That way the tearoff could be
> reused.
> mChildElements could go to nsNSElementTearoff. This all would make
> nsNSElementTearoff sizeof(void*)
> larger but could reduce the number of created tearoffs. Though, creating slots
> for always when
> the tearoff is used is perhaps a bit heavy. Someone should test this a bit.)

A lot of this uglyness will go away once we can break ABI compat. Then we can just add these methods directly to nsIDOMElement and not have to mess with tearoffs.

Comment 8

[Alex Vincent [:WeirdAl]]

Comment on attachment 332284 [details] [diff] [review]
Patch v2

>diff --git a/dom/public/idl/core/nsIDOMNSElement.idl b/dom/public/idl/core/nsIDOMNSElement.idl

I probably don't need to tell you this, but... rev the UUID?

Comment 9

[Peter Van der Beken [:peterv]]

Comment on attachment 332284 [details] [diff] [review]
Patch v2

>diff --git a/content/base/src/nsGenericElement.cpp b/content/base/src/nsGenericElement.cpp

>+nsNSElementTearoff::GetFirstElementChild(nsIDOMElement** aResult)

>+  nsAttrAndChildArray& children = mContent->mAttrsAndChildren;

const? Here and in the other methods.

>diff --git a/content/base/src/nsGenericElement.h b/content/base/src/nsGenericElement.h

>+class nsNSElementTearoff : public nsIDOMNSElement

(In reply to comment #7)
> It has to live before in order to be a friend.

You could just forward declare nsNSElementTearoff before nsGenericElement.

>diff --git a/content/base/test/test_elementTraversal.html b/content/base/test/test_elementTraversal.html

>+  for(i--, e = c.lastElementChild; e; e = e.previousElementSibling, i--) {
>+    is(e.textContent, contents[i], "g element contents");

"wrong ..."

>diff --git a/dom/public/idl/core/nsIDOMNSElement.idl b/dom/public/idl/core/nsIDOMNSElement.idl

>+  readonly attribute nsIDOMElement firstElementChild;
>+  readonly attribute nsIDOMElement lastElementChild;
>+  readonly attribute nsIDOMElement previousElementSibling;
>+  readonly attribute nsIDOMElement nextElementSibling;
>+
>+  /**
>+   * Returns the number of child nodes that are nsIDOMElements.
>+   *
>+   * Defined by the ElementTraversal spec.
>+   */
>+  readonly attribute unsigned long childElementCount;
>+  
>+  /**
>+   * Returns a live nsIDOMNodeList of the current child elements.
>+   */
>+  readonly attribute nsIDOMNodeList mozChildElements;

Shouldn't these be on a nsIDOMElementTraversal interface? If not, you need to rev the IID.
Have we proposed .childElements to the W3C?

Comment 10

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

> >+nsNSElementTearoff::GetFirstElementChild(nsIDOMElement** aResult)
> 
> >+  nsAttrAndChildArray& children = mContent->mAttrsAndChildren;
> 
> const? Here and in the other methods.

Hmm.. we don't do a whole lot of that when just local to one method, but it couldn't hurt i guess.

> > It has to live before in order to be a friend.
> 
> You could just forward declare nsNSElementTearoff before nsGenericElement.

I think gcc might frown on that. I'll have the tryserver test it.

> Shouldn't these be on a nsIDOMElementTraversal interface? If not, you need to
> rev the IID.

I really think we should stop creating separate interfaces just because W3C writes their specs that way. They don't take things like vtable overhead and such into account when drafting the specs, and it doesn't affect web authors anyway. Once we unfreeze all interfaces for moz2 I'm planning on merging this stuff onto nsIDOMElement.

> Have we proposed .childElements to the W3C?

Yes, apple thought it was too complex to implement :(

Apparently there is going to be a separate spec just for this one property instead... Another solution is to call the property .children if that doesn't complicate the compat story with IE.

Comment 11

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

In any case I hope to rename it something different from .mozChildElements before release.

Comment 12

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

Attachment: Patch to check in

It worked fine to just forward declare the class.

I also renamed mozChildElements to children. That implements exactly what safari implements for that property, and is a subset of what IE implements.

Comment 13

[Boris Zbarsky [:bzbarsky]]

So why not just use the nsContentList for the next/previous thing?  Slow indexOf?  If we sped up indexOf on content lists, could we nix a bunch of code here?

Comment 14

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

Mostly it simply didn't occur to me :)

But i'd be worried about the cost of the nsContentList actually, since we would force the parent to create one. There's both the memory cost with that, and the cost of having more nsIMutationObservers.

The reason I used it for ChildElementCount is that I suspect people will do:

while (i = 0; i < elem.childElementCount; i++) {
  ...
}

Or do stuff like the example in the spec. In both cases we want access to the count fast.

I think next/previous is going to be fast enough with the optimizations we have on nsAttrAndChildArray::IndexOfChild. To speed that up further we could add a version of nsAttrAndChildArray::ChildAt that stores the requested index in the internal cache and use that version in the next/previous(Element)Sibling implementations.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Yeah, if we expect people to use the traversal api without getting the count, then we can save the memory.  If they do use the count, then we lose on memory anyway...

I'm not worried about next/previous being slow per se, just not as fast as they could be, on pages with lots of textnodes.  But that "could be" depends on having the indexOf optimization on content lists, like I said.

I doubt the ChildAt thing is worth doing; the IndexOf and increment is just as fast, no?

Comment 16

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

Using a contentlist I think is a fairly minimal gain. The only win is that you're doing indexOf on a list that's around half the size (only elements vs. alternating textnodes and elements). However with the optimizations we have in nsAttrAndChildArray::IndexOfChild the list size doesn't matter anyway so the win seems negligable.

I do think that most use cases for using firstElementChild/nextElementSibling does not use a count at all, so in those cases we will save memory.

Doing the childAt trick means that we'll search fewer nodes, and in most cases hit the right index on the first node. Not a big win no, possibly not each measurable.

Comment 17

[Boris Zbarsky [:bzbarsky]]

It looks like this checkin merge added a duplicate "friend class nsNSElementTearoff" to nsGenericElement.h, leading to bazillions of warnings during compile.

Comment 18

[Boris Zbarsky [:bzbarsky]]

I removed one of those duplicate friend decls.

Comment 19

[Eric Shepherd [:sheppy]]

This is now documented among the stuff in the Element object's docs.