395496 - Attribute mutation listeners (DOMAttrModified) added to toolkit widgets

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[[e-mail]]

As of bug 360220 all menulists now add DOMAttrModified event listeners. I have verified that this triggers calls to nsPIDOMWindow::SetMutationListeners(). My understanding is that this will have an impact on the performance of all SetAttribute calls in all windows that contain menulists.

I am not aware of an obvious alternative to these event listeners, however note that in the addressbook sidebar panel an observer listens to changes in the datasource underlying the RDF template and manually refreshes the menulist.

Comment 1

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

I would strongly suggest we try to find another solution. Doing this will have a big impact on all attribute mutations. Though I guess it would be good to get perf numbers before doing anything too rash.

Maybe we can come up with some other notification mechanism based on delayed events based on nsIMutationObserver and nsIRunnables.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So what windows contain menulists?  And how much attribute mutation happens in them?

Comment 3

[Neil Deakin]

Note also that tabbrowser also has some DOMAttrModified listeners, so the answer to which windows this affects, is all the ones where performance really matters.

Comment 4

[[e-mail]]

(In reply to comment #3)
>Note also that tabbrowser also has some DOMAttrModified listeners
Ah, but at least in this case there are obvious alternatives.

Comment 5

[[e-mail]]

Hmm, can we use Add/RemoveBroadcastListenerFor for this?

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Note also that tabbrowser also has some DOMAttrModified listeners

Uh, it does?  Why?

We have the whole observer mechanism for a reason.  Why aren't we using it?

Comment 7

[[e-mail]]

Attachment: Possible patch

If I copied the GetDidInitialReflow stuff from OverlayForwardReference::Resolve into SynchroniseBroadcastListener then I could remove the attribute copies too.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 280930 [details] [diff] [review]
Possible patch

Looks ok, but I don't follow comment 7....

Comment 9

[[e-mail]]

>If I copied the GetDidInitialReflow stuff from OverlayForwardReference::Resolve
>into SynchroniseBroadcastListener then I could remove the attribute copies too.

>Looks ok, but I don't follow comment 7....
Resolve calls GetDidInitialReflow to tell what it should pass for aNotify, but SynchronizeBroadcastListener always calls SetAttr with an aNotify of PR_FALSE so I have to manually copy the attribute to ensure that everyone notices.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Oh, that was the explanation for why you kept the existing setAttribute() calls?

Comment 11

[[e-mail]]

Comment on attachment 280930 [details] [diff] [review]
Possible patch

menulist perf win

Comment 12

[[e-mail]]

Filed bug 396439 on the aNotify issue.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 280930 [details] [diff] [review]
Possible patch

a=bzbarsky

Comment 14

[[e-mail]]

menulist.xml fixed; tabbrowser.xml still needs help.

Comment 15

[pal-moz]

feed preview is broken by this check-in.
http://forums.mozillazine.org/viewtopic.php?p=3068646#3068646

Comment 16

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #15)
> feed preview is broken by this check-in.
> http://forums.mozillazine.org/viewtopic.php?p=3068646#3068646

Please file a new bug?

Comment 17

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #15)
> feed preview is broken by this check-in.
> http://forums.mozillazine.org/viewtopic.php?p=3068646#3068646

I filed bug 397225; please include your screenshots and Actual Results...

Comment 18

[Asaf Romano (gone)]

Let's file a follow up for tabbrowser, it's in browser/ now anyway.