Closed Bug 58523 Opened 20 years ago Closed 15 years ago

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

Categories

(SeaMonkey :: UI Design, defect, P3)

defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: bugzilla, Assigned: iamawalrus)

References

Details

(Keywords: fixed-seamonkey1.1b, helpwanted, Whiteboard: relnote-user)

Attachments

(13 files, 4 obsolete files)

1.43 KB, patch
Details | Diff | Splinter Review
2.99 KB, patch
timeless
: review-
Details | Diff | Splinter Review
9.95 KB, patch
neil
: review-
Details | Diff | Splinter Review
30.54 KB, patch
neil
: review+
Details | Diff | Splinter Review
24.34 KB, patch
bryner
: review+
Details | Diff | Splinter Review
11.00 KB, patch
neil
: review-
Details | Diff | Splinter Review
10.41 KB, patch
neil
: review+
Details | Diff | Splinter Review
9.64 KB, patch
neil
: review+
eagle.lu
: superreview-
Details | Diff | Splinter Review
9.67 KB, patch
bzbarsky
: superreview+
Details | Diff | Splinter Review
10.65 KB, patch
Details | Diff | Splinter Review
2.31 KB, patch
dragtext
: review+
mkaply
: superreview+
Details | Diff | Splinter Review
16.64 KB, patch
neil
: review+
Details | Diff | Splinter Review
16.61 KB, patch
bryner
: superreview+
Details | Diff | Splinter Review
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?
pessimistic about this getting fixed any time soon, so adding relnoteRTM kw.
Keywords: relnoteRTM
i tried a more simple test: just selecting mail to launch. still, only the
browser is launched when restarting. clarifying summary.
Summary: -P switch unable to launch more than one product → -P switch unable to launch anything other than the browser
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.
Whiteboard: relnote-user
*** Bug 60045 has been marked as a duplicate of this bug. ***
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!
Summary: -P switch unable to launch anything other than the browser → -P, -nosplash switches unable to launch anything other than the browser
Since Don has left, Vishy is taking his bugs in bulk, pending reassignment.
thanks,
	Vishy
Assignee: don → vishy
*** Bug 62221 has been marked as a duplicate of this bug. ***
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.
Netscape Nav triage team: this is not a Netscape beta stopper.
Keywords: helpwanted, nsbeta1-
*** Bug 66115 has been marked as a duplicate of this bug. ***
Marking nsbeta1- bugs as future to get off the radar.
Target Milestone: --- → Future
*** Bug 90828 has been marked as a duplicate of this bug. ***
OS: Linux → All
Summary: -P, -nosplash switches unable to launch anything other than the browser → -P, -nosplash, -console switches unable to launch anything other than the browser
*** Bug 68316 has been marked as a duplicate of this bug. ***
->bill to triage
Assignee: vishy → 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?
Target Milestone: Future → ---
Status: NEW → ASSIGNED
Setting target milestone to Future.  Not high enough priority.
Target Milestone: --- → Future
*** Bug 123818 has been marked as a duplicate of this bug. ***
Summary: -P, -nosplash, -console switches unable to launch anything other than the browser → -P, -nosplash, -splash, -console switches unable to launch anything other than the browser
*** Bug 143709 has been marked as a duplicate of this bug. ***
*** Bug 148049 has been marked as a duplicate of this bug. ***
*** Bug 153404 has been marked as a duplicate of this bug. ***
*** Bug 160128 has been marked as a duplicate of this bug. ***
Summary: -P, -nosplash, -splash, -console switches unable to launch anything other than the browser → -P, -nosplash, -splash, -console, -ProfileManager, -installer switches unable to launch anything other than the browser
*** Bug 176986 has been marked as a duplicate of this bug. ***
*** Bug 177009 has been marked as a duplicate of this bug. ***
*** Bug 179615 has been marked as a duplicate of this bug. ***
*** Bug 36231 has been marked as a duplicate of this bug. ***
*** Bug 98577 has been marked as a duplicate of this bug. ***
Summary: -P, -nosplash, -splash, -console, -ProfileManager, -installer switches unable to launch anything other than the browser → -P, -nosplash, -splash, -console, -ProfileManager, -installer switches unable to launch anything other than the browser (mail does not open on startup)
*** Bug 197981 has been marked as a duplicate of this bug. ***
*** Bug 200502 has been marked as a duplicate of this bug. ***
*** Bug 200827 has been marked as a duplicate of this bug. ***
*** Bug 215593 has been marked as a duplicate of this bug. ***
Attached patch patch (obsolete) — Splinter Review
preliminary patch for discussing
IMHO, MAC box may use the "heedStartupPrefs" argument, while *NIX and Win box not.
so just ignore that argument when not using MAC box.
Hi Bill Law,
Could you please take a look at this patch?
The fix of this bug is very important for *nix users.
Thanks!
Attachment #131164 - Flags: review?(jag)
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.
Attachment #131164 - Attachment is obsolete: true
Attachment #131164 - Flags: review?(jag)
*** Bug 237912 has been marked as a duplicate of this bug. ***
*** Bug 248622 has been marked as a duplicate of this bug. ***
*** Bug 248911 has been marked as a duplicate of this bug. ***
Summary: -P, -nosplash, -splash, -console, -ProfileManager, -installer switches unable to launch anything other than the browser (mail does not open on startup) → -P, -nosplash, -splash, -console, -ProfileManager, -installer switches (arguments) unable to launch anything other than the browser (mail does not open on startup)
Attached patch another proposalSplinter Review
I filed a similiar bug https://bugzilla.mozilla.org/show_bug.cgi?id=265065 on
bugzilla
Comment on attachment 162657 [details] [diff] [review]
another proposal

Can you give r? Thanks
Attachment #162657 - Flags: review?(law)
Boying, I don't think Bill Law would be looking at this bug anymore.
Comment on attachment 162657 [details] [diff] [review]
another proposal

Can you give r? Thanks
Attachment #162657 - Flags: review?(law) → review?(jag)
Comment on attachment 162657 [details] [diff] [review]
another proposal

Can you give sr? Thanks
Attachment #162657 - Flags: superreview?(neil.parkwaycc.co.uk)
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.
Attachment #162657 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview-
Attachment #162657 - Flags: review?(jag)
Attachment #168935 - Flags: review?(law)
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
Attachment #168935 - Flags: review?(law) → review-
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
BTW: Since Law is gone, who is the response engineer for this bug?
No longer depends on: 265065
*** Bug 265065 has been marked as a duplicate of this bug. ***
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.
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 on attachment 168944 [details] [diff] [review]
use -up50 to re-generate the patch

Can you give r? Thanks
Attachment #168944 - Flags: review?(neil.parkwaycc.co.uk)
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.
Attachment #168944 - Flags: review?(neil.parkwaycc.co.uk) → review-
Or if it does compile, it won't link.
I have tested the patch on solaris,linux and windows XP.
Attachment #172248 - Flags: review?(neil.parkwaycc.co.uk)
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.
Attachment #172248 - Flags: review?(neil.parkwaycc.co.uk) → review+
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.
Attached patch a revisionSplinter Review
I re-made the patch according to Neil's comment. Thanks Neil. Can you give r?
Thanks
Attachment #172319 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 172319 [details] [diff] [review]
a revision

Could you reivew toolkit parts? Thanks
Attachment #172319 - Flags: review?(neil.parkwaycc.co.uk) → review?(bryner)
Comment on attachment 172319 [details] [diff] [review]
a revision

Did you forget to include the diff for toolkit/xre/nsAppRunner.cpp?
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 on attachment 172319 [details] [diff] [review]
a revision

Neil has give r except "toolkit" part
Attachment #172319 - Flags: review?(bryner) → review?(darin)
Comment on attachment 172319 [details] [diff] [review]
a revision

Can you give r? Thanks
Attachment #172319 - Flags: review?(darin) → review?(neil.parkwaycc.co.uk)
Comment on attachment 172319 [details] [diff] [review]
a revision

r=me for xpfe changes.
Attachment #172319 - Flags: review?(neil.parkwaycc.co.uk) → review+
Comment on attachment 172319 [details] [diff] [review]
a revision

Can you reivew the "toolkit" part? Thanks
Attachment #172319 - Flags: review+ → review?(hyatt)
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=
Attachment #172319 - Flags: review?(hyatt)
Comment on attachment 172319 [details] [diff] [review]
a revision

Can you review "toolkit" part?
Attachment #172319 - Flags: review?(darin)
Comment on attachment 172319 [details] [diff] [review]
a revision

Can you give r?
Attachment #172319 - Flags: review?(darin) → review?(bryner)
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.
Attachment #172319 - Flags: review?(bryner) → review+
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 on attachment 172319 [details] [diff] [review]
a revision

sr? thanks
Attachment #172319 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #172319 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Well I agree with bryner, so I just removed the obsolete declaration.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Reopening because bare URLs on the command line were getting ignored.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #176008 - Attachment is obsolete: true
Attached patch a revised patch (obsolete) — Splinter Review
Fixed the issue found by neil.
Attachment #176697 - Flags: review?(neil.parkwaycc.co.uk)
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.
Attachment #176697 - Flags: review?(neil.parkwaycc.co.uk) → review-
*** Bug 270842 has been marked as a duplicate of this bug. ***
Attachment #176697 - Attachment is obsolete: true
Attached patch another oneSplinter Review
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
Attachment #177592 - Flags: review?(neil.parkwaycc.co.uk)
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.
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;
       }
     }
   }
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.
(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 on attachment 177592 [details] [diff] [review]
another one

I don't think we should handle out-of-order arguments.
Attachment #177592 - Flags: review?(neil.parkwaycc.co.uk) → review-
But what will happen when user launches mozilla like this:
mozilla -browser -mail mailtoURL www.google.com
(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.
Attachment #178547 - Flags: review?(neil.parkwaycc.co.uk)
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.
Attachment #178547 - Flags: review?(neil.parkwaycc.co.uk) → review+
(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.
(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.
I filed bug 288075 on the -browser <URL> confusion.
Attachment #178887 - Flags: superreview?(jag)
Attachment #178887 - Flags: review?(neil.parkwaycc.co.uk)
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.
Attachment #178887 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attachment #178887 - Flags: superreview?(jag) → superreview-
Attached patch revisedSplinter Review
Attachment #179809 - Flags: superreview?(bzbarsky)
I'll try to look at this in the next few days.
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
Attachment #179809 - Flags: superreview?(bzbarsky) → superreview+
Thanks Neil and BZ's comments. This patch is for checking in.
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?
Assignee: law → robin.lu
Status: REOPENED → NEW
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
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)
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"?
Attachment #182316 - Flags: superreview?(mozilla)
Attachment #182316 - Flags: review?(dragtext)
(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.
Attachment #182316 - Flags: review?(dragtext) → review+
Comment on attachment 182316 [details] [diff] [review]
Same change for OS/2

sr=mkaply
Attachment #182316 - Flags: superreview?(mozilla) → superreview+
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?
The problem is that NS_ConvertNativeToUnicode isn't available from
nsAppRunner.cpp so someone needs to come up with a rewrite of the patch.
Since the patch is ready, can anyone checkin the patch?
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?
put all patches together
Attachment #195838 - Flags: superreview?(bzbarsky)
Attachment #195838 - Flags: review?(neil.parkwaycc.co.uk)
I won't be able to get to this for a least a few weeks; I suggest trying a
different sr....
Attachment #195838 - Flags: superreview?(bzbarsky) → superreview?(jag)
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.
Attachment #195838 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attachment #196613 - Flags: superreview?(alecf)
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.
Attachment #196613 - Attachment is obsolete: true
Attachment #196613 - Flags: superreview?(alecf) → superreview-
Neil, Thanks for your suggestion.
Attachment #196735 - Flags: superreview?(alecf)
Attachment #196735 - Flags: superreview?(alecf) → superreview?(bryner)
Attachment #196735 - Flags: superreview?(bryner) → superreview+
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
Status: NEW → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Robin, can you see if this caused bug 310803?
Could this have caused Bug 310409?
Depends on: 310409
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.
More flag spam...
Component: Cmd-line Features → XP Apps
Product: Core → Mozilla Application Suite
Target Milestone: Future → ---
Attachment #196735 - Flags: approval-seamonkey1.1b?
Attachment #196735 - Flags: approval-seamonkey1.1b? → approval-seamonkey1.1b+
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.
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.
Quick Launch has been deleted with Bug 361682. 

VERIFIED
Status: RESOLVED → VERIFIED
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.
Attachment #195838 - Flags: superreview?(jag)
You need to log in before you can comment on or make changes to this bug.