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

RESOLVED FIXED

Status

()

Core
Plug-ins
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Margaret, Assigned: Margaret)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 579245 [details] [diff] [review]
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).
Attachment #579245 - Flags: feedback?(blassey.bugs)
(Assignee)

Comment 1

6 years ago
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?
Attachment #579245 - Flags: feedback?(joshmoz)
(Assignee)

Comment 2

6 years ago
Created attachment 579810 [details] [diff] [review]
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.
Attachment #579245 - Attachment is obsolete: true
Attachment #579245 - Flags: feedback?(joshmoz)
Attachment #579245 - Flags: feedback?(blassey.bugs)
Attachment #579810 - Flags: review?(blassey.bugs)
(Assignee)

Updated

6 years ago
Blocks: 708464
Attachment #579810 - Flags: review?(blassey.bugs) → review?(joshmoz)
Attachment #579810 - Flags: review?(joshmoz) → review?(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.
Attachment #579810 - Flags: review?(jst) → review+
(Assignee)

Comment 4

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9b9d90ff8e1
(Assignee)

Comment 5

6 years ago
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.
(Assignee)

Comment 6

6 years ago
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 :(
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);
pushed that to try https://tbpl.mozilla.org/?tree=Try&rev=645ad4589609
(Assignee)

Comment 9

6 years ago
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.
Attachment #580656 - Flags: review?(blassey.bugs)
(Assignee)

Comment 10

6 years ago
(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.
(In reply to Brad Lassey [:blassey] from comment #8)
> pushed that to try https://tbpl.mozilla.org/?tree=Try&rev=645ad4589609

this ran green
Attachment #580656 - Flags: review?(blassey.bugs) → review+
(Assignee)

Updated

6 years ago
Attachment #579810 - Attachment is obsolete: true
pushed https://hg.mozilla.org/mozilla-central/rev/bc84b3376e14
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 13

6 years ago
Click not working.
(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).
Depends on: 720400
You need to log in before you can comment on or make changes to this bug.