348573 - [FIX]PresShell should get notifications through the binding manager

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

We've talked about this on and off for a while.  We should just do it.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Same as diff -w

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 233541 [details] [diff] [review]
Same as diff -w

>Index: content/xbl/src/nsBindingManager.cpp

>+nsBindingManager::AddObserver(nsIDocumentObserver* aObserver)
>+nsBindingManager::RemoveObserver(nsIDocumentObserver* aObserver)

Inline these to save code?


>Index: content/base/src/nsDocument.cpp
>@@ -826,24 +826,23 @@ nsDocument::Init()
>   mBindingManager = bindingManager;
> 
>   // The binding manager must always be the first observer of the document.
>   mObservers.PrependObserver(bindingManager);
>   nsINode::nsSlots* slots = GetSlots();
>   NS_ENSURE_TRUE(slots &&
>                  slots->mMutationObservers.PrependObserver(bindingManager),
>                  NS_ERROR_OUT_OF_MEMORY);
>   // Prepend self as mutation-observer whether we need it or not (some
>   // subclasses currently do, other don't). This is because the code in
>-  // nsNodeUtils always notifies the first observer first, even when going
>-  // backwards, expecting the first observer to be the document.
>-  // If we remove that hack, we can move the below registring out to the leaf
>-  // classes.
>+  // nsNodeUtils always notifies the first observer first, expecting the
>+  // first observer to be the document.  If we remove that hack, we can
>+  // move the below registring out to the leaf classes.

Didn't you just fix that hack? (Though you'd still have to prepend since I think the leaf classes might want to get notified first).

>Index: content/base/src/nsTObserverArray.h
>+    // XXXbz do we still want to keep ReverseIterator around?
>+

I'd say no.


>+// XXXbz I wish I didn't have to pass in the observer type, but I
>+// don't see a way to get it out of array_.
>+#define NS_OBSERVER_ARRAY_NOTIFY_OBSERVERS(array_, obstype_, func_, params_) \

If you made this into an templetized (inline) function you could. But i'm not sure if that's worth it.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Inline these to save code?

They're virtual methods anyway.  Unless you mean push the member up to nsIBindingManager and inline there?

> Didn't you just fix that hack? 

I wasn't sure.  At this point "hack" refers to the assumption that the document is first in the list.  ;)

> I'd say no.

OK, I'll remove it.

> If you made this into an templetized (inline) function

Then I couldn't pass through the func and args in a reasonable way.... :(

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> > Inline these to save code?
> 
> They're virtual methods anyway.  Unless you mean push the member up to
> nsIBindingManager and inline there?

Either way is fine with me.

> > Didn't you just fix that hack? 
> 
> I wasn't sure.  At this point "hack" refers to the assumption that the 
> document is first in the list.  ;)

Yeah, the hack is gone so we can add just as we need. That said, i'm not sure which is cleaner, i'm sure we'll in the future have to make all nsDocuments observe themselfs so it's not that important to me to push it out to the leaves.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 7

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Actually, that patch breaks other consumers (like the DOM inspector listeners).

I think what we _really_ want is for all mutation listeners to go through the binding manager.  I'll write up a followup to do that.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

One thing that i'm not super exited about is that adding an observer to the document will behave so different from adding an observer to, say, the documentElement.

But I can't think of any problems that that will bring up in reality so I think it's fine.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Just to clarify, the problems that i'm worried about is for users that attaches listeners to random nsINodes, like nsContentList

Comment 12

[Boris Zbarsky [:bzbarsky]]

But those shouldn't care about XBL insertion points, right?

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, that was my hope too.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Checked in the followup patch.  Fixed.