While profiling mail I noticed that the offline load handler fired 4 times. This sounds somewhat excessive!
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?
> 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 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
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 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+
Attachment #115880 - Attachment is obsolete: true
Comment on attachment 116413 [details] [diff] [review] Remove "safety" code r=jag.
Attachment #116413 - Flags: review?(jaggernaut) → review+
16 years ago
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
Hmm... possible ½% Txul win :-)
Fix checked in... 5 months ago :-[
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.