1080302 - Use double-dash options in Firefox's help message

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Nicholas Nethercote [inactive]]

Firefox accepts options in:
- single-dash form (e.g. -safe-mode);
- double-dash form (e.g. --safe-mode);
- and on Windows, forward-slash form (e.g. /safe-mode).

This is true for both long options and short options (e.g. -P).

The help message only advertises the single-dash form, though, which is kind of old-fashioned and strange for long options. So we should change the help message to use double-dash form for long options.

And then we should change |mach run| to use double-dash forms for -remote, -foreground, and -noprofile.

Comment 1

[Mike Hommey [:glandium]]

This is where those strings are:
http://dxr.mozilla.org/mozilla-central/search?q=helpInfo&case=true

Comment 2

[Mike Hommey [:glandium]]

There is also:
http://dxr.mozilla.org/mozilla-central/search?q=GetHelpInfo&case=true

Comment 3

[Nicholas Nethercote [inactive]]

And there's a bunch at http://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#1507.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Convert all --foo options to -foo options (but both are still accepted)

In this patch I've taken the aggressive option: I tried to change every -foo
arg to --foo. I used grep heavily for this.

It's possible I've missed some, but in theory that shouldn't
matter since both forms are allowed. Except... there are a few places that
roll their own option parsing, and these places are generally less flexible
than the main option parsing routines. I've fixed all the ones I could find,
but it's possible I missed some.

[Hmm, I just realized I haven't considered .py files. There'll be a bunch more
needed there. Sigh.]

A more conservative option would be to just change -foo options that appear in
printed messages (e.g. help messages, error messages) and comments, but leave
alone -foo options in code.

glandium, I'm asking you for feedback because I figure you'll recognize many of
these obscure options.

I did a full try run yesterday with a patch that 95% matches this one, and it
was all green.

Here is a list of all the options I changed, grouped by how they are
registered. I've attached comments to the notable ones.

> Ones registered with CheckArg():
> -appomni
> -console      * in nsNativeAppSupportWin.cpp I added support for --console
> -createprofile
> -dump-args
> -foreground
> -greomni
> -help
> -install-xpi  * I removed the single reference, which was in a comment
> -install
> -metro-update
> -migration
> -new-instance
> -no-remote
> -offline
> -override
> -process-updates
> -ProfileManager
> -profile     * in gtk/webapprt.cpp I added support for --profile
>              * in win/webapprt.cpp I added support for --profile
>              * in GeckoApp.java, added support for --profile
> -purgecaches
> -register
> -requestPending
> -reset-profile
> -safe-mode
> -UILocale
> -version
> 
> Ones registered with handleFlagWithParam():
> -start-debugger-server
> --runapp            * all uses were already --runapp
> -screen
> -chrom
> -remote
> -new-window
> -new-tab
> -private-window
> -search
> -url                * in MetroContracts.cpp I added support for --url
> -file
> -width
> -height
> -alert
> -webapp
> -recording
> -recording-output
> -reftest
> -tps
> -tpsphase
> -tpslogfile
> -print-xpcom-dir
> -print-xpcom-dirlist
> 
> Ones registered with handleFlag():
> --ignore-unused-engines   * help msg was already double-dash
> -oop
> -browser
> -preferences
> -silent
> -private
> -osint
> -setDefaultBrowser
> -jsconsole          * has two handlers
> -jsdebugger
> -webide
> -reftestnocache
> -reftestskipslowtests
> -marionette
> 
> Other:
> -remove         * in gtk/webapprt.cpp I added support for --remove
> --a11y          * one mention in a comment in testing/mochitest/redirect.html
> -bookmark       * used in several .java files, but I couldn't find a handler
>                   anywhere
> 
> - browser/metro/shell/commandexecutehandler/CommandExecuteHandler.cpp has
> --metro-restart, --metro-update, --desktop-restart, --launchmetro, which are
>  already in double-dash form. Plus -Embedding which is in single-dash and I
>  left alone because it looks system-y.

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8502940 [details] [diff] [review]
Convert all --foo options to -foo options (but both are still accepted)

Review of attachment 8502940 [details] [diff] [review]:
-----------------------------------------------------------------

See below for some of the most obvious problems I found. To make this smoother, I think it should be split in several parts:
- First part adds support for double-dash arguments to things that only support single-dash currently.
- Second part changes all help messages to show double-dash arguments
- Third part changes internal option passing to use double-dash arguments. The latter would need to be reviewed by relevant people for the impacted components (e.g. cf. below, Marco Castelluccio for webapprt)

::: browser/components/shell/nsWindowsShellService.cpp
@@ +149,5 @@
>  
>  #define APP_REG_NAME L"Firefox"
>  #define VAL_FILE_ICON "%APPPATH%,1"
> +#define VAL_OPEN "\"%APPPATH%\" --osint --url \"%1\""
> +#define OLD_VAL_OPEN "\"%APPPATH%\" --requestPending --osint --url \"%1\""

You can't change this that way. See changeset 09362c5dceaf that added OLD_VAL_OPEN, it's apparently used for transition from an old value to new one.

::: mobile/android/base/GeckoApp.java
@@ +1146,5 @@
>                          profileName = m.group(1);
>                      }
>                  }
>  
> +                if (args.contains("--profile")) {

args is a string, not a list of strings. So in practice, this means the existing code is going to be happy with --profile. (It also means the existing code is likely very buggy)

::: python/mozbuild/mozbuild/mach_commands.py
@@ +798,2 @@
>          help='Do not pass the -foreground argument by default on Mac.')
> +    @CommandArgument('--noprofile', '-n', action='store_true', group=prog_group,

Those should probably be changed independently.

::: toolkit/components/remote/nsXRemoteService.cpp
@@ +276,5 @@
>      FindExtensionParameterInCommand("DESKTOP_STARTUP_ID",
>                                      cmd, '\n',
>                                      &desktopStartupID);
>  
> +    const char* argv[3] = {"dummyappname", "--remote", aCommand};

I'm killing this :)

::: toolkit/webapps/WebappOSUtils.jsm
@@ +364,5 @@
>        let process = Cc["@mozilla.org/process/util;1"]
>                        .createInstance(Ci.nsIProcess);
>  
>        process.init(exeFile);
> +      process.runAsync(["--remove"], 1, (aSubject, aTopic) => {

This may not work properly and might need to stay -remove forever: I think this can be used to invoke any old version of the webapprt, including those that only support -remove. Better check with Marco Castelluccio.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Add support for --foo options where it's not already present

I added support for /foo forms where appropriate, except in |mach run|... I
could do it there too if you think it's worthwhile.

I left the horrible -profile handling in GeckoApp.java unchanged, since it will
match -profile or --profile. Let sleeping dogs lie.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Convert all mentions of -foo options to --foo options in comments, help messages and error messages

This is all the safe stuff: help messages, error messages, comments.

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Convert -foo options to --foo in |mach run|

Comment 9

[Nicholas Nethercote [inactive]]

I'm going to skip the harder stuff (actual code changes) for the moment.

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8503149 [details] [diff] [review]
(part 1) - Add support for --foo options where it's not already present

Review of attachment 8503149 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/mach_commands.py
@@ +848,5 @@
>  
>          if not background and sys.platform == 'darwin':
>              args.append('-foreground')
>  
> +        if '-profile' not in params and '--profile' not in params and '-P' not in params and not noprofile:

maybe:
  if all(p not in params for p in ('-profile', '--profile', '-P')) and not noprofile:

::: webapprt/win/webapprt.cpp
@@ +447,5 @@
>    // Check if the runtime was executed with the "-profile" argument
>    for (int i = 1; i < argc; i++) {
> +    if (!strcmp(argv[i], "-profile") ||
> +        !strcmp(argv[i], "--profile") ||
> +        !strcmp(argv[i], "/profile")) {

Not sure we need to go all the way to support /arg when it's not currently supported.

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 8503151 [details] [diff] [review]
(part 2) - Convert all mentions of -foo options to --foo options in comments, help messages and error messages

Review of attachment 8503151 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/gaia/run-b2g.c
@@ +39,3 @@
>      fflush(stdout);
>      fflush(stderr);
>      execle(full_path, full_path, "-profile", full_profile_path, NULL, envp);

Maybe add a comment that the intent is to switch to --args and that for now it keeps using -arg even though it says --arg and that's not an error in the printf, but rather one in the exec.

::: browser/components/nsBrowserContentHandler.js
@@ +419,3 @@
>          // NS_ERROR_ABORT so that the xremote code knows to return a failure
>          // back to the handling code.
>          throw NS_ERROR_ABORT;

I bitrotted you :)

@@ +547,2 @@
>  #endif
> +             "  --search     <term> Search <term> with your default search engine.\n",

Can you remove those spaces between --search and <term> and realign?

::: browser/components/shell/nsWindowsShellService.cpp
@@ +97,5 @@
>  //   as aliases to the class:
>  //
>  //   HKCU\SOFTWARE\Classes\FirefoxHTML\
>  //     DefaultIcon                      (default)         REG_SZ     <apppath>,1
> +//     shell\open\command               (default)         REG_SZ     <apppath> --osint --url "%1"

mmmm changing that without changing the related code kind of makes the comment untruthful as to what the current values are expected to be in the registry.

::: toolkit/xre/nsAppRunner.cpp
@@ +3268,5 @@
>    }
>  
>    ar = CheckArg("safe-mode", true);
>    if (ar == ARG_BAD) {
> +    PR_fprintf(PR_STDERR, "Error: argument --safe-mode is invalid when argument --osint is specified\n");

I wonder why we sometimes use PR_fprintf and sometimes Output. Especially when PR_fprintf will print nothing on non-debug builds because firefox is not attached to a console on windows, and Output does open a message box instead, but that's unrelated to this bug.

Comment 12

[Nicholas Nethercote [inactive]]

> Not sure we need to go all the way to support /arg when it's not currently supported.

I'll keep it just to make things as consistent as possible with the main option handling code.

Comment 13

[Nicholas Nethercote [inactive]]

> ::: browser/components/shell/nsWindowsShellService.cpp
> @@ +97,5 @@
> >  //   as aliases to the class:
> >  //
> >  //   HKCU\SOFTWARE\Classes\FirefoxHTML\
> >  //     DefaultIcon                      (default)         REG_SZ     <apppath>,1
> > +//     shell\open\command               (default)         REG_SZ     <apppath> --osint --url "%1"
> 
> mmmm changing that without changing the related code kind of makes the
> comment untruthful as to what the current values are expected to be in the
> registry.

Ok, I'll revert all the changes in this file.

Comment 15

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a02e1ebafc13
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb44ac1b1f1e
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0354403d695

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a02e1ebafc13
https://hg.mozilla.org/mozilla-central/rev/cb44ac1b1f1e
https://hg.mozilla.org/mozilla-central/rev/e0354403d695