739575 - Alternate content is shown for Flash objects when "Tap to Play" is enabled for plugins

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

I guess this is also a Core->Plugins issue (like bug 739048), rather than Fennec issue, so filing it there.

See testcase, make sure you have "Tap to Play" for plugins enabled in your settings in Fennec to reproduce it.

Expected result:
- A "Tap here to activate plugin" placeholder is shown

Actual result:
- A link is shown which comes from the alternate content inside the <object> tag.

Tested on the Samsung Galaxy Nexus, Android 4.0.2.

Comment 1

[Dana Keeler (she/her) [:keeler]]

Attachment: proposed fix

In nsObjectLoadingContent::GetPluginSupportState, if aContent is determined to have alternate content, ePluginOtherState is returned. This prevents ePluginClickToPlay from being returned (which results in the "click/tap to play" box not being shown). This patch returns ePluginClickToPlay if that's what the actual plugin support state is, and follows the old behavior otherwise.

Comment 3

[Josh Aas]

Comment on attachment 614629 [details] [diff] [review]
proposed fix

Review of attachment 614629 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsObjectLoadingContent.cpp
@@ +1991,5 @@
>    }
>  
> +  PluginSupportState pluginDisabledState = GetPluginDisabledState(aContentType);
> +  if (pluginDisabledState == ePluginClickToPlay) return pluginDisabledState;
> +  else return (hasAlternateContent ? ePluginOtherState : pluginDisabledState);

Please use the if/else structure used elsewhere in this code, and always with scope braces.

if () {
  ...
} else {
  ...
}

Comment 4

[Dana Keeler (she/her) [:keeler]]

Attachment: fix v2 + test case

Asking for review again to make sure I interpreted your feedback correctly. Also, I included a mochi test case.

Comment 5

[Dana Keeler (she/her) [:keeler]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=3a1b3ce0b0bc

Comment 6

[Dana Keeler (she/her) [:keeler]]

Attachment: fix + test

Re-uploading patch to properly set title (to prepare for checkin-needed).
Carrying over r+ from joshmoz.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5e876a76c57

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/c5e876a76c57

Comment 9

[alex_mayorga]

Works now on Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:15.0) Gecko/20120429 Firefox/15.0a1 ID:20120429040300