866390 - Add a default about:config pref for plugin activation

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[JK]

There should be an option that allows an user to set plugins to always activate / ask to activate / never activate by default, and an option to reset all plugins to the default setting.

Compare to the extension updates (update add-ons automatically, reset all add-ons to update automatically/manually).

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

I'm worried this would just be adding noise to the Add-ons Manager UI, without providing much benefit.

(In reply to JK from comment #0)
> Compare to the extension updates (update add-ons automatically, reset all
> add-ons to update automatically/manually).

But we don't have a similar way to enable/disable all extensions; although plugins do have slightly different considerations. And in the past, Boriss has argued for removing the "Reset all add-ons to update automatically" menuitem (combining it with "Update add0ons automatically" menuitem).

Comment 2

[JK]

This is problematic because new installed plugins are enabled by default.

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

AFAIK, the plan is to make new plugins default to click-to-play... but I can't find the bug (I bet dkeeler knows). I just don't think we should have UI to change that default.

Comment 4

[Georg Fritzsche [:gfritzsche]]

(In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from comment #3)
> AFAIK, the plan is to make new plugins default to click-to-play...

The plan is indeed to make most plugins click-to-play by default, which is just not in yet.
I don't think we have a bug filed specifically for this yet.

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Well, bug 558194 is about making "unknown" plugins click-to-play by default. I think we're waiting on the UX improvements before moving forward with any sort of default setting. For what it's worth, I agree with Blair in comment 1. (Also, it would be pretty easy to write an addon that provides this functionality.)

Comment 6

[JK]

(In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from comment #3)
> AFAIK, the plan is to make new plugins default to click-to-play... but I
> can't find the bug (I bet dkeeler knows). I just don't think we should have
> UI to change that default.

But will it be ready for the next "train"?

An about:config preference would be fine.

Comment 7

[Mardeg]

(In reply to JK from comment #6)
> (In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from
> comment #3)
> An about:config preference would be fine.
It seems logical to repurpose plugins.click_to_play for this job since it's previous functionality was close enough.
Bug 866935 is looking to remove it.

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler]]

(In reply to Mardeg from comment #7)
> (In reply to JK from comment #6)
> > (In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from
> > comment #3)
> > An about:config preference would be fine.
> It seems logical to repurpose plugins.click_to_play for this job since it's
> previous functionality was close enough.
> Bug 866935 is looking to remove it.

I think this is a good way to go. In fact, this would restore the old behavior that current users of click-to-play expect (i.e. that if "plugins.click_to_play" is true, plugins are click-to-play).

Comment 9

[Benjamin Smedberg]

There are a couple things here:

* The user-set state of a plugin is stored in plugin.state.<name>
* We should have a matching pref to set the default state of newly-discovered plugins. That pref should be called plugin.default.state and have the same 0/1/2 settings. For now until we fix the CtP doorhanger, that should still default to enabled.
* Currently I believe we try to import a plugin's disabled state from the previously-existing pluginreg.dat. We should make sure that we don't do this repeatedly and that pluginreg.dat doesn't override other settings.

I don't think we should expose a UI function to reset all plugins to a particular state. Users can go through and click on the set of plugins they have.

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler]]

In light of comment 9, I think there's two bugs here:
1. People are confused by how the meaning of plugins.click_to_play changed after bug 549697. I think it would be nice to put that back to how it was (particularly with how click-to-play works on Fennec).
2. For those that don't use plugins.click_to_play, we need this new plugin.default.state pref.

Comment 11

[Benjamin Smedberg]

People are going to be even more confused when we remove click-to-play and make it click-to-show-doorhanger anyway.

I'm a little worried that I may not get the new doorhanger done for this cycle, so I think we need to at least keep the "Ask to Activate" feature hidden until that work is done.

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: patch

I think this does what you said in comment 9.

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler]]

This is now actually pretty important due to click-to-play being essentially broken on Android.

Comment 14

[Benjamin Smedberg]

Comment on attachment 749516 [details] [diff] [review]
patch

>diff --git a/dom/plugins/base/nsPluginTags.cpp b/dom/plugins/base/nsPluginTags.cpp

>+  enabledState = Preferences::GetInt("plugin.default.state",
>+                                     ePluginState_MaxValue);

I'm pretty sure you want a real C++ default here. I suggest nsIPluginTag::STATE_ENABLED.

r=me with that

Comment 15

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: patch v1.1

Thanks for the review - made the change, carrying over r+.
Try: https://tbpl.mozilla.org/?tree=Try&rev=fed58b2c58c0
Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/b24292f986aa

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b24292f986aa

Comment 17

[Paul Silaghi, QA [:pauly]]

I've been testing this in today's nightly.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #9)
> * We should have a matching pref to set the default state of
> newly-discovered plugins. That pref should be called plugin.default.state
> and have the same 0/1/2 settings.
0 - all plugins are "never activate"
1 - all plugins are "always activate"
2 - default state, does nothing

> * The user-set state of a plugin is stored in plugin.state.<name>
plugin.state.flash = 0 --> never activate
plugin.state.flash = 1 --> ask to activate
plugin.state.flash = 2 --> always activate

So, I would say the numbers don't match between prefs?

Comment 18

[Benjamin Smedberg]

> 0 - all plugins are "never activate"
> 1 - all plugins are "always activate"
> 2 - default state, does nothing

Paul, are you saying that this is your observed behavior? It doesn't match what the code appears to implement.

Comment 20

[:Margaret Leibovic]

I think we should uplift this patch to Aurora. Without it, "tap to activate" plugins are broken on Android (see bug 869366).

Comment 21

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 750497 [details] [diff] [review]
patch v1.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 549697
User impact if declined: click-to-play won't work on Android (plugins will just automatically activate)
Testing completed (on m-c, etc.): landed on m-c
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 22

[Paul Silaghi, QA [:pauly]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #18)
> > 0 - all plugins are "never activate"
> > 1 - all plugins are "always activate"
> > 2 - default state, does nothing
> 
> Paul, are you saying that this is your observed behavior?
Yes

Comment 23

[Paul Silaghi, QA [:pauly]]

Setting plugin.default.state to 0/1/2 makes all the plugins in addons manager to never activate/always activate/no change

Comment 24

[Georg Fritzsche [:gfritzsche]]

(In reply to Paul Silaghi [QA] from comment #23)
> Setting plugin.default.state to 0/1/2 makes all the plugins in addons
> manager to never activate/always activate/no change

This is weird, i don't see that behaviour (it behaves as expected for me) and i can't think of a combination of plugin.default.state & plugin.X.state settings that would behave that way.

Note that the addon manager doesn't pick up the changes immediately, you need to at least switch between it's tabs to see an effect.

Comment 25

[Dana Keeler (she/her) (use needinfo) [:keeler]]

If I set plugins.click_to_play to false, I get the behavior described by Paul (which is expected, I believe). With plugins.click_to_play=true, I get the correct behavior.

Comment 26

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #24)
> Note that the addon manager doesn't pick up the changes immediately, you
> need to at least switch between it's tabs to see an effect.

Well, that's a bug in itself... can you confirm and file?

Comment 27

[Lukas Blakk [:lsblakk] use ?needinfo]

Is comment 25 saying the QA results are the expectation for this patch? Can we get verification that the settings 0/1/2 is working as intended?

Comment 28

[Paul Silaghi, QA [:pauly]]

It seems I was somehow tricked by my out-of-date plugins (which had the 'ask to activate' option with plugins.click_to_play=false) or by the addon manager's UI (comment 24). Sorry for the confusion.
I confirm the behavior from comment 25. If that's the expected behavior with plugins.click_to_play=false and true, then everything is ok.
Verified fixed nightly 24.0a1 (2013-05-20) Win 7 x64.

Comment 29

[Paul Silaghi, QA [:pauly]]

One more question:
What should happen if I set couple of plugins to 'never activate' and then change plugin.default.state to 1 or 2 ? The 'never activate' plugins are not reset.

Comment 30

[Georg Fritzsche [:gfritzsche]]

That's(In reply to Paul Silaghi [QA] from comment #29)
> What should happen if I set couple of plugins to 'never activate' and then
> change plugin.default.state to 1 or 2 ? The 'never activate' plugins are not
> reset.

That's expected: the setting for those plugins is saved and overrides the default setting.

Comment 31

[Georg Fritzsche [:gfritzsche]]

(In reply to Blair McBride [:Unfocused] (Limited availability.) from comment #26)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #24)
> > Note that the addon manager doesn't pick up the changes immediately, you
> > need to at least switch between it's tabs to see an effect.
> 
> Well, that's a bug in itself... can you confirm and file?

This is when directly changing the prefs for plugin.state.PluginName. 
We do fire "plugin-info-updated" when it's changed on the tags, but we don't monitor the prefs, so directly changing those not resulting in updates is expected.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/401338d98748

Comment 33

[Paul Silaghi, QA [:pauly]]

Verified fixed the plugin.default.state pref in aurora 23.0a2 (2013-05-23)

Comment 34

[Tomer Cohen :tomer]

Removing plugins.click_to_play is causing all plugins to load instantly, without a visible message indicating why it happen or how to turn on click_to_play again. I, as an advanced user updated my Aurora to find that some plugins load again opened about:config and found that the preference is still there (because it set by the user) and the browser just ignore it. 

I'd suggest having more work on the migration process. We can (and should) run a code during the migration process to turn on similar function to click_to_play by default on most common plugins. Also, it could help us to periodically run such code and not only during the migration, so we won't break suggestions to set plugins.click_to_play to true which exists all over the web.

Comment 35

[Georg Fritzsche [:gfritzsche]]

The behavior behind the click_to_play pref was never exposed in any UI and was subject to change.
The actual click-to-play changes (aside from block-listing) will come in Firefox 24 and will have visible UI and sensible default states for plugins.