1106353 - [gcli] Extend `addon` to handle click-to-play

Status: RESOLVED FIXED

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

Description

[AJ Jordan [:strugee] (he/they)]

Plugins have slightly different contexts than addons: they can be click-to-play in addition to simply enabled or disabled. They should have their own command that has the ability to set a plugin's click-to-play state and get a plugin's click-to-play state, and possibly get a list of all plugins.

Comment 1

[AJ Jordan [:strugee] (he/they)]

Actually, looking further at the `addon` command, this should probably be `addon ctp` or somesuch instead of a separate command.

Comment 2

[AJ Jordan [:strugee] (he/they)]

Attachment: patch v1 (no tests)

WIP patch. It doesn't have tests but I'd like to get feedback first.

(N.b.: this is my first time patching Mozilla *and* my first time taking a crack at Mercurial. Hopefully everything is formatted correctly.)

Comment 3

[AJ Jordan [:strugee] (he/they)]

Attachment: patch v2

Here's a new patch. It includes bugfixes and tests, although I wasn't able to test the tests, as my test harness is broken. Hence all the TODO comments.

Comment 4

[Joe Walker [:jwalker]]

IIRC, Mike wrote the addons command, so he should be able to help if needed.

Comment 5

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

Comment on attachment 8530919 [details] [diff] [review]
patch v2

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

This looks good overall... do you know how to run the test?

Comment 6

[AJ Jordan [:strugee] (he/they)]

Hey, sorry I missed this!

I know how to run tests in principle, but I haven't had time to actually look into writing them for this feature. I looked into it at the time I wrote the patch, but the addon stuff is gnarly. I [asked on firefox-dev][1] a while ago, and over Thanksgiving break I'll rebase and hopefully finish this up.

 [1]: https://mail.mozilla.org/pipermail/firefox-dev/2015-October/003467.html

Comment 7

[AJ Jordan [:strugee] (he/they)]

Attachment: patch v3

Mostly this just rebases on top of current fx-team. There's still a couple failing tests that I need to look 
into.

Comment 8

[AJ Jordan [:strugee] (he/they)]

Attachment: patch v4

OK, this patch has (as far as I can tell) complete tests that all pass.

There's definitely some wonky stuff going on, but I'm not sure exactly what it is. In particular, some of the tests that I wrote are missing "markup" object keys, since I couldn't figure out what that did. Tests still pass fine, though.

Comment 9

[AJ Jordan [:strugee] (he/they)]

Attachment: gcli-extend-addon-to-handle-click-to-play-v4-git.patch

Accidentally attached a hg-style patch instead of a git-style patch.

Comment 10

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

Comment on attachment 8703460 [details] [diff] [review]
gcli-extend-addon-to-handle-click-to-play-v4-git.patch

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

Awesome work... I have to say that I am surprised that this is your first Mozilla patch because the quality of you code is excellent.

I don't think you have access to our try server (runs our tests on various operating systems) so I have initiated the try process for you. You can see the current process at:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3467e5cb25bd

If there are no failures relating to your patch I will mark this as r+ (ready to land)

Just a couple of nits that need to be addressed:

::: devtools/client/commandline/test/browser_cmd_addon.js
@@ +177,5 @@
> +        args: {
> +          command: { name: 'addon ctp' },
> +          addon: {
> +            value: function(addon) {
> +              is(addon.name, 'Mochitest', 'mochitest'); /* TODO */

Not sure what this TODO is for.

@@ +184,5 @@
> +          }
> +	}
> +      },
> +      exec: {
> +        output: 'Mochitest 1.0 cannot be set to click-to-play because it is not a plugin.' /* TODO */

Not sure what this TODO is for.

::: devtools/shared/locales/en-US/gclicommands.properties
@@ +854,5 @@
> +# ctp' command when an attempt is made to set an addon to click-to-play,
> +# but the addon is not a plugin.
> +addonCantCtp=%S cannot be set to click-to-play because it is not a plugin.
> +
> +# LOCALIZATION NOTE (addonCantCtp) Used in the output of the 'addon

This should be addonNoCtp

Comment 11

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

A few failures but none related to any code you have touched. This is generally caused by race issues and shouldn't concern you.

If you can fix the nits I mention in comment 10 then we will be able to land your patch.

Comment 12

[AJ Jordan [:strugee] (he/they)]

Attachment: patch v5

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #10)

> Awesome work... I have to say that I am surprised that this is your first
> Mozilla patch because the quality of you code is excellent.

Thanks, that means a lot! I have some previous JS experience and I've been lurking on the lists for quite a while, so that helps :)

> Not sure what this TODO is for.

There's nothing to do here, I just forgot to save.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/117b84fe44ce

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/117b84fe44ce

Comment 15

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Please mention STR for verification.


Component: 
Name 			Firefox
Version 		46.0b4
Build ID 		20160322075646
Update Channel 	        beta
User Agent 		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS                      Windows 7 SP1 x86_64

Actual Results: 
Not able to test.

Expected Results: 
STR should be clear.