Closed Bug 855124 Opened 9 years ago Closed 3 years ago

GCLI should provide a way to open add-on preferences

Categories

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

21 Branch
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED INCOMPLETE

People

(Reporter: wraithan, Assigned: sykopomp)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attachment #731686 - Flags: review?(jwalker)
Assignee: nobody → sykopomp
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.
Attachment #731686 - Flags: review?(jwalker)
Attached patch v2 (obsolete) — Splinter 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. 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.
Attachment #731686 - Attachment is obsolete: true
Attachment #733163 - Flags: review?(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;
Attachment #733163 - Flags: review?(jwalker)
Depends on: 792815
Attached patch v3Splinter Review
This fix adds a dependency on bug 792815 due to the .createPromise() thing. Previously, the patch applied with an automatic merge.
Attachment #733163 - Attachment is obsolete: true
Attachment #733264 - Flags: review?(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?
Attachment #733264 - Flags: review?(jwalker)
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! :))
Status: NEW → ASSIGNED
Blocks: GCLICMD
Product: Firefox → DevTools
Closing these as incomplete because component is going away. https://bugzilla.mozilla.org/show_bug.cgi?id=1491875
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → INCOMPLETE
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.