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.

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

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

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.

FWIW I'm working on tests right now. Just wanted to get those patches posted earlier so you can start looking

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?

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

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.

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

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

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

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

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?

That comment was incorrect, this study is still targeted at 52 if possible.

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