Closed
Bug 81712
Opened 24 years ago
Closed 24 years ago
-turbo mode should not preload homepage
Categories
(Core Graveyard :: Cmd-line Features, defect, P1)
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla0.9.2
People
(Reporter: cathleennscp, Assigned: law)
References
Details
(Whiteboard: patch available)
Attachments
(2 files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
if home page is set to a page that will do popup ads, it will cause probelems when we're pre-loading using -turbo mode.
Summary: home page with popup will cause problem when in -turbo mode → homepage with popup will cause problem when in -turbo mode
*** Bug 83139 has been marked as a duplicate of this bug. ***
Comment 3•24 years ago
|
||
Maybe this is stating the obvious, but couldn't this bug be rephrased as "-turbo mode should not load home page"?
Comment 4•24 years ago
|
||
Updating summary based on dup and Matthew Miller's comments.
Summary: homepage with popup will cause problem when in -turbo mode → -turbo mode should not preload homepage
Comment 5•24 years ago
|
||
I don't know if this is not going to happen anyway but I'd like to remind you who'll implement the fix for this bug that the contents of the current Sidebar tab shouldn't be loaded either: The last tab selected when closing the last Navigator window will be loaded on startup, and this tab may (like what I experienced with a tab written by me) ask for WWW authentication...
need this for -turbo. set target milestone to 0.9.2
Target Milestone: --- → mozilla0.9.2
This bug overlaps with a few other of the "-turbo mode" bugs. I'm thinking we might need to back off a bit on this feature and not keep a browser window open and hidden. Doing that leads to this bug, plus the bugs related to these hidden windows hanging around in the Tasks menu, the problem with mail not opening nav windows, the problems related to window sizing, etc. An alternative might be to open a navigator window, but then close it immediately (while blocking home page load, sidebar load, etc.). That will load the .xul-related libraries, cache some stuff, etc. and hopefully still be fast enough. The only other thing that would need be added is code that stops the app from terminating when the final top-level window closes. This change would permit us to back out some of the other stuff added for bug 75599 (navigator.js changes and some of the code in nsNativeAppSupportWin.cpp).
Whiteboard: time:3days
Updated•24 years ago
|
QA Contact: sairuh → paw
Comment 8•24 years ago
|
||
nav+pdt triage: m0.9.2, P1, startup performance is critical.
Keywords: nsbeta1+
Priority: -- → P1
Assignee | ||
Comment 10•24 years ago
|
||
Here's the plan of attack: When starting up in turbo mode, we want to open a Nav window (to load shared libraries, cache .xul, etc.), but not let it, or any other windows, become visible. We will then immediately close that window. The "window close" logic that detects that there are no more top-level windows and terminates the application will be modified to not do that if we're running in turbo mode. File->Exit will turn off turbo mode and then do what it already does (closing windows, which will now cause the app to quit because it's no longer in server mode). All code dealing with the "cached" Nav window can then go away. This includes the changes to Nav window close logic in navigator.xul/navigator.js. Specifics: 1. Ensure1Window will (before opening a normal browser window), check for turbo mode and instead open a "special" nav window: - to the url "about:blank" (avoiding user home page) - with window feature toolbar=no (to suppress sidebar) - with some magic arg that navigator.js can check (see below); this is legit; we use the same technique to pass charset info, for example. Probably, this window will need to be re-parented as it is now to ensure it remains hidden, suppress events that cause grief, etc. 2. navigator.js (or navigatorOverlay.js, not sure which it is) will, in its onload handler, look for the magic arg that indicates that it is the "turbo mode" window. That code will then close it before any damage occurs (e.g., putting up the "default browser" alert). 3. Tweak http://lxr.mozilla.org/seamonkey/source/xpfe/appshell/src/nsAppShellService.cpp# 777 slightly so it doesn't quit if we're in turbo mode (i.e., nsINativeAppSupport::GetIsServerMode returns true). 4. Tweak http://lxr.mozilla.org/seamonkey/source/xpfe/global/resources/content/globalOver lay.js#1 so that it fetches and unsets turbo mode. If the user cancels out, then reset turbo mode if it was previously set. This doesn't sound too hard. I should have a patch tomorrow.
Assignee | ||
Comment 11•24 years ago
|
||
Assignee | ||
Comment 12•24 years ago
|
||
I attached a patch with the fix for this bug and lots of other -turbo mode bugs (81708, 81714, 82804, 83569, and 83783). This removes all the code checked in for bug 75599 that dealt with the "cached" browser window. Instead, this code opens a browser window and immediately closes it. To keep the app running, I tweaked code (as described above) in nsAppShellService::UnregisterTopLevelWindow and in globalOverlay.js. If somebody would please review this and super-review it, I'll go for approval from drivers@mozilla.org and check this in.
Comment 13•24 years ago
|
||
This looks great. A couple comments/nits: // Make sure its for our service/topic. - if ( FindTopic( hsz1 ) < topicCount ) { + if ( FindTopic( hsz1 ) != -1 ) { This makes sense; is it related to this bug? + turboMode = false; Need to declare this (var). + window.setTimeout( "window.close()", 100 ); Anyone know why close() won't work here immediately? jst? Major nit here (since everything passed in at this point will likely be a DOM window), but: -HWND hwndForDOMWindow( nsIDOMWindowInternal *window ) { +HWND hwndForDOMWindow( nsISupports *window ) { Change the function name (hwndForWindow?) to account for this somewhat greater flexibility? +// - No toolbar (so there's no sidebar panels loaded, either) Actually, I think someone (pink?) found that the active panel is loaded on startup even if the sidebar is hidden, although that's a separate bug. Why does no toolbar ensure no sidebar? Both check the same arg? Stuff like dialog=no makes me queasy, I'd hate to see turbo=yes get added. Can't we just assume that the existence of a 'turbo' argument means turbo mode? Otherwise, sr=blake if Syd is okay with this. Should we expect slower (re) start times? I didn't notice any in my debug build.
Assignee | ||
Comment 14•24 years ago
|
||
re: topic check change Yes, that's a stray line from another fix I had in my tree. See bug 84562. I figured I'd just leave it since otherwise that bug would never get fixed. re: *var* turboMode Good catch; thanks. I've fixed it. re: setTimeout for the window.close() I didn't look into this to find out why it didn't work. I don't want to try to push through some low-level window/widget close logic change at this point so I took the path of least resistance. Since previously the window was left open forever (or until the made it visible and then closed it), we not exposing ourselves to any additional risk by keeping it open for 100 extra milliseconds (I don't think). Maybe another bug should be opened for this? I do know that window.close() in an onload handler works in general (maybe only in dialogs?). re: sidebar panel may open That could be a problem if it happens soon enough, and, forces a pop-up window. The latter is unlikely and I don't see any way around it aside from fixing the bug you mention. toolbar=no removes the sidebar because toolbar=no is "standard" JS and pop-up windows (like netscape.com ads) use that to remove browser chrome (and they don't want those ad windows to have sidebars, either). re: dialog=no and turbo=yes These are different kinds of things, though (the former is a Window "feature", the latter just a navigator.js pseudo-interface convention. As I mentioned above, this (the technique used for turbo=yes) is the same technique by which we pass in charset= (and some trickier magic to deal with focus). I'm not sure if you're asking whether just plain "turbo" would suffice (vs. "turbo=yes")? It might, but the code I cribbed for charset= implied some sort of convention that I chose to adopt as well. Maybe someday we'll have need for more flexibility (e.g., turbo=full-throttle that will leave the window open to gain even more speed improvement). Anyway, I'd just as soon leave it that way. re: slower start times Starting Mozilla for real (the second time, while already running in turbo mode) will take longer to open a browser window because it will have to make one from scratch, instead of just showing the "cached" one. The time will be the same as what Ctrl-N takes to open a new window (give or take a millisecond or two). That was deemed a fair trade for the 6 bugs. Thanks, Blake.
Comment 15•24 years ago
|
||
Sounds good. Sure, I agree the minor start time increase is a worthwhile increase; I was just curious. And you're right that we might want future types turbo startup; can't hurt to be future compatible.
Comment 16•24 years ago
|
||
Er, worthwhile *tradeoff*, sorry... Cc'ing syd so this spam isn't entirely useless.
Assignee | ||
Comment 18•24 years ago
|
||
Removing "review" keyword. I'm emailing drivers@mozilla.org to get approval for checkin today.
Keywords: review
Comment 19•24 years ago
|
||
a= asa@mozilla.org for checkin to the trunk. (on behalf of drivers)
Blocks: 83989
Assignee | ||
Comment 20•24 years ago
|
||
Thanks, Asa. Resolving FIXED.
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•