The default bug view has changed. See this FAQ.

Plugin placeholder text for the unsupported platforms is misleading

RESOLVED FIXED in Firefox 15

Status

()

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

People

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

Tracking

({late-l10n})

Trunk
mozilla17
ARM
Android
late-l10n
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox15 fixed, firefox16 fixed, blocking-fennec1.0 -, fennec15+)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
Follow-up from bug 704520, comment 6:
"
So, technically, this is correct. The reason there's no placeholder is because there is a flash plugin on the system an we're loading and initializing it. But, because its honeycomb, we can't draw anything. This patch will prevent us from finding any plugins on honeycomb (and ICS).

Madhava, we might want to have different text in the placeholder for these platforms. Something to the effect of "Plugins are not supported on this platform" rather than "A plugin is needed to display this content"
"

So that is currently the issue on Fennec Native on Honeycomb (Android 3).

A plugin placeholder is shown with the text "A plugin is needed to display this content", instead of something like "Plugins are not supported on this platform".

(the bug for supporting Flash on Honeycomb is bug 687267)
(Assignee)

Comment 1

5 years ago
That text is generated by toolkit code, so we'd need to change this on the platform side to create another error case for "plugins not supported on this platform". I'm not sure how complicated this is.
(Reporter)

Comment 2

5 years ago
So does this bug need to be moved into toolkit? Or Core->Plugins?
I guess this would also have l10n implications?
(Reporter)

Updated

5 years ago
blocking-fennec1.0: --- → ?

Updated

5 years ago
blocking-fennec1.0: ? → -
(Assignee)

Comment 3

5 years ago
Let's move this to Core->Plugins, since it probably requires some platform support to let us know that plugins aren't supported on a given platform.
Component: General → Plug-ins
Product: Fennec Native → Core
QA Contact: general → plugins
(Reporter)

Updated

5 years ago
Blocks: 744060
changing the summary to refocus this bug on handling the blocked tegras
Summary: Plugin placeholder text for the unsupported Honeycomb platform is too vague → Plugin placeholder text for the unsupported platforms is misleading
tracking-fennec: --- → 15+
Keywords: late-l10n
(Assignee)

Updated

5 years ago
Assignee: nobody → margaret.leibovic

Comment 5

5 years ago
Margaret, do you know what you need here?

As Erin/Brad said, string freeze is supposed to be Friday, or in due time for Build 3.
(Assignee)

Comment 6

5 years ago
(In reply to Axel Hecht [:Pike] from comment #5)
> Margaret, do you know what you need here?
> 
> As Erin/Brad said, string freeze is supposed to be Friday, or in due time
> for Build 3.

I'm working on this now and hope to have a patch ready today. I'm just waiting on Ian for a final call on what string we want to use.
(Assignee)

Comment 7

5 years ago
Created attachment 645868 [details] [diff] [review]
WIP

This patch follows Brad's suggestion to check if getenv("MOZ_PLUGIN_PATH")[0] == '\0' to see if we're not supporting plugins on a given platform, since that's what we're doing in GeckoApp to disable these plugins [1]. However, I'm finding that this condition is also true on my Nexus S without flash installed, so I think we need to be checking something else.
Attachment #645868 - Flags: feedback?(blassey.bugs)
(Assignee)

Comment 8

5 years ago
(In reply to Margaret Leibovic [:margaret] from comment #7)
> Created attachment 645868 [details] [diff] [review]
> WIP
> 
> This patch follows Brad's suggestion to check if
> getenv("MOZ_PLUGIN_PATH")[0] == '\0' to see if we're not supporting plugins
> on a given platform, since that's what we're doing in GeckoApp to disable
> these plugins [1].

Thinking about it more, this feels a bit too much like a dirty hack anyway. Can we set a pref or something instead?
So as far as a string is concerned, this feels like one of those "This is embarrassing…" situations, where we are not providing something the user was expecting (never mind Adobe dropping Flash support, or our user being on a crappy device), and the appropriate thing to do would be to own up to it in an apologetically human way:

"Funny story. Flash doesn't quite work on this device. But we're working on it. We promise. (OK, so maybe that wasn't so funny after all.)"

- or - 

"It's not you, it's us. Firefox can't play Flash on this device."
(In reply to Margaret Leibovic [:margaret] from comment #8)
> (In reply to Margaret Leibovic [:margaret] from comment #7)
> > Created attachment 645868 [details] [diff] [review]
> > WIP
> > 
> > This patch follows Brad's suggestion to check if
> > getenv("MOZ_PLUGIN_PATH")[0] == '\0' to see if we're not supporting plugins
> > on a given platform, since that's what we're doing in GeckoApp to disable
> > these plugins [1].
> 
> Thinking about it more, this feels a bit too much like a dirty hack anyway.
> Can we set a pref or something instead?

Alternatively, in getPluginDirectories() if we hit one of the conditions where we are disabling plugins (which I think is only tegra right now) we can set another env var to signify that. Since we run this check on every run, I don't think setting a pref (which would be persistent and would involve disk io) is the right thing to do.
Comment on attachment 645868 [details] [diff] [review]
WIP

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

looks good, though I think setting a separate env var might be better (as suggested in my last comment).

::: content/base/src/nsObjectLoadingContent.cpp
@@ +1221,5 @@
>            state |= NS_EVENT_STATE_HANDLER_CRASHED;
>            break;
>          case ePluginUnsupported:
> +          // Check to see if plugins are entirely disabled on this platform.
> +          if (getenv("MOZ_PLUGIN_PATH")[0] == '\0') {

when checking an env var, you also need to check for null so:

{
   char* plugnPath = getenv("MOZ_PLUGIN_PATH");
   if (plugnPath && plugnPath[0])
....

Also note the curly bracket, since you should scope variables in a case statement.
Attachment #645868 - Flags: feedback?(blassey.bugs) → feedback+
(Assignee)

Comment 12

5 years ago
Created attachment 645964 [details] [diff] [review]
patch

This patch creates a new environment variable if we're blocking plugins. Do we need review from someone else for the content/layout/toolkit bits of this patch?

I still haven't updated the string. I'm not sure how I feel about trying to be really humorous, but I prefer the second string Ian suggested over the first one, mainly because it's shorter. I think we should just get to the point, since there may not be a lot of room for a long string, and there's nothing the user can do about it, anyway (although I guess we do want to avoid 1-star "I HATE YOOOOUUUUU" reviews because of this). I won't land this patch until we come to a decision.
Attachment #645868 - Attachment is obsolete: true
Attachment #645964 - Flags: review?(blassey.bugs)
(Assignee)

Comment 13

5 years ago
Created attachment 645965 [details] [diff] [review]
patch

Whoops, wrong patch.
Attachment #645964 - Attachment is obsolete: true
Attachment #645964 - Flags: review?(blassey.bugs)
Attachment #645965 - Flags: review?(blassey.bugs)
Comment on attachment 645965 [details] [diff] [review]
patch

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

::: mobile/android/base/GeckoAppShell.java
@@ +316,5 @@
>              String[] dirs = context.getPluginDirectories();
> +            // Check to see if plugins were blocked.
> +            if (dirs == null) {
> +                GeckoAppShell.putenv("MOZ_PLUGINS_BLOCKED=1");
> +                return;

should probably call GeckoAppShell.putenv("MOZ_PLUGIN_PATH=") before returning so we don't change behavior
Attachment #645965 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 15

5 years ago
Comment on attachment 645965 [details] [diff] [review]
patch

Dolske, can you review the toolkit bits of this? Just adding another plugin failure case.
Attachment #645965 - Flags: review?(dolske)
Comment on attachment 645965 [details] [diff] [review]
patch

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

r=* I HATE YOOOOUUUUU per comment 12 ;-)

::: content/base/src/nsObjectLoadingContent.cpp
@@ +1226,5 @@
> +          if (pluginsBlocked && pluginsBlocked[0] == '1') {
> +            state |= NS_EVENT_STATE_TYPE_UNSUPPORTED_PLATFORM;
> +          } else {
> +            state |= NS_EVENT_STATE_TYPE_UNSUPPORTED;
> +          }

Would it make sense to move this into nsObjectLoadingContent::GetPluginDisabledState(), and add a ePluginUnsupportedPlatform? Hmm, the more I think about it probably not.

The other random thought would be that since the motivation here is entirely front-end, to not do change anything in platform code. Instead, have the front-end pluginProblem do the envvar check itself, and control message display with an attribute...
  :-moz-type-unsupported .msgUnsupported:not([byPlatform])
  :-moz-type-unsupported .msgUnsupportedPlatform[byPlatform]

But that would be rewriting the whole patch, and I don't feel strongly enough about it to require that. :)

Use PR_GetEnv here.

Finally, I'd suggest only calling getenv() only once (setting a bool for future checks). I'd suspect libc implementations tend not to be very optimized, and so having each plugin instance calling getenv() is probably slightly sucky. Fair to do this in a followup, but would also be simple to do here. :)

::: toolkit/locales/en-US/chrome/mozapps/plugins/plugins.dtd
@@ +21,5 @@
>  <!ENTITY pluginWizard.finalPage.restart.label                "&brandShortName; needs to be restarted for the plugin(s) to work.">
>  
>  <!ENTITY missingPlugin                                       "A plugin is needed to display this content.">
> +<!-- LOCALIZATION NOTE (unsupportedPlatform): Mobile only. Plugins are not supported on some mobile platforms. -->
> +<!ENTITY unsupportedPlatform                                 "Plugins are not supported on this platform.">

"Plugins are not supported on this device."?
Attachment #645965 - Flags: review?(dolske) → review+
(Assignee)

Comment 17

5 years ago
(In reply to Margaret Leibovic [:margaret] from comment #12)
> I won't land this patch until we come to a decision.

I take back what I said. Brad convinced me to just land this now, and we can update the string tomorrow if we want to, since that's the easier part.

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

(In reply to Justin Dolske [:Dolske] from comment #16)

> Finally, I'd suggest only calling getenv() only once (setting a bool for
> future checks). I'd suspect libc implementations tend not to be very
> optimized, and so having each plugin instance calling getenv() is probably
> slightly sucky. Fair to do this in a followup, but would also be simple to
> do here. :)

I wasn't sure about where to store this variable, and we wanted to land this ASAP, so I punted on this. If you suggest how to do it, I can do it in a follow-up :)
Target Milestone: --- → mozilla17
(Assignee)

Comment 18

5 years ago
Comment on attachment 645965 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: users on devices where we chose to block Flash will get a misleading error message
Testing completed (on m-c, etc.): just landed on inbound
Risk to taking this patch (and alternatives if risky): low-risk, just adds a new error case for the plugin problem UI
String or UUID changes made by this patch: one string (an error message for the case where we blocked Flash on a device)

We need to decide on a final string before actually uplifting the patches.
Attachment #645965 - Flags: approval-mozilla-beta?
Attachment #645965 - Flags: approval-mozilla-aurora?
(In reply to Ian Barlow (:ibarlow) from comment #9)
> So as far as a string is concerned, this feels like one of those "This is
> embarrassing…" situations, where we are not providing something the user was
> expecting (never mind Adobe dropping Flash support, or our user being on a
> crappy device), and the appropriate thing to do would be to own up to it in
> an apologetically human way:
> 
> "Funny story. Flash doesn't quite work on this device. But we're working on
> it. We promise. (OK, so maybe that wasn't so funny after all.)"
> 
> - or - 
> 
> "It's not you, it's us. Firefox can't play Flash on this device."

Maybe I'm just not in a joking mood, but I think the humor of the first string might be missing the mark here. This is different than the "oops we crashed" screen in that the users are going to see this all the damn time.

I like the second better, but it still might be more jovial than we want.

It might be good to have a "for more information click here" link that takes those users to a SUMO page explaining the situation.
https://hg.mozilla.org/mozilla-central/rev/f566af469969
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 21

5 years ago
(In reply to Margaret Leibovic [:margaret] from comment #18)
> Comment on attachment 645965 [details] [diff] [review]
> patch
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): n/a
> User impact if declined: users on devices where we chose to block Flash will
> get a misleading error message
> Testing completed (on m-c, etc.): just landed on inbound
> Risk to taking this patch (and alternatives if risky): low-risk, just adds a
> new error case for the plugin problem UI
> String or UUID changes made by this patch: one string (an error message for
> the case where we blocked Flash on a device)
> 
> We need to decide on a final string before actually uplifting the patches.

Note, I'm not sure that changing this string trivial, now that it landed on central, and is exposed to localizers there.

Also, for the uplift it should be noted that this technically breaks string freeze for all apps on aurora/beta, as it's adding a toolkit string. It might only be used on mobile, but all other desktop products will go red for all locales. That'll at least need an extra message.
(In reply to Brad Lassey [:blassey] from comment #19)

> This is different than the "oops we
> crashed" screen in that the users are going to see this all the damn time.

Oh fine, that's a fair point. ;)

> It might be good to have a "for more information click here" link that takes
> those users to a SUMO page explaining the situation.

Alright, but perhaps we can pare it down a bit. Something like:

--

We're very sorry, but Firefox can't play Flash on this device. Learn more »

--

where the 'learn more' link takes users to said sumo page
(Assignee)

Comment 23

5 years ago
(In reply to Ian Barlow (:ibarlow) from comment #22)

> where the 'learn more' link takes users to said sumo page

Do we have a SUMO page to use here? If we don't have one made yet, I'll still need a URL to add for the link. Or I could land the "Learn More..." string, but hide the link until we have a page to use.
Status: RESOLVED → UNCONFIRMED
Ever confirmed: false
Resolution: FIXED → ---
Target Milestone: mozilla17 → ---
(Assignee)

Updated

5 years ago
Status: UNCONFIRMED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
(Assignee)

Updated

5 years ago
Blocks: 777805
Comment on attachment 645965 [details] [diff] [review]
patch

[Triage Comment]
With this approval, it's my understanding that this is the last string to break the FN15 freeze.
Attachment #645965 - Flags: approval-mozilla-beta?
Attachment #645965 - Flags: approval-mozilla-beta+
Attachment #645965 - Flags: approval-mozilla-aurora?
Attachment #645965 - Flags: approval-mozilla-aurora+
(Assignee)

Updated

5 years ago
No longer blocks: 777805
Depends on: 777805
(Assignee)

Comment 25

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/61afffa83517
https://hg.mozilla.org/releases/mozilla-beta/rev/ea1b3c3822cf
status-firefox15: --- → fixed
status-firefox16: --- → fixed
(Assignee)

Updated

5 years ago
Duplicate of this bug: 751706
You need to log in before you can comment on or make changes to this bug.