Closed Bug 395496 Opened 17 years ago Closed 17 years ago

Attribute mutation listeners (DOMAttrModified) added to toolkit widgets

Categories

(Toolkit :: UI Widgets, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: neil, Assigned: neil)

References

Details

(Keywords: helpwanted, perf)

Attachments

(1 file)

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.
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.
Summary: Attribute listeners added to toolkit widgets → Attribute mutation listeners (DOMAttrModified) added to toolkit widgets
So what windows contain menulists?  And how much attribute mutation happens in them?
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.
(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.
Hmm, can we use Add/RemoveBroadcastListenerFor for this?
> 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?
Attached patch Possible patchSplinter Review
If I copied the GetDidInitialReflow stuff from OverlayForwardReference::Resolve into SynchroniseBroadcastListener then I could remove the attribute copies too.
Attachment #280930 - Flags: review?(enndeakin)
Attachment #280930 - Flags: review?(bzbarsky)
Attachment #280930 - Flags: review?(enndeakin) → review+
Comment on attachment 280930 [details] [diff] [review]
Possible patch

Looks ok, but I don't follow comment 7....
Attachment #280930 - Flags: review?(bzbarsky) → review+
>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.
Oh, that was the explanation for why you kept the existing setAttribute() calls?
Comment on attachment 280930 [details] [diff] [review]
Possible patch

menulist perf win
Attachment #280930 - Flags: approval1.9?
Filed bug 396439 on the aNotify issue.
Comment on attachment 280930 [details] [diff] [review]
Possible patch

a=bzbarsky
Attachment #280930 - Flags: approval1.9? → approval1.9+
menulist.xml fixed; tabbrowser.xml still needs help.
Keywords: helpwanted
feed preview is broken by this check-in.
http://forums.mozillazine.org/viewtopic.php?p=3068646#3068646
(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?
(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...

Assignee: nobody → neil
Assignee: neil → nobody
Let's file a follow up for tabbrowser, it's in browser/ now anyway.
Assignee: nobody → neil
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Depends on: 413631
Depends on: 1059146
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: