Documents in which Flash is blacklisted should not advertise Flash's existence in navigator.plugins.

@bsmedberg Let me know if I should request additional/different reviewers.

Comment on attachment 8832220 [details] Bug 1323064 - Remove Flash from navigator.plugins when Flash is blacklisted in the document https://reviewboard.mozilla.org/r/108568/#review109950 This is not the right way to do this. We can safely assume that Flash is the only plugin, so we don't need to check for Flash in particular any more. With that, this should all happen through nsPluginArray::AllowPlugins() -> nsDocShell::PluginsAllowedInCurrentDoc() -> nsDocument::GetAllowPlugins and you should just add a DocumentFlashClassification check in nsDocument::GetAllowPlugins. ::: dom/base/nsPluginArray.cpp:334 (Diff revision 1) > // This only supports one hidden plugin > return Preferences::GetCString("plugins.navigator.hidden_ctp_plugin").Equals(aName); > } > > +static bool > +PluginIsFlash(nsPluginTag* aPlugin) { Formatting nit, opening brace of a function should be in column 1 of the next line.

Comment on attachment 8832220 [details] Bug 1323064 - Remove Flash from navigator.plugins when Flash is blacklisted in the document https://reviewboard.mozilla.org/r/108568/#review109950 Having nsDocument::GetAllowPlugins return False causes Flash to be blocked via a different code path resulting in a Plugin Fallback Type of PLUGIN_USER_DISABLED instead of PLUGIN_SUPPRESSED. Is that ok?

Is there any difference is user-visible behavior? Or does this affect telemetry at all? Not having PLUGIN_SUPPRESSED seems fine to me.

I believe the user-visible behavior is the same. I am not sure how it affects telemetry.

I mean the telemetry you and Felipe are specifically adding for this shield study. I'm confident it doesn't affect any other telemetry.

Comment on attachment 8832220 [details] Bug 1323064 - Remove Flash from navigator.plugins when Flash is blacklisted in the document https://reviewboard.mozilla.org/r/108568/#review110388 LGTM. The outparam in nsDocument::GetAllowPlugins is ugly but let's not fix that here!

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/8649cbdc0969 Remove Flash from navigator.plugins when Flash is blacklisted in the document r=bsmedberg

Comment on attachment 8832220 [details] Bug 1323064 - Remove Flash from navigator.plugins when Flash is blacklisted in the document 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]: Flash will be present in navigator.plugins on pages that are strictly denied from loading Flash. [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]: Not for the feature independently. We'll do QE on the study as a whole to make sure all pieces work as expected [List of other uplifts needed for the feature/fix]: bug 1307604 [Is the change risky?]: No [Why is the change risky/not risky?]: Outside of test code, only adds a check to nsDocument::GetAllowPlugins. This check will only change behavior when Flash blocking is turned on and the page is denied from loading Flash. [String changes made/needed]: none

Comment on attachment 8832220 [details] Bug 1323064 - Remove Flash from navigator.plugins when Flash is blacklisted in the document For shield study purpose. Aurora53+.

Comment on attachment 8832220 [details] Bug 1323064 - Remove Flash from navigator.plugins when Flash is blacklisted in the document this was deemed too risky for beta