80208 - creating too many popup listeners?

Status: RESOLVED FIXED

(SeaMonkey :: General, defect, P2)

Description

[(not reading, please use [e-mail] instead)]

creating too many popup listeners.  (his number was 57 for the browser window).

talking to dave, he mentioned that he thinks we're creating too many pop 
listeners.

possibly creating them inside of generated content (like that other bug where 
we were putting onclick and oncommand handlers in generated content).

helping with browser performance, I'll take a look and see if this is the 
case.  (in browser and in the rest of the product.)

Comment 1

[(not reading, please use [e-mail] instead)]

0.9.1, I hope.

Comment 2

[(not reading, please use [e-mail] instead)]

moving to 0.9.2, 0.9.1 is too soon.

Comment 3

[(not reading, please use [e-mail] instead)]

I don't expect to fix this in 0.9.1, but it's one of my top priorities for 
overal performance improvements.  so focusing on it now.

Comment 4

[scottputterman]

moving to 0.9.2 

Comment 5

[scottputterman]

any ideas on what kind of improvement we expect fixing this to make?

Comment 6

[Cathleen]

we expect to help improve window open performance 

Comment 7

[scottputterman]

adding PDT+

Comment 8

[scottputterman]

moving to 0.9.3

Comment 9

[scottputterman]

moving to 0.9.4

Comment 10

[(not reading, please use [e-mail] instead)]

slide to 0.9.5

Comment 11

[(not reading, please use [e-mail] instead)]

back to trudelle.  this would be worth investigating, I just don't have time to
help.

Comment 12

[(not reading, please use [e-mail] instead)]

I'll let trudelle set the target milestone.

Comment 13

[Peter Trudelle]

->pchen for perf analysis and triage

Comment 14

[Paul Chen]

marking p2 and mozilla0.9.6

Comment 15

[Paul Chen]

After talking with Hyatt, it looks like we're overusing tooltip="aTooltip" too
far down the xul hierarchy (i.e on each toolbar button as opposed to just once
on the toolbar itself). After hacking around some, I got the number of observers
down to 43 or so just by moving tooltip="aTooltip" up the hierarchy. However,
Hyatt told me that with bug 93839, we will do away with having to specify
tooltip="aTooltip" and just create one popuplistener that knows all the
tooltiptexts. Sounds great, and it's a 0.9.6 bug to boot. So I am going to mark
this as a dup of that bug since that appears to be the right fix.

*** This bug has been marked as a duplicate of 93839 ***

Comment 16

[Paul Chen]

Sorry for the spam, hyatt wants me to clean this up because it will make his job
easier for 93839. I'll try to catch more of these before I post a patch

Comment 17

[Paul Chen]

Attachment: first pass at patch, it's just here so I don't loose it and can easily apply it on other machines

Comment 18

[Paul Chen]

Attachment: only create tooltip listeners at main toolbar, personal toolbar and statusbar

Comment 19

[Paul Chen]

Attachment: remove tooltip="aTooltip" since it's now provided by statusbar

Comment 20

[Paul Chen]

Attachment: remove tooltip="aTooltip" since it's now provided by statusbar

Comment 21

[Paul Chen]

Attachment: remove tooltip="aTooltip" from offline-status because it's now provided by statusbar

Comment 22

[Paul Chen]

Assuming I did this right under linux, I count *only* 34 popup listeners being
created now. Well, it's a jump in the right direction. ;-)

Comment 23

[Paul Chen]

Attachment: updated navigator.xul patch, moved tooltip="aTooltip" up from first hbox in nav-bar to toolbar itself to enable tooltips in print and throbber

Comment 24

[jag (Peter Annema)]

Comment on attachment 53363 [details] [diff] [review]
updated navigator.xul patch, moved tooltip="aTooltip" up from first hbox in nav-bar to toolbar itself to enable tooltips in print and throbber

r=jag

Comment 25

[jag (Peter Annema)]

Comment on attachment 53226 [details] [diff] [review]
remove tooltip="aTooltip" from offline-status because it's now provided by statusbar

r=jag

Comment 26

[jag (Peter Annema)]

Comment on attachment 53225 [details] [diff] [review]
remove tooltip="aTooltip" since it's now provided by statusbar

r=jag

Comment 27

[jag (Peter Annema)]

Comment on attachment 53224 [details] [diff] [review]
remove tooltip="aTooltip" since it's now provided by statusbar

r=jag

Comment 28

[Paul Chen]

Attachment: latest patch, navigator.xul only, don't fiddle with statusbar

Comment 29

[Paul Chen]

Ok, latest fix. I don't change anything in the statusbar anymore because that
would require me to go modify every other window in the app so that their
statusbars have tooltip="aTooltip". Hyatt will just have to go fix that when he
lands his bug. ;-)

Comment 30

[Alec Flett]

Comment on attachment 54081 [details] [diff] [review]
latest patch, navigator.xul only, don't fiddle with statusbar

sr=alecf

Comment 31

[Blake Ross]

Comment on attachment 54081 [details] [diff] [review]
latest patch, navigator.xul only, don't fiddle with statusbar

r=blake

Comment 32

[Paul Chen]

fix checked into trunk