The default bug view has changed. See this FAQ.

GCLI needs a command to control addons

RESOLVED FIXED in Firefox 16

Status

()

Firefox
Developer Tools: Console
P3
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jwalker, Assigned: miker)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [gclicommands])

Attachments

(1 attachment, 3 obsolete attachments)

> plugins list
> plugins enable <name> --global
> plugins disable <name> --global

(or something)
Triage. Filter on PEGASUS.
Priority: -- → P3
No longer blocks: 659052
GCLI Triage.
Target Milestone: --- → Firefox 15
Blocks: 745773
No longer blocks: 745773
Target Milestone: Firefox 15 → Firefox 16
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
Created attachment 637079 [details]
Command
Attachment #637079 - Flags: review?(jwalker)
Depends on: 685526
Comment on attachment 637079 [details]
Command

Here's a hack until we have true async types:

AddonManager.getAddonsByTypes(["plugin"], function(addons) {
  // cache names
  gcli.addCommand(addonCommandSpec);
});

I wonder if there is an event that is published by the addon manager when the list changes?


Also, so we can share the list of plugin names with others, we should do:

// Create a custom type for plugin names.

function PluginNameType() {
  this.stringifyProperty = 'name';
}

PluginNameType.prototype = Object.create(SelectionType.prototype);

PluginNameType.prototype.name = 'pluginname';

PluginNameType.prototype.lookup = function() {
  return pluginList.map(function(plugin) {
    return { name: plugin.name, value: plugin };
  }, this);
};

// This is a hack, we should avoid doing this
Components.utils.import("resource:///modules/devtools/Require.jsm");
var types = require('gcli/types');

types.registerType(PluginNameType);
Attachment #637079 - Flags: review?(jwalker)
Just to make sure we don't lose things - did you see Pimm's contributions:
https://gist.github.com/2998412
pimm is often around on IRC so we might be able to get his help - pimmhogeling
Summary: GCLI needs a command to control plugins → GCLI needs a command to control addons
Don't forget Panos's work in https://bugzilla.mozilla.org/attachment.cgi?id=636849
Blocks: 768998
No longer depends on: 685526
Duplicate of this bug: 729696
Filter on teabags
Whiteboard: [gclicommands]
Pimm's contributions are awesome!

As discussed on irc we could add the following:
- addon list <type> where type is one of dictionary, extension, locale, plugin or theme
- Partial string matching (maybe regex matching)

We should run the addons tests.

I also need to add some addon events.
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #9)
> - Partial string matching (maybe regex matching)

Partial string matching should be done automatically by GCLI, and I think that we'll get confused if we try to add regexp matching to this.
github repo https://github.com/Pimm/gcli-addon
We should also include <type> in enable / disable and install / uninstall

Comment 13

5 years ago
Code is now at https://github.com/Pimm/gcli-addon

If anyone needs write access, mail me (or mention me on Twitter: @pimmhogeling)

Comment 14

5 years ago
Installing an add-on can take quite a while. Feedback is an absolute must.

Therefore, I'd say this "bug" depends on #768414, perhaps.
(In reply to pimmhogeling from comment #14)
> Installing an add-on can take quite a while. Feedback is an absolute must.
> 
> Therefore, I'd say this "bug" depends on #768414, perhaps.

I thought we decided that we would file a followup for install/uninstall. In that case, the followup would depend on bug 768414.
Blocks: 771066
(In reply to Panos Astithas [:past] from comment #15)
> I thought we decided that we would file a followup for install/uninstall. In
> that case, the followup would depend on bug 768414.

We did, bug 771066 logged.
Created attachment 639649 [details] [diff] [review]
v1
Attachment #637079 - Attachment is obsolete: true
Attachment #639649 - Flags: review?(jwalker)
Blocks: 751904
Comment on attachment 639649 [details] [diff] [review]
v1

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

(I only briefly looked at the AddonManager API usage, not anything else.)

::: browser/devtools/commandline/GcliCommands.jsm
@@ +579,5 @@
> +    }
> +
> +    function representDisabledAddon(aAddon) {
> +      return "<li class=\"gcli-addon-disabled\">" +
> +        "<![CDATA[" + aAddon.name + "\u2002" + aAddon.version + "]]></li>";

Might want to add something to indicate whether an addon is pending enable/disable/uninstall/update here and in representEnabledAddon. Can check for that using addon.pendingOperations and the various AddonManager.PENDING_* constants (ie, addon.pendingOperations & AddonManager.PENIDNG_UNINSTALL).

@@ +603,5 @@
> +      let enabledAddons = [];
> +      let disabledAddons = [];
> +
> +      aAddons.forEach(function(aAddon) {
> +        if (aAddon.userDisabled) {

Note that .userDisabled isn't a measure of whether an add-on is actually enabled or disabled. Should be using .isActive here.

@@ +660,5 @@
> +    let name = aAddon.name + " " + aAddon.version;
> +    return name.trim();
> +  }
> +
> +  let addonNameCache = [];

This is going to get outdated - at the very least, you know it gets outdated when the enable/disable commands are run.

@@ +713,5 @@
> +        let name = representAddon(addon);
> +
> +        if (!addon.userDisabled) {
> +          this.resolve("<![CDATA[" +
> +            gcli.lookupFormat("addonAlreadyEnabled", [name]) + "]]>");

If the user has enabled a disabled non-restartless add-on and hasn't yet restarted, then userDisabled will be true even though the add-on won't be enabled until after Firefox next starts up. Check addon.pendingOperations & AdonManager.PENDING_ENABLE. (Same for the add-on disable command.)
Attachment #639649 - Flags: review?(jwalker)
Comment on attachment 639649 [details] [diff] [review]
v1

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

::: browser/devtools/commandline/GcliCommands.jsm
@@ +551,5 @@
> + */
> +gcli.addCommand({
> +  name: "addon",
> +  description: {
> +    "root": gcli.lookup("addonDesc")

We've got a mix of 2 ways to do l10n here.
What we mean is just:
 description: gcli.lookup("addonDesc")

@@ +567,5 @@
> +  params: [{
> +    name: 'type',
> +    type: {
> +      name: 'selection',
> +      data: ["dictionary", "extension", "locale", "plugin", "theme"]

Did we have a rationale for not having an "all" option (and maybe having that as the defaultValue?

@@ +672,5 @@
> +    },
> +    description: {
> +      "root": gcli.lookup("addonNameDesc")
> +    }
> +  };

Ug.
We really need to expose addType().

That way we could have:
{
  name: "name",
  type: "addonName",
  description: gcli.lookup("addonNameDesc")
}

And I don't think we'd feel the pain of the repetition and want to pull it out. Maybe even types could expose descriptions.

  { name: "name", type: "addonName" }

Anyway, not your job. Just thinking out loud.

::: browser/devtools/commandline/test/browser_gcli_addon.js
@@ +38,5 @@
> +      DeveloperToolbarTest.checkInputStatus({
> +        typed: "addon enable Test_Plug-in_1.0.0.0",
> +        status: "VALID"
> +      });
> +      DeveloperToolbarTest.exec({ completed:false });

So we're executing "addon enable Test_Plug-in_1.0.0.0" here. I think we need to make the command we're executing more explicit (but that's not your job, the test framework should be better here)

How about we exec "addon list plugin" and check that "Test_Plug-in_1.0.0.0" is part of the output?
I think that would be:

DeveloperToolbarTest.exec({
  typed: "addon list plugin",
  completed: false,
  outputMatch: /Test_Plug-in_1.0.0.0/
});

::: browser/themes/winstripe/devtools/gcli.css
@@ +127,5 @@
> +.gcli-addon-disabled {
> +  opacity: 0.6;
> +  text-decoration: line-through;
> +}
> +

This is a picky nit, I know, but please could you move .gcli-addon-disabled after all the .gcli-menu items. Unless we keep things in sections it's going to turn into a pigs ear in a few years.
Created attachment 640314 [details] [diff] [review]
patch II - the sequel

(In reply to Blair McBride (:Unfocused) from comment #18)
> Comment on attachment 639649 [details] [diff] [review]
> v1
> 
> Review of attachment 639649 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (I only briefly looked at the AddonManager API usage, not anything else.)
> 
> ::: browser/devtools/commandline/GcliCommands.jsm
> @@ +579,5 @@
> > +    }
> > +
> > +    function representDisabledAddon(aAddon) {
> > +      return "<li class=\"gcli-addon-disabled\">" +
> > +        "<![CDATA[" + aAddon.name + "\u2002" + aAddon.version + "]]></li>";
> 
> Might want to add something to indicate whether an addon is pending
> enable/disable/uninstall/update here and in representEnabledAddon. Can check
> for that using addon.pendingOperations and the various
> AddonManager.PENDING_* constants (ie, addon.pendingOperations &
> AddonManager.PENIDNG_UNINSTALL).
> 

Done, I now add pending operations in brackets after the addon name.

> @@ +603,5 @@
> > +      let enabledAddons = [];
> > +      let disabledAddons = [];
> > +
> > +      aAddons.forEach(function(aAddon) {
> > +        if (aAddon.userDisabled) {
> 
> Note that .userDisabled isn't a measure of whether an add-on is actually
> enabled or disabled. Should be using .isActive here.
> 

Changed

> @@ +660,5 @@
> > +    let name = aAddon.name + " " + aAddon.version;
> > +    return name.trim();
> > +  }
> > +
> > +  let addonNameCache = [];
> 
> This is going to get outdated - at the very least, you know it gets outdated
> when the enable/disable commands are run.
> 

Added an addonListener to handle this.

> @@ +713,5 @@
> > +        let name = representAddon(addon);
> > +
> > +        if (!addon.userDisabled) {
> > +          this.resolve("<![CDATA[" +
> > +            gcli.lookupFormat("addonAlreadyEnabled", [name]) + "]]>");
> 
> If the user has enabled a disabled non-restartless add-on and hasn't yet
> restarted, then userDisabled will be true even though the add-on won't be
> enabled until after Firefox next starts up. Check addon.pendingOperations &
> AdonManager.PENDING_ENABLE. (Same for the add-on disable command.)

I now add pending operations in brackets after the addon name.

(In reply to Joe Walker from comment #19)
> Comment on attachment 639649 [details] [diff] [review]
> v1
> 
> Review of attachment 639649 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/commandline/GcliCommands.jsm
> @@ +551,5 @@
> > + */
> > +gcli.addCommand({
> > +  name: "addon",
> > +  description: {
> > +    "root": gcli.lookup("addonDesc")
> 
> We've got a mix of 2 ways to do l10n here.
> What we mean is just:
>  description: gcli.lookup("addonDesc")
> 

Fixed

> @@ +567,5 @@
> > +  params: [{
> > +    name: 'type',
> > +    type: {
> > +      name: 'selection',
> > +      data: ["dictionary", "extension", "locale", "plugin", "theme"]
> 
> Did we have a rationale for not having an "all" option (and maybe having
> that as the defaultValue?
> 

Added

> @@ +672,5 @@
> > +    },
> > +    description: {
> > +      "root": gcli.lookup("addonNameDesc")
> > +    }
> > +  };
> 
> Ug.
> We really need to expose addType().
> 
> That way we could have:
> {
>   name: "name",
>   type: "addonName",
>   description: gcli.lookup("addonNameDesc")
> }
> 
> And I don't think we'd feel the pain of the repetition and want to pull it
> out. Maybe even types could expose descriptions.
> 
>   { name: "name", type: "addonName" }
> 
> Anyway, not your job. Just thinking out loud.
> 

+1 from me ;o)

> ::: browser/devtools/commandline/test/browser_gcli_addon.js
> @@ +38,5 @@
> > +      DeveloperToolbarTest.checkInputStatus({
> > +        typed: "addon enable Test_Plug-in_1.0.0.0",
> > +        status: "VALID"
> > +      });
> > +      DeveloperToolbarTest.exec({ completed:false });
> 
> So we're executing "addon enable Test_Plug-in_1.0.0.0" here. I think we need
> to make the command we're executing more explicit (but that's not your job,
> the test framework should be better here)
> 
> How about we exec "addon list plugin" and check that "Test_Plug-in_1.0.0.0"
> is part of the output?
> I think that would be:
> 
> DeveloperToolbarTest.exec({
>   typed: "addon list plugin",
>   completed: false,
>   outputMatch: /Test_Plug-in_1.0.0.0/
> });
> 

That would be nice. Unfortunately checking the output in addition to using "completed: false" fails because the output has not yet been built.

> ::: browser/themes/winstripe/devtools/gcli.css
> @@ +127,5 @@
> > +.gcli-addon-disabled {
> > +  opacity: 0.6;
> > +  text-decoration: line-through;
> > +}
> > +
> 
> This is a picky nit, I know, but please could you move .gcli-addon-disabled
> after all the .gcli-menu items. Unless we keep things in sections it's going
> to turn into a pigs ear in a few years.

Done
Attachment #639649 - Attachment is obsolete: true
Attachment #640314 - Flags: review?(jwalker)
Comment on attachment 640314 [details] [diff] [review]
patch II - the sequel

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

I think we could s/cliArguments/args/g and extract some of the nested functions up a level.
But we also need to get this in. So I suggest that we raise a bug 'Polish needed in GCLI addons command'.
OK?
Attachment #640314 - Flags: review?(jwalker) → review+
Blocks: 772376
Created attachment 640544 [details] [diff] [review]
v3

Bug 772376 created for tidying the code.

Also renamed cliArguments.
Attachment #640314 - Attachment is obsolete: true
Added to master patch in bug 768998
Copy / paste error ... added to master patch in but 771555.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.