707886 - Platform support for non-e10s click-to-play plugins

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[:Margaret Leibovic]

Attachment: WIP

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

[:Margaret Leibovic]

Comment on attachment 579245 [details] [diff] [review]
WIP

Josh, what do you think of this approach? Also will your patch in bug 90268 affect this work, apart from bit-rot?

Comment 2

[:Margaret Leibovic]

Attachment: patch

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

[Johnny Stenback (:jst)]

Comment on attachment 579810 [details] [diff] [review]
patch

+nsObjectLoadingContent::PlayPlugin()
+{
+  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 4

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9b9d90ff8e1

Comment 5

[:Margaret Leibovic]

Backed out due to bustage :(

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b4a90374698

I'll try fixing it and push to try server instead of mozilla-inbound.

Comment 6

[:Margaret Leibovic]

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

[Brad Lassey [:blassey] (use needinfo?)]

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

[Brad Lassey [:blassey] (use needinfo?)]

pushed that to try https://tbpl.mozilla.org/?tree=Try&rev=645ad4589609

Comment 9

[:Margaret Leibovic]

Attachment: 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

[:Margaret Leibovic]

(In reply to Brad Lassey [:blassey] from comment #8)
> pushed that to try https://tbpl.mozilla.org/?tree=Try&rev=645ad4589609

Oops, I didn't notice this. I pushed my patch to try also:
https://tbpl.mozilla.org/?tree=Try&rev=424b32fb6529.

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Brad Lassey [:blassey] from comment #8)
> pushed that to try https://tbpl.mozilla.org/?tree=Try&rev=645ad4589609

this ran green

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

pushed https://hg.mozilla.org/mozilla-central/rev/bc84b3376e14

Comment 13

[Andrew]

Click not working.

Comment 14

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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