58523 - -P, -nosplash, -splash, -console, -ProfileManager, -installer switches (arguments) unable to launch anything other than the browser (mail does not open on startup)

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect, P3)

Description

[sairuh (rarely reading bugmail)]

not sure if this is expected behavior. my gut reaction is that this doesn't act
intuitively correctly. anyhow, the details. this issue occurred on winNT and
linux running 2000.10.30.09-n6 [commercial branch]; n/a to Mac since this
involves a prompt-only option.

1. went to the Preferences dialog, selected the Appearance category.
2. selected all of the products to launch on startup: mail, browser and editor.
3. click OK to save and dismiss prefs.
4. exit the app.
5. restart the app at the commandline with the following:
		./netscape -P [profile]
insert your own profile name above [o'course]...

expected: upon launch, mail, editor and browser windows should appear.

result: only the browser window appears.

note: if i use only ./netscape [or, on winNT, doubleclick the Netscape 6 desktop
icon or the icon in an explorer window], this does work --even tho' i need to
select my profile first.

Asa/Blake/Timeless, is this a problem in the trunk?

Comment 1

[sairuh (rarely reading bugmail)]

pessimistic about this getting fixed any time soon, so adding relnoteRTM kw.

Comment 2

[sairuh (rarely reading bugmail)]

i tried a more simple test: just selecting mail to launch. still, only the
browser is launched when restarting. clarifying summary.

Comment 3

[sairuh (rarely reading bugmail)]

relnote info: [as the summary sez ;-] you'll only be able to open a browser
window if you use the -P switch --even if you have your profile setup to launch
with mail and/or the editor.

Comment 4

[sairuh (rarely reading bugmail)]

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

Comment 5

[sairuh (rarely reading bugmail)]

i dupped bug 60045, thinking it might be caused by the same problem here [since
the end result is the same]. could anyone confirm this [or explain otherwise]?
thx!

Comment 6

[Viswanath Ramachandran]

Since Don has left, Vishy is taking his bugs in bulk, pending reassignment.
thanks,
	Vishy

Comment 7

[sairuh (rarely reading bugmail)]

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

Comment 8

[[e-mail]]

As I mentioned in bug 62221 I think the problem is that
http://lxr.mozilla.org/seamonkey/source/xpfe/bootstrap/nsAppRunner.cpp#999
only checks for an empty command line.
It should launch general preference windows if no other windows were created.
It should only launch the default browser window if there are still no windows.

Comment 9

[Marcia Knous [:marcia]]

Netscape Nav triage team: this is not a Netscape beta stopper.

Comment 10

[sairuh (rarely reading bugmail)]

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

Comment 11

[Paul Chen]

Marking nsbeta1- bugs as future to get off the radar.

Comment 12

[Tuukka Tolvanen (sp3000)]

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

Comment 13

[R.K.Aa.]

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

Comment 14

[sairuh (rarely reading bugmail)]

->bill to triage

Comment 15

[Bill Law]

First, I've changed the target milestone so this bug really gets triaged.

Secondly, Neil is write about the cause, code-wise.  The line has changed now,
though, it is at
http://lxr.mozilla.org/seamonkey/source/xpfe/bootstrap/nsAppRunner.cpp#1235.

I've been hacking on that code a bit (see bug 88123).

I think it would be relatively easy to modify DoCommandLines to eliminate the
"heedStartupPrefs" argument.  Instead, it would process command line options,
then, as Neil suggests, process the startup prefs, then lastly, open a browser
window.

The only difference in the outcome would be that "mozilla -foobar" (including
the case where "foobar" is -P "profile") would open nav/mailnews/composer
instead of just a browser.

There may be some corner cases.  Thoughts, anybody?

Comment 16

[Bill Law]

Setting target milestone to Future.  Not high enough priority.

Comment 17

[Matthias Versen [:Matti]]

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

Comment 18

[Aleksey Nogin]

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

Comment 19

[Tuukka Tolvanen (sp3000)]

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

Comment 20

[timeless]

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

Comment 21

[R.K.Aa.]

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

Comment 22

[R.K.Aa.]

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

Comment 23

[R.K.Aa.]

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

Comment 24

[Simon Montagu :smontagu]

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

Comment 25

[Torben]

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

Comment 26

[Michael Lefevre]

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

Comment 27

[R.K.Aa.]

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

Comment 28

[timeless]

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

Comment 29

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 30

[Andrew Schultz]

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

Comment 31

[Simford]

Attachment: patch

preliminary patch for discussing

Comment 32

[Simford]

IMHO, MAC box may use the "heedStartupPrefs" argument, while *NIX and Win box not.
so just ignore that argument when not using MAC box.

Comment 33

[Simford]

Hi Bill Law,
Could you please take a look at this patch?
The fix of this bug is very important for *nix users.
Thanks!

Comment 34

[Stefan Borggraefe]

Comment on attachment 131164 [details] [diff] [review]
patch

This patch doesn't work, when you select to start with MailNews in the
Preferences and additionally use the -mail command-line option for example. You
end up with *two* MailNews-Windows in this situation.

I think the command line options should only add to the Preferences options if
the same component isn't selected in the Preferences already. So this patch is
a little bit to simple.

Comment 35

[R.K.Aa.]

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

Comment 36

[R.K.Aa.]

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

Comment 37

[R.K.Aa.]

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

Comment 38

[Boying Lu]

Attachment: another proposal

Comment 39

[Boying Lu]

I filed a similiar bug https://bugzilla.mozilla.org/show_bug.cgi?id=265065 on
bugzilla

Comment 40

[Boying Lu]

Comment on attachment 162657 [details] [diff] [review]
another proposal

Can you give r? Thanks

Comment 41

[sairuh (rarely reading bugmail)]

Boying, I don't think Bill Law would be looking at this bug anymore.

Comment 42

[Boying Lu]

Comment on attachment 162657 [details] [diff] [review]
another proposal

Can you give r? Thanks

Comment 43

[Boying Lu]

Comment on attachment 162657 [details] [diff] [review]
another proposal

Can you give sr? Thanks

Comment 44

[[e-mail]]

Comment on attachment 162657 [details] [diff] [review]
another proposal

Um... no, sorry :-( Some of these arguments are provided by optional
components, so you can't hard code a list.
Looking at the code, I think it may need reorganizing slightly. The way it
seems to work now, is this:
1. If there are command line options, apply them, otherwise apply the startup
preferences.
2. If there are no windows open, then open a browser window.
What could resolve the problem is as follows:
1. Apply any command line options.
2. If there are no windows open, apply the startup preferences.
3. If there are still no windows open, then open a browser window.

Comment 45

[Boying Lu]

Attachment: apply command line options first, if no window is opened, then apply the starup preferences

Comment 46

[timeless]

Comment on attachment 168935 [details] [diff] [review]
apply command line options first, if no window is opened, then apply the starup preferences

in no particular order:
* law is gone
* you're removing lots of code, where is it going?
* use -up100 or something so there's enough context for your poor reviewers

Comment 47

[Boying Lu]

Attachment: use -up50 to re-generate the patch

Basically, the patch modifies two location:
1. Change the protot type of the function DoCommandLines from:
nsresult DoCommandLines(nsICmdLineService* cmdLineArgs, 
	PRBool heedGeneralStartupPrefs, PRBool *windowOpened)
to
nsresult DoCommandLines(nsICmdLineService* cmdLineArgs, PRBool *windowOpened)
i.e. heedGeneralStartupPrefs is not needed any more.  

So the codes to set defaultStartup are removed.

The reason is that the original logical is unreasonable.
heedGeneralStartupPrefs is set to true only when there is no command line
options. Base the this logic, startup preferences take effect ONLY when there
is no command line options.

2. Reorginized the codes in DoCommandLines()
Now the current logic is (as Neil said):
  a. apply command line options first.
  b. if no window is opened, then apply the startup perferences

Comment 48

[Boying Lu]

BTW: Since Law is gone, who is the response engineer for this bug?

Comment 49

[timeless]

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

Comment 50

[timeless]

Comment on attachment 168944 [details] [diff] [review]
use -up50 to re-generate the patch

please explain why you're removing this code without putting similar code
/somewhere/:
>-  // This will go away once Components are handling there own commandlines
>-  // if we have no command line arguments, we need to heed the
>-  // "general.startup.*" prefs
>-  // if we had no command line arguments, argc == 1.
>-
>   PRBool windowOpened = PR_FALSE;
>-  PRBool defaultStartup;
>-#if defined(XP_MAC) || defined(XP_MACOSX)
>-  // On Mac, nsCommandLineServiceMac may have added synthetic
>-  // args. Check this adjusted value instead of the raw value.
>-  PRInt32 processedArgc;
>-  cmdLineArgs->GetArgc(&processedArgc);
>-  defaultStartup = (processedArgc == 1);
>-#if defined(XP_MACOSX)
>-  // On OSX, we get passed two args if double-clicked from the Finder.
>-  // The second is our PSN. Check for this and consider it to be default.
>-  if (argc == 2 && processedArgc == 2) {
>-    ProcessSerialNumber ourPSN;
>-    if (::MacGetCurrentProcess(&ourPSN) == noErr) {
>-      char argBuf[64];
>-      sprintf(argBuf, "-psn_%ld_%ld", ourPSN.highLongOfPSN, ourPSN.lowLongOfPSN);
>-      if (!strcmp(argBuf, argv[1]))
>-        defaultStartup = PR_TRUE;
>-    }
>-  }
>-#endif /* XP_MACOSX */
>-#else
>-  defaultStartup = (argc == 1);
>-#endif

as to your question about engineers, there probably isn't one. neil/biesi/i and
maybe alecf probably are the closest things to owners.

Comment 51

[Boying Lu]

OK, I removed these codes because these codes just do one thing: set
defaultStartup, which is used in DoCommandLines(...) passed by the second
parameter heedGeneralStartupPrefs.  This parameter is used to indicate if using
"startup preferences" to launch mozilla or not.

In the original implementation, "startup" perferences (general.startup.*) are
used only when there is no command line options. I.e. only when mozilla-bin is
launched with no options.  I think this incorrect. For example if we launch
mozilla with following command:  mozilla -P foo, Mozilla should be launched
according to the  settings in  "general.startup.*",which is set in profile foo.
But actually, just browser window is opened. "general.startup.*" are not taken
effect at all.

In my patch, defaultStartup is not needed any more. I.e. the second parameter of
DoCommandLines heedGeneralStartupPrefs is not needed any more.  So I removed the
codes of setting the variable defaultStartup.

Comment 52

[Boying Lu]

Comment on attachment 168944 [details] [diff] [review]
use -up50 to re-generate the patch

Can you give r? Thanks

Comment 53

[[e-mail]]

Comment on attachment 168944 [details] [diff] [review]
use -up50 to re-generate the patch

Although this patch works fine on Linux it won't even compile on Windows or
OS/2 because you've only patched two of the five references to DoCommandLines.

Comment 54

[[e-mail]]

Or if it does compile, it won't link.

Comment 55

[Boying Lu]

Attachment: add support for windows and OS2

I have tested the patch on solaris,linux and windows XP.

Comment 56

[[e-mail]]

Comment on attachment 172248 [details] [diff] [review]
add support for windows and OS2

Nits:

>+    //first apply command line options 
This and the following block will need to be outdented.
[You could then use diff -w to make the patch easier to review.]
You also have an unnecessary trailing space.

>+      if (NS_FAILED(rv)) return rv;
return rv; should be on its own line.

>+        rv = appStartup->CreateStartupState(width, height, windowOpened);
Indentation is (more) wrong.

>+      if (NS_FAILED(rv)) return rv;
return rv; on its own line again please.

>   NS_TIMELINE_LEAVE("appStartup->CreateHiddenWindow");
> 
>-  // This will go away once Components are handling there own commandlines
>-  // if we have no command line arguments, we need to heed the
>-  // "general.startup.*" prefs
>-  // if we had no command line arguments, argc == 1.
> 
>   PRBool windowOpened = PR_FALSE;
You've left a double blank line here, please delete one of them.

Comment 57

[[e-mail]]

Comment on attachment 172248 [details] [diff] [review]
add support for windows and OS2

Nits:

>+    //first apply command line options 
This and the following block will need to be outdented.
[You could then use diff -w to make the patch easier to review.]
You also have an unnecessary trailing space.

>+      if (NS_FAILED(rv)) return rv;
return rv; should be on its own line.

>+        rv = appStartup->CreateStartupState(width, height, windowOpened);
Indentation is (more) wrong.

>+      if (NS_FAILED(rv)) return rv;
return rv; on its own line again please.

>   NS_TIMELINE_LEAVE("appStartup->CreateHiddenWindow");
> 
>-  // This will go away once Components are handling there own commandlines
>-  // if we have no command line arguments, we need to heed the
>-  // "general.startup.*" prefs
>-  // if we had no command line arguments, argc == 1.
> 
>   PRBool windowOpened = PR_FALSE;
You've left a double blank line here, please delete one of them.

>Index: toolkit/xre/nsAppRunner.h
My review does not extend to toolkit.
Please ask someone relevant whether they have the same bug.

Comment 58

[Boying Lu]

Attachment: a revision

I re-made the patch according to Neil's comment. Thanks Neil. Can you give r?
Thanks

Comment 59

[Boying Lu]

Comment on attachment 172319 [details] [diff] [review]
a revision

Could you reivew toolkit parts? Thanks

Comment 60

[Brian Ryner (not reading)]

Comment on attachment 172319 [details] [diff] [review]
a revision

Did you forget to include the diff for toolkit/xre/nsAppRunner.cpp?

Comment 61

[Boying Lu]

I didn't modify toolkit/xre/nsAppRunner.cpp. I modified the prototype and 
implentation of DoCommandLines(), which is implemented in 
xpfe/bootstrap/nsAppRunner.cpp. I just change the declaration of DoCommandLines
() in toolkit/xre/*.h files.

Comment 62

[Boying Lu]

Comment on attachment 172319 [details] [diff] [review]
a revision

Neil has give r except "toolkit" part

Comment 63

[Boying Lu]

Comment on attachment 172319 [details] [diff] [review]
a revision

Can you give r? Thanks

Comment 64

[[e-mail]]

Comment on attachment 172319 [details] [diff] [review]
a revision

r=me for xpfe changes.

Comment 65

[Boying Lu]

Comment on attachment 172319 [details] [diff] [review]
a revision

Can you reivew the "toolkit" part? Thanks

Comment 66

[Asaf Romano (gone)]

Comment on attachment 172319 [details] [diff] [review]
a revision

hyatt is gone, if your request is about the toolkit bits only, ask a "new
toolkit" peer for r=

Comment 67

[Boying Lu]

Comment on attachment 172319 [details] [diff] [review]
a revision

Can you review "toolkit" part?

Comment 68

[Boying Lu]

Comment on attachment 172319 [details] [diff] [review]
a revision

Can you give r?

Comment 69

[Brian Ryner (not reading)]

Comment on attachment 172319 [details] [diff] [review]
a revision

Oh, I see, this function doesn't even exist in the toolkit code.  Plesae just
remove the declaration in toolkit/xre/nsAppRunner.h and r=me.

Comment 70

[Boying Lu]

Thanks Ryner for your reviewing. But I don't think the declaration can be 
removed from toolkit/xre/nsAppRunner.h because the DoCommandLine() is called in
nsNativeAppSupportWin.cpp, which needs a prototype declaration.

Comment 71

[Boying Lu]

Comment on attachment 172319 [details] [diff] [review]
a revision

sr? thanks

Comment 72

[Boying Lu]

Attachment: a new patch OS2 relate codes are removed

Comment 73

[[e-mail]]

Well I agree with bryner, so I just removed the obsolete declaration.

Comment 74

[[e-mail]]

Reopening because bare URLs on the command line were getting ignored.

Comment 75

[Boying Lu]

Attachment: a revised patch

Fixed the issue found by neil.

Comment 76

[[e-mail]]

Comment on attachment 176697 [details] [diff] [review]
a revised patch

OK, so I spent all morning trying to figure out how the browser launches URLs.

As you know, with the old code, having any command line arguments causes the 
startup preferences to be ignored. Instead we call Ensure1Window which calls
OpenBrowserWindow which calls GetURLToLoad which scans the command line.

Now in the new scheme of things the url is ignored as it doesn't have a handler
(yes, -url is magic), so we launch the general startup prefs.

So, I think that the correct fix is to put in an extra call to GetURLToLoad and
if it finds a URL then open a browser window and don't process general startup
preferences.

Comment 77

[R.K.Aa.]

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

Comment 78

[Boying Lu]

Attachment: another one

The following logic is used in DoCommandLines():

If command line options cause any window opened then 
   all startup perferences are ignored
else if (an url is in command line) && (browser window is not opened)
   open a browser window with the url
   ignore startup perferences
else 
   startup perferences will be used

Comment 79

[[e-mail]]

Sorry for not reviewing this patch, but my bugmail inexplicably stopped working.
I'll have to have a look at it tomorrow, but in the mean time I'm wondering why
you still have all those explicit checks for opening browser windows.

Comment 80

[Boying Lu]

Because I think when user launch mozilla with " mozilla -browser www.google.com"
only a browser will be opened with www.google.com. This is handled by the codes:

+  PRBool startBrowser = !strcmp(aParam,"browser");
   nsXPIDLCString cmdResult;
-  rv = cmdLineArgs->GetCmdLineValue(commandLineArg, getter_Copies(cmdResult));
+
+  if (startBrowser)
+    rv = cmdLineArgs->GetURLToLoad(getter_Copies(cmdResult));
+
+  if (cmdResult.IsEmpty())
+    rv = cmdLineArgs->GetCmdLineValue(commandLineArg, getter_Copies(cmdResult));
+
+
   if (NS_FAILED(rv)) return rv;
 #ifdef DEBUG_CMD_LINE
   printf("%s, cmdResult = %s\n",commandLineArg,cmdResult.get());
@@ -658,7 +662,7 @@ LaunchApplicationWithArgs(const char *co
         rv = handler->GetOpenWindowWithArgs(&openWindowWithArgs);
         if (NS_FAILED(rv)) return rv;
 
-        if (openWindowWithArgs) {
+        if (openWindowWithArgs || startBrowser) {

When user launch mozilla with "mozilla www.google.com", a browser window will be
opened and the corresponding codes are:

+  // second if browser window is not opened then check if there is an url 
+  // in the command line
+  if (!browserWindowIsOpened) {
+    nsXPIDLCString urlToLoad;
+    rv = cmdLineArgs->GetURLToLoad(getter_Copies(urlToLoad));
+    if (NS_SUCCEEDED(rv) && !urlToLoad.IsEmpty()) {
+      nsAutoString url;
+      nsXPIDLCString chromeUrlForTask;
+      nsCOMPtr<nsICmdLineHandler> handler(
+         
do_GetService("@mozilla.org/commandlinehandler/general-startup;1?type=browser",
&rv));
+
+      // convert the cmdLine URL to Unicode
+      rv = NS_CopyNativeToUnicode(nsDependentCString(urlToLoad), url);
+      if (NS_SUCCEEDED(rv)) {
+        rv = handler->GetChromeUrlForTask(getter_Copies(chromeUrlForTask));
+        if (NS_SUCCEEDED(rv))
+          rv = OpenWindow(chromeUrlForTask,url,width,height);
+        if (NS_SUCCEEDED(rv))
+          *windowOpened = PR_TRUE;
       }
     }
   }

Comment 81

[[e-mail]]

Sorry for the delay, I forgot to CC myself on the bug.
In the "mozilla -browser www.google.com" case, the www.google.com is eaten by
the -browser, so GetCmdLineValue will return www.google.com while GetURLToLoad
will be blank
In the "mozilla www.google.com" case, GetCmdLineValue is not used, while
GetURLToLoad will return www.google.com

(In reply to comment #80)
> Because I think when user launch mozilla with " mozilla -browser www.google.com"
> only a browser will be opened with www.google.com. This is handled by the codes:
> 
> +  PRBool startBrowser = !strcmp(aParam,"browser");
>    nsXPIDLCString cmdResult;
> -  rv = cmdLineArgs->GetCmdLineValue(commandLineArg, getter_Copies(cmdResult));
I think I snipped too much here, never mind, but I think only GetCmdLineValue
can give the right answer at this point.

> When user launch mozilla with "mozilla www.google.com", a browser window will be
> opened and the corresponding codes are:
> 
> +  // second if browser window is not opened then check if there is an url 
> +  // in the command line
> +  if (!browserWindowIsOpened) {
> +    nsXPIDLCString urlToLoad;
> +    rv = cmdLineArgs->GetURLToLoad(getter_Copies(urlToLoad));
Here I'd say if GetURLToLoad manages to return something then we can assume that
a browser window wasn't opened otherwise it would have stolen the URL earlier.

Comment 82

[Boying Lu]

(In reply to comment #81)
> Sorry for the delay, I forgot to CC myself on the bug.
> In the "mozilla -browser www.google.com" case, the www.google.com is eaten by
> the -browser, so GetCmdLineValue will return www.google.com while GetURLToLoad
> will be blank
> In the "mozilla www.google.com" case, GetCmdLineValue is not used, while
> GetURLToLoad will return www.google.com
> 
> (In reply to comment #80)
> > Because I think when user launch mozilla with " mozilla -browser www.google.com"
> > only a browser will be opened with www.google.com. This is handled by the codes:
> > 
> > +  PRBool startBrowser = !strcmp(aParam,"browser");
> >    nsXPIDLCString cmdResult;
> > -  rv = cmdLineArgs->GetCmdLineValue(commandLineArg, getter_Copies(cmdResult));
> I think I snipped too much here, never mind, but I think only GetCmdLineValue
> can give the right answer at this point.
> 

If we only call GetCmdLineValue(), it will return null when "mozilla -browser
-mail mailToURL www.google.com". What I want is a browser window will be opened
with "www.google.com" in this case.

> > When user launch mozilla with "mozilla www.google.com", a browser window will be
> > opened and the corresponding codes are:
> > 
> > +  // second if browser window is not opened then check if there is an url 
> > +  // in the command line
> > +  if (!browserWindowIsOpened) {
> > +    nsXPIDLCString urlToLoad;
> > +    rv = cmdLineArgs->GetURLToLoad(getter_Copies(urlToLoad));
> Here I'd say if GetURLToLoad manages to return something then we can assume that
> a browser window wasn't opened otherwise it would have stolen the URL earlier.

When user starts mozilla with "mozilla -browser -mail mailtoURL www.google.com",
GetURLToLoad() still returns "www.google.com" even though a browser window is
opened.

Comment 83

[[e-mail]]

Comment on attachment 177592 [details] [diff] [review]
another one

I don't think we should handle out-of-order arguments.

Comment 84

[Boying Lu]

But what will happen when user launches mozilla like this:
mozilla -browser -mail mailtoURL www.google.com

Comment 85

[[e-mail]]

(In reply to comment #84)
>But what will happen when user launches mozilla like this:
>mozilla -browser -mail mailtoURL www.google.com
What I expect to see is:
1) a browser window open to the default page
2) a mail window (although I think you meant to use -compose!)
3) a browser window open to www.google.com
Ideally for 3) we should support the "open new windows in tabs" preference but
that is way beyond the scope of this bug.

Comment 86

[Boying Lu]

Attachment: modify the codes according to neil's comments

Comment 87

[[e-mail]]

Comment on attachment 178547 [details] [diff] [review]
modify the codes according to neil's comments

>+  PRBool startBrowser = !strcmp(aParam,"browser");
>-        if (openWindowWithArgs) {
>+        if (openWindowWithArgs || startBrowser) {
I think you left startBrowser in by mistake. r=me if you remove it.

Comment 88

[Boying Lu]

(In reply to comment #87)
> (From update of attachment 178547 [details] [diff] [review] [edit])
> >+  PRBool startBrowser = !strcmp(aParam,"browser");
> >-        if (openWindowWithArgs) {
> >+        if (openWindowWithArgs || startBrowser) {
> I think you left startBrowser in by mistake. r=me if you remove it.
> 
I think startBrowser can't be removed. Because when user launch mozilla with
"mozilla -browser www.google.com", a browser window should be opened with
www.google.com. But the corresponding nsICmdHandler (handler) is
nsBrowserContentHandler. handler->GetOpenWindowWithArgs(&openWindowWithArgs)
will set openWindowWithArgs to FALSE. So even "www.google.com" will be saved as
"-browser"'s value, it will not be used when opening a browser window.
This snippet is used to handle this case.

Comment 89

[[e-mail]]

(In reply to comment #88)
Then that's a bug in our implementation of -browser (in fact, -browser is
implemented as a synonym of -chrome) and it should be fixed there, not here.

Comment 90

[[e-mail]]

I filed bug 288075 on the -browser <URL> confusion.

Comment 91

[Boying Lu]

Attachment: remove "startBrowser" related codes

Comment 92

[[e-mail]]

Comment on attachment 178887 [details] [diff] [review]
remove "startBrowser" related codes

>+    // convert the cmdLine URL to Unicode
>+    rv = NS_CopyNativeToUnicode(nsDependentCString(urlToLoad), url);
>+    if (NS_SUCCEEDED(rv)) {
>+      rv = handler->GetChromeUrlForTask(getter_Copies(chromeUrlForTask));
>+      if (NS_SUCCEEDED(rv))
>+        rv = OpenWindow(chromeUrlForTask,url,width,height);
>+      if (NS_SUCCEEDED(rv))
>+        *windowOpened = PR_TRUE;
>     }
The braces look a little inconsistent here, I think local style in this file is
to always use {}s with if. r=me with this fixed (no new review needed).


>@@ -1247,30 +1271,7 @@ static nsresult main1(int argc, char* ar
>   // if we had no command line arguments, argc == 1.
I've just noticed this comment - I guess it's no longer useful, so we should
delete it.

Comment 93

[Boying Lu]

Attachment: revised

Comment 94

[Boris Zbarsky [:bzbarsky]]

I'll try to look at this in the next few days.

Comment 95

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 179809 [details] [diff] [review]
revised

So... general comment on creating diffs:

1)  Use the -p option
2)  Use at least 6 lines of context
3)  If you do whitespace cleanup, post a -w diff as well.

It makes patches a lot easier to review (and hence gets them review faster). 
Reviewing large whitespace-only changes for correctness is pretty painful....

>Index: xpfe/bootstrap/nsAppRunner.cpp
>+  // if there is any window opened, startup perferences are ignored

"preferences"

>+        rv = OpenWindow(chromeUrlForTask,url,width,height);

Please put spaces after those commas.

sr=bzbarsky with that

Comment 96

[Boying Lu]

Attachment: the patch for checkin

Thanks Neil and BZ's comments. This patch is for checking in.

Comment 97

[Frank Wein [:mcsmurf]]

Same as for Bug 266259, you actually needed approval since the tree is frozen.
Just take a look at http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey,
it mentions when the tree is frozen. Well, I guess the patch should be backed
out or post-approval should be requested?

Comment 98

[Jo Hermans]

And it caause the beast build to fail too :

nsAppRunner.cpp
/cygdrive/c/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/build/cygwin-wrapper
cl -FonsAppRunner.obj -c  -DXPCOM_GLUE -DMOZ_APP_NAME="mozilla"
-DMOZ_APP_DISPLAYNAME="Mozilla" -DMOZ_APP_VERSION="1.8b2" -DOSTYPE=\"WINNT5.0\"
-DOSARCH=\"WINNT\" -DBUILD_ID=2005043001
-I/cygdrive/c/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap
-I. -I../../dist/include/xpcom -I../../dist/include/string
-I../../dist/include/webbrwsr -I../../dist/include/widget
-I../../dist/include/dom -I../../dist/include/necko -I../../dist/include/content
-I../../dist/include/pref -I../../dist/include/appshell
-I../../dist/include/toolkitcomps -I../../dist/include/appcomps
-I../../dist/include/gfx -I../../dist/include/xpinstall
-I../../dist/include/windowwatcher -I../../dist/include/embed_base
-I../../dist/include/embedcomponents -I../../dist/include/docshell
-I../../dist/include/locale -I../../dist/include/profile
-I../../dist/include/chrome -I../../dist/include/xremoteclient -I!
 ../../dist/
include/jprof -I../../dist/include/xpconnect -I../../dist/include/intl
-I../../dist/include/appcomps -I../../dist/include/apprunner
-I../../dist/include -I../../dist/include/nspr    -I../../dist/sdk/include
-I/usr/X11R6/include     -I/usr/X11R6/include -TP -nologo -W3 -Gy
-FdnsAppRunner.pdb  -DNDEBUG -DTRIMMED -Zi -O1 -UDEBUG -DNDEBUG
-DWIDGET_DLL=\"libwidget_windows.dll\" -DGFXWIN_DLL=\"libgfx_windows.dll\"  -MD
          -I/usr/X11R6/include -DMOZILLA_CLIENT -D_MOZILLA_CONFIG_H_
-DMOZILLA_VERSION=\"1.8b2\" -DMOZILLA_VERSION_MAJOR=1 -DMOZILLA_VERSION_MINOR=8
-DHAVE_SNPRINTF=1 -D_WINDOWS=1 -D_WIN32=1 -DWIN32=1 -DXP_WIN=1 -DXP_WIN32=1
-DHW_THREADS=1 -DWINVER=0x400 -DSTDC_HEADERS=1 -DWIN32_LEAN_AND_MEAN=1
-DNO_X11=1 -D_X86_=1 -DD_INO=d_ino -DMOZ_DEFAULT_TOOLKIT=\"windows\"
-DMOZ_SUITE=1 -DMOZ_BUILD_APP=suite -DMOZ_DISTRIBUTION_ID=\"org.mozilla\"
-DOJI=1 -DIBMBIDI=1 -DMOZ_VIEW_SOURCE=1 -DACCESSIBILITY=1 -DMOZ_XPINSTALL=1
-DMOZ_JSLOADER=1 -DMOZ_XTF=1 -DMOZ_MATHML=1 -DMOZ_LOGGING!
 =1 -DMOZ_US
ER_DIR=\"Mozilla\" -DMOZ_XUL=1 -DMOZ_PROFILESHARING=1 -DMOZ_PROFILELOCKING=1
-DMOZ_BYPASS_PROFILE_AT_STARTUP=1 -DMOZ_DLL_SUFFIX=\".dll\" -DJS_THREADSAFE=1
-DNS_PRINT_PREVIEW=1 -DNS_PRINTING=1 -DMOZILLA_LOCALE_VERSION=\"1.8b2\"
-DMOZILLA_REGION_VERSION=\"1.8b2\" -DMOZILLA_SKIN_VERSION=\"1.5\" 
/cygdrive/c/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp
nsAppRunner.cpp
c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp(730)
: error C2065: 'NS_CopyNativeToUnicode' : undeclared identifier
c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp(730)
: error C2440: 'type cast' : cannot convert from 'class nsCString_external' to
'class nsDependentCString_external'
        No constructor could take the source type, or constructor overload
resolution was ambiguous

Comment 99

[Robert Kaiser]

backed out as it has no approval and breaks windows builds with the following lines:

c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp(730)
: error C2065: 'NS_CopyNativeToUnicode' : undeclared identifier
c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp(730)
: error C2440: 'type cast' : cannot convert from 'class nsCString_external' to
'class nsDependentCString_external'
        No constructor could take the source type, or constructor overload
resolution was ambiguous

(see creature tinderbox)

Comment 100

[Peter Weilbacher]

Attachment: Same change for OS/2

I suspect that it also needs the same fix in nsNativeAppSupportOS2.cpp as on
Windows. Along the lines of this patch. (Actually, the patch in attachment
172248 [details] [diff] [review] already did that before it was revised.)

Robin, when you create a new patch can you change the comment in
nsNativeAppSupport.h to say "from nsNativeAppSupportWin.cpp and
nsNativeAppSupportOS2.cpp"?

Comment 101

[Robin Lu]

(In reply to comment #100)
> Robin, when you create a new patch can you change the comment in

It is Boying Brian Lu's patch, however, checked in by me. 

I am very sorry I missed the trunk frozen warning.

Comment 102

[Mike Kaply [:mkaply]]

Comment on attachment 182316 [details] [diff] [review]
Same change for OS/2

sr=mkaply

Comment 103

[Peter Weilbacher]

Is anyone thinking of getting this (attachment 182241 [details] [diff] [review] and attachment 182316 [details] [diff] [review])
into the trunk now or are you waiting till after the releases?

Comment 104

[[e-mail]]

The problem is that NS_ConvertNativeToUnicode isn't available from
nsAppRunner.cpp so someone needs to come up with a rewrite of the patch.

Comment 105

[Boying Lu]

Since the patch is ready, can anyone checkin the patch?

Comment 106

[Peter Weilbacher]

I don't see that anybody attached a new patch and the last several comments
after the first checkin attempt indicate that definitely a new patch is
required. So why do you think that it is still ready for checkin?

Comment 107

[Boying Lu]

Attachment: fixed the compiler error

put all patches together

Comment 108

[Boris Zbarsky [:bzbarsky]]

I won't be able to get to this for a least a few weeks; I suggest trying a
different sr....

Comment 109

[[e-mail]]

Comment on attachment 195838 [details] [diff] [review]
fixed the compiler error 

>+    nsAutoString url;
>+    nsXPIDLCString chromeUrlForTask;
>+    nsCOMPtr<nsICmdLineHandler> handler(
>+      do_GetService("@mozilla.org/commandlinehandler/general-startup;1?type=browser", &rv));
You don't do anything with this rv.

>+    // convert the cmdLine URL to Unicode
>+    url = NS_ConvertUTF8toUTF16(urlToLoad);
This is wrong. It's probably simplest to inline it into the OpenWindow call,
otherwise use NS_ConvertUTF8toUTF16 url(urlToLoad); not nsAutoString url;

r=me with these nits fixed.

Comment 110

[Boying Lu]

Attachment: revised according to Neil's comment

Comment 111

[[e-mail]]

Comment on attachment 196613 [details] [diff] [review]
revised according to Neil's comment

>+    nsXPIDLCString chromeUrlForTask;
>+    nsCOMPtr<nsICmdLineHandler> handler(
>+      do_GetService("@mozilla.org/commandlinehandler/general-startup;1?type=browser"));
>+
>+    // convert the cmdLine URL to Unicode
>+    NS_ConvertUTF8toUTF16 url(urlToLoad);
>+    rv = handler->GetChromeUrlForTask(getter_Copies(chromeUrlForTask));
>+
>+    if (NS_SUCCEEDED(rv)) {
>+      rv = OpenWindow(chromeUrlForTask, url, width, height);
>+    }
Actually it would probably be clearer still if you moved the declarations
nearer to the uses i.e. move chromeUrlForTask to just before
handler->GetChromeUrlForTask and url to just before OpenWindow.

Comment 112

[Boying Lu]

Attachment: changed according to Neil's suggestion

Neil, Thanks for your suggestion.

Comment 113

[Ginn Chen]

Checking in xpfe/bootstrap/nsAppRunner.cpp;
/cvsroot/mozilla/xpfe/bootstrap/nsAppRunner.cpp,v  <--  nsAppRunner.cpp
new revision: 1.444; previous revision: 1.443
done
Checking in xpfe/bootstrap/nsNativeAppSupport.h;
/cvsroot/mozilla/xpfe/bootstrap/nsNativeAppSupport.h,v  <--  nsNativeAppSupport.h
new revision: 1.16; previous revision: 1.15
done
Checking in xpfe/bootstrap/nsNativeAppSupportWin.cpp;
/cvsroot/mozilla/xpfe/bootstrap/nsNativeAppSupportWin.cpp,v  <-- 
nsNativeAppSupportWin.cpp
new revision: 1.125; previous revision: 1.124
done
Checking in xpfe/bootstrap/nsNativeAppSupportOS2.cpp;
/cvsroot/mozilla/xpfe/bootstrap/nsNativeAppSupportOS2.cpp,v  <-- 
nsNativeAppSupportOS2.cpp
new revision: 1.68; previous revision: 1.67
done

Comment 114

[Stephen Donner [:stephend] Not actively reading bugmail]

Robin, can you see if this caused bug 310803?

Comment 115

[OstGote!]

Could this have caused Bug 310409?

Comment 116

[Mike Cowperthwaite]

I haven't tested this extensively, but the original reported case (-P) -- which was my problem also -- was indeed fixed for Seamonkey 1.5.  Thanks, Boying Lu, 
and Neil too for helping him get it right.

Comment 117

[[e-mail]]

More flag spam...

Comment 118

[Cass Witkowski]

Has this bug resurfaced in SeaMonkey 1.1 for windows?  I have no command line option and have selected to start up mail and browser at startup but only the browser window starts up.

Comment 119

[Mike Cowperthwaite]

I see that symptom if I enable Quick Start.

xref bug 368698, bug 367242.

See also bug 368299 -- a different problem, but one that I can reproduce as well.

Comment 120

[:Hb]

Quick Launch has been deleted with Bug 361682. 

VERIFIED

Comment 121

[Mark Banner (:standard8)]

Comment on attachment 195838 [details] [diff] [review]
fixed the compiler error 

Dropping old review request on an old bug/patch. This code has all moved now anyway, if it is still required, please re-file on a new bug.