771066 - GCLI command to control addons needs install & uninstall adding

Status: RESOLVED INCOMPLETE

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

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

addon (un)install dictionary <name>
addon (un)install extension <name>
addon (un)install locale <name>
addon (un)install plugin <name>
addon (un)install theme <name>

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: addon (un)install command (WIP)

This implementation needs a little tidying up and tests adding.

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Works just fine.

I have simplified the command ... all we need is:
addon (un)install <name>

There is no need to complicate this by adding unnecessary filter criteria.

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Rebased

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Green on try

Comment 5

[Joe Walker [:jwalker]]

Comment on attachment 653487 [details] [diff] [review]
Rebased

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

I think we need to get UI feedback on this, and I'd like to take a look, (just a code review so far).

::: browser/devtools/commandline/CmdAddon.jsm
@@ +13,5 @@
>  XPCOMUtils.defineLazyModuleGetter(this, "AddonManager",
>                                    "resource://gre/modules/AddonManager.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "AddonRepository",
> +                                  "resource://gre/modules/AddonRepository.jsm");
> +let helpers = {

Arg, so I guess it's really annoying that we just used 'helpers' in the testing framework.
I don't suppose there is an actual namespace clash anywhere, but there could be some mental confusion. I'll leave it up to you if you'd like to change it.

@@ +442,5 @@
> +    description: gcli.lookup("addonUninstallDesc"),
> +    params: [nameParameter],
> +
> +    exec: function(aArgs, aContext) {
> +      if (/Test Plug-in/gi.test(aArgs.name)) {

Hmm, I'm not sure that we should be doing this. It feels quite dangerous to me. What do you think?

::: browser/devtools/commandline/test/browser_cmd_addon.js
@@ +48,5 @@
>    });
> +    // We have no internet access from our testing servers so it is not
> +  // possible to fully test addon install & uninstall. For this reason we
> +  // only test the input.
> +  DeveloperToolbarTest.checkInputStatus({

I expect this will be bit-rotted after the recent landings, as we unbitrot, please could we convert to helpers.check();

::: browser/devtools/shared/DeveloperToolbar.jsm
@@ +661,5 @@
>  {
>    if (this._panel == null || this.document == null || !this._panel.state == "closed") {
>      return
>    }
> +  this._frame.height = this.document.body.scrollHeight + 2;

We should probably avoid magic numbers where we can, and certainly document them when we can't. What is this about?

@@ +666,1 @@
>    this._frame.width = this._input.clientWidth + 2;

Yes I know.

::: browser/locales/en-US/chrome/browser/devtools/gcli.properties
@@ -370,5 @@
>  # LOCALIZATION NOTE (introBody): The text displayed at the top of the output
>  # for the help command, just before the list of commands. This text is wrapped
>  # inside a link to a localized MDN article
>  introBody=For more information see MDN.
> -

Accidental change?

::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
@@ +512,5 @@
>  
> +# LOCALIZATION NOTE (addonInstallDesc) A very short description of the
> +# 'addon install <name>' command. This string is designed to be shown in a menu
> +# alongside the command name, which is why it should be as short as possible.
> +addonInstallDesc=Install the specified add-on from addons.mozilla.org

Lets have a discussion on '.' use and create a standard.

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Addressed reviewers comments

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Joe Walker [:jwalker] from comment #5)
> Comment on attachment 653487 [details] [diff] [review]
> Rebased
> 
> Review of attachment 653487 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think we need to get UI feedback on this, and I'd like to take a look,
> (just a code review so far).
> 
> ::: browser/devtools/commandline/CmdAddon.jsm
> @@ +13,5 @@
> >  XPCOMUtils.defineLazyModuleGetter(this, "AddonManager",
> >                                    "resource://gre/modules/AddonManager.jsm");
> > +XPCOMUtils.defineLazyModuleGetter(this, "AddonRepository",
> > +                                  "resource://gre/modules/AddonRepository.jsm");
> > +let helpers = {
> 
> Arg, so I guess it's really annoying that we just used 'helpers' in the
> testing framework.
> I don't suppose there is an actual namespace clash anywhere, but there could
> be some mental confusion. I'll leave it up to you if you'd like to change it.
> 

Changed to addonHelpers.

> @@ +442,5 @@
> > +    description: gcli.lookup("addonUninstallDesc"),
> > +    params: [nameParameter],
> > +
> > +    exec: function(aArgs, aContext) {
> > +      if (/Test Plug-in/gi.test(aArgs.name)) {
> 
> Hmm, I'm not sure that we should be doing this. It feels quite dangerous to
> me. What do you think?
> 

Changed to a simple check for addon.uninstall (it looks like test add-ons don't have that method).

> ::: browser/devtools/commandline/test/browser_cmd_addon.js
> @@ +48,5 @@
> >    });
> > +    // We have no internet access from our testing servers so it is not
> > +  // possible to fully test addon install & uninstall. For this reason we
> > +  // only test the input.
> > +  DeveloperToolbarTest.checkInputStatus({
> 
> I expect this will be bit-rotted after the recent landings, as we unbitrot,
> please could we convert to helpers.check();
> 

Done.

> ::: browser/devtools/shared/DeveloperToolbar.jsm
> @@ +661,5 @@
> >  {
> >    if (this._panel == null || this.document == null || !this._panel.state == "closed") {
> >      return
> >    }
> > +  this._frame.height = this.document.body.scrollHeight + 2;
> 
> We should probably avoid magic numbers where we can, and certainly document
> them when we can't. What is this about?
> 

This was to work around the issue with a css list-style-image. When we call resize() the image is not fully initialized and the document was then 2px larger than before the image was fully loaded.

Anyhow, I have changed it not to use list-style-image so it works far better now without any need for magic numbers.

> @@ +666,1 @@
> >    this._frame.width = this._input.clientWidth + 2;
> 
> Yes I know.
> 

I have fixed that to use the boundingClientRect so no need to add 2 any longer.

> ::: browser/locales/en-US/chrome/browser/devtools/gcli.properties
> @@ -370,5 @@
> >  # LOCALIZATION NOTE (introBody): The text displayed at the top of the output
> >  # for the help command, just before the list of commands. This text is wrapped
> >  # inside a link to a localized MDN article
> >  introBody=For more information see MDN.
> > -
> 
> Accidental change?
> 

Removed

> ::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
> @@ +512,5 @@
> >  
> > +# LOCALIZATION NOTE (addonInstallDesc) A very short description of the
> > +# 'addon install <name>' command. This string is designed to be shown in a menu
> > +# alongside the command name, which is why it should be as short as possible.
> > +addonInstallDesc=Install the specified add-on from addons.mozilla.org
> 
> Lets have a discussion on '.' use and create a standard.

We already did ... The only reason I have not included a '.' is because it ends with a url and the . can be confusing.

I will change it because that seems to be your preference.

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: GCLI Progress

Joe asked me to get UX feedback about the gcli progress dialog.

This is displayed when a gcli command has been run and we are waiting for feedback (in this case "addon install firebug"). We have no idea of % progress etc. so I have used a twisty instead.

Does this work for you?

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Rebased

I see what you meant, that was major bitrot!

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Changed throbber

Comment 11

[Joe Walker [:jwalker]]

Attachment: alternative spinner

What do you think?

Comment 12

[Joe Walker [:jwalker]]

Specifically it wouldn't grow the height of the "loading...." area, and it's in our colours.

Comment 13

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Sure

Comment 14

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Now uses jwalker's throbber

Comment 15

[Joe Walker [:jwalker]]

New component triage. Filter on "Lobster Thermidor aux crevettes with a Mornay sauce"

Comment 16

[Sebastian Zartner [:sebo]]

This bug lies around for more than 4 years already. Its status is ASSIGNED, though you didn't assign yourself, Mike.
So, what's the status of this?

Sebastian

Comment 17

[Sebastian Zartner [:sebo]]

I unassign this bug now to give somebody else the chance to pick this up.

Sebastian

Comment 18

[Firefox Bug Husbandry Bot]

Per bug 1491875, this component has been closed, and the affected code is being removed from Firefox. Closing this bug as incomplete.