Last Comment Bug 739048 - "Tap here to activate plugin" placeholder shown for unsupported plugin content
: "Tap here to activate plugin" placeholder shown for unsupported plugin content
Status: VERIFIED FIXED
: testcase
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: mozilla14
Assigned To: :Margaret Leibovic
:
Mentors:
http://people.mozilla.org/~mwargers/t...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-25 04:31 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2012-04-04 16:03 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta+


Attachments
testcase (102 bytes, text/html)
2012-03-25 04:31 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
patch (1.61 KB, patch)
2012-03-27 09:45 PDT, :Margaret Leibovic
no flags Details | Diff | Splinter Review
patch v2 (1.80 KB, patch)
2012-03-27 15:44 PDT, :Margaret Leibovic
jaas: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2012-03-25 04:31:24 PDT
Created attachment 609104 [details]
testcase

You need to have Plugins set to "Tap to play" in the settings for this bug.
Tested on the Galaxy Nexus, Android 4.0.2, using latest Fennec Native build.

Steps to reproduce:
- Visit testcase

Expected result:
- "A plugin is needed to display this content" placholder on the page

Actual result:
- "Tap here to activate plugin" placeholder on the page, which doesn't do anything when being tapped upon
Comment 1 Aaron Train [:aaronmt] 2012-03-25 12:16:26 PDT
This sounds like a general core plugin-container click-to-play issue. Does this happen on Firefox desktop too?
Comment 2 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-03-25 16:36:08 PDT
I don't know, I don't have a click-to-play desktop build. Since when is click-to-play working on desktop?
Comment 3 :Margaret Leibovic 2012-03-26 17:51:33 PDT
When I tested this myself, after tapping the "Tap here to activate plugin" overlay, the overlay message switched to the "A plugin is needed to display this content" message. I was discussing this with Jared, and it seems this is happening because we need to try to activate the plugin object before we can know if the appropriate plugin type is installed.

We're using the :-moz-state-unsupported pseduo-class to decide whether or not to show the unsupported plugin overlay, and that's determined by the NS_EVENT_STATE_TYPE_UNSUPPORTED state. Following the code paths through MXR, it looks like we're returning NS_ERROR_PLUGIN_CLICKTOPLAY before we check to see if the plugin is actually missing:

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsObjectLoadingContent.cpp#490

So we always end up in the click to play state instead of the unsupported state.

Josh, I was wondering if you knew the right way to go about fixing this issue. Can we just switch around nsObjectLoadingContent::IsPluginEnabledForType to check pluginHost->IsPluginEnabledForType before jumping to conclusions about click to play?

Moving this over to Core->Plugins, since it will affect desktop as well.
Comment 4 Josh Aas 2012-03-27 00:09:47 PDT
(In reply to Margaret Leibovic [:margaret] from comment #3)

> Josh, I was wondering if you knew the right way to go about fixing this
> issue. Can we just switch around
> nsObjectLoadingContent::IsPluginEnabledForType to check
> pluginHost->IsPluginEnabledForType before jumping to conclusions about click
> to play?

If that works and doesn't break anything on try it seems fine to me.
Comment 5 :Margaret Leibovic 2012-03-27 09:45:25 PDT
Created attachment 609756 [details] [diff] [review]
patch

I'll push to try, but this patch was working locally for me. Following the idea in my last comment, it checks to see if the plugin is enabled before returning NS_ERROR_PLUGIN_CLICKTOPLAY.

I also changed a |return false| in there to |return NS_ERROR_FAILURE|, since false isn't a nsresult.
Comment 6 :Margaret Leibovic 2012-03-27 09:55:54 PDT
https://tbpl.mozilla.org/?tree=Try&rev=b1bc9153cd4c
Comment 7 Jared Wein [:jaws] (please needinfo? me) 2012-03-27 11:16:02 PDT
Comment on attachment 609756 [details] [diff] [review]
patch

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

::: content/base/src/nsObjectLoadingContent.cpp
@@ +489,5 @@
> +  nsCOMPtr<nsIPluginHost> pluginHostCOM(do_GetService(MOZ_PLUGIN_HOST_CONTRACTID));
> +  nsPluginHost *pluginHost = static_cast<nsPluginHost*>(pluginHostCOM.get());
> +  if (!pluginHost) {
> +    return NS_ERROR_FAILURE;
> +  }

NS_ENSURE_TRUE(pluginHost, NS_ERROR_FAILURE);
Comment 8 Josh Aas 2012-03-27 14:38:48 PDT
Comment on attachment 609756 [details] [diff] [review]
patch

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

This is perfectly functional as far as I can tell but I would like to see the revision before it goes in.

::: content/base/src/nsObjectLoadingContent.cpp
@@ +489,5 @@
> +  nsCOMPtr<nsIPluginHost> pluginHostCOM(do_GetService(MOZ_PLUGIN_HOST_CONTRACTID));
> +  nsPluginHost *pluginHost = static_cast<nsPluginHost*>(pluginHostCOM.get());
> +  if (!pluginHost) {
> +    return NS_ERROR_FAILURE;
> +  }

No, leave it the way you have it in this patch. That's how we do the checks in plugin code. I don't like NS_ENSURE_TRUE here, it seems too much like we're intending to check for an actual boolean TRUE value as opposed to non-NULL.

@@ +492,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsresult rv = pluginHost->IsPluginEnabledForType(aMIMEType.get());
> +  if (NS_SUCCEEDED(rv) && !mShouldPlay) {

I don't like these in the same if statement, it seems overly clever. Please check for NS_FAILED here and return rv if it failed, then check mShouldPlay if it succeeded. The logic is more clear that way.

Also, please add a comment indicating that the order of operations matters here so that we don't mess it up again in the future.

@@ -492,5 @@
>  
> -  nsCOMPtr<nsIPluginHost> pluginHostCOM(do_GetService(MOZ_PLUGIN_HOST_CONTRACTID));
> -  nsPluginHost *pluginHost = static_cast<nsPluginHost*>(pluginHostCOM.get());
> -  if (!pluginHost) {
> -    return false;

Nice catch!
Comment 9 :Margaret Leibovic 2012-03-27 15:44:16 PDT
Created attachment 609926 [details] [diff] [review]
patch v2
Comment 11 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-03-29 08:41:08 PDT
https://hg.mozilla.org/mozilla-central/rev/15a4628f412e
Comment 12 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-03-31 14:49:14 PDT
Verified fixed in today's trunk build on the Samsung Galaxy SII.
Comment 13 :Margaret Leibovic 2012-04-03 08:27:13 PDT
Comment on attachment 609926 [details] [diff] [review]
patch v2

[Approval Request Comment]
Mobile only. Beta blocker.
Comment 14 :Margaret Leibovic 2012-04-03 08:28:21 PDT
(In reply to Margaret Leibovic [:margaret] from comment #13)
> Comment on attachment 609926 [details] [diff] [review]
> patch v2
> 
> [Approval Request Comment]
> Mobile only. Beta blocker.

Er, mass approval request fail. This is platform code, but it should only affect applications where click to play plugins are enabled, which for now is just mobile.
Comment 15 Alex Keybl [:akeybl] 2012-04-04 16:03:45 PDT
Comment on attachment 609926 [details] [diff] [review]
patch v2

[Triage Comment]
No risk to desktop Aurora because click-to-play is mobile only for now. Approved for Aurora 13.

Note You need to log in before you can comment on or make changes to this bug.