offline load handler never removed and fires multiple times

RESOLVED FIXED

Status

SeaMonkey
UI Design
RESOLVED FIXED
16 years ago
10 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: neil@parkwaycc.co.uk)

Tracking

({perf})

Trunk
x86
Windows 95

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

1.20 KB, patch
jag (Peter Annema)
: review+
(not reading, please use seth@sspitzer.org instead)
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

16 years ago
While profiling mail I noticed that the offline load handler fired 4 times.
This sounds somewhat excessive!
(Assignee)

Comment 1

16 years ago
Created attachment 97529 [details] [diff] [review]
Proposed patch
(Assignee)

Comment 2

16 years ago
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.
Keywords: patch, perf, review
why isn't the right fix to fix our xul so that we only load that file once?
(Assignee)

Comment 4

16 years ago
> 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.
(Assignee)

Comment 5

16 years ago
Comment on attachment 97529 [details] [diff] [review]
Proposed patch

This is wrong; the right way is for everybody to use bubbling load event
listeners.
Attachment #97529 - Attachment is obsolete: true
(Assignee)

Comment 6

16 years ago
Created attachment 115880 [details] [diff] [review]
Better patch
(Assignee)

Comment 7

16 years ago
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).
Attachment #115880 - Flags: superreview?(sspitzer)
Attachment #115880 - Flags: review?(jaggernaut)

Comment 8

16 years ago
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?).
Attachment #115880 - Flags: review?(jaggernaut) → review+
(Assignee)

Comment 9

16 years ago
Created attachment 116413 [details] [diff] [review]
Remove "safety" code
(Assignee)

Updated

16 years ago
Attachment #115880 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #116413 - Flags: superreview?(sspitzer)
Attachment #116413 - Flags: review?(jaggernaut)

Comment 10

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

r=jag.
Attachment #116413 - Flags: review?(jaggernaut) → review+
Attachment #115880 - Flags: superreview?(sspitzer)
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.
Attachment #116413 - Flags: superreview?(sspitzer) → superreview+
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.
Assignee: blaker → neil
(Assignee)

Comment 13

16 years ago
Hmm... possible ½% Txul win :-)
(Assignee)

Comment 14

15 years ago
Fix checked in... 5 months ago :-[
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Product: Core → Mozilla Application Suite

Updated

10 years ago
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.