Status

()

defect
RESOLVED FIXED
11 years ago
2 months ago

People

(Reporter: sicking, Assigned: sicking)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(1 attachment, 2 obsolete attachments)

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

Got a patch, just need to write some tests.
Posted patch Patch to fix (obsolete) — Splinter Review
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.
Assignee: nobody → jonas
Status: NEW → ASSIGNED
Attachment #331273 - Flags: superreview?(jst)
Attachment #331273 - Flags: review?(jst)
Should probably add mChildElements to cycle collection.
Do all the methods work properly with XUL?
Is EnsureContentsGenerated() eventually called?
Comment on attachment 331273 [details] [diff] [review]
Patch to fix

Doh! On both accounts.
Attachment #331273 - Flags: superreview?(jst)
Attachment #331273 - Flags: review?(jst)
Posted patch Patch v2 (obsolete) — Splinter Review
Attachment #331273 - Attachment is obsolete: true
Attachment #332284 - Flags: superreview?
Attachment #332284 - Flags: review?
Attachment #332284 - Attachment is patch: true
Attachment #332284 - Attachment mime type: application/octet-stream → text/plain
Attachment #332284 - Flags: superreview?(peterv)
Attachment #332284 - Flags: superreview?
Attachment #332284 - Flags: review?(Olli.Pettay)
Attachment #332284 - Flags: review?
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
Attachment #332284 - Flags: review?(Olli.Pettay) → review+
> >+  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 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 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?
Attachment #332284 - Flags: superreview?(peterv) → superreview+
> >+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.

In any case I hope to rename it something different from .mozChildElements before release.
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.
Attachment #332284 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
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?
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.
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?
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.
It looks like this checkin merge added a duplicate "friend class nsNSElementTearoff" to nsGenericElement.h, leading to bazillions of warnings during compile.
I removed one of those duplicate friend decls.
Depends on: 453307
This is now documented among the stuff in the Element object's docs.
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.