901749 - Fix what preference "Activate all plugins by default" in SeaMonkey preferences window sets

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Frank Wein [:mcsmurf]]

The preference "Activate all plugins by default" in SeaMonkey preferences (Advanced->Script&Plugins) currently sets plugins.click_to_play to true. That's not correct anymore, the relevant pref is now plugin.default.state. plugin.default.state=0 means "deactivate all plugin", 1 means click-to-play, 2 means activate all plugins. This preference reflects to the GUI when looking at about:addons -> Plugins. The plugins.click_to_play pref just displays the "Ask to activate" UI in about:addons. It does not CTP (de)activate any plugin (see Bug 866390).

Comment 1

[Frank Wein [:mcsmurf]]

Bug 549697 changed the pref, so currently all release trains are affected. Marking wontfix for SeaMonkey 2.20, too late to fix it.

Comment 3

[Frank Wein [:mcsmurf]]

Neil: Do you know where in the SeaMonkey code I could place migration code for a pref? I need to check once if plugins.click_to_play is set to true and based on that set another pref (plugin.default.state). This can happen after all the UI from browser startup has been displayed, so this does not necessarily need to happen during startup.

Comment 4

[Philip Chee]

Frank:
Firefox puts this sort of migration code in their nsBrowserGlue.js

Comment 5

[[e-mail]]

Interesting, the Add-ons Manager has a bug whereby setting the default state to 1 only displays properly if plugins.click_to_play is true...

(In reply to Frank Wein from comment #3)
> Neil: Do you know where in the SeaMonkey code I could place migration code
> for a pref? I need to check once if plugins.click_to_play is set to true and
> based on that set another pref (plugin.default.state). This can happen after
> all the UI from browser startup has been displayed, so this does not
> necessarily need to happen during startup.

So, would _updatePrefs in nsSuiteGlue.js be suitable?

Comment 6

[Joachim Otahal]

See https://bugzilla.mozilla.org/show_bug.cgi?id=866390 - the behaviour is inhereted from Firefox which does exactly the same thing wrong.
The last post says: "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."
So, maybe, instead of breaking our brains, it may already fixed in the v24 alpha/beta tree? Seems like they want to include a domain based whitelist/blacklist for the plugins to be enabled or click-to-play.

Comment 7

[Frank Wein [:mcsmurf]]

Firefox 24 has a new, bit different UI regarding how enabling/blocking plugin works. When you click on a click-to-play blocked plugin there, a doorhanger UI (that's the GUI that opens when click the plugin icon in the URL bar) opens where you can choose to enable/block this plugin type and remember that decision for this page. But if you're interested in that, I suggest you download a current Firefox beta build :).
The problem with migrating plugins.click_to_play pref probably cannot be solved though. The problem is the core code changed the meaning of the plugins.click_to_play pref. These days setting plugins.click_to_play to true basically means: Activate the click-to-play UI in the about:addons plugins UI. But in our code we don't know from what version you updated from, though the application could make some guesses (but in a sane way I think).

Comment 8

[Frank Wein [:mcsmurf]]

Attachment: Patch

Neil: This patch fixes the pref window and also enables the CTP UI under about:addons by setting click_to_play to true (I see no reason why we should hide the CTP feature there). This patch is not quite accurate as it ignores the difference between 0 and 1 for plugin.default.state (though 0 can only happen when a user manually sets that pref). The checkbox only gets checked when the pref has the "enable all plugins" state. This patch is branch-safe as it does not change any strings. Not sure if we should handle the "0" pref value separately. I don't think many users will use that, after all there is still the plugins.disabled pref.

Comment 9

[[e-mail]]

Comment on attachment 792824 [details] [diff] [review]
Patch

So, on trunk, should we follow up by changing to something that will cope with a value of 0 too?

>+                onsynctopreference="return document.getElementById('scripts_pane').WritePluginDefaultState()"/>
Note: you can just write "return this.checked ? 2 : 1;" here. You have various choices for onsyncfrompreferences, such as "return document.getElementById(this.getAttribute('preference')).value == 2;".

Comment 10

[[e-mail]]

Comment on attachment 792824 [details] [diff] [review]
Patch

r=me with above nits fixed.

Comment 11

[Frank Wein [:mcsmurf]]

Attachment: Patch for checkin

Ah, good ideas to shorten the code like that.

Yes, for trunk I have additional ideas in mind. But there probably the "plugins.disable" pref then needs to be removed from the GUI. Explaining the difference to the user between plugins.disable = true and plugins.default.state=0 is not worth it.

Comment 12

[Frank Wein [:mcsmurf]]

Pushed: https://hg.mozilla.org/comm-central/rev/f50955f8a0c3

Comment 13

[Frank Wein [:mcsmurf]]

Comment on attachment 792866 [details] [diff] [review]
Patch for checkin

[Approval Request Comment]
Regression caused by (bug #): Bug 866390
User impact if declined: Preference in pref window does not behave like it should
Testing completed (on m-c, etc.): Works fine on mozilla-central
Risk to taking this patch (and alternatives if risky): Small patch, very low risk, changes some XUL code only.
String changes made by this patch: -

Comment 14

[Frank Wein [:mcsmurf]]

Pushed to aurora: https://hg.mozilla.org/releases/comm-aurora/rev/839a532b06b8
Pushed to beta: https://hg.mozilla.org/releases/comm-beta/rev/6513e29c9aff

=> FIXED, I'll open a new bug for better UI in prefs

Joachim: Unfortunately we could not migrate the old click_to_play pref, but at least we've fixed the UI now. The problem is Mozilla devs decided to change the meaning of that preference which is usually not a good thing to do. This makes pref migration basically too difficult.