26104 - move broadcaster maintenance into nsXULDocument

Status: RESOLVED FIXED

(Core :: XUL, defect, P2)

Description

[Chris Waterson]

Currently, broadcaster/observers are implemented by maintaining the "listener" 
list in the broadcasting element. This causes two problems:

1. HTML elements cannot be broadcasters.

2. When a listener is removed from the content model, the broadcaster cannot 
reliably be notified that the element is "leaving" (i.e., relying on the values 
of attributes in the content model is fragile). See bug 18127.

By promoting this functionality to the XUL document, we solve both problems.

Comment 1

[Paul MacQuiddy]

spam, open xptoolkit qa contact moving over to jrgm

Comment 2

[Chris Waterson]

future projects.

Comment 3

[Chris Waterson]

Attachment: work-in-progress

Comment 4

[Chris Waterson]

Attachment: test case: label should start as ``bar'', change to ``baz'' when clicked

Comment 5

[Chris Waterson]

Attachment: working patch

Comment 6

[Chris Waterson]

Okay, attachment 54371 [details] [diff] [review] is ready for review:

1. I've moved the broadcaster APIs from nsIDOMXULElement to nsIDOMXULDocument
   (although nobody uses them).

2. I moved all of the broadcaster synchronization and maintenance from 
   nsXULElement to nsXULDocument. Update happens via the 
    nsIDocument::AttributeChanged notification.

3. I fixed the broadcaster mechanism so that you could actually observe a
   list of attributes. (The previous implementation was broken in this regard.)

4. Noted where ownership model may have a scary dangling pointer (cf.
   bug 18127). Didn't fix it, because I don't want to fight leaks and the
   case is obscure.

I'll try to collect some footprint numbers later today.

Comment 7

[Chris Waterson]

shaver, you could review this one, too, with your new XUL element super-powers.

Comment 8

[Chris Waterson]

Mmm, 10KB requested on a single browser window.

Comment 9

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 54371 [details] [diff] [review]
working patch

>+    aNodeInfo->GetNameAtom(*getter_AddRefs(tagName));
>+    if ((tagName.get() == nsXULAtoms::broadcaster) ||
>+        (tagName.get() == nsXULAtoms::command) ||
>+        (tagName.get() == nsXULAtoms::key)) {
>+        return NS_STYLE_HINT_NONE;

Eschew .get()!

>+    if (mBroadcasterMap)
>+        PL_DHashTableDestroy(mBroadcasterMap);
>+

How common are broadcasters?  Better to save the extra allocation and inline
the PLDHashTable?  (Probably not, but I felt I should ask.)

>+    entry->mListeners.~nsCheapVoidArray();

You don't see manual destructor invocation often enough, I say.

>+        // We are observing the broadcaster, but is this the right
>+        // attribute?
>+        nsAutoString listeningToAttribute;
>+        listener->GetAttr(kNameSpaceID_None, nsXULAtoms::attribute, listeningToAttribute);
>+
>+        if (! listeningToAttribute.Equals(attrName))
>+            continue;

Can we not observe all attributes, as well, with the "*"?

>-  NS_WARN_IF_FALSE(mNumCapturers >= 0, "Number of capturers has become negative");
>+  NS_ASSERTION(mNumCapturers >= 0, "Number of capturers has become negative");

(Thank you.)

Fix those nits, and assuage my observing-* fears, and I'll stamp my r.

Comment 10

[Chris Waterson]

You are the wind beneath my wings, shaver! Attaching a get-eschewing, fancy dtor
calling commented, star pushing patch. As best I can tell, there are only a few
XUL files using broadcasters (navigator, editor, maybe some mail). I figure
dialogs n' stuff didn't need 'em.

Comment 11

[Chris Waterson]

Attachment: all that and then some

Comment 12

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 54443 [details] [diff] [review]
all that and then some

money. r=shaver

Comment 13

[David Hyatt]

Eek, so I need to update the XUL spec, since the APIs moved to the document.

Comment 14

[David Hyatt]

sr=hyatt

Comment 15

[Chris Waterson]

Attachment: after merging with changes from bug 68871

Comment 16

[Chris Waterson]

Changes checked in.