Closed Bug 276588 Opened 20 years ago Closed 19 years ago

Rework toolkit command-line handling

Categories

(Toolkit :: Startup and Profile System, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.8beta1

People

(Reporter: benjamin, Assigned: benjamin)

References

()

Details

Attachments

(11 files, 3 obsolete files)

3.88 KB, patch
caillon
: first-review+
Details | Diff | Splinter Review
38.28 KB, patch
mconnor
: first-review+
Details | Diff | Splinter Review
136.80 KB, patch
darin.moz
: first-review+
Details | Diff | Splinter Review
5.06 KB, patch
caillon
: first-review+
Details | Diff | Splinter Review
52.30 KB, patch
Details | Diff | Splinter Review
7.15 KB, patch
darin.moz
: first-review+
Details | Diff | Splinter Review
3.61 KB, patch
Details | Diff | Splinter Review
1.85 KB, patch
darin.moz
: first-review+
Details | Diff | Splinter Review
5.80 KB, patch
benjamin
: first-review+
Details | Diff | Splinter Review
999 bytes, patch
Details | Diff | Splinter Review
1.07 KB, patch
sicking
: first-review+
Details | Diff | Splinter Review
This is the bug for http://wiki.mozilla.org/index.php/XUL:Command_Line_Handling

My target is 1.8a6, because this is a basic requirement for the xulrunner; I
think I have a good handle on the toolkit apps, but I do *not* have a good
handle on the suite, because of the multitude of command-line flags and some
bizarre interactions between them. So I wish to consider this a toolkit-only bug
for the time being, with an easy upgrade path for the suite (I'm configuring the
build system so that's it would be easy to plug this code into the suite without
a lot of extraneous changes.

I'm going to break this up into several patches:

1) the core commandline interfaces and impl, with nsAppRunner.cpp hookup
2) the command-line handlers, most of which are written in JS (probably one
patch for firefox/xulrunner and a separate one for tbird)
3) the remoting hookup for xremote/winDDE/appleevents (each separate, because
different people need to review them)
darin, let me know if you're not going to get to these reviews by alpha6.
Attachment #169979 - Flags: first-review?(darin)
I don't think I'm going to get to do real xremote remoting per the
nsICommandLine API until after alpha, but this patch preserves the status-quo
for existing xremote usage.
Attachment #169981 - Flags: first-review?(caillon)
Depends on: 276592
Comment on attachment 169979 [details] [diff] [review]
Core commandline APIs and impl, with startup-hookup

>Index: toolkit/components/commandlines/public/nsICommandLine.idl
>+  /**
>+   * A helper method which will find a flag and remove it in one step.
>+   *
>+   * @param aFlag  The flag name to find and remove.
>+   * @return       Whether the flag was found.
>+   */
>+  boolean handleFlag(in AString aFlag, in boolean aCaseSensitive);

Missed @param aCaseSensitive, though it's obvious what it does. :)

A couple of other ones don't have any @param/@return info, though they are all
obvious from the main comment.

>Index: toolkit/components/commandlines/public/nsICommandLineHandler.idl
>+  /**
>+   * When the app is launched with the -help argument, this attribute
>+   * is retrieved and displayed to the user (on stdout). The text should
>+   * have embedded newlines which wrap at 76 columns, and should include
>+   * a newline at the end. By convention, the right column which contains flag
>+   * descriptions begins at the 24th character.
>+   */
>+  readonly attribute AUTF8String helpText;

Does this comment mean that the help text, as returned by the attribute, should
include the flag (and param) at the left edge and the description wrapped
within columns 24 - 76?
Comment on attachment 169979 [details] [diff] [review]
Core commandline APIs and impl, with startup-hookup

>Index: toolkit/components/commandlines/public/nsICommandLine.idl

>+   * On *nix, flags of the form --flag are normalized to -flag. --flag=param
>+   * are normalized to the form -flag param.

Does this include MacOS X?  If so, probably worth stating that explicitly.

>+   * @return       The indexth argument, or -1 if not found.

So the string "-1" will be returned?  Or what?

>+  /**
>+   * Find a command-line flag.
>+   *
>+   * @param aFlag          The flag name to locate. Do not include the intitial
>+   *                       hyphen.

s/intitial/initial/

>+   * A helper method which will find a flag and remove it in one step.

It would be good to define up front what the terms "flag" and "argument" mean
in this interface.  Some people treat them as synonyms, others assume "flags"
have values while "arguments" do not.  Until I saw handleFlagWithParam, I
assumed that handleFlag would remove the flag together with its value....

>+   * If the flag cannot be found, returns null (which is not quite the same as an
>+   * empty string).

Are all the various AStrings we have nullable?	That is, could a C++ caller use
this method?  Heck, is at least nsString/nsAutoString nullable?

>+   * STATE-REMOTE_EXPLICIT is a remote command line explicitly redirected 

Is there a reason that's a dash and not an underscore?

>+  attribute boolean preventDefault;

Do we really want to allow setting this to false?

I'd think a more reasonable setup would be a pair as follows:

  readonly attribute boolean doDefaultAction;
  void preventDefaultAction();

or something.

>Index: toolkit/components/commandlines/public/nsICommandLineHandler.idl

>+   * is retrieved and displayed to the user (on stdout). The text should
>+   * have embedded newlines which wrap at 76 columns, and should include
>+   * a newline at the end.

Do we have debug asserts on this?  If not, I'd add them.

>Index: toolkit/components/commandlines/public/nsICommandLineRunner.idl

This interface is badly underdocumented.

I assume these interfaces replace nsICmdLineService and nsICmdLineHandler,
right?	I don't see a way to get the program name via these apis; have we
decided that it's not needed anymore?
> Missed @param aCaseSensitive, though it's obvious what it does. :)

Will fix.

> A couple of other ones don't have any @param/@return info, though they are all
> obvious from the main comment.

Will fix as appropriate.

> include the flag (and param) at the left edge and the description wrapped
> within columns 24 - 76?

Yes. Better wording is welcome. I should probably make some examples also.

> Does this include MacOS X?  If so, probably worth stating that explicitly.

It does currently, I will add a comment.

> 
> >+   * @return       The indexth argument, or -1 if not found.
> 
> So the string "-1" will be returned?  Or what?

Oh, that's a bogus comment I copied from "findFlag". It either returns the
argument or throws an out-of-bounds error.

> It would be good to define up front what the terms "flag" and "argument" mean
> in this interface.  Some people treat them as synonyms, others assume "flags"
> have values while "arguments" do not.  Until I saw handleFlagWithParam, I
> assumed that handleFlag would remove the flag together with its value....

Ah, good point. How about this comment:
 * DEFINITIONS:
 * "arguments" are any values found on the command line.
 * "flags" are switches. In normalized form they are preceded by a single dash.
 * Some flags may take "parameters", e.g. "-url <param>" or
 *   "-install-xpi <param>"
 *

> Are all the various AStrings we have nullable?	That is, could a C++ caller use
> this method?  Heck, is at least nsString/nsAutoString nullable?

Yes. nsAString.SetIsVoid is available in all of our string classes.

> Is there a reason that's a dash and not an underscore?

Typo, fixed.

> >+  attribute boolean preventDefault;
> 
> Do we really want to allow setting this to false?

Yes. The "default action" is application-dependent (it is not performed by the
command-line code). So one of the "last" command line handlers "reads" the
preventDefault attribute, and if it is false, opens a browser window or performs
some other appropriate default action.

> >Index: toolkit/components/commandlines/public/nsICommandLineRunner.idl
> 
> This interface is badly underdocumented.

Yes, but it's internal/private (and non-scriptable to boot, at least until some
JS caller can convince me why they might need it). I will add some basic docs.

> I assume these interfaces replace nsICmdLineService and nsICmdLineHandler,
> right?	I don't see a way to get the program name via these apis; have we
> decided that it's not needed anymore?

Yes, nsICmdLine* will be gone. For the appname, it's more like "not an
applicable question in a xulrunner world", since the xulrunner binary will be
doing the launching, and it may be manually launched from a directory, or from a
custom-created mac bundle, or from the /usr/bin/xulrunner finder-stub.
I won't have a chance to review this until Jan 3rd.
> Ah, good point. How about this comment:
>  * DEFINITIONS:
>  * "arguments" are any values found on the command line.
>  * "flags" are switches. In normalized form they are preceded by a single dash.

Is this the right place to mention the "how it does it" vs "what it does"
distinction?  Or is it just me that thinks of command-line arguments that way?

> Yes. The "default action" is application-dependent (it is not performed by the
> command-line code).

Yes, but if a handler prevents default, do we want to allow a different handle
to "unprevent" it?  What's the use case?

> > This interface is badly underdocumented.
> 
> Yes, but it's internal/private (and non-scriptable to boot

So?  Imo we should document internal interfaces just as well as external ones...
A bunch of the nits fixed, and I got win32 DDE in this patch. A lot of the
logic to open new tabs or windows was pushed into the command line handlers, so
it's a lot more - than +. I'll attach the main firefox command line handler
shortly.
Attachment #169979 - Attachment is obsolete: true
Attachment #170049 - Flags: first-review?(darin)
Attachment #169979 - Flags: first-review?(darin)
Brian, since you reviewed danm's original code for DDE external link handling,
I'm giving this one to you. I really hope to land this before alpha6, so if you
can't get to the review let me know and I will ask around.
Attachment #170050 - Flags: first-review?(bryner)
Comment on attachment 170049 [details] [diff] [review]
Core commandline APIs and impl with startup-hookup and win32 DDE remoting (version 2)

>Index: toolkit/components/commandlines/public/nsICommandLine.idl

>+   * Find a flag with an argument and remove both.

s/argument/parameter/ ?

>+  attribute boolean preventDefault;

I'm still not clear on why we want to allow a handler to reenable the default
action after another handler has prevented it....  That just doesn't make much
sense to me.

>Index: toolkit/components/commandlines/public/nsICommandLineRunner.idl
>+  void init(in long argc, in nsCharPtrArray argv,
>+            in nsIFile workingDir, in unsigned long state);

What's "state"?

(Note that I'm just looking at the new apis, not any of the rest of this patch.
 Apart from the above nits, they look very reasonable to me.)
Comment on attachment 170049 [details] [diff] [review]
Core commandline APIs and impl with startup-hookup and win32 DDE remoting (version 2)

Somehow this patch got mangled. I'm trying again.
Attachment #170049 - Attachment is obsolete: true
Attachment #170049 - Flags: first-review?(darin)
Attachment #170147 - Flags: first-review?(darin)
Comment on attachment 170147 [details] [diff] [review]
Core commandline APIs and impl, version 2.1

>Index: toolkit/components/commandlines/

nit: maybe "commandline" instead of "commandlines"  ... there is
only one command-line right?


>Index: toolkit/components/commandlines/public/Makefile.in

>+XPIDL_MODULE = commandline

nit: not commandlines?


>Index: toolkit/components/commandlines/public/nsICommandLine.idl

>+   * On *nix and mac flags of the form --flag are normalized to -flag. --flag=param
>+   * are normalized to the form -flag param.

nit: this suggests that --flag is not supported on windows.  is there a point
to that restriction?


>+   * @param aStart  Index to begin removing.
>+   * @param aEnd    Index to end removing.

Is aEnd inclusive or exclusive?  I.e., if I pass 3 & 4 would that remove
2 arguments or only 1?


>+   * STATE_REMOTE_AUTO     is a remote command line automatically redirected to
>+   *                       this instance.
>+   * STATE_REMOTE_EXPLICIT is a remote command line explicitly redirected to
>+   *                       this instance using xremote/windde/appleevents.

when does STATE_REMOTE_AUTO happen?


>+  /**
>+   * There may be a command-line handler which performs a default action if
>+   * there was no explicit action on the command line (open a default browser
>+   * window, for example). This flag allows the default action to be prevented.
>+   */
>+  attribute boolean preventDefault;

how is the default handler configured?


>+  nsIURI resolveURI(in AString aArgument);

general comment about this interface: since arguments are ASCII valued,
would it perhaps be better to use ACString here instead?


>Index: toolkit/components/commandlines/src/nsCommandLine.cpp

Had to stop reviewing here... will continue later.
> general comment about this interface: since arguments are ASCII valued,

They're not (eg filenames can easily be non-ASCII).
> They're not (eg filenames can easily be non-ASCII).

whoops, you're of course right.  i was thinking in terms of flags :-/
Comment on attachment 170147 [details] [diff] [review]
Core commandline APIs and impl, version 2.1

>Index: toolkit/components/commandlines/src/nsCommandLine.cpp

>+static const nsDefaultStringComparator caseCmp;
>+static const nsCaseInsensitiveStringComparator caseICmp;

Is there really any advantage to putting these here?  Constructing
an instance of one of these classes should correspond to a single
instruction to assign the vtable pointer.  Doing that tiny bit of
work in FindFlag would seem fine to me.


>+nsCommandLine::FindFlag(const nsAString& aFlag, PRBool aCaseSensitive, PRInt32 *aResult)
>+{
>+  NS_ENSURE_ARG(aFlag.Length() > 0);

nit: NS_ENSURE_ARG(!aFlag.IsEmpty());

or should this just be a NS_ASSERTION?	the comparsions will
fail if aFlag is empty.


>+  for (f = 0; f < mArgs.Count(); ++f) {
>+    nsString &arg = *mArgs[f];
>+
>+    if (arg.Length() >= 2 && arg[0] == PRUnichar('-')) {
>+      nsDependentSubstring realarg(arg, 1, PRUint32(-1));
>+      if (realarg.Equals(aFlag, aCaseSensitive ? c : ci)) {

why not move the aCaseSensitive check outside of the loop?
you could also rewrite this like so:

  if (aFlag.Equals(Substring(arg, 1), comparator)) {
    ...
  }

that would avoid the explicit use of nsDependentSubstring,
which is generally discouraged.

also, |arg| should be declared |const|, right?


>+nsCommandLine::RemoveArguments(PRInt32 aStart, PRInt32 aEnd)
...
>+  NS_ENSURE_ARG_MAX(aEnd, mArgs.Count());

hmm.. shouldn't this be |NS_ENSURE_ARG_MAX(aEnd, mArgs.Count() - 1)|?


>+  for (PRInt32 i = aEnd; i >= aStart; --i) {
>+    mArgs.RemoveStringAt(i);

...given that aEnd appears to be inclusive.


>+nsCommandLine::HandleFlagWithParam(const nsAString& aFlag, PRBool aCaseSensitive,

>+  if (found == -1) {
>+    aResult.SetIsVoid(PR_TRUE);

Whoa.. are you sure you want to use SetIsVoid here??  Why not Truncate?
Oh, ic... the IDL says that the return value may be null in some cases.
This API is confusing.	I would prefer if it returned boolean like 
handleFlag and then also returned an outparam that was an AString instead
of using the Void property.  Or hmm...

+  /**
+   * Find a flag with a parameter and remove both. This is a helper
+   * method that combines "findFlag" and "removeArguments" in one step.
+   *
+   * @return	null (a void astring) if the flag is not found. The parameter
value
+   *		if found. Note that null and the empty string are not the same.
+   * @throws	NS_ERROR_INVALID_ARG if the flag exists without a parameter
+   *
+   * @param aFlag The flag name to find and remove.
+   * @param aCaseSensitive Whether to do case-sensitive flag search.
+   */

You say that NS_ERROR_INVALID_ARG will be thrown in the case where
we have a flag but no parameter.  That appeared to be covered by the
case of NS_OK with an empty return ("").  Why do you need an exception
then?

I think there should be some consistency in the API for handleFlag and
handleFlagWithParam.  Either use exceptions to report non-existent flags,
or use a boolean return value, but be consistent :-)


>+  if (* mArgs[found]->get() == '-') {

You could also use the |First()| method.


>+NS_IMETHODIMP
>+nsCommandLine::ResolveFile(const nsAString& aArgument, nsIFile* *aResult)

No OS/2 impl ;-)


>+#elif defined(XP_UNIX)
...
>+  nsAutoString newpath;
>+  mWorkingDir->GetPath(newpath);
>+
>+  newpath.Append('/');
>+  newpath.Append(aArgument);
>+
>+  rv = lf->InitWithPath(newpath);

Remember that it is more efficient to work with native
file paths when inside XP_UNIX code.

It seems to me that this file resolving code should live
in xpcom/io along side the nsLocalFile implementations.


>+NS_IMETHODIMP
>+nsCommandLine::ResolveURI(const nsAString& aArgument, nsIURI* *aResult)
>+{
>+  // This is, paradoxically enough, simpler than resolving a path. The only
>+  // "odd" case that needs special handling is windows-style C:\Foo paths, which
>+  // are misinterpreted as a protocol instead of a path.

Hmm... I think it should be an error to call this function with a file path.
If a consumer is unsure, then they should call ResolveFile first, no?


>+    // Is this a path with a drive letter?
>+    if (':' == *reader) {
>+      // Treat this as an absolute path.
>+      nsCOMPtr<nsILocalFile> lf (do_CreateInstance(NS_LOCAL_FILE_CONTRACTID));
>+      NS_ENSURE_TRUE(lf, NS_ERROR_OUT_OF_MEMORY);
>+
>+      return io->NewFileURI(lf, aResult);

So, "x://blah" is assumed to be an invalid URL?


>+EnumRun(nsCString& aEntry, void* aData)

nit: |const nsCString& aEntry|


>+  if (rv == NS_ERROR_ABORT)
>+    return PR_FALSE;
>+
>+  return PR_TRUE;

return rv != NS_ERROR_ABORT;


>+EnumHelp(nsCString& aEntry, void* aData)

nit: |const nsCString& aEntry|


>Index: toolkit/xre/nsAppRunner.cpp

>       NS_TIMELINE_LEAVE("appStartup->CreateHiddenWindow");
>       NS_ENSURE_SUCCESS(rv, 1);
>-
>+ 

nit: kill the whitespace addition.


>Index: toolkit/xre/nsNativeAppSupportWin.cpp

>     NS_IMETHOD SendRequest( const char *cmd ) {
...
>+        char* cmdbuf = (char*) malloc(cmdlen + MAX_PATH + 1);
...
>         HWND newWin = (HWND)::SendMessage( mHandle, WM_COPYDATA, 0, (LPARAM)&cds );
>         if ( newWin ) {
>             ::SetForegroundWindow( newWin );
>         }
>         return NS_OK;

where is |cmdbuf| freed?  in the WM_COPYDATA handler?  or you could
just free it after SendMessage returns since the message handler will
run synchronously.


>+                if (NS_ERROR_ABORT == cmdLine->Run())
>+                    return FALSE;
>+
>+                return TRUE;

nit:		  return cmdLine->Run() != NS_ERROR_ABORT;


>Index: toolkit/mozapps/extensions/public/nsIExtensionManager.idl

> [scriptable, uuid(c3515b0f-99f4-453b-805e-1fdf5724d6d9)]
> interface nsIExtensionManager : nsISupports
> {
>   // Apprunner hooks
>-  boolean start(in boolean aIsDirty);
>+  boolean start(in nsICommandLine aCmdLine, in boolean aIsDirty);
> 
>   /** 
>    * Returns true if mismatches were found and the app needs to restart. 
>    */
>   boolean checkForMismatches();
> 
>-  void handleCommandLineArgs();
>+  void handleCommandLineArgs(in nsICommandLine cmdline);

interface UUID changes are needed here.


>Index: xpfe/browser/src/nsBrowserInstance.cpp

> NS_IMETHODIMP    
> nsBrowserInstance::StartPageCycler(PRBool* aIsPageCycling)
> {
>+  *aIsPageCycling = PR_FALSE;
>+
>+#ifndef MOZ_XUL_APP

Are you sure the page cycler isn't used by any of the tinderboxen?
I don't remember.


>+#ifndef MOZ_XUL_APP
> //*****************************************************************************
> // nsBrowserInstance: Helpers
> //*****************************************************************************

I missed the reason for these being disabled.
Attachment #170147 - Flags: first-review?(darin) → first-review-
>+nsCommandLine::ResolveFile

just going by the function name here - I'm assuming this takes a filename and
returns an absolute file, relative to some known base. would it be easier/less
code to use nsIURI::Resolve here? I.e., construct an nsIFileURI from the base
path (NS_NewFileURI) and call ->Resolve on that when needed (or NS_NewURI), and
convert the result into a file/file path again?
> just going by the function name here - I'm assuming this takes a filename and
> returns an absolute file, relative to some known base.

biesi: i think the relative filename can have path separators :(

Since I already added the CVSdir to make the patch, can we stick with
commandlines? At least in the DDE world, there is more than one ;)

Timeless made me not-honor --flags on windows. We don't currently, and I'm
trying to keep behavior as similar to current behavior as possible.

> when does STATE_REMOTE_AUTO happen?

When you launch a second firefox.exe -commandline, it will be auto-remoted to
the first instance with STATE_REMOTE_AUTO. If DDE openURL() is used, then the
commandline will be STATE_REMOTE_EXPLICIT. right now all of xremote is explicit,
but I hope to do generic command-line remoting in the 1.8b timeframe.

> how is the default handler configured?

Each app does what they want. For Firefox see my other patch, which registers a
"y-default" category and handles any remaining URLs on the command line.

> Is there really any advantage to putting these here?  Constructing

I was trying to use ?: and kept getting compiler errors. Neil pointed out that I
needed to use casts to disambiguate it.

OK, here is how I designed handleFlag() and handleFlagWithParam(): With
well-formed command lines, they should not throw an exception. i.e.:

A: firefox.exe
B: firefox -chrome
C: firefox -chrome chrome://inspector/content/
D: firefox -chrome ""

    handleFlag("chrome")           handleFlagWithParam(
A:  returns false;                 returns null;
B:  returns true;                  throws NS_ERROR_INVALID_ARG
C:  returns true;                  returns "chrome://inspector/content/"
D:  returns true;                  returns ""

Does this make sense? Can the IDL comment be clearer?

> >+NS_IMETHODIMP
> >+nsCommandLine::ResolveFile(const nsAString& aArgument, nsIFile* *aResult)
> 
> No OS/2 impl ;-)

Oops, OS/2 matches windows; I fixed the #if

> It seems to me that this file resolving code should live
> in xpcom/io along side the nsLocalFile implementations.

Yes it should, but let's not hold up this patch. I'll file a followup to convert
it over and make a nsILocalFile2 interface.

> Hmm... I think it should be an error to call this function with a file path.
> If a consumer is unsure, then they should call ResolveFile first, no?

This is necessary to support Firefox "Open With..." and it's not a bad thing in
general. Basically, people right-click HTML files and choose "Open With Firefox"
frequently (command line is firefox.exe C:\Path\To\File.htm), and get "C is not
a registered protocol." This works around the problem.

> So, "x://blah" is assumed to be an invalid URL?

Yes. There aren't any one-letter protocols, and IE would treat them as file
paths anyway so it's unlikely there will be.


> >+EnumRun(nsCString& aEntry, void* aData)
> 
> nit: |const nsCString& aEntry|

The nsStringArray enumerator passes a mutable string.

> >Index: xpfe/browser/src/nsBrowserInstance.cpp

> Are you sure the page cycler isn't used by any of the tinderboxen?
> I don't remember.

yes, I checked with dbaron.


> >+#ifndef MOZ_XUL_APP
> > //*****************************************************************************
> > // nsBrowserInstance: Helpers

> I missed the reason for these being disabled.

These handlers are app-specific. The Firefox patch attached to this bug contains
the new handlers for -url -chrome -new-window -width -height and the default URL
handler.
Updated with comments and nits. Also, I have changed
nsICommandLineHandler.helpText to .helpInfo, because when I was implementing
the new handler for DOMI, I realized that there's a conflict with
nsICmdLineHandler (semantically different properties with the same name).
Attachment #170147 - Attachment is obsolete: true
Attachment #170254 - Flags: first-review?(darin)
Ok, I'm fine with the handleFlag and handleFlagWithParam API.  I understand that
you are trying to make it convenient for JS callers.  NS_ERROR_INVALID_ARG seems
to suggest that aFlag is somehow out-of-range or an illegal parameter value. 
Instead, it is really the case that that exception is reached when the user
input is bad, right?  So, maybe that is the wrong name for the exception. 
NS_ERROR_NOT_AVAILABLE might be clearer, but that's just my opinion :-/
Comment on attachment 170254 [details] [diff] [review]
Core commandline APIs and impl, version 2.2

>+interface nsICommandLine : nsISupports
...
>+  readonly attribute long length;
...
>+  AString getArgument(in long aIndex);
...
>+  void removeArguments(in long aStart, in long aEnd);

nit: can these integer values be unsigned?  then you wouldn't need to
bounds check the parameters.  findFlag could return unsigned long too,
and then you could make PR_UINT32_MAX indicate not found.  or, you
could just keep it signed if that is more convenient for JS callers.


>+  nsDefaultStringComparator caseCmp;
>+  nsCaseInsensitiveStringComparator caseICmp;
>+  nsStringComparator& c = aCaseSensitive ?

nit: these could probably all be declared |const| fwiw.


>+NS_IMETHODIMP
>+nsCommandLine::ResolveURI(const nsAString& aArgument, nsIURI* *aResult)

As discussed over IRC, this guy should call ResolveFile first and then
failover to URI parsing if that fails.


r=darin
Attachment #170254 - Flags: first-review?(darin) → first-review+
Comment on attachment 170254 [details] [diff] [review]
Core commandline APIs and impl, version 2.2

toolkit/components/commandlines/public/nsICommandLine.idl
+ * Represents the command line used to invoke a XUL application. 

why? are commandlines inherently tied to XUL apps?

+  readonly attribute long length;

can you make this unsigned?

in fact, it would seem like a good idea to make all indices unsigned. 

findFlag:
+   * @param aFlag	    The flag name to locate. Do not include the initial
+   *			    hyphen.

maybe s/Do/Does/? not sure...
+   * @return		    The position of the flag in the command line.

what if the flag is not found? Does this throw an exception, or does it return
a special value?


Why are arguments removed? would it do any harm to leave them in there?


Do any of these functions do bounds checking? If so, can you document what
happens if the index is out of bounds?

handleFlagWithParam:
+   * @return	null (a void astring)

Make that AString?

unsure if void strings are a good idea... I guess they make this easier to use
from JS... 

You are a bit inconsistent with placement of @param vs @return
(handleFlagWithParam vs handleFlag); and with the alignment of @param
documentation (handleFlag* vs findFlag, removeArguments, etc)

What is STATE_REMOTE_AUTO? I think this needs a better comment and/or an
example...

I agree with bz that preventing a setPreventDefault(PR_FALSE) would be a good
thing... 


Why does resolveURI accept file paths? shouldn't the caller use resolveFile
first if desired? (or does it not handle them, and the comment is a bit
misleading re C:\ paths?)

toolkit/components/commandlines/public/nsICommandLineHandler.idl
+   * a window), and remove the arguments from the command-line array.

which array? you mean from the commandline object?

I am unsure if requiring helpInfo to be formatted in a special way is a good
thing... what if I, say, want to generate an HTML file showing valid command
line options? (For displaying it in the Mozilla Help, say). I guess this
specific use case would not be harmed, but others may...

maybe I'm just worrying too much about this.

toolkit/components/commandlines/public/nsICommandLineRunner.idl
is it useful to separate init from run? can I call run several times?

merging the methods would mean that callers are unable to call run on a
non-inited object
oh, I see you responded to some of that earlier...

(In reply to comment #19)
> > Hmm... I think it should be an error to call this function with a file path.
> > If a consumer is unsure, then they should call ResolveFile first, no?
> 
> This is necessary to support Firefox "Open With..." and it's not a bad thing in
> general.

Why? Why can the -chrome handler not call both resolveFile and resolveURI for
the string? in fact, it seems that there is not much point in resolveURI, since
callers could just call resolveFile, and if that fails use NS_NewURI

Comment on attachment 170254 [details] [diff] [review]
Core commandline APIs and impl, version 2.2

one more thing: this interface is kinda unusable once mozilla uses unicode APIs
on windows (GetCommandLineW, CommandLineToArgvW), since then there is no char*
but a WCHAR*/PRUnichar*...

ok, now some implementation comments:

toolkit/components/commandlines/src/nsCommandLine.cpp
+nsCommandLine::FindFlag
+  NS_ENSURE_ARG(!aFlag.IsEmpty());

this seems a bit pointless to me...


why not declare the |PRInt32 f| where it is first used?

+    if (arg.Length() >= 2 && arg[0] == PRUnichar('-')) {

arg.First()?

+nsCommandLine::HandleFlagWithParam
+  if (found == mArgs.Count()) {

hm, I don't see how this can return .Count()... I think you want Count() - 1
here?

+nsCommandLine::ResolveFile
+  return CallQueryInterface(newfile, aResult);

ew, nsILocalFileMac is a nsIFile... why not:
  NS_ADDREF(*aResult = newfile);
  return NS_OK;

hm, several places in this function do that

+    // If it's a relative path, the Init is *going* to fail.

that's right. but that should not be different on the other platforms, as
that's documented in nsIFile... 

+nsCommandLine::ResolveURI
+      nsCOMPtr<nsILocalFile> lf (do_CreateInstance(NS_LOCAL_FILE_CONTRACTID));
+      NS_ENSURE_TRUE(lf, NS_ERROR_OUT_OF_MEMORY);
+
+      return io->NewFileURI(lf, aResult);

I don't see lf being initialized anywhere?

+      char* colon = PL_strchr(dup, ':');

I think you can use libc's strchr here... that one's likely to be faster

+    NS_ASSERTION(text.Length() == 0 || text.Last() == '\n', "Help text from
command line handlers should end in a newline.");

can you linebreak this somewhere?

toolkit/xre/nsAppRunner.cpp
+  rv = cmdline->GetHelpText(text);
+  if (NS_SUCCEEDED(rv))
+    printf("%s", text.get());

can we show some error message if this fails?

+      nsCOMPtr<nsICommandLineRunner> cmdLine
+	 (do_CreateInstance("@mozilla.org/toolkit/command-line;1"));
+      NS_ENSURE_TRUE(cmdLine, 1);

varying return values (instead of always 1) might make debugging easier,
maybe..

+      rv = dirSvc->Get(NS_OS_CURRENT_WORKING_DIR, NS_GET_IID(nsIFile),
+			getter_AddRefs(workingDir));

NS_GetSpecialDirectory?

+      /* Special-case services that need early access to the command
+	  line. */

ew... do the priorities from the category not suffice?

toolkit/xre/nsNativeAppSupportWin.cpp
+#define MOZ_DEBUG_DDE 1

guess you won't check that in?

+	     if ( windowID.Equals( "" ) ) {

.IsEmpty()

+    // Parse command line args according to MS spec

toolkit/mozapps/extensions/public/nsIExtensionManager.idl

hm, why does start and handleCommandLineArgs both need a nsICommandLine
instance?
When we get around to using the windows unicode APIs, we can add an InitW method
to nsICommandLineRunner. That interface is not meant to be frozen.

> hm, I don't see how this can return .Count()... I think you want Count() - 1
> here?

Good catch.

> +    // If it's a relative path, the Init is *going* to fail.
> 
> that's right. but that should not be different on the other platforms, as
> that's documented in nsIFile... 

The implementations do not match the documentation at all.

> +  rv = cmdline->GetHelpText(text);
> +  if (NS_SUCCEEDED(rv))
> +    printf("%s", text.get());
> 
> can we show some error message if this fails?

What kind? "Help information not available."?

> +      /* Special-case services that need early access to the command
> +	  line. */
> 
> ew... do the priorities from the category not suffice?

Not in this unfortunate case. Once you've started loading chrome windows (the
hidden window in particular) it's too late to go back and change locales.

> hm, why does start and handleCommandLineArgs both need a nsICommandLine
> instance?

I don't understand the question.
>> hm, why does start and handleCommandLineArgs both need a nsICommandLine
>> instance?
>I don't understand the question.

well, both start and handleCommandLineArgs take an nsICommandLine argument. why
is that needed? shouldn't it be sufficient to have one method dealing with
commandlines?
Comment on attachment 169981 [details] [diff] [review]
xremote workaround patch

r=caillon
Attachment #169981 - Flags: first-review?(caillon) → first-review+
Depends on: 277309
Attachment #170551 - Flags: first-review?(mscott)
This is the xulrunner default handler, which is also useful for sunbird and
probably NVU when that comes back to roost. It can safely be included with
firefox/tbird also, since those apps will not declare toolkit.defaultChromeURI
Attachment #170582 - Flags: first-review?(darin)
Attached patch Sunbird, rev. 1Splinter Review
Now we can get rid of that horrible, horrible hack in the sunbird main()
function!
Attachment #170586 - Flags: first-review?(mvl)
Comment on attachment 170582 [details] [diff] [review]
xulrunner default handler

So, I take it there is a reason why you didn't want to just make the code that
listens for browser.chromeURL listen to this new pref as well?
Comment on attachment 170586 [details] [diff] [review]
Sunbird, rev. 1

I don't think this will compile on windows... xre_main takes 3 parameters, no?
Comment on attachment 170255 [details] [diff] [review]
DOMI: Use runtime detection to support nsICmdLineHandler and nsICommandLineHandler

>+    catman.addCategoryEntry("command-line-handler",
>+                            "m-inspector",
>+                            INSPECTOR_CMDLINE_CONTRACTID, true, true);

Does the "m-" prefix signify anything special?
Benjamin thanks for taking on this big task. Are there any test builds available
that we can test these changes on? If not, maybe its worth while for you to cut
a quick branch, put all these changes on a branch and then we can either
individually make test builds on our own or we can have the release team spin up
some test builds for thunderbird and firefox. Then we can do some testing on the
changes to make sure we aren't breaking anything before the changes actually
land. We can also get our QA teams to take a look at the test builds as well. 

I'd like to avoid the bumpy large toolkit landings we've had in the past where
we have to spend sometime after things land getting things running again.

What do you think?
> I don't think this will compile on windows... xre_main takes 3 parameters, no?

Yes, stet that change, I'll just keep calling main(). No new patch unless mvl
asks for it.

> Does the "m-" prefix signify anything special?

The category entries are processes alphabetically, and per the handling
convention handlers with ordinary priority get prefixed with "m"

> Benjamin thanks for taking on this big task. Are there any test builds available
> that we can test these changes on? If not, maybe its worth while for you to cut
> a quick branch, put all these changes on a branch and then we can either

I have debug builds, but that's a huge download. I can look at making a branch.
Also, if you have a list of QA smoketests for command lines and DDE, I can run
these to make sure I haven't missed any important logic.
> So, I take it there is a reason why you didn't want to just make the code that
> listens for browser.chromeURL listen to this new pref as well?

There isn't any code in xulrunner that listens to browser.chromeURL any more.
That pref is now browser-specific (and could probably be hardcoded, but I'm a wuss).
Comment on attachment 170582 [details] [diff] [review]
xulrunner default handler

I know it's ugly, but for compatibility reasons, perhaps it'd be nice to
support browser.chromeURL if toolkit.defaultChromeURI is not specified.  Or,
will firefox override this default command line handler with its own when built
on top of xulrunner?
Attachment #170582 - Flags: first-review?(darin) → first-review+
Firefox will not declare toolkit.defaultChromeURI, so the generic handler will
not be used. There is specialized startup logic in nsBrowserContentHandler.js
which handles single-window browsing mode and other good stuff like that.
Scott and others, I have made the COMMANDLINES_20050109_BRANCH which contains
the changes for this bug, please do QA there as possible.
awesome. Pulling the branch right now....
Attachment #170255 - Flags: first-review?(caillon) → first-review+
Build error on the branch:

MOZILLA_SKIN_VERSION=\"1.5\"  -D_MOZILLA_CONFIG_H_ -DMOZILLA_CLIENT /cygdrive/c/
build/trees/cmdline/mozilla/xpfe/components/winhooks/nsWindowsHooks.cpp
nsWindowsHooks.cpp
c:/build/trees/cmdline/mozilla/xpfe/components/winhooks/nsWindowsHooks.cpp(54) :
 fatal error C1083: Cannot open include file: 'nsICmdLineService.h': No such fil
e or directory
make[6]: *** [nsWindowsHooks.obj] Error 2
make[6]: Leaving directory `/cygdrive/c/build/trees/cmdline/mozilla/xpfe/compone
nts/winhooks'
make[5]: *** [libs] Error 2
make[5]: Leaving directory `/cygdrive/c/build/trees/cmdline/mozilla/xpfe/compone
nts'
the winhooks fix was in my tree, it's now on the branch
I'll pick this back up when I get back home tonight (it's on my home machine).

One more error as I head out the door:

ERSION=\"1.5\"  -D_MOZILLA_CONFIG_H_ -DMOZILLA_CLIENT /cygdrive/c/build/trees/cm
dline/mozilla/toolkit/xre/nsNativeAppSupportWin.cpp
nsNativeAppSupportWin.cpp
c:/build/trees/cmdline/mozilla/toolkit/xre/nsNativeAppSupportWin.cpp(592) : erro
r C2446: '!=' : no conversion from 'unsigned int' to 'unsigned int (__stdcall ns
ICommandLineRunner::*)(void)'
        There are no conversions from integral values to pointer-to-member value
s
c:/build/trees/cmdline/mozilla/toolkit/xre/nsNativeAppSupportWin.cpp(592) : erro
r C2040: '!=' : 'unsigned int (__stdcall nsICommandLineRunner::*)(void)' differs
 in levels of indirection from 'unsigned int'
make[4]: *** [nsNativeAppSupportWin.obj] Error 2
make[4]: Leaving directory `/cygdrive/c/build/trees/cmdline/mozilla/toolkit/xre'

Benjamin fixed the build problem I reported in comment #45.

The next build problem after that is:

nsCommandLine.cpp
c:/build/trees/cmdline/mozilla/toolkit/components/commandlines/src/nsCommandLine
.cpp(604) : error C2552: 'closure' : non-aggregates cannot be initialized with i
nitializer list
make[2]: *** [nsCommandLine.obj] Error 2
make[2]: Leaving directory `/cygdrive/c/build/trees/cmdline/mozilla/toolkit/comp
Scott, I am perplexed by comment #46, it works for me on mac/linux/msvc.net2003

Could you try

-  HelpClosure closure = { aResult, catman };
+  HelpClosure closure = { aResult, (nsICategoryManager*) catman };

To see if MSVC6 might not be auto-casting properly? Otherwise, I suspect the
reference initializer, but I have no clue why that wouldn't compile.
Nevermind, Neil and I found a way to work around MSVC6 stupidity using an
explicit constructor.
Observations:

1) JS error on startup:
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED)
 [nsIPrefBranch.getCharPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  loc
ation: "JS frame :: file:///C:/build/trees/cmdline/mozilla/dist/bin/components/n
sDefaultCLH.js :: clh_handle :: line 81"  data: no]
************************************************************

2) Failure case: Passing a mailto url directly on the command line:
thunderbird.exe mailto:foo@foo.com

Everything else is checking out so far. I tested -addressbook, -compose, mailto
urls with spaces, and DDE requests for mailto urls. I still have some more test
to run but so far so good. 
The JS error is expected and should be thrown; I don't see any way to suppress
the debug output.

> 2) Failure case: Passing a mailto url directly on the command line:
> thunderbird.exe mailto:foo@foo.com

That doesn't work on trunk currently, because it was implemented on the branch
with #ifdefs, see nsAppShellService.cpp, revision 1.212.6.6, and see my comment
in nsMailDefaultHandler.js in the patch. I will have a followup patch which will
enable uri-handling, but that requires some additional work in the appstartup code.
Attachment #170050 - Flags: first-review?(bryner) → first-review?(mconnor)
Attachment #170551 - Flags: first-review?(mscott) → first-review+
Fixed on trunk, with the following bustage fixes:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/browser/components&command=DIFF_FRAMESET&file=nsBrowserContentHandler.js&rev1=1.2&rev2=1.3&root=/cvsroot
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/toolkit/xre&command=DIFF_FRAMESET&file=nsAppRunner.cpp&rev1=1.61&rev2=1.62&root=/cvsroot

If there are changes in command-line handling logic worthy of a bug, please open
a new bug instead of reopening this one.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.8beta
This broke extensions/layout-debug in a firefox build on windows:

c:/builds/moz-trunk/mozilla/extensions/layout-debug/src/nsLayoutDebugCLH.h(43) :
fatal error C10
83: Cannot open include file: 'nsICmdLineHandler.h': No such file or directory

nsICmdLineHandler.h doesn't exist in a firefox build anymore.
This supplementary patch fixes the ResolveURI method to try only absolute
pathnames before using the ioservice to resolve the URI.
Attachment #171653 - Flags: first-review?(darin)
Comment on attachment 171653 [details] [diff] [review]
Make ::ResolveURI work under all the necessary conditions [checked in]

r=darin (looks good to me)

Though, as I said on IRC, I think it may be good to similarly call Normalize in
ResolveFile on XP_WIN instead of using _fullpath there.

BTW, does ResolveFile handle UNC file paths (e.g., \\server\dir\file)?
Attachment #171653 - Flags: first-review?(darin) → first-review+
Comment on attachment 171653 [details] [diff] [review]
Make ::ResolveURI work under all the necessary conditions [checked in]

+  // First, we try to init the argument as an absolute file path. If this
doesn't
+  // work, it is an absolute or relative URI.

isn't a relative file path be more likely than a relative URI? (..\foo vs
../foo, on windows)
> isn't a relative file path be more likely than a relative URI? (..\foo vs
> ../foo, on windows)

The URL parser can deal with either of those properly with a file:// base URL. 
You might recall that I recently added code to nsURLHelperWin.cpp to support
backslashes in file:// URLs.
This is modeled pretty closely on the thunderbird patch above.
Attachment #171697 - Flags: first-review?(bsmedberg)
Comment on attachment 171697 [details] [diff] [review]
fix extensions/layout-debug

I'm a little worried that you don't pass an argument to
windowWatcher->openWindow. I suggest that you pass the commandline service as
an argument even if you don't plan to use it. See bug 277798 for the
inconsistencies in how windowwatcher handles null args.
Attachment #171697 - Flags: first-review?(bsmedberg) → first-review+
Attachment #171653 - Attachment description: Make ::ResolveURI work under all the necessary conditions → Make ::ResolveURI work under all the necessary conditions [checked in]
Not sure if this has been addressed already or if it should be handled in a new
bug (or even if it's the expected behavior), if that's the case, sorry for the spam.

External links without http:// prefix do not work anymore, for example clicking
a www.foo.com link in Thunderbird opens a blank new window. Also command prompt
commands firefox.exe www.foo.com (or .. foo.com) do not work. Firefox.exe
foo.com results in The file /C:/Program Files/Mozilla Firefox/foo.com cannot be
found. Please check the location and try again.

Same goes for all external apps as far as I can see, clicking a www.foo.com link
in mIRC (shoddy but the most popular irc-client) causes the same 'error'. The
file /D:/Programs/mIRC/www.foo.com cannot be found. Please check the location
and try again.
As far as I can tell, this breaks my cygwin/Mingw/Win32 build with the following


Creating Resource file: module.res
/cygdrive/e/mozilla/source/mozilla/build/cygwin-wrapper windres -O coff --use-te
mp-file -DOSTYPE=\"WINNT5.1\" -DOSARCH=\"WINNT\" -DHAVE_DEPENDENT_LIBS --include
-dir ../../../../dist/include/xpcom --include-dir ../../../../dist/include/strin
g --include-dir ../../../../dist/include/necko --include-dir ../../../../dist/in
clude/unicharutil --include-dir ../../../../dist/include/toolkitcomps --include-
dir ../../../../dist/include --include-dir ../../../../dist/include/nspr --inclu
de-dir . -o module.res /cygdrive/e/mozilla/source/mozilla/obj/thunderbird/toolki
t/components/commandlines/src/module.rc
rm -f commandlines.dll
/cygdrive/e/mozilla/source/mozilla/build/cygwin-wrapper g++ -march=athlon-xp -mm
mx -msse -m3dnow -mno-cygwin -shared -o commandlines.dll  nsCommandLine.o   ./mo
dule.res          -L../../../../dist/lib -lxpcom -lxpcom_core -L../../../../dist
/bin -L../../../../dist/lib -lnspr4 -lplc4 -lplds4 ../../../../dist/lib/libunich
arutil_s.a   -lm
../../../../dist/lib/libunicharutil_s.a(nsUnicharUtils.o)(.text+0x4b6): In funct
ion `Z11ToLowerCaseRK9nsAStringRS_':
e:/mozilla/source/mozilla/intl/unicharutil/util/nsUnicharUtils.cpp:173: undefine
d reference to `_imp___ZN9nsAString9SetLengthEj@8'
../../../../dist/lib/libunicharutil_s.a(nsUnicharUtils.o)(.text+0x65c): In funct
ion `Z11ToUpperCaseRK9nsAStringRS_':
e:/mozilla/source/mozilla/intl/unicharutil/util/nsUnicharUtils.cpp:257: undefine
d reference to `_imp___ZN9nsAString9SetLengthEj@8'
../../../../dist/lib/libunicharutil_s.a(nsUnicharUtils.o)(.text+0x6f5): In funct
ion `Z29CaseInsensitiveFindInReadableRK9nsAStringR17nsReadingIteratorIwES4_':
e:/mozilla/source/mozilla/intl/unicharutil/util/nsUnicharUtils.cpp:265: undefine
d reference to `_imp___Z14FindInReadableRK9nsAStringR17nsReadingIteratorIwES4_RK
18nsStringComparator'
../../../../dist/lib/libunicharutil_s.a(nsUnicharUtils.o)(.text$_ZN9nsAString12B
eginWritingER17nsWritingIteratorIwE[nsAString::BeginWriting(nsWritingIterator<wc
har_t>&)]+0xb): In function `ZNK17nsReadingIteratorIwE3getEv':
e:/mozilla/source/mozilla/obj/thunderbird/intl/unicharutil/util/../../../dist/in
clude/string/nsStringIterator.h: undefined reference to `_imp___ZN9nsAString17Ge
tWritableBufferEPPw@8'
../../../../dist/lib/libunicharutil_s.a(nsUnicharUtils.o)(.text$_ZN9nsAString10E
ndWritingER17nsWritingIteratorIwE[nsAString::EndWriting(nsWritingIterator<wchar_
t>&)]+0xb):e:/mozilla/source/mozilla/obj/thunderbird/intl/unicharutil/util/../..
/../dist/include/string/nsStringIterator.h: undefined reference to `_imp___ZN9ns
AString17GetWritableBufferEPPw@8'
../../../../dist/lib/libunicharutil_s.a(nsUnicharUtils.o)(.text$_ZN11nsSubstring
12BeginWritingERPw[nsSubstring::BeginWriting(wchar_t*&)]+0x8):e:/mozilla/source/
mozilla/obj/thunderbird/intl/unicharutil/util/../../../dist/include/string/nsStr
ingIterator.h: undefined reference to `_imp___ZN11nsSubstring13EnsureMutableEv@4
'
../../../../dist/lib/libunicharutil_s.a(nsUnicharUtils.o)(.text$_ZNK9nsAString12
BeginReadingER17nsReadingIteratorIwE[nsAString::BeginReading(nsReadingIterator<w
char_t>&) const]+0xb):e:/mozilla/source/mozilla/obj/thunderbird/intl/unicharutil
/util/../../../dist/include/string/nsStringIterator.h: undefined reference to `_
imp___ZNK9nsAString17GetReadableBufferEPPKw@8'
../../../../dist/lib/libunicharutil_s.a(nsUnicharUtils.o)(.text$_ZNK9nsAString10
EndReadingER17nsReadingIteratorIwE[nsAString::EndReading(nsReadingIterator<wchar
_t>&) const]+0xb):e:/mozilla/source/mozilla/obj/thunderbird/intl/unicharutil/uti
l/../../../dist/include/string/nsStringIterator.h: undefined reference to `_imp_
__ZNK9nsAString17GetReadableBufferEPPKw@8'
../../../../dist/lib/libunicharutil_s.a(nsUnicharUtils.o)(.text$_Z14CallGetServi
ceI17nsICaseConversionEjPKcPPT_[unsigned int CallGetService<nsICaseConversion>(c
har const*, nsICaseConversion**)]+0x77):e:/mozilla/source/mozilla/obj/thunderbir
d/intl/unicharutil/util/../../../dist/include/string/nsStringIterator.h: undefin
ed reference to `_imp___Z14CallGetServicePKcRK4nsIDPPv'
../../../../dist/lib/libunicharutil_s.a(nsUnicharUtils.o)(.text$_ZN8nsCOMPtrI18n
sIObserverServiceE36assign_from_gs_contractid_with_errorERK33nsGetServiceByContr
actIDWithErrorRK4nsID[nsCOMPtr<nsIObserverService>::assign_from_gs_contractid_wi
th_error(nsGetServiceByContractIDWithError const&, nsID const&)]+0x12):e:/mozill
a/source/mozilla/obj/thunderbird/intl/unicharutil/util/../../../dist/include/str
ing/nsStringIterator.h: undefined reference to `_imp___ZNK33nsGetServiceByContra
ctIDWithErrorclERK4nsIDPPv@12'
collect2: ld returned 1 exit status
make[6]: *** [commandlines.dll] Error 1
make[6]: Leaving directory `/cygdrive/e/mozilla/source/mozilla/obj/thunderbird/t
oolkit/components/commandlines/src'
make[5]: *** [libs] Error 2
make[5]: Leaving directory `/cygdrive/e/mozilla/source/mozilla/obj/thunderbird/t
oolkit/components/commandlines'
make[4]: *** [libs] Error 2
make[4]: Leaving directory `/cygdrive/e/mozilla/source/mozilla/obj/thunderbird/t
oolkit/components'
make[3]: *** [libs] Error 2
make[3]: Leaving directory `/cygdrive/e/mozilla/source/mozilla/obj/thunderbird/t
oolkit'
make[2]: *** [libs] Error 2
make[2]: Leaving directory `/cygdrive/e/mozilla/source/mozilla/obj/thunderbird'
make[1]: *** [alldep] Error 2
make[1]: Leaving directory `/cygdrive/e/mozilla/source/mozilla/obj/thunderbird'
make: *** [alldep] Error 2
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #170050 - Flags: first-review?(mconnor) → first-review+
David, if there is bustage on cygwin, please file a new bug on bsmedberg.  Also
keep in mind that cygwin is a tier-3 platform and doesn't get blocker-like
consideration.
Status: REOPENED → RESOLVED
Closed: 19 years ago19 years ago
Resolution: --- → FIXED
(In reply to comment #62)
> David, if there is bustage on cygwin, please file a new bug on bsmedberg.  Also
> keep in mind that cygwin is a tier-3 platform and doesn't get blocker-like
> consideration.

Bug #279079 filed.

By the way, where does one find information on what is considered tier-1, tier-2
and tier-3 platforms?
http://www.mozilla.org/build/faq.html#supported lists the various build platforms.
Blocks: 279093
Checked in a bustage fix for xulrunner on windows (crash on startup), 

Index: nsNativeAppSupportWin.cpp
===================================================================
RCS file: /cvsroot/mozilla/toolkit/xre/nsNativeAppSupportWin.cpp,v
retrieving revision 1.15
diff -u -r1.15 nsNativeAppSupportWin.cpp
--- nsNativeAppSupportWin.cpp   17 Jan 2005 18:50:16 -0000      1.15
+++ nsNativeAppSupportWin.cpp   20 Jan 2005 15:01:57 -0000
@@ -334,10 +334,10 @@
 
 void
 nsNativeAppSupportWin::CheckConsole() {
-    for ( int i = 1; i < __argc; i++ ) {
-        if ( strcmp( "-console", __argv[i] ) == 0
+    for ( int i = 1; i < gArgc; i++ ) {
+        if ( strcmp( "-console", gArgv[i] ) == 0
              ||
-             strcmp( "/console", __argv[i] ) == 0 ) {
+             strcmp( "/console", gArgv[i] ) == 0 ) {
             // Users wants to make sure we have a console.
             // Try to allocate one.
             BOOL rc = ::AllocConsole();

I think this caused bug 279164
I was pleased and suprised to find some (XP_BEOS) code had been written
although it had obviously never been built (I think we've got some tinderboxes
on the way). There are a few build errors (missing include, and a few syntax
errors) which are fixed by this patch.

With this it seems to open links (relative, absolute and http://www.site.com)
from the command line correctly - something that the BeOS port has never even
done before! Thanks very much for this!
(In reply to comment #58)
> Created an attachment (id=171697) [edit]
> fix extensions/layout-debug
> 
> This is modeled pretty closely on the thunderbird patch above.

Firefox builds with layout-debug enabled don't work, the command-line handler
screws things up somehow. I'm trying to figure out why.
Okay, one obvious problem is that we do

+    rv = aCmdLine->HandleFlag(NS_LITERAL_STRING("layoutdebug"),
+                              PR_FALSE, &found);
+    NS_ENSURE_SUCCESS(rv, rv);
+

And then we never check found, we just launch the layout debugger no matter what :-)
Attached patch obvious fixSplinter Review
Note that if you start with -layoutdebug, then the layout debugger still
doesn't actually work, but at least builds without that command-line option
will now start.
Attachment #172215 - Flags: first-review?(bryner)
Attachment #172215 - Flags: first-review?(bryner) → first-review+
Attachment #172207 - Attachment description: Fix building under BeOS → Fix building under BeOS [checked in]
So.. this checkin causes assertions on startup:

###!!! ASSERTION: Relative paths not allowed: 'Error', file
/home/bzbarsky/mozilla/debug/mozilla/xpcom/io/nsLocalFileUnix.cpp, line 287

I just ran 'firefox http://whatever'

Benjamin, would you like a new bug on that?
bz, yes please (we should probably remove that assertion, since we expect the
method to fail if we're feeding it a relative path).
yes, the assertion should be removed. I should not have added it. (it also
causes assertions in helper app code)
Attachment #170586 - Flags: first-review?(mvl)
Benjamin, is there anyway we can get rid of this exception on startup:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED)
 [nsIPrefBranch.getCharPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  loc
ation: "JS frame :: file:///C:/build/trees/cmdline/mozilla/dist/bin/components/n
sDefaultCLH.js :: clh_handle :: line 81"  data: no]
************************************************************

even if it's just a try/catch, I don't think we should really be throwing the
exception if it's to be expected. 
(In reply to comment #75)
> Benjamin, is there anyway we can get rid of this exception on startup:
> 
> ************************************************************
> * Call to xpconnect wrapped JSObject produced this error:  *
> [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED)
>  [nsIPrefBranch.getCharPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  loc
> ation: "JS frame :: file:///C:/build/trees/cmdline/mozilla/dist/bin/components/n
> sDefaultCLH.js :: clh_handle :: line 81"  data: no]
> ************************************************************
> 
> even if it's just a try/catch, I don't think we should really be throwing the
> exception if it's to be expected. 
> 

New bug? This one is marked fixed. Was it on the 23rd? Thanks.
There is already a bug about the intentional exception.
Depends on: 303836
Blocks: 87127
Comment on attachment 170255 [details] [diff] [review]
DOMI: Use runtime detection to support nsICmdLineHandler and nsICommandLineHandler

>+    if (args || cmdLine.handleFlag("inspector", false)) {
>+      var wwatch = Components.classes["@mozilla.org/embedcomp/window-watcher;1"]
>+                             .getService(nsIWindowWatcher);
>+      wwatch.openWindow(null, "chrome://inspector/content/", "_blank",
>+                        "chrome,dialog=no", args);
>+    }
Bug 340233 filed on the two bugs with this code.
Depends on: 340233
(In reply to comment #27)
> When we get around to using the windows unicode APIs, we can add an InitW
> method
> to nsICommandLineRunner. That interface is not meant to be frozen.

I guess this issue can be dealt with in bug 333805. bug 359200 and bug 282285 seem also related.
(In reply to comment #48)
>Nevermind, Neil and I found a way to work around MSVC6 stupidity using an
>explicit constructor.
IMHO it's technically accurate because HelpClosure wasn't a POD type.
Flags: in-testsuite?
Component: XRE Startup → Startup and Profile System
QA Contact: nobody → startup
You need to log in before you can comment on or make changes to this bug.