855124 - GCLI should provide a way to open add-on preferences

Status: RESOLVED INCOMPLETE

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

Description

[Wraithan (Chris McDonald) [:wraithan]]

You can list, enable, and disable currently. It would be nice if I could open preferences from there as well instead of having to open about:addons.

Comment 1

[Josh Marchán]

Attachment: `addon configure` command implemented

Comment 2

[Joe Walker [:jwalker]]

Comment on attachment 731686 [details] [diff] [review]
`addon configure` command implemented

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

::: browser/devtools/commandline/BuiltinCommands.jsm
@@ +310,5 @@
> +
> +    gcli.addCommand({
> +      name: "addon configure",
> +      description: gcli.lookup("addonConfigureDesc"),
> +      params: [nameParameter],

Ug. I raised bug 857520 - GCLI addon commands should use a "addon" type rather than an object literal

So we can do
    params:[ name:"name", type:"addon" ]
here and elsewhere.

@@ +330,5 @@
> +            window.switchToTabHavingURI(
> +              addon.optionsURL, true);
> +            break;
> +          default:
> +            message = gcli.lookupFormat("addonNotConfigurable", [name]);

Or reject the promise with this string?

@@ +332,5 @@
> +            break;
> +          default:
> +            message = gcli.lookupFormat("addonNotConfigurable", [name]);
> +          }
> +          this.resolve(message);

Which would mean that we wouldn't need 'message'.

@@ +335,5 @@
> +          }
> +          this.resolve(message);
> +        }
> +        let deferred = context.defer();
> +        AddonManager.getAllAddons(configure.bind(deferred, args.name));

Having |this| be a deferred seems unexpected. Is there why's it like this?

::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
@@ +587,5 @@
>  addonDisabled=%S disabled.
>  
> +# LOCALIZATION NOTE (addonConfigureDesc) A very short description of the
> +# 'addon configure' command.
> +addonConfigureDesc=Open the preferences dialog for the specified add-on.

Nit: GCLI descriptions are titles, and we try to avoid periods at the end of titles.

Comment 3

[Josh Marchán]

Attachment: v2

I don't know why 'this' was considered appropriate for the deferred. In this particular case, I looked at what 'addon enable/disable' were doing. I've shifted the deferred into a proper argument, but perhaps it would be prudent to make sure this change is done to those commands as well.

I've also done the message/.reject() thing, and fixed addonNotConfigurable's text.

I understood that [nameParameter] should stay for now, and let the other ticket take care of it.

Comment 4

[Joe Walker [:jwalker]]

Comment on attachment 733163 [details] [diff] [review]
v2

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

> I don't know why 'this' was considered appropriate for the deferred.
> In this particular case, I looked at what 'addon enable/disable' were doing.

No idea how that got in, but you're right.
So, totally optional, but for bonus points you could fix that code too. No need for a separate bug.

::: browser/devtools/commandline/BuiltinCommands.jsm
@@ +331,5 @@
> +            break;
> +          default:
> +            deferred.reject(gcli.lookupFormat("addonNotConfigurable", [name]));
> +          }
> +          deferred.resolve();

There's a bug here in that we may reject and then resolve the promise, right?

@@ +335,5 @@
> +          deferred.resolve();
> +        }
> +        let deferred = context.defer();
> +        AddonManager.getAllAddons(configure.bind(null, deferred, args.name));
> +        return deferred.promise;

I think you could also do this, which might be simpler?

    let deferred = context.defer();
    
    AddonManager.getAllAddons(function() {
      let addon = findAddon(args.name, addons);
      ...
      deferred.resolve();
    });
    
    return deferred.promise;

Comment 5

[Josh Marchán]

Attachment: v3

This fix adds a dependency on bug 792815 due to the .createPromise() thing. Previously, the patch applied with an automatic merge.

Comment 6

[Joe Walker [:jwalker]]

Comment on attachment 733264 [details] [diff] [review]
v3

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

Thanks.
So we also need to add some tests. I think it should be simple to extend browser_cmd_addon.js with some extra cases, and an exec where we check that there is a new window open.

I'm assuming that you don't have access to try? I'll get that setup for you while you add some tests, ok?

Comment 7

[Josh Marchán]

ah good, I wasn't sure about the testing situation with gcli commands -- take your time, I'm flying out for a couple of days and may not hack on stuff until Sunday or Monday.

I've never used try, so I don't think I have access to it (I don't even know where it is! :))

Comment 8

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Closing these as incomplete because component is going away. https://bugzilla.mozilla.org/show_bug.cgi?id=1491875