88844 - Turbo mode should run mozilla from registry

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect, P2)

Description

[Daniel Mario Vega (Restore MNG Support --> #18574)]

When "Turbo mode" is activated Mozilla should be run from Registry, not from
Start Menu. The normal way to launch background applications is from :

(System Wide Setting)
HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion\Run

Or from

(Personal Setting)
HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Run


Reproducible: Always
Steps to Reproduce:
Activate turbo mode in installer or advanced preferences

Actual Results:  A shortcut is created in Startup Menu

Expected Results:  Mozilla should be start from registry as a background process

See comments on bug 81149

This is the way many applications start. ICQ, Yahoo Messenger, RealPlayer,
Norton Antivirus, Babylon Translator, and many many more.

If the installer is run by an administrator we should set the option for all
users (or ask the administrator what he wants to do.)

There are some ways to see if a user has admin privileges. But none of them are
perfect, and most of them do not work in Windows 9x

http://www.mvps.org/vcfaq/sdk/21.htm
http://support.microsoft.com/support/kb/articles/q118/6/26.asp

The easiest way (and perhaps safest) is to test for access denied errors when
creating the registry entry.

Comment 1

[Markus Gerstel]

I disagree. Using the autostart-approach every user can see what programs will
start on system startup easily.
Mozilla takes up 16 to 20 MB RAM in Turbomode, and hiding will only make user's
life difficult. If you don't want Mozilla's Turbo Mode in autostart you can
always hide it in at least 7 different locations in the registry.

Comment 2

[Asa Dotzler [:asa]]

->law. xpapps

Comment 3

[Boris Zbarsky [:bzbarsky]]

ccing timeless

Comment 4

[Daniel Mario Vega (Restore MNG Support --> #18574)]

The "StartUp folder" is for end users to add or remove applications manually
(e.g. Microsoft Office Shortcut Bar).

Applications that need to run components at startup should use the Registry
Interface. If this behaviour is optional, the application should have a
preference option to enable or disable it (like the one in Mozilla Preferences).

This is how 99% of Windows applications work

More applications that use the registry :

WinAmp Agent
AOL Instant Messenger
MSN Messenger

Comment 5

[Corrado Berti]

registry solution is better for me.
In this case the bug 87035 should be
change to invalid.

Comment 6

[Dimitrios]

With all respect, I totally disagree with this bug. If the two methods for
loading an app at Windows startup (Startup folder and registry) are identical
regarding to the level of application priority (and I am sure they are), Startup
folder method is more honest and much easier for handling if something goes
wrong. E.g. uninstaller fails or someone deletes the Mozilla files manually. How
do you expect from an average user (even not as ignorant as the "average Joe"
example) to remove the registry key? For such a user, regedit (or other registry
editors) isn't a toy to play with. Btw, I am very happy that e.g. MSOffice "fast
start" feature (OSA.EXE) can be easily removed from the Startup folder, instead
of hacking the registry files.

Comment 7

[Bozhan Boiadzhiev]

using registry seems to me like native way... to win32 or NT
using start up is someting like to write in autoexec.bat in win3.xx...
and is there any plans to do something about linux window managers to use
something like "turbo mode" in windows?

Comment 8

[Jason Belcher]

Personally, I think it should be in the registry, just to avoid extra clutter in
the Start menu (imagine if everything used the Startup folder). However it isn't
really important. Let's focus on this stuff when the major bugs are fixed.

Severity: trivial, anyone?

Comment 9

[Jason Belcher]

Personally, I think it should be in the registry, just to avoid extra clutter in
the Start menu (imagine if everything used the Startup folder). However it isn't
really important. Let's focus on this stuff when the major bugs are fixed.

Severity: trivial, anyone?

Comment 10

[Martin Poirier]

You could also use
HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion\RunServices, which
enable to load the app at startup without showing in the Close application
dialog obtained by pressing CTRL-ALT-DEL. Another solution would be to use the
Win.ini LOAD= or RUN= parameters, though this is a bit old-style but would make
this setting available for Win 3.1, for what it's worth ;). Anyway, I agree that
startup folder is more convenient, but if we go for the registry, I suggest
HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion\RunServices in
order to mimic IE behaviour to be loaded with the OS and not as a separate app.

Comment 11

[Daniel Mario Vega (Restore MNG Support --> #18574)]

I'm not 100% sure, but I believe that "RunServices" does not work on NT/2000. I
think it only works on Win95/98/ME

Comment 12

[Blake Ross]

--> me

Comment 13

[Syd Logan]

Attachment: patch for 0.9.4 branch, mozilla

Comment 14

[Syd Logan]

Attachment: Patch for 0.9.4 branch, commercial

Comment 15

[Syd Logan]

I'm handling the installer part of this, the patches above are for that. Blake
has the rest of it.

Comment 16

[Syd Logan]

Attachment: patch for mozilla based on ssu comments

Comment 17

[Syd Logan]

Attachment: great tabs, less filling

Comment 18

[Syd Logan]

Attachment: always tell the installer to remove the key

Comment 19

[Sean Su]

r=ssu for patch id=48684.

Comment 20

[Syd Logan]

Attachment: same as above but for ns tree

Comment 21

[Sean Su]

Blake, the ns patch above is assuming that your code will use "Netscape" instead 
of "Mozilla" for the var name set in the Run key for the ns tree.  If you're 
setting something other than "Netscape", please let Syd know.

Other than the above comment, r=ssu for patch id=48685.

Comment 22

[Daniel Mario Vega (Restore MNG Support --> #18574)]

Some comments on attachment 48684 [details] [diff] [review] and attachment 48685 [details] [diff] [review]

    + fTemp = fileExe;
    + fileExe = getFolder("file:///", fTemp);
    + newKey = fileExe + ' -turbo';
    + winreg.setValueString(subkey, valname, newKey);

If fileExe contains any space, all the string must be enclosed between quotes
("). I don't know how "getFolder" works. Perhaps it's already quoted.
In line 83 of browser.jst :

http://lxr.mozilla.org/seamonkey/source/xpinstall/packager/windows/browser.jst#83

we are trying to see if the current user is admin (or something like that), so
we set shortcuts for all users instead the current one.

Should we do the same for turbo mode ?

I mean, should we create the registry key on HKEY_LOCAL_MACHINE instead of
HKEY_CURRENT_USER in that case ?

Comment 23

[Blake Ross]

Attachment: patch to app startup/prefs

Comment 24

[Syd Logan]

To respond to the above questions:

Notice that the string *is* quoted. In the JavaScript language, single and
double quotes can be used for this purpose.

As to the other question, I believe that we decided to put this in
HKEY_CURRENT_USER. I checked with Sean on this issue before creating the patch.

Comment 25

[Sean Su]

Daniel, you are correct.  The path does not show up quoted when created under 
the Run registry key.  However, I've tested under NT4 (sp6) and Win2k.  Under 
both OS, they seem to be working fine.

We'll need to test this under the other OS'es now: Win95, Win98, WinMe, and 
WinXP.

We only want to set this under HKEY_CURRENT_USER.  It was decided that we set 
only under this key for now due to the time constraint in codeing up an 
appropriate UI to let the user decide between the two (or do both).

Comment 26

[Sean Su]

I forgot to mention that the path I tested with had spaces in it:
  c:\program files\mozilla.org\mozilla\mozilla.exe -turbo

Comment 27

[Blake Ross]

In my patch, disregard

-        appShell->UnregisterTopLevelWindow( 0 );
+        appShell->Quit();

While testing my changes, I noticed that danm's change yesterday to
::UnregisterTopLevelWindow() had broken exiting from the turbo systray icon. 
But I see that bug 98792 is handling this (same patch).

Sean, can you take a look at this patch, disregarding the hardcoded "Mozilla" (I
still need to find out how to get the value from the app)?

Comment 28

[Daniel Mario Vega (Restore MNG Support --> #18574)]

Sean, you are correct. It works ok without the quotes. I think I am a big mouth.
I tested this on NT and Win98 and worked fine. 

I wonder how Windows realize which part of the string is the command and which
part are the parameters. If I try to execute the same string (without quotes)
from "Start -> Run..." it fails. But doing the same from the registry it works fine.

Sorry for the spam.

Comment 29

[Blake Ross]

Attachment: patch to app startup/prefs (updates nsIWindowsHooks documentation)

Comment 30

[Blake Ross]

Attachment: diff -u20 per reviewer request

Comment 31

[Conrad Carlen (not reading bugmail)]

Overall, looks good. One little cosmetic beef in nsAppRunner.cpp:

+    PRBool serverMode = PR_FALSE;
+    nativeApps->GetIsServerMode(&serverMode);
+    if (!serverMode) {  // okay, so it's not -turbo
+      HKEY key;
+      LONG result = ::RegOpenKeyEx(HKEY_CURRENT_USER,
"Software\\Microsoft\\Windows\\CurrentVersion\\Run", 0, KEY_QUERY_VALUE, &key);

Can't this be put into NS_CreateNativeAppSupport() or
nsNativeAppSupportWin::Start()?
Since we're no longer using the pref service, this can be done earlier, even
before XPCOM is inited. It would make for less #ifdef XP_WIN32 cruft in apprunner.

Comment 32

[Blake Ross]

Attachment: updated patch for app startup/prefs and exit dialog

Comment 33

[Conrad Carlen (not reading bugmail)]

Comment on attachment 48813 [details] [diff] [review]
updated patch for app startup/prefs and exit dialog

+    attribute boolean shouldShowUI;
 - this could probably use the same amount of comments so graciously provided for the other params.

+    PRBool   mShownTurboDialog;
 - does this need to be in the base class? Seems pretty specific to the Windows impl.

Since those are pretty minor complaints, r=ccarlen

Comment 34

[Ben Goodger (use ben at mozilla dot org for email)]

I agree with conrad wrt the mShownTurboDialog member, surely it wouldn't 
require much to move it into nsNativeAppSupportWin, unless we're planning to 
offer this same functionality on other platforms (?)

Otherwise, sr=[e-mail]

Comment 35

[Ben Goodger (use ben at mozilla dot org for email)]

Oh, forgot to mention. Use hbox/vbox instead of box with attrs, and autostretch 
is deprecated. Fix those too. 

Comment 36

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 48813 [details] [diff] [review]
updated patch for app startup/prefs and exit dialog

sr=[e-mail] with the above conditions

Comment 37

[Blake Ross]

Yeah I moved it. We'll want to show the dialog on other platforms if they 
support turbo, though.

Also I'll update the xul, it's a little out of date because I just took it from 
the 0.9.2 branch. Thanks.

Comment 38

[Asa Dotzler [:asa]]

Comment on attachment 48813 [details] [diff] [review]
updated patch for app startup/prefs and exit dialog

a=asa for checkin to 0.9.4

Comment 39

[Blake Ross]

Comment on attachment 48684 [details] [diff] [review]
always tell the installer to remove the key

sr=blake

Comment 40

[Blake Ross]

Comment on attachment 48685 [details] [diff] [review]
same as above but for ns tree

sr=blake

Comment 41

[Syd Logan]

Checked my part in for the installer, ns tree, mozilla tree, trunck and branch.

Comment 42

[Daniel Mario Vega (Restore MNG Support --> #18574)]

On attachment 48813 [details] [diff] [review] we are using a registry key value named "Mozilla
QuickLaunch" but on attachment 48684 [details] [diff] [review] and attachment 48685 [details] [diff] [review] we are using a key
value named "Mozilla". Is this correct ? 

Comment 43

[Daniel Mario Vega (Restore MNG Support --> #18574)]

My last comment was not completely right. Attachment 48685 [details] [diff] uses a keyValue named
"Netscape" . So we have 3 files, and each file uses a differen key. I understand
that ns tree and mozilla tree shpould use different names. But I dont understand
why we have 3 names for the same key.

Comment 44

[Syd Logan]

Attachment: Use Mozilla as app name, trunk version of patch

Comment 45

[Syd Logan]

Attachment: Use Mozilla as app name, branch version

Comment 46

[Suresh Duddi (gone)]

Attachment: Not using nsIHttpProtocolHandler to get run key. Using static run key to match installer.

Comment 47

[Sean Su]

r=ssu for syd's two patches: id=49173 and id=49174

Comment 48

[Sean Su]

r=ssu for Dp's patch too: id=49175

Comment 49

[Conrad Carlen (not reading bugmail)]

I just AIM'd with blake who asked if I could make the call to the http happen
later after XPCOM was known to be inited. While looking at that (which  was
looking to be non-trivial), syd's and dp's patches showed up.

+// The key that is used to write the quick launch appname in the windows registry
+#define NS_QUICKLAUNCH_RUN_KEY "Mozilla Quick Launch"

Is it possible to make this definition get setup by something in the makefiles
so that it can be different between mozilla and commercial?

Comment 50

[Daniel Mario Vega (Restore MNG Support --> #18574)]

Just an aditional comment. If I understand the code correctly, then as a side
effect of attachment 49173 [details] [diff] [review], attachment 49174 [details] [diff] [review] and attachment 49175 [details] [diff] [review], it is not
possible to have both Mozilla and Netscape browsers installed on the same
machine and using both the quick launch feature. Because they both use the same
registry key value and so they overwrite each other. Also if we enable the quick
launch feature in one browser, the "Quick Launch" preference option in the other
one will be misleading.

Comment 51

[Syd Logan]

Life's not perfect, we have a 0.9.4 to get out, NS won't be releasing for some
time, so let's get this in so at least both, independent of each other, actually
work when we wake up tomorrow. This turbo stuff has been down too long.

Comment 52

[Brendan Eich [:brendan]]

Ewww, hard tabs:

+            if ( result == ERROR_SUCCESS ) {

+
			// XXX To make absolutely sure, we should check the value to see if this is

+
			// XXX us or one of our predecessors - bascially distinguish betn mozilla and

+
			// XXX mozilla based browsers
+                SetIsServerMode( PR_TRUE );

+
		}

Fix these, and sr=[e-mail].

/be

Comment 53

[Asa Dotzler [:asa]]

a=asa for checkin to 0.9.4 branch

Comment 54

[Syd Logan]

Attachment: trunk: actually turn on the code dp added, doh. got rid of tabs per /be (I hope)

Comment 55

[Syd Logan]

Attachment: This patch also needs to go on the branch

Comment 56

[Ferdinand]

On the trunk (win2k cvs 2001091303) -turbo makes mozilla unusable now.
It will only load 1 navigator window, any additional windows that are loaded
exist in memory but not visually. They show under Tasks as emtpy lines.

Comment 57

[Ferdinand]

sorry, spoke too soon, seeing it happen without quick launch too now

Comment 58

[Suresh Duddi (gone)]

Attachment: Reenabling turbo on launch

Comment 59

[Suresh Duddi (gone)]

When syd checked in my patch last night, he enabled code that should have been
#if 0 - This will cause xpcom to get double initialized, putting the app in a
unstable state. This will happen only in the case where:
- turbo mode was enabled
- user quits turbo-moded mozilla by right clicking on the lizard icon in the systray
- restarts mozilla
The patch "Reenabling turbo on launch" fixes it. 

Comment 60

[Conrad Carlen (not reading bugmail)]

Comment on attachment 49243 [details] [diff] [review]
Reenabling turbo on launch

I think having another method on nsINativeAppSupport which is called *after* xpcom is initialized is a good thing. The name setupServerMode(), since it's only done for one feature on one platform could be a bit more generic. Maybe what's currently called start() could be renamed initialize() and setupServerMode() becomes start()?

Also, since what's happening in setupServerMode() is now happening after xpcom is initialized, can we go back to getting the app name in some non-static way? If the problem pointed out by Daniel is true, we should avoid that.

Comment 61

[Conrad Carlen (not reading bugmail)]

For why I think we need more generic methods on nsINativeAppSupport which are
called at startup, see attachment=49227 from bug 98566.

Comment 62

[Suresh Duddi (gone)]

We thought users wont want fast start for every mozilla and mozilla based
browser. Hence the static name instead of different name. Whichever browser last
enabled fast start should win.

Since we are targetting this for 0.9.4, I would rather minimize the changes and
make them post the branch.

Marking TFV 0.9.4 to get in on the radar.

Comment 63

[chris hofmann]

if we don't get this sorted out for the mozilla release we will want to get it
sorted out on the branch afterwords...

Comment 64

[Sean Su]

Dp's latest patch (id=49243) does also fix a startup crash (in debug builds 
only) as well.  There is also a comperable crash on exit which has been filed as 
bug 99553.

I've noticed that the Mozilla Quick Launch variable in the Run key sometimes 
gets automatically (and mysteriouly) removed at the first startup immediately 
after the install is completed.  I have not been able to reproduce it 
consistently.  It might not have anything to do with this patch, but thought I 
just mention it.  I'm still investigating it.

Comment 65

[jlarsen]

I know this is not the proper place to post such comments, but personally I'm
very dissapointed in you people. Running apps from Registry has been always
viewed as the "sneaky" way of auto loading apps, so that the common user can't
remove the app easily. I think this is completly opposite the spirit of mozilla,
and I'm very dissapointed that this bug didn't get more debate. :(

Comment 66

[Mitesh Shah]

I think the patch (attachment 48813 [details] [diff] [review]) has not checked in properly into the 0.9.4 
branch. The branch lost some of the code which is not part of this patch but 
part of bug 92447. Particularly, I am talking about 
xpfe/bootstrap/nsAppRunner.cpp 
The following changes are not yet landed on the branch but it seems that it's 
already applied to the branch as part of the fix 48813. This breaks the 
ReadConfig/AutoConfig functionality on the branch.

Index: mozilla/xpfe/bootstrap/nsAppRunner.cpp
===================================================================
RCS file: /cvsroot/mozilla/xpfe/bootstrap/nsAppRunner.cpp,v
retrieving revision 1.305
diff -u -2 -r1.305 nsAppRunner.cpp
--- nsAppRunner.cpp     2001/08/29 14:02:05     1.305
+++ nsAppRunner.cpp     2001/08/31 20:25:36
@@ -937,6 +937,4 @@
         rv = prefs->ResetPrefs(); 
         if (NS_FAILED(rv)) return rv; 
-        rv = prefs->ReadConfigFile(); 
-        if (NS_FAILED(rv)) return rv; 
         rv = prefs->ReadUserPrefs(nsnull); 
         if (NS_FAILED(rv)) return rv; 
@@ -944,7 +942,4 @@
     else 
     {
-        rv = prefs->ReadConfigFile(); 
-        if (NS_FAILED(rv)) return rv; 
-
         nsCOMPtr<nsIAppShellService> 
appShellService(do_GetService(kAppShellServiceCID, &rv));
         if (NS_FAILED(rv)) return rv;

Comment 67

[Bill Law]

Mitesh,

I remember a conflict merging that diff onto the branch.  I couldn't wrap my 
tired brain around what was going on and apparently erred in resolving the 
conflict the way I did.

Can you work out how that code should now look on the branch?  My recollection 
is that the 3 patches I had to work with (one from bug 88844, one from another 
bug, and the actual cvs diffs) had me sufficiently confused that I wasn't sure 
how to proceed.

If need be, we should open a separate bug (if there isn't one already).  Conrad 
Carlen might be able to help out; I think he likely understands the changes 
that were made here.

Comment 68

[Mitesh Shah]

Actually, the fix from bug 92447 is also going to be checked in to the branch 
and it should be fine after that. In that case, we probably don't need to do a 
separate check in but need to modify the fix from bug 92447.

Comment 69

[Suresh Duddi (gone)]

Attachment: [dp]Redoing relaunch enabling of turbomode and fixing XPCOM double initialization

Comment 70

[Suresh Duddi (gone)]

BTW, my patch on 9/14 16:13 is for the branch.

Comment 71

[Bill Law]

That latest patch looks good, dp.  r=law

I've applied it, built, and tested on my branch build and it fixes the problem.

Comment 72

[Bill Law]

Blake, can we reassign this bug to dp so he can track landing this latest 
patch?  Or, dp, do you want to take it?

Comment 73

[Suresh Duddi (gone)]

I will take this bug. Blake protest if you like to do it differently.

Comment 74

[Asa Dotzler [:asa]]

0.9.4 is out the door.

Comment 75

[Suresh Duddi (gone)]

Comment on attachment 49396 [details] [diff] [review]
[dp]Redoing relaunch enabling of turbomode and fixing XPCOM double initialization

Bill reviewed this.

Comment 76

[Bill Law]

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

Comment 77

[Bill Law]

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

Comment 78

[Grace Bush]

changing QA contact- 

Comment 79

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

instead of re-using b, please use isServerMode and shouldShowUI.  what ever
nanoseconds you hope to gain by re-using the bool you loose in readability.

fix that and sr=sspitzer

Comment 80

[Alec Flett]

Comment on attachment 49396 [details] [diff] [review]
[dp]Redoing relaunch enabling of turbomode and fixing XPCOM double initialization

sr=alecf

Comment 81

[Jaime Rodriguez, Jr.]

PDT+. Check it in today.

Comment 82

[Suresh Duddi (gone)]

Fix checked into trunk and branch.

Comment 83

[Grace Bush]

verified on trunk and branch - builds 2001092505