Last Comment Bug 707886 - Platform support for non-e10s click-to-play plugins
: Platform support for non-e10s click-to-play plugins
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: unspecified
: All All
-- normal with 1 vote (vote)
: ---
Assigned To: :Margaret Leibovic
: Benjamin Smedberg [:bsmedberg]
Depends on: 720400
Blocks: 702653 708464
  Show dependency treegraph
Reported: 2011-12-05 23:18 PST by :Margaret Leibovic
Modified: 2012-01-28 15:21 PST (History)
13 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

WIP (16.50 KB, patch)
2011-12-05 23:18 PST, :Margaret Leibovic
no flags Details | Diff | Splinter Review
patch (17.92 KB, patch)
2011-12-07 13:11 PST, :Margaret Leibovic
jst: review+
Details | Diff | Splinter Review
updated patch (19.03 KB, patch)
2011-12-10 09:33 PST, :Margaret Leibovic
blassey.bugs: review+
Details | Diff | Splinter Review

Description User image :Margaret Leibovic 2011-12-05 23:18:32 PST
Created attachment 579245 [details] [diff] [review]

Right now there is e10s-specific code in place to support click-to-play plugins in XUL fennec, but this approach won't work for fennec native or desktop Firefox, so it needs to be changed.

This bug is just about adding the basic support for click-to-play, and we can work in follow-up bugs to create more fine-tuned preferences like the ones described in bug 549697. Also, since it seems like some of these preferences are only wanted on desktop, we shouldn't block on them for mobile.

I still need to add a way to pref this on/off, but this WIP patch works (tested on desktop with a slimmed down version of the front-end patch in bug 549697).
Comment 1 User image :Margaret Leibovic 2011-12-06 11:25:26 PST
Comment on attachment 579245 [details] [diff] [review]

Josh, what do you think of this approach? Also will your patch in bug 90268 affect this work, apart from bit-rot?
Comment 2 User image :Margaret Leibovic 2011-12-07 13:11:14 PST
Created attachment 579810 [details] [diff] [review]

I'll add the mobile UI in another bug, but this patch includes a pref that disables this by default. Tested on desktop, flash plays normally.
Comment 3 User image Johnny Stenback (:jst, 2011-12-08 17:08:09 PST
Comment on attachment 579810 [details] [diff] [review]

+  mHasBeenClickedToPlay = true;
+  return LoadObject(mURI, true, mContentType, true);

This as written lets a webpage QI an embed or what not to nsIObjectLoadingContent and call playPlugin() (separate bug, but no fix for that in the near future), which I don't think we want to permit. I'd say we should check that the caller if this method is chrome, and if that's not the case do nothing. IOW, ad a check for nsContentUtils::IsCallerChrome() at the very top of this method and return NS_OK if that returns false.

r=jst with that fixed.
Comment 5 User image :Margaret Leibovic 2011-12-09 14:59:17 PST
Backed out due to bustage :(

I'll try fixing it and push to try server instead of mozilla-inbound.
Comment 6 User image :Margaret Leibovic 2011-12-09 18:03:46 PST
So, the problem was caused by setting the default value of aHasBeenClickedToPlay to false in the methods I changed in nsPluginHost. Changing that to true fixed the bustage, since that enables the plugins as expected. However, that also breaks the click to play functionality, so I'm going to have to invest some more effort in this to fix it :(
Comment 7 User image Brad Lassey [:blassey] (use needinfo?) 2011-12-10 08:29:05 PST
How about:

nsresult IsPluginEnabledForType(const char* aMimeType) { return IsPluginEnabledForType(aMimeType, !mozilla::Preferences::GetBool("plugins.click_to_play", false)); }
nsresult IsPluginEnabledForExtension(const char* aExtension, const char* &aMimeType) { return IsPluginEnabledForExtension(aExtension, aMimeType, !mozilla::Preferences::GetBool("plugins.click_to_play", false)); }
nsresult IsPluginEnabledForType(const char* aMimeType,
                                bool aHasBeenClickedToPlay);
nsresult IsPluginEnabledForExtension(const char* aExtension, const char* &aMimeType,
                                     bool aHasBeenClickedToPlay);
Comment 8 User image Brad Lassey [:blassey] (use needinfo?) 2011-12-10 09:07:45 PST
pushed that to try
Comment 9 User image :Margaret Leibovic 2011-12-10 09:33:49 PST
Created attachment 580656 [details] [diff] [review]
updated patch

Brad, is this what you meant? It works for me locally (testing on desktop), but I'm going to push it to try to make sure it doesn't cause any bustage.

I also updated the variable names from aHasBeenClickedToPlay/mHasBeenClickedToPlay to aShouldPlay/mShouldPlay, since that's less confusing. And I moved the pref from firefox.js to all.js, since that's where it should have been in the first place.
Comment 10 User image :Margaret Leibovic 2011-12-10 09:44:58 PST
(In reply to Brad Lassey [:blassey] from comment #8)
> pushed that to try

Oops, I didn't notice this. I pushed my patch to try also:
Comment 11 User image Brad Lassey [:blassey] (use needinfo?) 2011-12-10 11:08:09 PST
(In reply to Brad Lassey [:blassey] from comment #8)
> pushed that to try

this ran green
Comment 12 User image Brad Lassey [:blassey] (use needinfo?) 2011-12-14 11:36:42 PST
Comment 13 User image Andrew 2011-12-17 12:20:21 PST
Click not working.
Comment 14 User image David Keeler [:keeler] (use needinfo?) 2011-12-17 12:25:13 PST
(In reply to Andrew from comment #13)
> Click not working.

If you're on mobile, this might be from bug 710885. For desktop, see bug 711552 (the UI hasn't landed, so you'll need a patch to enable it).

Note You need to log in before you can comment on or make changes to this bug.