81712 - -turbo mode should not preload homepage

Status: VERIFIED FIXED

(Core Graveyard :: Cmd-line Features, defect, P1)

Description

[Cathleen]

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.

Comment 1

[Cathleen]

Attachment: test home page, will create popup window

Comment 2

[Bill Law]

*** Bug 83139 has been marked as a duplicate of this bug. ***

Comment 3

[Matthew Miller]

Maybe this is stating the obvious, but couldn't this bug be rephrased as "-turbo
mode should not load home page"?

Comment 4

[Jesse Ruderman]

Updating summary based on dup and Matthew Miller's comments.

Comment 5

[Jens Hatlak (:InvisibleSmiley)]

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...

Comment 6

[Cathleen]

need this for -turbo.
set target milestone to 0.9.2

Comment 7

[Bill Law]

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).

Comment 8

[Viswanath Ramachandran]

nav+pdt triage: m0.9.2, P1, startup performance is critical. 

Comment 9

[Viswanath Ramachandran]

*** Bug 81714 has been marked as a duplicate of this bug. ***

Comment 10

[Bill Law]

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.

Comment 11

[Bill Law]

Attachment: Proposed solution

Comment 12

[Bill Law]

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 [e-mail] and check this in.

Comment 13

[Blake Ross]

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.

Comment 14

[Bill Law]

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

[Blake Ross]

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

[Blake Ross]

Er, worthwhile *tradeoff*, sorry...

Cc'ing syd so this spam isn't entirely useless.

Comment 17

[Syd Logan]

Looks ok to me, r=syd

Comment 18

[Bill Law]

Removing "review" keyword.  I'm emailing [e-mail] to get approval 
for checkin today.

Comment 19

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to the trunk.
(on behalf of drivers)

Comment 20

[Bill Law]

Thanks, Asa.

Resolving FIXED.

Comment 21

[Ian Tommins ('thelem')]

*** Bug 82988 has been marked as a duplicate of this bug. ***

Comment 22

[Paul Wyskoczka]

Verified in 2001062703