Closed
Bug 739048
Opened 10 years ago
Closed 10 years ago
"Tap here to activate plugin" placeholder shown for unsupported plugin content
Categories
(Core Graveyard :: Plug-ins, defect)
Tracking
(blocking-fennec1.0 beta+)
VERIFIED
FIXED
mozilla14
Tracking | Status | |
---|---|---|
blocking-fennec1.0 | --- | beta+ |
People
(Reporter: martijn.martijn, Assigned: Margaret)
References
()
Details
(Keywords: testcase)
Attachments
(2 files, 1 obsolete file)
102 bytes,
text/html
|
Details | |
1.80 KB,
patch
|
jaas
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
This sounds like a general core plugin-container click-to-play issue. Does this happen on Firefox desktop too?
Reporter | ||
Comment 2•10 years ago
|
||
I don't know, I don't have a click-to-play desktop build. Since when is click-to-play working on desktop?
![]() |
||
Updated•10 years ago
|
blocking-fennec1.0: --- → ?
Updated•10 years ago
|
Assignee: nobody → margaret.leibovic
blocking-fennec1.0: ? → beta+
Assignee | ||
Comment 3•10 years ago
|
||
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.
Component: General → Plug-ins
Product: Fennec Native → Core
QA Contact: general → plugins
(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.
Assignee | ||
Comment 5•10 years ago
|
||
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.
Attachment #609756 -
Flags: review?(joshmoz)
Assignee | ||
Comment 6•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=b1bc9153cd4c
Comment 7•10 years ago
|
||
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!
Attachment #609756 -
Flags: review?(joshmoz)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #609756 -
Attachment is obsolete: true
Attachment #609926 -
Flags: review?(joshmoz)
Attachment #609926 -
Flags: review?(joshmoz) → review+
Assignee | ||
Comment 10•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/15a4628f412e
Target Milestone: --- → mozilla14
Comment 11•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/15a4628f412e
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 12•10 years ago
|
||
Verified fixed in today's trunk build on the Samsung Galaxy SII.
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 13•10 years ago
|
||
Comment on attachment 609926 [details] [diff] [review] patch v2 [Approval Request Comment] Mobile only. Beta blocker.
Attachment #609926 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 14•10 years ago
|
||
(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•10 years ago
|
||
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.
Attachment #609926 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•11 days ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•