104401 - eliminate mOuter aggregate from nsXULElement

Status: RESOLVED FIXED

(Core :: XUL, defect, P1)

Description

[Chris Waterson]

Replace native mOuter aggregate cruft from nsXULElement with XBL magic.

Comment 1

[David Hyatt]

hewitt could do this when he starts working on <listbox>.  Want to reassign it
to him?

Comment 2

[Chris Waterson]

I'd love to!

Comment 3

[Joe Hewitt (gone)]

this is almost done but I'll wait until the opening of 0.9.7 to land it

Comment 4

[Joe Hewitt (gone)]

Attachment: patch to content, dom, and tree.xml

Comment 5

[Joe Hewitt (gone)]

Attachment: patch to xul/js for syntax changes

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 6

[Chris Waterson]

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.

Comment 7

[Blake Ross]

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

[David Hyatt]

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

[David Hyatt]

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.

Comment 10

[Joe Hewitt (gone)]

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?

Comment 11

[Chris Waterson]

r= or sr=waterson, then.

Comment 12

[David Hyatt]

r/sr=hyatt

Comment 13

[Joe Hewitt (gone)]

fixed