Closed
Bug 104401
Opened 23 years ago
Closed 23 years ago
eliminate mOuter aggregate from nsXULElement
Categories
(Core :: XUL, defect, P1)
Core
XUL
Tracking
()
RESOLVED
FIXED
mozilla0.9.8
People
(Reporter: waterson, Assigned: hewitt)
References
(Blocks 1 open bug)
Details
(Keywords: memory-footprint)
Attachments
(2 files)
(deleted),
patch
|
waterson
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
Replace native mOuter aggregate cruft from nsXULElement with XBL magic.
Reporter | ||
Updated•23 years ago
|
Comment 1•23 years ago
|
||
hewitt could do this when he starts working on <listbox>. Want to reassign it
to him?
Assignee | ||
Updated•23 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•23 years ago
|
||
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
Assignee | ||
Comment 4•23 years ago
|
||
Assignee | ||
Comment 5•23 years ago
|
||
I renamed clearItemSelection() to clearSelection, and converted multiple="true"
to seltype="multiple", so this patch includes updates all xul/js uses in the
tree.
Reporter | ||
Comment 6•23 years ago
|
||
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+
Comment 7•23 years ago
|
||
Case doesn't matter but convention is generally initial cap.
http://lxr.mozilla.org/seamonkey/source/content/events/src/nsEventListenerManage
r.cpp#2335
Comment 8•23 years ago
|
||
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.
Comment 9•23 years ago
|
||
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.
Assignee | ||
Comment 10•23 years ago
|
||
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?
Reporter | ||
Comment 11•23 years ago
|
||
r= or sr=waterson, then.
Comment 12•23 years ago
|
||
r/sr=hyatt
Assignee | ||
Updated•23 years ago
|
Target Milestone: mozilla0.9.7 → mozilla0.9.8
Assignee | ||
Comment 13•23 years ago
|
||
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.
Description
•