Last Comment Bug 725286 - Plugin placeholder text for the unsupported platforms is misleading
: Plugin placeholder text for the unsupported platforms is misleading
Status: RESOLVED FIXED
: late-l10n
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: mozilla17
Assigned To: :Margaret Leibovic
:
Mentors:
http://people.mozilla.org/~mwargers/t...
: 751706 (view as bug list)
Depends on: 777805
Blocks: 744060
  Show dependency treegraph
 
Reported: 2012-02-08 05:51 PST by Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
Modified: 2012-08-03 10:02 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
-
15+


Attachments
WIP (10.75 KB, patch)
2012-07-25 13:13 PDT, :Margaret Leibovic
blassey.bugs: feedback+
Details | Diff | Review
patch (25.10 KB, patch)
2012-07-25 17:33 PDT, :Margaret Leibovic
no flags Details | Diff | Review
patch (14.10 KB, patch)
2012-07-25 17:34 PDT, :Margaret Leibovic
blassey.bugs: review+
dolske: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Review

Description Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2012-02-08 05:51:34 PST
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)
Comment 1 :Margaret Leibovic 2012-02-08 10:04:58 PST
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.
Comment 2 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2012-02-27 10:10:03 PST
So does this bug need to be moved into toolkit? Or Core->Plugins?
I guess this would also have l10n implications?
Comment 3 :Margaret Leibovic 2012-02-27 17:11:37 PST
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.
Comment 4 Brad Lassey [:blassey] (use needinfo?) 2012-07-25 09:43:00 PDT
changing the summary to refocus this bug on handling the blocked tegras
Comment 5 Axel Hecht [:Pike] 2012-07-25 11:47:04 PDT
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.
Comment 6 :Margaret Leibovic 2012-07-25 12:00:12 PDT
(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.
Comment 7 :Margaret Leibovic 2012-07-25 13:13:08 PDT
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.
Comment 8 :Margaret Leibovic 2012-07-25 13:22:10 PDT
(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?
Comment 9 Ian Barlow (:ibarlow) 2012-07-25 13:26:49 PDT
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."
Comment 10 Brad Lassey [:blassey] (use needinfo?) 2012-07-25 16:03:24 PDT
(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 11 Brad Lassey [:blassey] (use needinfo?) 2012-07-25 16:06:56 PDT
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.
Comment 12 :Margaret Leibovic 2012-07-25 17:33:57 PDT
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.
Comment 13 :Margaret Leibovic 2012-07-25 17:34:31 PDT
Created attachment 645965 [details] [diff] [review]
patch

Whoops, wrong patch.
Comment 14 Brad Lassey [:blassey] (use needinfo?) 2012-07-25 17:43:27 PDT
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
Comment 15 :Margaret Leibovic 2012-07-25 17:57:35 PDT
Comment on attachment 645965 [details] [diff] [review]
patch

Dolske, can you review the toolkit bits of this? Just adding another plugin failure case.
Comment 16 Justin Dolske [:Dolske] 2012-07-25 19:24:26 PDT
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."?
Comment 17 :Margaret Leibovic 2012-07-25 21:49:49 PDT
(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 :)
Comment 18 :Margaret Leibovic 2012-07-25 21:54:02 PDT
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.
Comment 19 Brad Lassey [:blassey] (use needinfo?) 2012-07-25 21:56:01 PDT
(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.
Comment 20 Ed Morley [:emorley] 2012-07-26 05:07:37 PDT
https://hg.mozilla.org/mozilla-central/rev/f566af469969
Comment 21 Axel Hecht [:Pike] 2012-07-26 05:35:03 PDT
(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.
Comment 22 Ian Barlow (:ibarlow) 2012-07-26 09:34:24 PDT
(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
Comment 23 :Margaret Leibovic 2012-07-26 10:33:27 PDT
(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.
Comment 24 Alex Keybl [:akeybl] 2012-07-26 14:58:40 PDT
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.
Comment 26 :Margaret Leibovic 2012-08-03 10:02:41 PDT
*** Bug 751706 has been marked as a duplicate of this bug. ***

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