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
This sounds like a general core plugin-container click-to-play issue. Does this happen on Firefox desktop too?
I don't know, I don't have a click-to-play desktop build. Since when is click-to-play working on desktop?
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.
(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.
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.
https://tbpl.mozilla.org/?tree=Try&rev=b1bc9153cd4c
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 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!
Created attachment 609926 [details] [diff] [review] patch v2
https://hg.mozilla.org/integration/mozilla-inbound/rev/15a4628f412e
https://hg.mozilla.org/mozilla-central/rev/15a4628f412e
Verified fixed in today's trunk build on the Samsung Galaxy SII.
Comment on attachment 609926 [details] [diff] [review] patch v2 [Approval Request Comment] Mobile only. Beta blocker.
(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 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.
or
