Closed
Bug 240186
Opened 21 years ago
Closed 21 years ago
[FIX]GetElementsByAttribute should return live nodelists
Categories
(Core :: DOM: Core & HTML, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla1.8alpha1
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(1 file, 2 obsolete files)
|
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
|
Assignee | |
Comment 1•21 years ago
|
||
ccing a few more people, since this will affect the various birds.
|
|
Assignee | |
Comment 2•21 years ago
|
||
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•21 years ago
|
||
Yay, this sounds awesome!
|
|
Assignee | |
Comment 4•21 years ago
|
||
Attachment #145842 -
Attachment is obsolete: true
|
|
Assignee | |
Updated•21 years ago
|
Priority: -- → P1
Summary: GetElementsByAttribute should return live nodelists → [FIX]GetElementsByAttribute should return live nodelists
Target Milestone: --- → mozilla1.8alpha
|
|
Assignee | |
Comment 5•21 years ago
|
||
Comment on attachment 146101 [details] [diff] [review]
Patch
Neil, could you review the JS changes? jst, could you review the rest?
Attachment #146101 -
Flags: superreview?(jst)
Attachment #146101 -
Flags: review?(neil.parkwaycc.co.uk)
|
||
Comment 6•21 years ago
|
||
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.
Attachment #146101 -
Flags: review?(neil.parkwaycc.co.uk) → review+
|
|
Assignee | |
Comment 7•21 years ago
|
||
> 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•21 years ago
|
||
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.
|
|
Assignee | |
Comment 9•21 years ago
|
||
Ah, ok. Sure thing. Made that change.
|
|
||
Comment 10•21 years ago
|
||
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
Attachment #146101 -
Flags: superreview?(jst) → superreview+
|
|
Assignee | |
Comment 11•21 years ago
|
||
> 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•21 years ago
|
||
(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.
|
|
Assignee | |
Comment 13•21 years ago
|
||
Attachment #146101 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 14•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
Mea Culpa. I used the April 14th nightly which apparently didn't have the fix
in. Today's build works fine.
|
||
Comment 17•20 years ago
|
||
toolkit/browser bits have been relanded following aviary branch landing
Keywords: aviary-landing
|
||
Updated•20 years ago
|
Keywords: aviary-landing
|
||
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•