128377 - Generalize turbo code

Status: VERIFIED FIXED

(Core Graveyard :: QuickLaunch (AKA turbo mode), defect)

Description

[Joseph Elwell]

The Turbo code needs to be generalized to allow other applications to modify 
the system registry, without trouncing the quicklaunch support.

The proposal approved by Bill is to modify nsIWindowsHooks so that 
IsStatupTurboEnabled, startupTurboEnable and startupTurboDisable all pass in a 
string that is the command line argument that is to be dealt with. This way 
mail can add "-mail" to the "Mozilla Quick Launch" Key in a known documented 
manner without trouncing existing command line arguments.

Comment 1

[Joseph Elwell]

Attachment: patch.

Comment 2

[Bill Law]

(1) I think "command" should be "option" in the renamed methods.  It's 
really "commandLineOption" and "option" is a better abbreviation, IMHO.  
Thoughts?

(2)
+    void IsCommandEnabled(in wstring command, [retval] out boolean result);
 
That should be "isOptionEnabled" (lower case "i").  And just make the return 
type "boolean" instead of the "[retval] out boolean result"?  I.e.,

     boolean isOptionEnabled(in wstring option);

(3) Can you put the function {}s the way they're supposed to be in this file 
(the *right* way :-)?  People keep sneaking in this bogus style (or has the 
proper style become so out of style that it is now completely forbidden :-).

(4) Should the command/option args be "string" rather than "wstring"?  You're 
just converting them to "string" anyway.

(5) I think having to prepend a " " to the command/option arg is awkward.  Can 
we just do that inside the functions so callers don't have to?

(6) The string fiddling around your ::StartupAddCommand seems wrong.  Your 
foofile and keyvalue buffers may not be big enough and there is no checking on 
length when you strcat to it.  I'll need to spend a bit more time examining 
that code.

Comment 3

[Joseph Elwell]

1 & 2 I will do.

3 is odd. Where do you get your coding style guide? The new layout guide 
http://www.mozilla.org/newlayout/doc/codingconventions.html says this:
"Curly braces on the same line as the if, for, while or switch statement. Curly 
braces on a new line for method implementations."
which is completely opposite of what I just did. But I recall being told to at 
least do curly braces on a new line for if statements in the past. Oddly 
enough, and somewhat related, this document 
http://www.mozilla.org/projects/ef/coding_style.html says use 4 space tabs 
while the New layout guide says use 2. And I know I've been told to use 2 in 
the past.

4. i will do this.

5. I think it's awkward as well. I can add the space in manually, it just 
seemed like wasted effort when I could just document the input, rather than 
shuffling strings around, causing a rediculously minor amount of extra 
performance requirements.

6. Please spend as much time as neccessary. I'll put off trying to get this 
code in until next week.

Comment 4

[Alec Flett]

4 spaces for indentation unless the first line of the file (The emacs mode line)
says otherwise. And never EVER use tabs. 

I'll try to review this soon.

Comment 5

[Alec Flett]

Comment on attachment 72115 [details] [diff] [review]
patch.

>       }
>       var winhooksService = Components.classes["@mozilla.org/winhooks;1"].getService(Components.interfaces.nsIWindowsHooks);
>       if (winhooksService) {
>-        parent.isTurboEnabled = winhooksService.isStartupTurboEnabled();
>+        parent.isTurboEnabled = winhooksService.IsCommandEnabled(" -turbo");

I agree with bill, requiring the spaces is lame. the whole "documenting to
avoid doing work myself"
 is the wrong attitude here :)



>+  RegistryEntry startup ( HKEY_CURRENT_USER, "Software\\Microsoft\\Windows\\CurrentVersion\\Run", NS_QUICKLAUNCH_RUN_KEY, NULL );
>+  nsCString cargs = startup.currentSetting();
>+  if (cargs.Find(NS_ConvertUCS2toUTF8(command).get(), PR_TRUE, 0, _MAX_PATH) != -1)
>+    *_retval = PR_TRUE;

Not -1, use kNotFound.

>+void grabArgs(char* commandline, char** args)
>+{
>+  strtok(commandline, "\"");
>+  *args = strtok(NULL, "\"");
> }

this is bad! strtok() is verboten. Use nsCRT::strtok(). 
You'll have to maintain some state here with that.

>+      char keyvalue[_MAX_PATH] = { 0 };
>+      if ( rv ) {
>+        strcat(args, NS_ConvertUCS2toUTF8(command).get());
>+        strcat(keyvalue, "\"");
>+        strcat(keyvalue, fileName);
>+        strcat(keyvalue, "\"");
>+        strcat(keyvalue, args);
>+        RegistryEntry startup ( HKEY_CURRENT_USER, "Software\\Microsoft\\Windows\\CurrentVersion\\Run", NS_QUICKLAUNCH_RUN_KEY, keyvalue );
>+        startup.set();
>+      }

learn to use the string classes. this should be an nsCAutoString, and 
you should be using Append() not strcat()


>+    RegistryEntry startup ( HKEY_CURRENT_USER, "Software\\Microsoft\\Windows\\CurrentVersion\\Run", NS_QUICKLAUNCH_RUN_KEY, NULL );
>+    nsCString cargs = startup.currentSetting();
>+    char* args;
>+    char* s = ToNewCString(cargs);
>+    grabArgs(s, &args);
>+

ack! I'm not sure this allocation is necessary once you switch to
nsCRT::strtok() 
(I might be wrong though)
It kind of seems like you don't want strtok(), instead you want to use Find()

>+    nsCAutoString launchcommand;
>+    launchcommand.Assign(args);

all at once now:
+ nsCAutoString launchCommand(args);


>+    launchcommand.ReplaceSubstring( NS_ConvertUCS2toUTF8(command).get(), (char *) " ");
>+    launchcommand.CompressWhitespace(PR_FALSE, PR_TRUE);

Hmmm.. an interesting approach, but it seems wasteful. if we know the length of
command() why don't we just
launchcommand.Cut(0,command.Length());

Am I reading this wrong?

>+    nsCAutoString ufileName;
>+    ufileName.Assign("\"");

ufileName.Assign('\"'); - no need to create a literal string here.

>+    ufileName.Append(fileName);
>+    ufileName.Append("\"");

same here

>+    ufileName.Append(launchcommand);
>+    PRInt32 foo = launchcommand.CompareWithConversion("", PR_TRUE, -1);

>+    if (NS_SUCCEEDED(rv) && foo == 0)

Wow! A case-insensitive comparison with an empty string? What does "foo" mean?
real variables are NEVER named foo.

I think you mean:

if (NS_SUCCEEDED(rv) && !foo.IsEmpty())


>+    {
>+      HKEY key;
>+      ::RegOpenKeyEx( HKEY_CURRENT_USER, "Software\\Microsoft\\Windows\\CurrentVersion\\Run", 0, KEY_SET_VALUE, &key );

Maybe we ought to be #defining this registry key somewhere, I see it alot.


Plus, a ditto on all of bill's review comments.

Comment 6

[Alec Flett]

Comment on attachment 72115 [details] [diff] [review]
patch.

also, even if you can avoid that application you should never use free() for
something allocated with ToNewCString() - use nsMemory::Free()

Comment 7

[Bill Law]

Joe,

(3) was just me complaining about *my* style being subverted, contrary to the
"when in Rome..." rule at http://www.mozilla.org/hacking/reviewers.html.  I
can't tell you the number of times I've had to amend patches to files where I've
forgotten to do it the other way, and turnabout is fair play.  As to which way
is right or wrong...that's why I put the :-) in there.

Comment 8

[Joseph Elwell]

Alec: I didn't say *I* was avoiding work, as you can plainly see I've spent some
time in getting this generalization done in a meaningful fashion. I said
performance. That space needs to be there, and it would be minutely faster for
the caller to just have it. But it's a rediculous amount to worry about, and I
will be changing it.

"learn to use the string classes. this should be an nsCAutoString, and 
you should be using Append() not strcat()"
I know, generally, how and when to use the different and many string classes, if
you notice that registry key wants a char*, this is windows only code of course.
The code is littered enough with pushing strings from one class to another. Do
we need more shoving about?

Comment 9

[Alec Flett]

Joe - thanks for fixing the " " in the API.

I don't think it would be that much shuffling, and you prevent a possible buffer
overflow by using the string classes, and if you want to talk about speed, the
string classes will be faster than strcat(), which needs to find the end of the
string every time. Besides, you can go to |const char *| with a simple .get()

does the registry code require const char* or char*? and if it's the latter, is
it just a bad api, or are they actually writing out to the string? if it's the
former, a NS_CONST_CAST will be in order.

as for more shoving about, if you use the string classes from the get-go (for
instance, instead of declaring a buffer, which is in the code that you put in
the patch..) then you never end up shoving :)

Comment 10

[Joseph Elwell]

Alec: one thing I don't follow:
ufileName.Assign('\"'); - no need to create a literal string here.

Is there an easier way to encapsulate the command with quotes?

Comment 11

[Joseph Elwell]

Attachment: updated patch.

Comment 12

[Alec Flett]

What I mean is, instead of doing this:

foo.Assign("\"");

do this:
foo.Assign('\"');

The former appends a zero-terminated string, the latter appends a single
character... faster/easier for the string code, is all.

Comment 13

[Joseph Elwell]

I believe I did everything except for this:
>+    launchcommand.ReplaceSubstring( NS_ConvertUCS2toUTF8(command).get(), (char
*) " ");
>+    launchcommand.CompressWhitespace(PR_FALSE, PR_TRUE);

Hmmm.. an interesting approach, but it seems wasteful. if we know the length of
command() why don't we just
launchcommand.Cut(0,command.Length());

Does Cut do a find? I would imagine the first argument would need to be the
starting position in the string where i'm cutting from.

Comment 14

[Alec Flett]

Comment on attachment 72433 [details] [diff] [review]
updated patch.

>+        char foofile[_MAX_PATH] = { 0 };
>+        DWORD foosize = sizeof foofile;
>+        int rv = ::GetModuleFileName( NULL, fileName, sizeof fileName );

no foo! Please name these variables correctly.

>+    RegistryEntry startup ( HKEY_CURRENT_USER, "Software\\Microsoft\\Windows\\CurrentVersion\\Run", NS_QUICKLAUNCH_RUN_KEY, NULL );

so is this long registry key getting a #define or what? I see it 6 other times
in this pat

>+    nsCString cargs = startup.currentSetting();
>+    if (cargs.Find(option, PR_TRUE, 0, _MAX_PATH) != kNotFound)
>+        *_retval = PR_TRUE;
>     return NS_OK;

why _MAXPATH? cargs is a nsCString, you don't need a limit.


> }
>+        char fileName[_MAX_PATH];
>+        HKEY key;
>+        char foofile[_MAX_PATH] = { 0 };
>+        DWORD foosize = sizeof foofile;

again with the foo. 

>+            {
>+                nsCAutoString keyvalue("\"");
>+                keyvalue.Append(fileName);
>+                keyvalue.Append("\"");

append characters, not 1-byte strings
keyvalue.Append('\"');

>+                keyvalue.Append(args);
>+                keyvalue.Append(" ");

same here.

>+    RegistryEntry startup ( HKEY_CURRENT_USER, "Software\\Microsoft\\Windows\\CurrentVersion\\Run", NS_QUICKLAUNCH_RUN_KEY, NULL );
>+    nsCString cargs = startup.currentSetting();
>+    char* args;
>+    grabArgs(NS_CONST_CAST(char *, cargs.get()), &args);

woah, if you're going to NS_CONST_CAST (which is a good indication you're doing
the wrong thing) then at least add a comment.
In this case, it looks like this is safe because grabArgs uses nsCRT::strtok,
which only tweaks individual bytes of the string.
but it's still evil :)

>+
>+    nsCAutoString launchcommand(args);
>+    launchcommand.ReplaceSubstring(option, (char *) " ");
>+    launchcommand.CompressWhitespace(PR_FALSE, PR_TRUE);

to respond to your query in the bug: yes, you'll have to use Find() here.
And actually, looking at this code the above will simply FAIL unless the option
in question is at the end of the run line.

>+    nsCAutoString ufileName;
>+    ufileName.Assign("\"");

more byte vs. single-byute strin
>+    ufileName.Append(fileName);
>+    ufileName.Append("\"");
>+    ufileName.Append(launchcommand);
>+    if (NS_SUCCEEDED(rv) && launchcommand.IsEmpty())
>+    {

I don't understand why this NS_SUCCEEDED(rv) test is after all this
ufileName.Append stuff..
very confusing. nsresult checks should happen immediately after failure, or you
document why you're sticking code
in between the failure and the actual code

oh my goodness! looking back at the patch, I see that rv is an int (not an
nsresult) that comes from ::GetModuleFileName(), 
which has nothing to do with NS_SUCCEEDED().. a perfect reason to always follow
this pattern

>+        HKEY key;
>+        ::RegOpenKeyEx( HKEY_CURRENT_USER, "Software\\Microsoft\\Windows\\CurrentVersion\\Run", 0, KEY_SET_VALUE, &key );
>+        ::RegDeleteValue( key, NS_QUICKLAUNCH_RUN_KEY );
>+        ::RegCloseKey( key );
>+    }
>+    else
>+    {
>+        startup.setting = ufileName;
>+        startup.set();
>+    }

Not only that, but the value of ufileName only needs to be calculated inside
this else {}

Comment 15

[Joseph Elwell]

Attachment: phase 3 :)

Comment 16

[Alec Flett]

Comment on attachment 72491 [details] [diff] [review]
phase 3 :)

Looks close, but I still don't understand why you insist on passing in 4
arguments to
.Find() - it has default arguments, use 'em! (and besides, if there is a good
reason for the 3rd and 4th
argument, you should have added them as a knee-jerk reaction to my review)

If the reviewer isn't clear, or is wrong in a suggestion, then assume anyone
else would probably make the same mistake.. and document the code.

>+    RegistryEntry startup ( HKEY_CURRENT_USER, RUNKEY, NS_QUICKLAUNCH_RUN_KEY, NULL );
>+    nsCString cargs = startup.currentSetting();
>+    char* args;
>+//grabArgs desperately wants a non const char* to tokenize.
>+//this is evil :)
>+    grabArgs(NS_CONST_CAST(char *, cargs.get()), &args);

indentation.. and comments explain WHY you're doing something, they don't just 
rewrite the code in english. Why does it need a non-const char? (the answer
being, 
that it modifies the incoming buffer) Why is this safe?

>+
>+    nsCAutoString launchcommand(args);
>+    
>+    PRInt32 optionlocation = launchcommand.Find(option, PR_TRUE, 0, _MAX_PATH);
>+    // modify by one to get rid of the space we prepended in StartupAddOption
>+    launchcommand.Cut(optionlocation - 1, nsCRT::strlen(option) + 1);
>+
>+    char fileName[_MAX_PATH];
>+    int rv = ::GetModuleFileName( NULL, fileName, sizeof fileName );
>+	
>+    if (rv)
>+    {
>+        if (launchcommand.IsEmpty())
>+        {
>+            HKEY key;
>+            ::RegOpenKeyEx( HKEY_CURRENT_USER, RUNKEY, 0, KEY_SET_VALUE, &key );
>+            ::RegDeleteValue( key, NS_QUICKLAUNCH_RUN_KEY );
>+            ::RegCloseKey( key );
>+        }
>+        else
>+        {
>+            nsCAutoString ufileName;
>+            ufileName.Assign('\"');
>+            ufileName.Append(fileName);
>+            ufileName.Append('\"');
>+            ufileName.Append(launchcommand);
>+            startup.setting = ufileName;
>+            startup.set();
>+        }
>+    }
>     return NS_OK;
> }
>

Comment 17

[Bill Law]

>+    if (cargs.Find(option, PR_TRUE, 0, _MAX_PATH) != kNotFound)

This isn't quite right.  The entry in the registry is "<path> -turbo -aim" and
"<path>" can be _MAX_PATH bytes long.  So "-turbo" or "-aim" won't necessarily
start in the first _MAX_PATH bytes.

>+        char fileName[_MAX_PATH];
>+        HKEY key;
>+        char exefile[_MAX_PATH] = { 0 };
>+        DWORD exesize = sizeof exefile;
>+        int rv = ::GetModuleFileName( NULL, fileName, sizeof fileName );
>+        LONG result = ::RegOpenKeyEx( HKEY_CURRENT_USER, RUNKEY, 0,
KEY_QUERY_VALUE, &key );
>+        char* args;
>+        if (result == ERROR_SUCCESS)
>+        {
>+            LONG result = ::RegQueryValueEx( key, NS_QUICKLAUNCH_RUN_KEY,
NULL, NULL, (LPBYTE)exefile, &exesize );
>+            ::RegCloseKey( key );

This is broken for the same reason.  You might try:
   nsCString exeFile = RegistryEntry(HKEY_CURRENT_USER,
                                     RUNKEY,
                                     NS_QUICKLAUNCH_RUN_KEY,
                                     "").currentSetting();

Which might be better for other reasons, too (I believe in "the less code, the
better").  Also, "exeFile" is a misnomer.  It's more like "currentEntry" or
"oldKeyValue" or something.

>+            char keyvalue[_MAX_PATH] = { 0 };

This is leftover from previous revisions of the patch (and no longer needed)?

>+            if ( rv )
>+            {
>+                nsCAutoString keyvalue("\"");
>+                keyvalue.Append(fileName);
>+                keyvalue.Append('\"');
>+                keyvalue.Append(args);
>+                keyvalue.Append(' ');
>+                keyvalue.Append(option);

If you use ::GetShortPathName (or thisApplication()), then you don't need the
quotes around the executable path (which may simply things).  I really think you
should use thisApplication() to be consistent with the rest of this module.

>+            ::RegOpenKeyEx( HKEY_CURRENT_USER, RUNKEY, 0, KEY_SET_VALUE, &key );
>+            ::RegDeleteValue( key, NS_QUICKLAUNCH_RUN_KEY );
>+            ::RegCloseKey( key );

I believe RegistryEntry::set() will delete the entry if its setting==0.

>+            nsCAutoString ufileName;

I kind of like "foo" better (at least it has some personality).  "newSetting" or
somesuch would have told me more about what you're doing with this variable.

Comment 18

[Joseph Elwell]

Alec: I accidently forgot to remove the 3rd and 4th parameters in the Find. I
have done so. Can you please produce a comment that you feel would justify the
use of an NS_CONST_CAST?

Bill: In StartupAddOption I would rather not switch to using the RegistryEntry
class. In complicates string usage even more. But more importantly there's no
way to find out if a key exists or not using the class. If currentSetting
returned a null pointer if this statement failed, that might be more useful:
http://lxr.mozilla.org/seamonkey/source/xpfe/components/winhooks/nsWindowsHooksUtil.cpp#529

Comment 19

[Joseph Elwell]

Attachment: I still needed the quotes. thisApplication does not quote it's string.

Comment 20

[Alec Flett]

Comment on attachment 72680 [details] [diff] [review]
I still needed the quotes. thisApplication does not quote it's string.

ok, this one time I'll write your comments, but I suggest you read the
"comments" chapter of "Code Complete" - you obviously are keen on comments,
which is good :) Just remember the point is not to restate what you're doing,
but to explain why you are doing it. (i.e. which includes why it is
safe/unsafe)

// exploiting the fact that nsString's storage is also a char* buffer.
// NS_CONST_CAST is safe here because nsCRT::strtok will only modify the
existing buffer

make sure the comment is indented correctly.

one other thing:
+    nsCString fileName = thisApplication();

stack-based strings should always be nsCAutoString/nsAutoString unless you're
pretty sure the string is longer than AutoString's internal buffer (in which
case you should comment as to why you're not using it)

sr=alecf with the above

Comment 21

[Blake Ross]

Do we need this for machv? I would rather not be touching turbo code for an
architecture issue with no immediate need.  If we do need this, or if you just
want to get it in, please test very well.  We can't afford to break turbo now.

Comment 22

[Joseph Elwell]

alec: I was hoping you'd write the comment, because the code is based on your
suggestions for that line. I'm not aware of the exact ramifications of dropping
the const, other than that it's not a good idea.

I used nsCString because that's what the function returns. Bill, do you have
comments on that?

Thanks for the super review.

Comment 23

[Joseph Elwell]

blake: yes. i'm testing thoroughly.

Comment 24

[Alec Flett]

ack! if you don't understand the ramifications of what you're doing, then you
shouldn't be doing it :)

- go learn about nsCRT::strtok() - note that it modifies the buffer that it
passes in, an inserts "null" characters, whacking the string that you passed in
- go look at the string classes - note that nsCAutoString derives from nsCString
- just because a function returns doesn't mean it's the proper class

the sad thing is, nobody should be returning nsCString objects from their
function. some day someone should go fix that function...this function will
create all sorts of ugly temporary objects, resulting in extra allocations, etc.

Comment 25

[Joseph Elwell]

Usually if something is const it's because you don't expect the value to change,
and if it does change you could end up with problems. What I don't know is why
the string was const in the first place, obviously it wasn't to prevent people
from altering individual characters in the string. Perchance it's because of
some memory allocation issue?

As for the nsCString, nsCAutoString issue, I will make that change. I'm grateful
for your String guide, but I was going to ask why nsCString and nsCAutoString
were left out.

Comment 26

[Joseph Elwell]

Note to self: I need to catch old registry keys that aren't quoted and toss 
them and replace wipth new key format.

Comment 27

[Bill Law]

>Bill: In StartupAddOption I would rather not switch to using the RegistryEntry
>class. In complicates string usage even more. But more importantly there's no
>way to find out if a key exists or not using the class. If currentSetting
>returned a null pointer if this statement failed, that might be more useful:

Well, maybe it does complicate things, but so would writing the code so it
worked.

I'm not sure you understood what I've said twice already: the registry value can
be considerably longer than _MAX_PATH bytes.  You can't rely on reading it into
a buffer that's only _MAX_PATH bytes long.

>I still needed the quotes. thisApplication does not quote it's string.

That's because it doesn't need to.  That's the whole point of ::GetShortPathName.

Comment 28

[Joseph Elwell]

Bill: What length do you think would be sufficient?

I need the quotes because that's how I parse out the arguments.

"Well, maybe it does complicate things, but so would writing the code so it
worked."

Are you talking about rewriting the RegistryEntry code being a lot of work? I
still don't think it would even work to use the RegistryEntry class in
StartUpAddOption because there's no way for me to use that class and know
whether or not the key even exists. I want to be able to check if the key
exists, not just if it's set to something.

Comment 29

[Bill Law]

RegistryEntry uses a 4K buffer.

I don't think you care whether the registry key exists or not.  Don't we get the
same end result if the key is missing as if it's blank?

Comment 30

[Joseph Elwell]

I will try and see if that works.

Comment 31

[Joseph Elwell]

Attachment: almost done. don't review this one yet. it works but i want to clean up strtok now after lunch

Comment 32

[Joseph Elwell]

Attachment: finished.

Comment 33

[Joseph Elwell]

Attachment: diff for installer to quote the executable string

Comment 34

[Bill Law]

You can simplify this code (to remove the registry entyr when the last option is
removed), by re-using code already in RegistryEntry::set:

+        HKEY key;
+        ::RegOpenKeyEx( HKEY_CURRENT_USER, RUNKEY, 0, KEY_SET_VALUE, &key );
+        ::RegDeleteValue( key, NS_QUICKLAUNCH_RUN_KEY );
+        ::RegCloseKey( key );

by simply replacing it with:

+        startup.set();

Also, one question: How will this deal with existing registry settings out there
that are missing the enclosing quotes around the executable name?  It seems like
we'll have to deal with that and it seems like adding "-aim" will lose "-turbo"
if the entry is there without the quotes?  That would be a bug and I think the
code needs to deal with that.

I don't think fixing the installer to add quotes is sufficient because that
won't fix the registry settings out there.

Once that issue is dealt with, then consider this r=law.

Comment 35

[Joseph Elwell]

I've thought alot about how to deal with registry settings that are out there
right now. I've tested this patch with values that aren't quoted and indeed if
the user adds -mail then the original -turbo is deleted. I can add a hack so
whenever we adjust the key value we first call  IsOptionEnabled("-turbo") and
hard code adding that one key back in. Note IsOptionEnabled doesn't bother
parsing out the args.

Is that reasonable?

Comment 36

[Sean Su]

Comment on attachment 73538 [details] [diff] [review]
diff for installer to quote the executable string

r=ssu for this patch.

You need to also create a similar patch to a similar file under the ns tree
(same sub path).

Comment 37

[Joseph Elwell]

Attachment: ding

Comment 38

[Bill Law]

Comment on attachment 73589 [details] [diff] [review]
ding

r=law

Comment 39

[Alec Flett]

Comment on attachment 73589 [details] [diff] [review]
ding

everything looks good, 'cept the comments around isOptionEnabled still say
"indicating that the app will preload when Windows starts" - I think it should
probably say "indicating that the app will start with the given option"

I notice that you still didn't change "nsCString cargs" to "nsCAutoString
cargs" and that you're casting to get from thisApplication() to nsCAutoString?
Is that cast really necessary? If the cast is not necessary then don't put it
in there. In fact, you should be able to say
nsCAutoString fileName(thisApplication());

I don't quite understand this:
+	     // check for the old style registry key that doesnot quote its
executable
+	     IsOptionEnabled("-turbo", &retval);
+	     if (retval)
+		 newsetting.Append(" -turbo");
+

since you just called IsOptionEnabled above.
I'm not sure how the comment relates, since I don't see you changing any state
that would make IsOptionEnabled() return anything different.

And actually, I'm not sure why you need some of these temporaries. for
instance:
+	 nsCAutoString fileName = (nsCAutoString) thisApplication();
+	 nsCAutoString ufileName;
+	 ufileName.Assign('\"');
+	 ufileName.Append(fileName);
+	 ufileName.Append('\"');

you could just say
+	 nsCAutoString ufileName;
+	 ufileName.Assign('\"');
+	 ufileName.Append(thisApplication());
+	 ufileName.Append('\"');

oh, and you don't have to change this, but in the future, if you're going to
have an entire function body inside an if (), you should just return early.. in
StartupAddOption() you could have just said

if (retval) return NS_OK;

and saved yourself lots of indenting.

Comment 40

[Joseph Elwell]

alecf: that comment line has a - in fron of it.
This comment http://bugzilla.mozilla.org/show_bug.cgi?id=128377#c35 explains the
new IsOptionEnabled check. I'm reusing the retval now. But the check only
happened if args == NULL, which meant we couldn't find any old arguments, even
though they were there before. (old style runkey)

If I don't cast, I get an error. I'm getting rid of some of the temporary
strings. I guess going through 6 revisions causes some artifacts.

Comment 41

[Joseph Elwell]

Attachment: tko

Comment 42

[Alec Flett]

Comment on attachment 73976 [details] [diff] [review]
tko

you have yet to explain the (nsCAutoString) casts :)
 and now they seem completely unnecessary... 

in some cases the casts are actually going to CREATE a temporary, resulting in
an additional string copy - that's why I keep bitching about it.

for example:
+	 ufileName.Append( (nsCAutoString) thisApplication());

this will likely create a temporary. Why do it? Append even takes an nsCString!

Comment 43

[Joseph Elwell]

Append works fine. I pulled out the cast for both appends. but this line:
nsCAutoString cargs = (nsCAutoString) startup.currentSetting();
will not compile if I don't leave the cast in.

I get cannot convert from 'class nsCString' to 'class nsCAutoString' No
constructor could take the source type, or constructor overload resolution was
ambiguous.

Comment 44

[Alec Flett]

ok, that's fine. This is why we shouldn't have functions that return concrete
classes.. *sigh*

anyway, just make "cargs" an nsCString and we'll be set.

Comment 45

[Peter Trudelle]

At this point in the release cycle, shouldn't this at least be nominated for a
release (or block an approved bug) if several engineers are going to spend
significant time on it?  Why is this needed?  

Comment 46

[Joseph Elwell]

trudelle: it blocks a bugscape bug 11992. i've nominated this bug in case that
helps.

Comment 47

[Joseph Elwell]

Attachment: final copy?

Comment 48

[Alec Flett]

Comment on attachment 74003 [details] [diff] [review]
final copy?

yay. well this certainly has been a learning experience for all involved (yes,
myself inclued) :)

sr=alecf as long as law is the reviewer

Comment 49

[Bill Law]

+NS_IMETHODIMP nsWindowsHooks::StartupAddOption(const char* option) {
+    NS_ASSERTION(option, "nsWindowsHooks::StartupAddOption requires something
like \"-turbo\"");
+    PRBool retval;
+    IsOptionEnabled(option, &retval);
+    if (retval) return NS_OK; //already in there
+    
+    RegistryEntry startup ( HKEY_CURRENT_USER, RUNKEY, NS_QUICKLAUNCH_RUN_KEY,
NULL );
+    nsCString cargs = startup.currentSetting();
+    nsCAutoString newsetting;
+    newsetting.Assign('\"');
+    newsetting.Append(thisApplication());
+    newsetting.Append('\"');
+    if (!cargs.IsEmpty())
+    {
+        char* args;
+        // exploiting the fact that nsString's storage is also a char* buffer.
+        // NS_CONST_CAST is safe here because nsCRT::strtok will only modify
+        // the existing buffer
+        grabArgs(NS_CONST_CAST(char *, cargs.get()), &args);
+        if (args != NULL)
+            newsetting.Append(args);
+        else
+        {
+            // check for the old style registry key that doesnot quote its
executable
+            if (retval)
+                newsetting.Append(" -turbo");
+        }


You seem to have dropped the requisite line:
+            IsOptionEnabled("-turbo", &retval);

somewhere between your "ding" and "tko" patches.  That Append call will never
execute, as it stands.  Or am I missing something?

Comment 50

[Joseph Elwell]

For some reason when I read alec's comment where he said I was calling
IsOptionEnabled twice I thought it was ok to remove the second call. Adding it back.

Comment 51

[Joseph Elwell]

Attachment: so this check is to make sure we don't remove the old -turbo for backwards compatibility

Comment 52

[Joseph Elwell]

I got confused by this:
"since you just called IsOptionEnabled above.
I'm not sure how the comment relates, since I don't see you changing any state
that would make IsOptionEnabled() return anything different."

The real answer is the second call is hardcoded to check for "-turbo" not the 
variable option. And it's to check for the old registry key value that doesn't 
quote it's executable.

Comment 53

[Bill Law]

Comment on attachment 74426 [details] [diff] [review]
so this check is to make sure we don't remove the old -turbo for backwards compatibility

r=law

re: comment 43

nsCAutoString cargs( startup.currentSettings() );

would work (I believe).  FWIW

Comment 54

[Syd Logan]

Comment on attachment 73538 [details] [diff] [review]
diff for installer to quote the executable string

sr=syd for adding quotes around the executable to make it easier to parse the
key.

Comment 55

[Alec Flett]

Comment on attachment 74426 [details] [diff] [review]
so this check is to make sure we don't remove the old -turbo for backwards compatibility

ok.. I still don't understand what the 2nd IsOptionEnabled does..because you're
checking for it above, and there's no change in state that indicates that
"retval" is going to have a different value than the first time youc hecked it,
above.

but whatever, if it works it works :)
sr=alecf
yay!

Comment 56

[Joseph Elwell]

alecf: thanks for the sr=. Just so you're clear here's the code in question:

IsOptionEnabled(option, &retval);
...
// check for the old style registry key that doesnot quote its executable
IsOptionEnabled("-turbo", &retval);
if (retval)
	newsetting.Append(" -turbo");

Notice the first IsOptionEnabled passes in option, which might be "-mail". The
second call hard codes a check for "-turbo" and adds "-turbo" onto the argument
string if it's found. This second call only gets called if after trying to parse
out the argument string from the registry we have an empty string. This happens
if the registry key is the old style where the executable isn't quoted.

Comment 57

[Alec Flett]

ah! now I get it. :)

Comment 58

[scalkins]

Tested on special build Joe gave me ...
Results:

1. Exisiting Turbo mode pref is remembered at reboot 
2. Re-starting Netcape with turbo mode and our new pref is OK
3. Turning off Turbo mode at will and exit/restarting or exit/rebooting works fine.

Comment 59

[Asa Dotzler [:asa]]

Comment on attachment 74426 [details] [diff] [review]
so this check is to make sure we don't remove the old -turbo for backwards compatibility

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 60

[Blake Ross]

Has this been checked in?

Comment 61

[Joseph Elwell]

yes. I was out sick the day after. marking fixed. ;)

Comment 62

[Grace Bush]

verified