276588 - Rework toolkit command-line handling

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Benjamin Smedberg]

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)

Comment 1

[Benjamin Smedberg]

Attachment: Core commandline APIs and impl, with startup-hookup

darin, let me know if you're not going to get to these reviews by alpha6.

Comment 2

[Benjamin Smedberg]

Attachment: xremote workaround patch

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.

Comment 3

[James Ross]

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 4

[Boris Zbarsky [:bzbarsky]]

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?

Comment 5

[Benjamin Smedberg]

> 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.

Comment 6

[Darin Fisher]

I won't have a chance to review this until Jan 3rd.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> 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...

Comment 8

[Benjamin Smedberg]

Attachment: Core commandline APIs and impl with startup-hookup and win32 DDE remoting (version 2)

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.

Comment 9

[Benjamin Smedberg]

Attachment: Firefox command-line handlers fixup, version 1

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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 11

[Benjamin Smedberg]

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.

Comment 12

[Benjamin Smedberg]

Attachment: Core commandline APIs and impl, version 2.1

Comment 13

[Darin Fisher]

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.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> general comment about this interface: since arguments are ASCII valued,

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

Comment 15

[Darin Fisher]

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

whoops, you're of course right.  i was thinking in terms of flags :-/

Comment 16

[Darin Fisher]

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.

Comment 17

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

>+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?

Comment 18

[Darin Fisher]

> 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 :(

Comment 19

[Benjamin Smedberg]

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.

Comment 20

[Benjamin Smedberg]

Attachment: Core commandline APIs and impl, version 2.2

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).

Comment 21

[Benjamin Smedberg]

Attachment: DOMI: Use runtime detection to support nsICmdLineHandler and nsICommandLineHandler

Comment 22

[Darin Fisher]

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 23

[Darin Fisher]

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

Comment 24

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

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

Comment 25

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

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 26

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

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?

Comment 27

[Benjamin Smedberg]

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.

Comment 28

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

>> 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 29

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 169981 [details] [diff] [review]
xremote workaround patch

r=caillon

Comment 30

[Benjamin Smedberg]

Attachment: Changes for thunderbird, rev 1

Comment 31

[Benjamin Smedberg]

Attachment: xulrunner default handler

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

Comment 32

[Benjamin Smedberg]

Attachment: Sunbird, rev. 1

Now we can get rid of that horrible, horrible hack in the sunbird main()
function!

Comment 33

[Darin Fisher]

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 34

[Darin Fisher]

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 35

[Christopher Aillon (sabbatical, not receiving bugmail)]

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?

Comment 36

[Scott MacGregor]

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?

Comment 37

[Benjamin Smedberg]

> 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.

Comment 38

[Benjamin Smedberg]

> 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 39

[Darin Fisher]

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?

Comment 40

[Benjamin Smedberg]

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.

Comment 41

[Benjamin Smedberg]

Scott and others, I have made the COMMANDLINES_20050109_BRANCH which contains
the changes for this bug, please do QA there as possible.

Comment 42

[Scott MacGregor]

awesome. Pulling the branch right now....

Comment 43

[Scott MacGregor]

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'

Comment 44

[Benjamin Smedberg]

the winhooks fix was in my tree, it's now on the branch

Comment 45

[Scott MacGregor]

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'

Comment 46

[Scott MacGregor]

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

Comment 47

[Benjamin Smedberg]

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.

Comment 48

[Benjamin Smedberg]

Nevermind, Neil and I found a way to work around MSVC6 stupidity using an
explicit constructor.

Comment 49

[Scott MacGregor]

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:[e-mail]

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. 

Comment 50

[Benjamin Smedberg]

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:[e-mail]

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.

Comment 51

[Benjamin Smedberg]

mconnor: Instead of reviewing the attached patch, please review the firefox
changes that were committed on the branch:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=COMMANDLINES_20050109_BRANCH&branchtype=match&dir=mozilla%2Fbrowser&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=all&mindate=&maxdate=&cvsroot=%2Fcvsroot

Comment 52

[Benjamin Smedberg]

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.

Comment 53

[Darin Fisher]

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.

Comment 54

[Benjamin Smedberg]

Attachment: Make ::ResolveURI work under all the necessary conditions [checked in]

This supplementary patch fixes the ResolveURI method to try only absolute
pathnames before using the ioservice to resolve the URI.

Comment 55

[Darin Fisher]

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)?

Comment 56

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

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)

Comment 57

[Darin Fisher]

> 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.

Comment 58

[Brian Ryner (not reading)]

Attachment: fix extensions/layout-debug

This is modeled pretty closely on the thunderbird patch above.

Comment 59

[Benjamin Smedberg]

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.

Comment 60

[TimoT]

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.

Comment 61

[David G King]

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

Comment 62

[Mike Connor [:mconnor]]

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.

Comment 63

[David G King]

(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?

Comment 64

[Mike Connor [:mconnor]]

http://www.mozilla.org/build/faq.html#supported lists the various build platforms.

Comment 65

[Axel Hecht]

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();

Comment 66

[Doug Wright]

I think this caused bug 279164

Comment 67

[Simon]

Attachment: Fix building under BeOS [checked in]

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!

Comment 68

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 69

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 :-)

Comment 70

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: obvious fix

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.

Comment 71

[Boris Zbarsky [:bzbarsky]]

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?

Comment 72

[Benjamin Smedberg]

bz, yes please (we should probably remove that assertion, since we expect the
method to fail if we're feeding it a relative path).

Comment 73

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

yes, the assertion should be removed. I should not have added it. (it also
causes assertions in helper app code)

Comment 74

[Boris Zbarsky [:bzbarsky]]

Bug 280351 filed.

Comment 75

[Scott MacGregor]

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. 

Comment 76

[Worcester12345]

(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.

Comment 77

[Benjamin Smedberg]

There is already a bug about the intentional exception.

Comment 78

[[e-mail]]

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.

Comment 79

[Jungshik Shin]

(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.

Comment 80

[[e-mail]]

(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.