Last Comment Bug 710159 - GCLI needs a command to control addons
: GCLI needs a command to control addons
Status: RESOLVED FIXED
[gclicommands]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Console (show other bugs)
: unspecified
: All All
: P3 normal with 1 vote (vote)
: Firefox 16
Assigned To: Michael Ratcliffe [:miker] [:mratcliffe]
:
Mentors:
: 729696 (view as bug list)
Depends on:
Blocks: 771066 751904 GCLICMD 772376
  Show dependency treegraph
 
Reported: 2011-12-13 02:27 PST by Joe Walker [:jwalker] (needinfo me or ping on irc)
Modified: 2012-07-16 05:13 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Command (2.95 KB, text/plain)
2012-06-27 04:45 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details
v1 (15.93 KB, patch)
2012-07-06 06:52 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Review
patch II - the sequel (17.11 KB, patch)
2012-07-09 11:47 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
jwalker: review+
Details | Diff | Review
v3 (17.06 KB, patch)
2012-07-10 03:01 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Review

Description Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-12-13 02:27:08 PST
> plugins list
> plugins enable <name> --global
> plugins disable <name> --global

(or something)
Comment 1 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-01-13 09:45:10 PST
Triage. Filter on PEGASUS.
Comment 2 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-04-29 11:48:39 PDT
GCLI Triage.
Comment 3 Michael Ratcliffe [:miker] [:mratcliffe] 2012-06-27 04:45:50 PDT
Created attachment 637079 [details]
Command
Comment 4 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-06-27 07:24:26 PDT
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);
Comment 5 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-06-29 01:51:38 PDT
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
Comment 6 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-06-29 02:15:17 PDT
Don't forget Panos's work in https://bugzilla.mozilla.org/attachment.cgi?id=636849
Comment 7 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-06-29 03:04:28 PDT
*** Bug 729696 has been marked as a duplicate of this bug. ***
Comment 8 Michael Ratcliffe [:miker] [:mratcliffe] 2012-06-29 03:10:28 PDT
Filter on teabags
Comment 9 Michael Ratcliffe [:miker] [:mratcliffe] 2012-06-29 03:36:33 PDT
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.
Comment 10 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-06-29 03:54:12 PDT
(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.
Comment 11 Michael Ratcliffe [:miker] [:mratcliffe] 2012-06-29 04:03:08 PDT
github repo https://github.com/Pimm/gcli-addon
Comment 12 Michael Ratcliffe [:miker] [:mratcliffe] 2012-06-29 04:07:46 PDT
We should also include <type> in enable / disable and install / uninstall
Comment 13 pimmhogeling 2012-06-29 04:11:17 PDT
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 pimmhogeling 2012-07-02 03:12:19 PDT
Installing an add-on can take quite a while. Feedback is an absolute must.

Therefore, I'd say this "bug" depends on #768414, perhaps.
Comment 15 Panos Astithas [:past] 2012-07-02 03:46:39 PDT
(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.
Comment 16 Michael Ratcliffe [:miker] [:mratcliffe] 2012-07-05 01:20:15 PDT
(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.
Comment 17 Michael Ratcliffe [:miker] [:mratcliffe] 2012-07-06 06:52:02 PDT
Created attachment 639649 [details] [diff] [review]
v1
Comment 18 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-07-07 05:28:18 PDT
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.)
Comment 19 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-07-09 03:46:04 PDT
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.
Comment 20 Michael Ratcliffe [:miker] [:mratcliffe] 2012-07-09 11:47:55 PDT
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
Comment 21 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-07-09 13:38:20 PDT
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?
Comment 22 Michael Ratcliffe [:miker] [:mratcliffe] 2012-07-10 03:01:56 PDT
Created attachment 640544 [details] [diff] [review]
v3

Bug 772376 created for tidying the code.

Also renamed cliArguments.
Comment 23 Michael Ratcliffe [:miker] [:mratcliffe] 2012-07-10 04:00:48 PDT
Added to master patch in bug 768998
Comment 24 Michael Ratcliffe [:miker] [:mratcliffe] 2012-07-10 04:03:25 PDT
Copy / paste error ... added to master patch in but 771555.

Note You need to log in before you can comment on or make changes to this bug.