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

Comment on attachment 783379 [details] [diff] [review] Test fixup part 1 - dom/plugins Try shows that i missed reftest & crashtest.

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

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

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.

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

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.

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.

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

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

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

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

(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 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 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

> > + // 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.

(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.

(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.

(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 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.

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 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.

(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.

(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 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.

(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.

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

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.

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

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

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

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.

Ah, thanks - that sounds obvious & right. https://tbpl.mozilla.org/?tree=Try&rev=ce95ce5c87f2

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

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?

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

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

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

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.

(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

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

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.

(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?

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.

Ok, would this work for you?

Comment on attachment 798875 [details] [diff] [review] Test fixup part 5 - layout, v2 Review of attachment 798875 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

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

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

(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)

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.