Add a mechanism to control plugin fallback content

RESOLVED FIXED in Firefox 52

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Felipe, Assigned: Felipe)

Tracking

(Depends on 2 bugs, Blocks 1 bug)

Trunk
mozilla54
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox52 fixed, firefox53 fixed, firefox54 fixed)

Details

Attachments

(4 attachments)

We want to have a mechanism to make Flash <object> content to prefer its fallback content even when Flash is installed, based on some rules. This bug is about implementing the mechanism itself, which should easily support adding new rules, and then these rules should be part of different bugs.

If activated (through a pref), the mechanism should call into the list of rules and run them. It should then default to prefer the fallback content unless any of the rules indicated that it should use the plugin.

This probably will happen in nsIObjectLoadingContent.

It shold also be easily toggable per site/session and measured, as detailed in the dependent bugs.
Blocks: 1282485
Blocks: 1282486
Blocks: 1282487
Blocks: 1282489
Blocks: 1282492
Assignee: nobody → felipc
Status: NEW → ASSIGNED
Priority: -- → P1
Depends on: 1090576
No longer depends on: 1282489
Depends on: 1310887
Depends on: 1316102
Duplicate of this bug: 1310887
Here's a pastebin with all the patches here and from the related rules bugs merged: https://pastebin.mozilla.org/8926279
Comment on attachment 8808755 [details]
Bug 1282484 - Add a mechanism to control plugin fallback content.

https://reviewboard.mozilla.org/r/91480/#review91602

::: browser/app/profile/firefox.js:649
(Diff revision 1)
> +// "always"     - always use favor fallback mode
> +// "follow-ctp" - activate if ctp is active for the given
> +//                plugin object (could be due to a plugin-wide
> +//                setting or a site-specific setting)
> +// "never"      - never use favor fallback mode
> +pref("plugins.favorfallback.mode", "always");

note: default state here to land will be follow-ctp or never.. Still to be decided

::: dom/base/nsObjectLoadingContent.cpp:3590
(Diff revision 1)
> +
> +  if (!thisContent->IsHTMLElement(nsGkAtoms::object)) {
> +    return false;
> +  }
> +
> +  // xxx to be filled

This will be filled by other patches already posted
Attachment #8808755 - Flags: review?(kyle)
Attachment #8808756 - Flags: review?(kyle)
Attachment #8808757 - Flags: review?(kyle)
Can you mark all of the "plugin fallback rule" bug as duplicate against this one and move those patches into this bug, and possible just compile all of the rules patches into one patch? This review is way too hard to reason about when it's split across 5 bugs, not to mention you have patches that bridge bugs in odd orders (bug 1282486 has patches that only work after bug 1282487 is applied, patches in bug 1282487 won't even compile, etc.)
Flags: needinfo?(felipc)
Sure. I posted a pastebin in comment 5 with that, but I'll bring everything to this bug. I was hoping MozReview would make it easier to see the correct patch order, but not really, it doesn't.
Flags: needinfo?(felipc)
FWIW I'm working on tests right now. Just wanted to get those patches posted earlier so you can start looking
Duplicate of this bug: 1282485
Duplicate of this bug: 1282486
Duplicate of this bug: 1282487
Duplicate of this bug: 1316102
Comment on attachment 8808755 [details]
Bug 1282484 - Add a mechanism to control plugin fallback content.

https://reviewboard.mozilla.org/r/91480/#review91656

Removing myself from review until new patches are done. Just r? me when those are ready to go. I've got some comments on these that I'll leave, nothing big though.

::: browser/app/profile/firefox.js:649
(Diff revision 1)
> +// "always"     - always use favor fallback mode
> +// "follow-ctp" - activate if ctp is active for the given
> +//                plugin object (could be due to a plugin-wide
> +//                setting or a site-specific setting)
> +// "never"      - never use favor fallback mode
> +pref("plugins.favorfallback.mode", "always");

This needs to be added to modules/libpref/init/all.js too.

::: dom/base/nsObjectLoadingContent.cpp:3566
(Diff revision 1)
> +    if (aIsPluginClickToPlay &&
> +        prefString.EqualsLiteral("follow-ctp")) {
> +      return true;
> +    }
> +
> +    if (prefString.EqualsLiteral("always")) {

Nit: Why not combine this with the logic above? If it's for readability of the two cases here, I guess that's ok.

::: dom/base/nsObjectLoadingContent.cpp:3586
(Diff revision 1)
> +    // Don't let custom fallback handlers run outside HTML, tags without a
> +    // determined type should always just be alternate content
> +    return false;
> +  }
> +
> +  if (!thisContent->IsHTMLElement(nsGkAtoms::object)) {

Nit: Why not combine this with the logic above?
Attachment #8808755 - Flags: review?(kyle)
Comment on attachment 8808756 [details]
Bug 1282484 - Cache the value of PreferFallback() because ShouldPlay() is called several times during the load process.

https://reviewboard.mozilla.org/r/91482/#review91666

::: browser/app/profile/firefox.js:653
(Diff revision 1)
>  // "never"      - never use favor fallback mode
>  pref("plugins.favorfallback.mode", "always");
>  
> +// The rules to follow when deciding whether an object has been
> +// provided with good fallback content.
> +pref("plugins.favorfallback.rules", "");

Needs to be added to modules/libpref/init/all.js
Attachment #8808756 - Flags: review?(kyle)
Oops, ok, just saw the rules additions in the 3rd patch, so .these patches did get updated and I didn't notice. Sorry, still kinda new to review board. I'll try to get these reviewed today.
Comment on attachment 8808755 [details]
Bug 1282484 - Add a mechanism to control plugin fallback content.

https://reviewboard.mozilla.org/r/91480/#review92392

r+ with nits from last review fixed.
Attachment #8808755 - Flags: review+
Comment on attachment 8808756 [details]
Bug 1282484 - Cache the value of PreferFallback() because ShouldPlay() is called several times during the load process.

https://reviewboard.mozilla.org/r/91482/#review92394
Attachment #8808756 - Flags: review+
Comment on attachment 8808757 [details]
Bug 1282484 - Write structure for pref-configurable fallback rules list, and include initial set of rules.

https://reviewboard.mozilla.org/r/91484/#review92398
Attachment #8808757 - Flags: review?(kyle) → review+
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f31082d1426
Add a mechanism to control plugin fallback content. r=qDot
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ff237346a69
Cache the value of PreferFallback() because ShouldPlay() is called several times during the load process. r=qDot
https://hg.mozilla.org/integration/mozilla-inbound/rev/babe5113f1ec
Write structure for pref-configurable fallback rules list, and include initial set of rules. r=qDot
https://hg.mozilla.org/mozilla-central/rev/9f31082d1426
https://hg.mozilla.org/mozilla-central/rev/2ff237346a69
https://hg.mozilla.org/mozilla-central/rev/babe5113f1ec
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Approval Request Comment
[Feature/Bug causing the regression]: This feature is needed for the Shield study to be run on Release 52 (bug 1335232), which we'll use to study the effect of making flash click-to-play by default.
[User impact if declined]: Can't run the study as intended
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: Bug 1307604 will also need to be uplifted for this study. But they don't need to be landed at the same time.
[Is the change risky?]: Somewhat yes, but only if enabled.
[Why is the change risky/not risky?]: This feature will not be enabled for release users, except a few that are recruited through Shield and offered to opt-in into the study.
[String changes made/needed]: none
Attachment #8832542 - Flags: review+
Attachment #8832542 - Flags: approval-mozilla-beta?
Attachment #8832542 - Flags: approval-mozilla-aurora?
Comment on attachment 8832542 [details] [diff] [review]
rollup patch for uplift, r=qDot

The meta bug (bug 1335232) mentions aiming this Shield study at 53. 
Are you changing that to 52 release instead?
Flags: needinfo?(felipc)
Attachment #8832542 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
That comment was incorrect, this study is still targeted at 52 if possible.
Flags: needinfo?(felipc)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #26)
> Comment on attachment 8832542 [details] [diff] [review]
> rollup patch for uplift, r=qDot
> 
> The meta bug (bug 1335232) mentions aiming this Shield study at 53. 
> Are you changing that to 52 release instead?

Sorry, that was a typo. The aim is 52 indeed
Comment on attachment 8832542 [details] [diff] [review]
rollup patch for uplift, r=qDot

We need this on 52 for a Shield study, let's try it with beta 4.
Attachment #8832542 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Duplicate of this bug: 1307606
Blocks: 1364505
You need to log in before you can comment on or make changes to this bug.