Closed Bug 1106353 Opened 9 years ago Closed 8 years ago

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

Categories

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

defect
Not set
normal

Tracking

(firefox46 fixed)

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed

People

(Reporter: strugee, Assigned: strugee, Mentored)

Details

Attachments

(1 file, 5 obsolete files)

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.
Blocks: GCLICMD
Component: Developer Tools: Console → Developer Tools: Graphic Commandline and Toolbar
Actually, looking further at the `addon` command, this should probably be `addon ctp` or somesuch instead of a separate command.
Summary: [gcli] Implement a command for plugins → [gcli] Extend `addon` to handle click-to-play
No longer blocks: GCLICMD
Attached patch patch v1 (no tests) (obsolete) — Splinter Review
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.)
Attachment #8530617 - Flags: feedback?(jwalker)
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #8530617 - Attachment is obsolete: true
Attachment #8530617 - Flags: feedback?(jwalker)
IIRC, Mike wrote the addons command, so he should be able to help if needed.
Flags: needinfo?(mratcliffe)
Assignee: nobody → alex
Flags: needinfo?(mratcliffe)
Mentor: mratcliffe
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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?
Attachment #8530919 - Flags: feedback+
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
Flags: needinfo?(alex)
Attached patch patch v3 (obsolete) — Splinter Review
Mostly this just rebases on top of current fx-team. There's still a couple failing tests that I need to look 
into.
Attachment #8530919 - Attachment is obsolete: true
Attached patch patch v4 (obsolete) — Splinter Review
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.
Attachment #8692813 - Attachment is obsolete: true
Attachment #8703408 - Flags: review?(mratcliffe)
Accidentally attached a hg-style patch instead of a git-style patch.
Attachment #8703408 - Attachment is obsolete: true
Attachment #8703408 - Flags: review?(mratcliffe)
Attachment #8703460 - Flags: review?(mratcliffe)
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
Attachment #8703460 - Flags: review?(mratcliffe) → review+
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.
Flags: needinfo?(alex)
Attached patch patch v5Splinter Review
(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.
Attachment #8703460 - Attachment is obsolete: true
Flags: needinfo?(alex)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/117b84fe44ce
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
[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.
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.