166219 - offline load handler never removed and fires multiple times

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[[e-mail]]

While profiling mail I noticed that the offline load handler fired 4 times.
This sounds somewhat excessive!

Comment 1

[[e-mail]]

Attachment: Proposed patch

Comment 2

[[e-mail]]

This saves about 1% of load time for mail, it probably affects other windows.

It may turn out that the subsequent "safety" code is unnecessary.

Comment 3

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

why isn't the right fix to fix our xul so that we only load that file once?

Comment 4

[[e-mail]]

> why isn't the right fix to fix our xul so that we only load that file once?

The load handler fires when a window loads, not when a script file loads.

It only needs to load for the top window, not for content frames.

http://lxr.mozilla.org/seamonkey/search?string=BM_navigatorLoad does it right.

Comment 5

[[e-mail]]

Comment on attachment 97529 [details] [diff] [review]
Proposed patch

This is wrong; the right way is for everybody to use bubbling load event
listeners.

Comment 6

[[e-mail]]

Attachment: Better patch

Comment 7

[[e-mail]]

Comment on attachment 115880 [details] [diff] [review]
Better patch

This fixes the problem by only listening for the main window load event
(content loads don't bubble, according to GlobalWindowImpl).

Comment 8

[jag (Peter Annema)]

Comment on attachment 115880 [details] [diff] [review]
Better patch

Could you add space after comma while you're touching that line (no new patch
needed of course).

r=jag

And yeah, it looks like we could get rid of that "safety" code (wanna make that
part of this patch?).

Comment 9

[[e-mail]]

Attachment: Remove "safety" code

Comment 10

[jag (Peter Annema)]

Comment on attachment 116413 [details] [diff] [review]
Remove "safety" code

r=jag.

Comment 11

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

Comment on attachment 116413 [details] [diff] [review]
Remove "safety" code

assuming you've confirmed that we aren't calling it twice for any windows that
use the utilityOverlay, sr=sspitzer

the "crude way" was part of my check in for bug #65704

please read over that bug and make sure that your change hasn't caused any
regressions.  (especially see comment #49)

question:  are we still loading utilityOverlay.xul twice in certain cases?

eyeballing the patch, it doesn't look like it would cause regressions, but
we've been getting a lot lately, so please double check.

Comment 12

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

as long as all the indicators (browser, compose, 3 pane, search, addrbook, etc)
do the right thing (stay in sync), and going offline from the File menu or the
offline widget in compose and the 3 pane still prompt the user (if configured
for that), that would be enough to smoke test this fix.

see http://www.mozilla.org/quality/mailnews/tests/mojo-offline-ui.html for some
more test cases.

Comment 13

[[e-mail]]

Hmm... possible ½% Txul win :-)

Comment 14

[[e-mail]]

Fix checked in... 5 months ago :-[