Closed Bug 899080 Opened 6 years ago Closed 6 years ago

Make plugins default to click-to-play

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla26
Tracking Status
relnote-firefox --- 26+

People

(Reporter: gfritzsche, Assigned: gfritzsche)

References

Details

(5 keywords)

Attachments

(8 files, 7 obsolete files)

1.09 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
2.63 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.70 KB, patch
roc
: review+
Details | Diff | Splinter Review
115.50 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
3.79 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
3.92 KB, patch
surkov
: review+
Details | Diff | Splinter Review
2.18 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
6.12 KB, patch
roc
: review+
Details | Diff | Splinter Review
We want to have all plugins except Flash default to click-to-play.
Attached patch Test fixup part 1 - dom/plugins (obsolete) — Splinter Review
Attachment #783379 - Flags: review?(benjamin)
Comment on attachment 783379 [details] [diff] [review]
Test fixup part 1 - dom/plugins

Try shows that i missed reftest & crashtest.
Attachment #783379 - Flags: review?(benjamin)
Next attempt, this should fix everything i've seen fail so far:
https://tbpl.mozilla.org/?tree=Try&rev=fdbd28d07d5b
Whiteboard: [needs Mozmill test update]
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.
Attachment #783379 - Attachment is obsolete: true
Attachment #784884 - Flags: review?(jmaher)
Attached patch Test fixup part 2 - dom/plugins (obsolete) — Splinter Review
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.
Attachment #784885 - Flags: review?(benjamin)
Attachment #784884 - Attachment description: testfix-1-reftest.patch → Test fixup part 1 - always enable test plugins in reftest/crashtest
Attached patch Test fixup part 3 - toolkit (obsolete) — Splinter Review
bsmedberg, hg log identified you as the most likely reviewer here.
Attachment #784888 - Flags: review?(benjamin)
Attachment #784888 - Attachment description: Test fixup part 2 - toolkit → Test fixup part 3 - toolkit
smaug, hg history suggest you may be a good reviewer here?
Attachment #784891 - Flags: review?(bugs)
Attached patch Test fixup part 5 - layout (obsolete) — Splinter Review
roc, can you review the layout test changes?
Is there maybe a common shared utility js file i could put things into?
Attachment #784893 - Flags: review?(roc)
Can we make the test plugin always be exempt from click-to-play? Except for the tests that are explicitly testing click-to-play?
Attachment #784894 - Flags: review?(roc)
Attached patch Test fixup part 7 - accessible (obsolete) — Splinter Review
Attachment #784897 - Flags: review?(surkov.alexander)
Attachment #784891 - Flags: review?(bugs) → review+
(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.
Attachment #784884 - Flags: review?(jmaher) → review-
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
Attachment #784897 - Flags: review?(surkov.alexander) → review+
> > +    // 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?
Flags: needinfo?(benjamin)
Attachment #783377 - Flags: review?(benjamin) → review+
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.
Attachment #784885 - Flags: review?(benjamin) → review-
Attachment #784888 - Flags: review?(benjamin) → review+
Flags: needinfo?(benjamin)
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?
Attachment #784885 - Attachment is obsolete: true
Attachment #786453 - Flags: review?(benjamin)
Depends on: 902075
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.
Attachment #786453 - Flags: review?(benjamin) → review+
(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.
Duplicate of this bug: 902219
(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.
Attachment #784893 - Flags: review?(roc) → review+
(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.
Status: NEW → ASSIGNED
Whiteboard: [needs Mozmill test update]
jmaher, does that look better?
Note that this adds resetting the plugins enabledState to their previous values.
Attachment #784884 - Attachment is obsolete: true
Attachment #789748 - Flags: review?(jmaher)
Duplicate of this bug: 906251
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.
Attachment #789748 - Flags: review?(jmaher) → review+
Depends on: 910266
No longer depends on: 910266
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.
Attachment #784897 - Attachment is obsolete: true
Attachment #797296 - Flags: review?(surkov.alexander)
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
Flags: needinfo?(dtownsend+bugmail)
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
Flags: needinfo?(dtownsend+bugmail)
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?
Attachment #797296 - Flags: review?(surkov.alexander) → review+
Does that look better for you Alexander? I added setTestPluginEnabledState() in common.js for setting the required enabled state.
Attachment #797296 - Attachment is obsolete: true
Attachment #797804 - Flags: review?(surkov.alexander)
Added "onPropertyChanged", "userDisabled" to the expected test_2 events.
Attachment #784888 - Attachment is obsolete: true
Attachment #797807 - Flags: review?(benjamin)
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
Attachment #797804 - Flags: review?(surkov.alexander) → review+
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?
Flags: needinfo?(roc)
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.
Flags: needinfo?(roc)
Ok, would this work for you?
Attachment #784893 - Attachment is obsolete: true
Attachment #798875 - Flags: review?(roc)
Comment on attachment 798875 [details] [diff] [review]
Test fixup part 5 - layout, v2

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

Thanks!
Attachment #798875 - Flags: review?(roc) → review+
Attachment #797807 - Flags: review?(benjamin) → review+
relnote-firefox: ? → ---
Keywords: feature
Target Milestone: mozilla26 → ---
relnote-firefox: --- → ?
Target Milestone: --- → mozilla26
(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)
Status: RESOLVED → VERIFIED
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.
Keywords: sec-want
Depends on: 926830
QA Contact: paul.silaghi
You need to log in before you can comment on or make changes to this bug.