:Felipe Gomes (needinfo for replies!) Assignee
	Description • 8 years ago

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.

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 5 • 8 years ago

Here's a pastebin with all the patches here and from the related rules bugs merged: https://pastebin.mozilla.org/8926279

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 6 • 8 years ago
mozreview-review

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

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)
	Comment 7 • 8 years ago

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

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 8 • 8 years ago

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.

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 12 • 8 years ago

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

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)
	Comment 17 • 8 years ago
mozreview-review

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?

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)
	Comment 18 • 8 years ago
mozreview-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/#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

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)
	Comment 19 • 8 years ago

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.

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)
	Comment 20 • 8 years ago
mozreview-review

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.

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)
	Comment 21 • 8 years ago
mozreview-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

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)
	Comment 22 • 8 years ago
mozreview-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

	Pulsebot
	Comment 23 • 8 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 24 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/9f31082d1426
https://hg.mozilla.org/mozilla-central/rev/2ff237346a69
https://hg.mozilla.org/mozilla-central/rev/babe5113f1ec

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 25 • 7 years ago

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

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 26 • 7 years ago

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?

	Benjamin Smedberg
	Comment 27 • 7 years ago

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

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 28 • 7 years ago

(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

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 29 • 7 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 30 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/70fa5013cfb8

	Ryan VanderMeulen [:RyanVM]
	Comment 31 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/51e2150d6f00