Note: There are a few cases of duplicates in user autocompletion which are being worked on.

"Tap here to activate plugin" placeholder shown for unsupported plugin content

VERIFIED FIXED in mozilla14

Status

()

Core
Plug-ins
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: Margaret)

Tracking

({testcase})

Trunk
mozilla14
ARM
Android
testcase
Points:
---

Firefox Tracking Flags

(blocking-fennec1.0 beta+)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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?
(Reporter)

Comment 2

5 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?
blocking-fennec1.0: --- → ?
Assignee: nobody → margaret.leibovic
blocking-fennec1.0: ? → beta+
(Assignee)

Comment 3

5 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

Comment 4

5 years ago
(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

5 years ago
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.
Attachment #609756 - Flags: review?(joshmoz)
(Assignee)

Comment 6

5 years ago
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 8

5 years ago
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

5 years ago
Created attachment 609926 [details] [diff] [review]
patch v2
Attachment #609756 - Attachment is obsolete: true
Attachment #609926 - Flags: review?(joshmoz)

Updated

5 years ago
Attachment #609926 - Flags: review?(joshmoz) → review+
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/15a4628f412e
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/15a4628f412e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Comment 12

5 years ago
Verified fixed in today's trunk build on the Samsung Galaxy SII.
Status: RESOLVED → VERIFIED
(Assignee)

Comment 13

5 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

5 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 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+
You need to log in before you can comment on or make changes to this bug.