899080 - Make plugins default to click-to-play

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

[Georg Fritzsche [:gfritzsche]]

We want to have all plugins except Flash default to click-to-play.

Comment 1

[Georg Fritzsche [:gfritzsche]]

Attachment: Let all plugins except Flash default to click-to-play

Comment 2

[Georg Fritzsche [:gfritzsche]]

Attachment: Test fixup part 1 - dom/plugins

Comment 3

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 783379 [details] [diff] [review]
Test fixup part 1 - dom/plugins

Try shows that i missed reftest & crashtest.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Next attempt, this should fix everything i've seen fail so far:
https://tbpl.mozilla.org/?tree=Try&rev=fdbd28d07d5b

Comment 5

[Jeff Hammel]

I'm not sure what, if any, the effect on mozmill will be; perhaps :whimboo could comment as to that (or :davehunt)?

Comment 6

[Georg Fritzsche [:gfritzsche]]

Via ctalbert this will require mozmill updates but is not extra-ordinary compared to other UI changes.
I'm not sure if i'm supposed to file a separate bug, just flagged it for needing mozmill test updates for now.

Comment 7

[Georg Fritzsche [:gfritzsche]]

Looking good on try now:
https://tbpl.mozilla.org/?tree=Try&rev=768e28e89e97

Patches coming up here later.

Comment 8

[Georg Fritzsche [:gfritzsche]]

Attachment: Test fixup part 1 - always enable test plugins in reftest/crashtest

This sets the test plugins to enabled for reftests & crashtests.
As those tests don't have special powers it's probably best to support their current behavior and make tests for click-to-play behavior etc. mochitests.

jmaher, i see you've previously review changes for reftest.js, so trying you first.

Comment 9

[Georg Fritzsche [:gfritzsche]]

Attachment: Test fixup part 2 - dom/plugins

Unsurprisingly, the biggest set of test-changes is in dom/plugins.
Updating them so they all set the plugin enabled state to what they need.

Comment 10

[Georg Fritzsche [:gfritzsche]]

Attachment: Test fixup part 3 - toolkit

bsmedberg, hg log identified you as the most likely reviewer here.

Comment 11

[Georg Fritzsche [:gfritzsche]]

Attachment: Test fixup part 4 - content

smaug, hg history suggest you may be a good reviewer here?

Comment 12

[Georg Fritzsche [:gfritzsche]]

Attachment: Test fixup part 5 - layout

roc, can you review the layout test changes?
Is there maybe a common shared utility js file i could put things into?

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can we make the test plugin always be exempt from click-to-play? Except for the tests that are explicitly testing click-to-play?

Comment 14

[Georg Fritzsche [:gfritzsche]]

Attachment: Test fixup part 6 - widget

Comment 15

[Georg Fritzsche [:gfritzsche]]

Attachment: Test fixup part 7 - accessible

Comment 16

[Georg Fritzsche [:gfritzsche]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> Can we make the test plugin always be exempt from click-to-play? Except for
> the tests that are explicitly testing click-to-play?

We can probably always set it to enabled up in the test harness, but to me it seemed best having the tests set the state they expect explicitly (especially with defaults possibly changing or state set unexpectedly from previous tests).
I'm not fixed on that though if that's undesired.

Comment 17

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 784884 [details] [diff] [review]
Test fixup part 1 - always enable test plugins in reftest/crashtest

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

::: layout/tools/reftest/reftest.js
@@ +281,5 @@
>      document.getElementById("reftest-window").appendChild(gBrowser);
>  #endif
>  
> +    // reftests should have the test plugins enabled, not click-to-play
> +    getTestPlugin().enabledState = CI.nsIPluginTag.STATE_ENABLED;

could you send in "Test Plug-in" as the name to getTestPlugin()?  that would get rid of the assumption of the default name.

@@ +282,5 @@
>  #endif
>  
> +    // reftests should have the test plugins enabled, not click-to-play
> +    getTestPlugin().enabledState = CI.nsIPluginTag.STATE_ENABLED;
> +    getTestPlugin("Second Test Plug-in").enabledState = CI.nsIPluginTag.STATE_ENABLED;

we can return null from getTestPlugin, this seems like a recipe for disaster and random oranges in the future.  Please adjust this to either use a try/catch or check the return value.

Comment 18

[alexander :surkov (:asurkov)]

Comment on attachment 784897 [details] [diff] [review]
Test fixup part 7 - accessible

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

::: accessible/tests/mochitest/elm/test_plugin.html
@@ +22,5 @@
> +    for (var tag of pluginTags) {
> +      if (tag.name == "Test Plug-in") {
> +        tag.enabledState = SpecialPowers.Ci.nsIPluginTag.STATE_ENABLED;;
> +      }
> +    }

it'd be good to have a short comment what it's for

Comment 19

[Benjamin Smedberg]

> > +    // reftests should have the test plugins enabled, not click-to-play
> > +    getTestPlugin().enabledState = CI.nsIPluginTag.STATE_ENABLED;
> 
> could you send in "Test Plug-in" as the name to getTestPlugin()?  that would
> get rid of the assumption of the default name.

Not without some platform ifdefs, because the name of the test plugin changes per-platform.

Comment 20

[Georg Fritzsche [:gfritzsche]]

(In reply to Joel Maher (:jmaher) from comment #17)
> > +    // reftests should have the test plugins enabled, not click-to-play
> > +    getTestPlugin().enabledState = CI.nsIPluginTag.STATE_ENABLED;
> 
> could you send in "Test Plug-in" as the name to getTestPlugin()?  that would
> get rid of the assumption of the default name.

Can do, but note this would just move the assumption from one function to another. Note that if we'd ever change the name of the test plugin  (unlikely) we would have to fix quite a few places across the code-base.

Comment 21

[Georg Fritzsche [:gfritzsche]]

(In reply to Benjamin Smedberg  [:bsmedberg] PTO until 4-Aug from comment #19)
> > could you send in "Test Plug-in" as the name to getTestPlugin()?  that would
> > get rid of the assumption of the default name.
> 
> Not without some platform ifdefs, because the name of the test plugin
> changes per-platform.

We use the "nice" plugin name to find its tag, not the filename, so this works across platforms.

Comment 22

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #16)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> > Can we make the test plugin always be exempt from click-to-play? Except for
> > the tests that are explicitly testing click-to-play?
> 
> We can probably always set it to enabled up in the test harness, but to me
> it seemed best having the tests set the state they expect explicitly
> (especially with defaults possibly changing or state set unexpectedly from
> previous tests).
> I'm not fixed on that though if that's undesired.

bsmedberg, dom/plugins is the only module that has a bigger number of tests affected by these patches - would you prefer the harnesses changing plugin states from the defaults here?

Comment 23

[Benjamin Smedberg]

Comment on attachment 784885 [details] [diff] [review]
Test fixup part 2 - dom/plugins

I don't mind setting the pref for each test, but we do need to set it back to its original value when the test completes. I'd add a function in utils.js to use SimpleTest.registerCleanupFunction to set and reset the value all in one function.

Comment 24

[Georg Fritzsche [:gfritzsche]]

Attachment: Test fixup part 2 - dom/plugins - v2

Moved setting and cleanup to one helper function. This should cover everything now.

I'll probably hold off with everything until after you're back from PTO?

Comment 25

[Benjamin Smedberg]

Comment on attachment 786453 [details] [diff] [review]
Test fixup part 2 - dom/plugins - v2

If you'd like to take the telemetry bug and land it all, I'm ok with that.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #16)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> > Can we make the test plugin always be exempt from click-to-play? Except for
> > the tests that are explicitly testing click-to-play?
> 
> We can probably always set it to enabled up in the test harness, but to me
> it seemed best having the tests set the state they expect explicitly
> (especially with defaults possibly changing or state set unexpectedly from
> previous tests).
> I'm not fixed on that though if that's undesired.

The current state for all tests (apart from the tests explicitly testing click-to-play) is that they expect the plugin to be enabled. It seems to me that any tests that are trying to test plugins, other than testing click-to-play, would want the plugin to be enabled. I don't see why we'd ever want to change that default for tests.

Comment 28

[Georg Fritzsche [:gfritzsche]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #26)
> The current state for all tests (apart from the tests explicitly testing
> click-to-play) is that they expect the plugin to be enabled. It seems to me
> that any tests that are trying to test plugins, other than testing
> click-to-play, would want the plugin to be enabled. I don't see why we'd
> ever want to change that default for tests.

Shouldn't our tests avoid assuming non-default behavior where possible? Otherwise we might get surprised later.
As long as it's a reasonable effort we can explicitly state per-test what behavior it expects (which makes reading and updating settings easier later on as well).
The only bigger amount of changes for this is in dom/plugins, all other modules don't have an unreasonable amount of changes for this (e.g. the ones in browser/ already set up their expected plugin states).

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 784893 [details] [diff] [review]
Test fixup part 5 - layout

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

I think forcing all plugin tests to include this boilerplate code is silly, but I don't care enough to keep fighting it.

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Jeff Hammel [:jhammel] from comment #5)
> I'm not sure what, if any, the effect on mozmill will be; perhaps :whimboo
> could comment as to that (or :davehunt)?

The only plugin we make use of in our Mozmill tests is Flash at the moment. So I don't think anything has to be changed. But thanks for noting this change! I really appreciate that.

Comment 31

[Georg Fritzsche [:gfritzsche]]

Attachment: Test fixup part 1, v2 - always enable test plugins in reftest/crashtest

jmaher, does that look better?
Note that this adds resetting the plugins enabledState to their previous values.

Comment 33

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 789748 [details] [diff] [review]
Test fixup part 1, v2 - always enable test plugins in reftest/crashtest

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

thanks for this.

Comment 34

[Georg Fritzsche [:gfritzsche]]

Rebased & pushed to try to be safe:
https://tbpl.mozilla.org/?tree=Try&rev=38ecf3a69e9e

Comment 35

[Georg Fritzsche [:gfritzsche]]

Attachment: Test fixup part 7 - accessible, v2

test_HTMLSpec.html is apparently new, i gave it the same treatment as test_plugin.html.

Comment 36

[Georg Fritzsche [:gfritzsche]]

Hm, there is a new failure in toolkit/mozapps/extensions/test/xpcshell/test_plugins.js.
I don't really get what this is trying to tell me, Mossop do you have an idea? 

https://tbpl.mozilla.org/php/getParsedLog.php?id=27171688&tree=Try&full=1#error1

> Too many events for {e2c52c1c-5ee1-cc23-15fa-35945fd58806} - See following stack:
> JS frame :: [...]/tests/xpcshell/tests/toolkit/mozapps/extensions/test/xpcshell-unpack/head_addons.js :: getExpectedEvent :: line 804
> JS frame :: [...]/tests/xpcshell/tests/toolkit/mozapps/extensions/test/xpcshell-unpack/head_addons.js :: AddonListener.onPropertyChanged :: line 826
> JS frame :: resource://gre/modules/AddonManager.jsm :: AMI_callAddonListeners :: line 1197
> JS frame :: resource://gre/modules/AddonManager.jsm :: AMP_callAddonListeners :: line 2107
> JS frame :: resource://gre/modules/PluginProvider.jsm :: <TOP_LEVEL> :: line 343
> JS frame :: /builds/slave/talos-slave/test/build/tests/xpcshell/tests/toolkit/mozapps/extensions/test/xpcshell-unpack/test_plugins.js :: run_test_2 :: line 119
> JS frame :: /builds/slave/talos-slave/test/build/tests/xpcshell/tests/toolkit/mozapps/extensions/test/xpcshell-unpack/test_plugins.js :: run_test_1/</< :: line 105
> JS frame :: resource://gre/modules/AddonManager.jsm :: safeCall :: line 83
> JS frame :: resource://gre/modules/AddonManager.jsm :: getAddonByID_safeCall :: line 1683
> JS frame :: resource://gre/modules/PluginProvider.jsm :: PL_getAddon :: line 119
> JS frame :: resource://gre/modules/AddonManager.jsm :: callProvider :: line 109
> JS frame :: resource://gre/modules/AddonManager.jsm :: getAddonByID_nextObject :: line 1681
> JS frame :: resource://gre/modules/AddonManager.jsm :: AOC_callNext :: line 184
> JS frame :: resource://gre/modules/AddonManager.jsm :: getAddonByID_safeCall :: line 1685
> JS frame :: resource://gre/modules/LightweightThemeManager.jsm :: LightweightThemeManager_getAddonByID :: line 363
> JS frame :: resource://gre/modules/AddonManager.jsm :: callProvider :: line 109
> JS frame :: resource://gre/modules/AddonManager.jsm :: getAddonByID_nextObject :: line 1681
> JS frame :: resource://gre/modules/AddonManager.jsm :: AOC_callNext :: line 184
> JS frame :: resource://gre/modules/AddonManager.jsm :: getAddonByID_safeCall :: line 1685
> JS frame :: resource://gre/modules/XPIProvider.jsm :: getAddonByID_getVisibleAddonForID :: line 3437
> JS frame :: resource://gre/modules/XPIProvider.jsm -> resource://gre/modules/XPIProviderUtils.js :: safeCallback/< :: line 153
> JS frame :: resource://gre/modules/XPIProvider.jsm -> resource://gre/modules/XPIProviderUtils.js :: getRepositoryAddon :: line 134
> JS frame :: resource://gre/modules/XPIProvider.jsm -> resource://gre/modules/XPIProviderUtils.js :: <TOP_LEVEL> :: line 1087

Comment 37

[Benjamin Smedberg]

The test has an "expected events" object:

https://hg.mozilla.org/try/file/38ecf3a69e9e/toolkit/mozapps/extensions/test/xpcshell/test_plugins.js#l113

So it only expects to receive "onDiabling" and "onDisabled" for the test plugin. But it appears that PluginProvider.jsm is also firing an "onPropertyChanged" -> "userDisabled" event:

https://hg.mozilla.org/try/file/38ecf3a69e9e/toolkit/mozapps/extensions/PluginProvider.jsm#l343

If that's expected/normal, then we just need to add that to the expected event array in the test.

Comment 38

[Georg Fritzsche [:gfritzsche]]

Ah, thanks - that sounds obvious & right.

https://tbpl.mozilla.org/?tree=Try&rev=ce95ce5c87f2

Comment 39

[Georg Fritzsche [:gfritzsche]]

Actually: https://tbpl.mozilla.org/?tree=Try&rev=a14ed7f12602

Comment 40

[alexander :surkov (:asurkov)]

Comment on attachment 797296 [details] [diff] [review]
Test fixup part 7 - accessible, v2

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

::: accessible/tests/mochitest/elm/test_HTMLSpec.html
@@ +1246,5 @@
> +    for (var tag of pluginTags) {
> +      if (tag.name == "Test Plug-in") {
> +        tag.enabledState = SpecialPowers.Ci.nsIPluginTag.STATE_ENABLED;;
> +      }
> +    }

can we have enablePlugins() function in common.js?

Comment 41

[Georg Fritzsche [:gfritzsche]]

Attachment: Test fixup part 7 - accessible, v3

Does that look better for you Alexander? I added setTestPluginEnabledState() in common.js for setting the required enabled state.

Comment 42

[Georg Fritzsche [:gfritzsche]]

Attachment: Test fixup part 3 - toolkit, v2

Added "onPropertyChanged", "userDisabled" to the expected test_2 events.

Comment 43

[alexander :surkov (:asurkov)]

Comment on attachment 797804 [details] [diff] [review]
Test fixup part 7 - accessible, v3

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

r=me, thank you for doing this

::: accessible/tests/mochitest/common.js
@@ +757,5 @@
> +/** Sets the test plugin(s) initially expected enabled state.
> + * It will automatically be reset to it's previous value after the test
> + * ends.
> + * @param aNewEnabledState the enabled state, e.g. SpecialPowers.Ci.nsIPluginTag.STATE_ENABLED
> + * @param aPluginName The name of the plugin - optional, defaults to "Test Plug-in"

nit: in a11y we do
@param aParamName  [in] description

Comment 44

[Georg Fritzsche [:gfritzsche]]

All green now:
https://tbpl.mozilla.org/?tree=Try&rev=f72ac6cf70bc

(In reply to alexander :surkov from comment #43)
> nit: in a11y we do
> @param aParamName  [in] description

Oh, i missed that. Will be fixed when landing.

Comment 45

[Brendan Eich [:brendan]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29)
> Comment on attachment 784893 [details] [diff] [review]
> Test fixup part 5 - layout
> 
> Review of attachment 784893 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think forcing all plugin tests to include this boilerplate code is silly,
> but I don't care enough to keep fighting it.

Copy-and-paste of this size, even in tests, is bad practice. No util.js around with which to share a function?

/be

Comment 46

[Brendan Eich [:brendan]]

For victims of Scandinavian banks, lame old plugin-based courseware, etc., how is the usability of CtP? Please point me to other bugs if not tracked here via blocking bugs.

/be

Comment 47

[Benjamin Smedberg]

We believe usability for most things that aren't hidden Flash to be very good now, but I've got telemetry for the UI interactions which I'll use to verify some assumptions we've made along the way. firefox-dev has most of the UX design history. Bug 738698 is the metabug and http://people.mozilla.com/~lco/CtP/current/130415%20CtP%20design%20spec.pdf is the most recent UX spec.

Comment 48

[Georg Fritzsche [:gfritzsche]]

(In reply to Brendan Eich [:brendan] from comment #45)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29)
> > Comment on attachment 784893 [details] [diff] [review]
> > Test fixup part 5 - layout
> > 
> > Review of attachment 784893 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I think forcing all plugin tests to include this boilerplate code is silly,
> > but I don't care enough to keep fighting it.
> 
> Copy-and-paste of this size, even in tests, is bad practice. No util.js
> around with which to share a function?

I don't see one in layout - Robert, would you mind me adding one?

Comment 49

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Just create an enableTestPlugin.js somewhere so that just the act of including that script enabled the test plugin without further API calls. I don't care where it lives.

Comment 50

[Georg Fritzsche [:gfritzsche]]

Attachment: Test fixup part 5 - layout, v2

Ok, would this work for you?

Comment 51

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 798875 [details] [diff] [review]
Test fixup part 5 - layout, v2

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

Thanks!

Comment 52

[Georg Fritzsche [:gfritzsche]]

Still all green on trunk:
https://tbpl.mozilla.org/?tree=Try&rev=93b65e65bc62

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e9fc39801fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/46a5c7cf1ec4
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bcbfbc9591b
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b9221731b0e
https://hg.mozilla.org/integration/mozilla-inbound/rev/af8b8c91286c
https://hg.mozilla.org/integration/mozilla-inbound/rev/1332d461d52e
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c3c7005f6af
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb82f789664b

Comment 53

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0e9fc39801fc
https://hg.mozilla.org/mozilla-central/rev/46a5c7cf1ec4
https://hg.mozilla.org/mozilla-central/rev/3bcbfbc9591b
https://hg.mozilla.org/mozilla-central/rev/4b9221731b0e
https://hg.mozilla.org/mozilla-central/rev/af8b8c91286c
https://hg.mozilla.org/mozilla-central/rev/1332d461d52e
https://hg.mozilla.org/mozilla-central/rev/1c3c7005f6af
https://hg.mozilla.org/mozilla-central/rev/eb82f789664b

Comment 54

[Paul Silaghi, QA [:pauly]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #0)
> We want to have all plugins except Flash default to click-to-play.
Verified fixed Win 7, Mac OS X 10.8.4, Ubuntu 13.04 in nightly 26.0a1 (2013-09-11)

Comment 55

[Andrey K]

(In reply to Georg Fritzsche [:gfritzsche] from comment #0)
> We want to have all plugins except Flash default to click-to-play.

Ha-ha.
we != all

Comment 56

[Asa Dotzler [:asa]]

This needs a good relnote. Quick and dirty is "All plug-ins in Firefox, with the exception of Flash plug-ins, are defaulted to 'click to play'.  This is probably also deserving of a blog post.

Comment 57

[Kohei Yoshino [:kohei]]

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/26/Site_Compatibility