240186 - [FIX]GetElementsByAttribute should return live nodelists

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

I believe that making this do what other nodelists do is the right thing, if
it's declared as returning an nsIDOMNodeList.

If anyone objects, please let me know and justify the objections.

Note that once this is done the only user of nsRDFDOMNodeList will be
nsXULElement::GetChildNodes (which _definitely_ needs to be fixed to not be
broken like it is), so we can look forward to eliminating nsRDFDOMNodeList.

Comment 1

[Boris Zbarsky [:bzbarsky]]

ccing a few more people, since this will affect the various birds.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: List of files I need to diff to get the patch

This is just a note-to-self.  Most of XUL/JS changes are just to avoid using
.length unnecessarily, but a few (especially in calendar) are correctness
changes that absolutely have to be made if this list becomes live.

Comment 3

[Johnny Stenback (:jst)]

Yay, this sounds awesome!

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146101 [details] [diff] [review]
Patch

Neil, could you review the JS changes?	jst, could you review the rest?

Comment 6

[[e-mail]]

Comment on attachment 146101 [details] [diff] [review]
Patch

Almost ok...

>-  nsString* mData;
>+  const nsAFlatString* mData;
>+  /**
>+   * Whether we own mData
>+   */
>+  PRPackedBool mOwnsData;
Aren't string buffers smart now, so you can do nsString mData; here and mData =
aData in the constructor, etc?

>-      if (regionElements.length) {
>-        contentList.selectItem(regionElements[0]);
>+      var regionElement = regionElements.item(0);
>+      if (regionElement) {
>+        contentList.selectItem(regionElement);
I prefer your FireFox version of these changes.

>+             selectedItem = dataEls.item(0) ? dataEls.item(0) : defaultItem;
selectedItem = dataEls.item(0) || defaultItem;
Note that someone's trying to fix the JS strict warnings in this code so you
might get a conflict...

>           // Find and select the menuitem that matches inputField's "value"
>           var arr = null;
>           var popup = this.menupopup;
> 
>           if (popup)
>             arr = popup.getElementsByAttribute('label', this.inputField.value);
> 
>-          if (arr && arr.length)
>-            this.setSelectionInternal(arr[0]);
>-          else
>-            this.setSelectionInternal(null);
>+          if (arr) {
>+            this.setSelectionInternal(arr.item(0));
This isn't the same. The old code would set the internal selection to null even
if there wasn't a menupopup. This is necessary so that the .selectedItem getter
returns null if you remove the menupopup. I think something like this would
work, if you prefer it to arr && arr.item(0):
var popup = this.menupopup;
var item = null;
if (popup)
  item = popup.getElementsByAttribute('label', this.inputField.value).item(0);
this.setSelectionInternal(item);

>-          return btns.length > 0 ? btns[0] : document.getAnonymousElementByAttribute(this._wizardButtons, "dlgtype", aDlgType);
>+          return btns[0] ? btns[0] : document.getAnonymousElementByAttribute(this._wizardButtons, "dlgtype", aDlgType);
Again, btns.item(0) ? btns[0] : as per your FireFox version.

>-   for( var i = 0;  i < eventBoxList.length; i++ ) {
>-      eventBox = eventBoxList[ i ];
>+   while (eventBoxList.item(0)) {
>+      eventBox = eventBoxList[0];
>       eventBox.parentNode.removeChild( eventBox );
>    }
Out of interest, does this continue searching the document from where it left
off, or does it start from the beginning every time, or does .item(N) do a
search every time anyway? Anyway, my point is that calendar might want to be
able to create a backward-compatible xpi so they would need code that would
work with both static and dynamic lists.

>Index: toolkit/components/console/content/consoleBindings.xml
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/console/content/consoleBindings.xml,v
>retrieving revision 1.3
>diff -u -p -d -u -8 -r1.3 consoleBindings.xml
>--- toolkit/components/console/content/consoleBindings.xml	22 Oct 2003 08:00:39 -0000	1.3
>+++ toolkit/components/console/content/consoleBindings.xml	14 Apr 2004 15:56:02 -0000
>@@ -257,17 +257,17 @@
>         <parameter name="aAttr"/>
>         <parameter name="aVal"/>
>         <body><![CDATA[
>           var kids = document.getAnonymousNodes(this);
>           for (var i = 0; i < kids.length; ++i) {
>             if (kids[i].getAttribute(aAttr) == aVal)
>               return kids[i];
>             var kids2 = kids[i].getElementsByAttribute(aAttr, aVal);
>-            if (kids2.length > 0)
>+            if (kids2.item(0))
>               return kids2[0];
>           }
>           return null;
>         ]]></body>
>       </method>
This code should just so not be here; it was removed from SeaMonkey, the caller
now uses document.getAnonymousElementByAttribute instead.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> so you can do nsString mData; here

An nsString is a lot bigger than a pointer, and most content lists have no
mData, so I'd rather keep this as is.

> I prefer your FireFox version of these changes.

Eh?  I don't see a FireFox version of that change in the patch....

> so you might get a conflict...

I recall.  Conflicts happen.  ;)

> This isn't the same.

Good point.  Will fix.  Something like:

  this.setSelectionInternal(arr ? arr.item(0) : null);

should work, right?

> Again, btns.item(0) ? btns[0] : as per your FireFox version.

Oops.  Forgot to fix that.  Fixed.

> Out of interest, does this continue searching the document from where it left

I believe it searches from beginning every time; I also believe we could change
that without too much hassle.... The code here may in fact be better off
deleting elements off the end perf-wise; I'm not quite sure.

> they would need code that would work with both static and dynamic lists

Removing off the end may be the only way to do it.

> This code should just so not be here

Wanna file a followup bug?  I'm trying to avoid making substantive changes to
any of the JS/XBL stuff here.

Thanks for the review!

Comment 8

[[e-mail]]

Hmm... I must have been seeing double. Anyway, where you wrote
>-      if (listSelection.length)
>-        list.selectedItem = listSelection[0];
>+      var firstListSelection = listSelection.item(0);
>+      if (firstListSelection)
>+        list.selectedItem = firstListSelection;
I would prefer
>-      if (listSelection.length)
>+      if (listSelection.item(0))
as you have done everywhere else.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Ah, ok.   Sure thing.  Made that change.

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 146101 [details] [diff] [review]
Patch

Very nice changes! Here's some minor comments:

- In nsContentList::~nsContentList():

-  delete mData;
+  if (mOwnsData) {
+    delete mData;
+  }

Seems like you could get by w/o adding mOwnsData here if you'd write the above
as:

+  if (mData && mData != &EmptyString()) {
+    delete mData;
+  }

- In nsContentList::AttributeChanged():

+  if (mState == LIST_DIRTY || !mFunc) {
+    // Either we're already dirty or this notification doesn't affect
+    // whether we might match aContent.
+    return;
+  }

Isn't !mFunc the more common case here? If so, test that first.

+  if (MayContainRelevantNodes(aContent->GetParent())) {
+    if (Match(aContent)) {
+      if (mElements.IndexOf(aContent) == -1) {
+	 // We now match a new node.  Just dirty ourselves.

Kinda sucks to do a linear search here, but I guess it should be a short list
in most cases.

- In nsXULElement::GetElementsByAttribute():

+    nsCOMPtr<nsIAtom> attrAtom(do_GetAtom(aAttribute));
+    NS_ENSURE_TRUE(attrAtom, NS_ERROR_OUT_OF_MEMORY);

-    GetElementsByAttribute(this, aAttribute, aValue, elements);
+    nsCOMPtr<nsIContentList> list =
+	 new nsContentList(GetDocument(),
+			   nsXULDocument::MatchAttribute,
+			   aValue,
+			   this,
+			   PR_TRUE,
+			   attrAtom,
+			   kNameSpaceID_None);

Seems like it might be worth changing NS_GetContentList() to take a callback
function as well, and get the caching benefits that normal nsContentList cases
get.

If you agree, either do that, or file a followup bug on that. Either way is
fine with me.

sr=jst

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Seems like you could get by w/o adding mOwnsData here

Oh, duh.  Excellent idea.

> Isn't !mFunc the more common case here? If so, test that first.

It is, and will do.

> Kinda sucks to do a linear search here

Yeah... I didn't want to dirty all content lists all the time on attr changes,
though.  I suppose I could just unconditionally dirty if a node had an attr
change and now we match it.  Would that be better?

> Seems like it might be worth changing NS_GetContentList()

I thought about that too, but I'd need to push more stuff up into
nsContentListKey to make sure the hashing is done right.  I'll file a followup
on this, since it may take some thought.

Comment 12

[Johnny Stenback (:jst)]

(In reply to comment #11)
> > Kinda sucks to do a linear search here
> 
> Yeah... I didn't want to dirty all content lists all the time on attr changes,
> though.  I suppose I could just unconditionally dirty if a node had an attr
> change and now we match it.  Would that be better?

Sounds better to me, yeah.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch updated to comments

Comment 14

[Boris Zbarsky [:bzbarsky]]

Checked in for 1.8a, marking fixed.  I've filed bug 240572 on the hashing issue.
 Bug 121495 covers the XUL childNodes work.

No real effect on Tp/Txul/Ts from this checkin, which is good.

Comment 15

[Mostafa Hosseini]

The changes to calendar, for example:

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/calendar/resources/content&command=DIFF_FRAMESET&root=/cvsroot&file=monthView.js&rev1=1.66&rev2=1.67

which was part of the patch are causing infinite loops.
Example line 671 of monthView.js ( the length of the nodelist is 1 and remains 1 )

Comment 16

[Mostafa Hosseini]

Mea Culpa. I used the April 14th nightly which apparently didn't have the fix
in. Today's build works fine.

Comment 17

[Ian Neal]

toolkit/browser bits have been relanded following aviary branch landing