Closed Bug 104401 Opened 23 years ago Closed 23 years ago

eliminate mOuter aggregate from nsXULElement

Categories

(Core :: XUL, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla0.9.8

People

(Reporter: waterson, Assigned: hewitt)

References

(Blocks 1 open bug)

Details

(Keywords: memory-footprint)

Attachments

(2 files)

Replace native mOuter aggregate cruft from nsXULElement with XBL magic.
Blocks: 104400
Status: NEW → ASSIGNED
Keywords: footprint
Priority: -- → P1
Target Milestone: --- → mozilla0.9.6
hewitt could do this when he starts working on <listbox>. Want to reassign it to him?
I'd love to!
Assignee: waterson → hewitt
Status: ASSIGNED → NEW
Blocks: 70858
Status: NEW → ASSIGNED
Blocks: 108219
this is almost done but I'll wait until the opening of 0.9.7 to land it
Target Milestone: mozilla0.9.6 → mozilla0.9.7
I renamed clearItemSelection() to clearSelection, and converted multiple="true" to seltype="multiple", so this patch includes updates all xul/js uses in the tree.
Comment on attachment 58814 [details] [diff] [review] patch to content, dom, and tree.xml Sweet justice. Just a few questions: conditional sr=waterson if you've got answers that make sense. ;-) >+ nsCOMPtr<nsIDOMDocumentEvent> doc(do_QueryInterface(mDocument)); >+ nsCOMPtr<nsIDOMEvent> event; >+ doc->CreateEvent(NS_LITERAL_STRING("Events"), getter_AddRefs(event)); Did you want this to be initial-caps? ^^^^^^ (Most event names I've seen are all lower-case.) >@@ -3340,7 +3251,7 @@ > parent = mParent; > } > >- if (retarget || (parent != mParent)) { >+ if (!retarget || (parent != mParent)) { Was this a latent bug? >@@ -4743,6 +4654,9 @@ > nsXULPrototypeElement* proto = mPrototype; > mPrototype = nsnull; > >+ if (proto->mNumAttributes == 0) >+ return NS_OK; >+ Won't this cause us to leak the prototype when it has no attributes? >+ <constructor> >+ <![CDATA[ >+ var els = this.getElementsByAttribute("selected", "true"); >+ this.selectedItems = []; >+ for (var i = 0; i < els.length; ++i) >+ this.selectedItems.push(els[i]); >+ ]]> This could cause some pain for larger RDF-based trees since it will cause a content model crawl. Any other way to do this? hyatt should review the other changes to the tree stuff in nsXULElement.cpp, I don't know that code at all.
Attachment #58814 - Flags: superreview+
Case doesn't matter but convention is generally initial cap. http://lxr.mozilla.org/seamonkey/source/content/events/src/nsEventListenerManage r.cpp#2335
The DOM spec uses "Events" with an initial cap. For these sorts of "event groups", the DOM spec uses initial cap notation. This is also the convention it is using with new events as well, e.g., DOMAttrModified, etc. I think the content model crawl is just fine actually, since this is going to become nothing more than a listbox by the time we're done with it. We may take an initial penalty now, but once we morph all trees into outliners, doing a crawl only for listboxes will be fine.
IMHO this change is completely wrong: - if (retarget || (parent != mParent)) { + if (!retarget || (parent != mParent)) { I'd be curious if this is an accident or if you were trying to fix something. The original code, though, is correct AFAICT.
I have no idea how the !retarget and proto->mNumAttributes stuff got into that patch, as I don't remember typing them at all. Ignore them, they will be removed. As for the selectedItems crawl, would it satisfy you if I did that lazily the first time selectedItems is accessed?
r= or sr=waterson, then.
r/sr=hyatt
Target Milestone: mozilla0.9.7 → mozilla0.9.8
fixed
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: jrgmorrison → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: