Last Comment Bug 687265 - Front-end support for flash on Android Fennec
: Front-end support for flash on Android Fennec
Status: RESOLVED FIXED
[QA+][inbound]
: compat, flashplayer, meta, mobile, pp
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: All Android
: P1 enhancement with 3 votes (vote)
: Firefox 10
Assigned To: Brad Lassey [:blassey] (use needinfo?)
:
Mentors:
Depends on: 630007 695826
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-17 11:36 PDT by Doug Turner (:dougt)
Modified: 2012-01-04 07:29 PST (History)
69 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
WIP 1 (15.49 KB, patch)
2011-09-21 13:04 PDT, Mark Finkle (:mfinkle) (use needinfo?)
no flags Details | Diff | Review
WIP 2 (22.24 KB, patch)
2011-09-27 22:51 PDT, Mark Finkle (:mfinkle) (use needinfo?)
no flags Details | Diff | Review
patch (19.64 KB, patch)
2011-09-28 14:20 PDT, Mark Finkle (:mfinkle) (use needinfo?)
mbrubeck: review+
blassey.bugs: review+
dolske: review-
Details | Diff | Review
patch that passes reftests (12.15 KB, patch)
2011-10-05 22:18 PDT, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Review
patch (14.18 KB, patch)
2011-10-07 10:56 PDT, Brad Lassey [:blassey] (use needinfo?)
dolske: review+
jst: review+
Details | Diff | Review

Description Doug Turner (:dougt) 2011-09-17 11:36:03 PDT
This bugs intent is to focus on the front end pieces of getting flash to work in fennec.


+++ This bug was initially created as a clone of Bug #630007 +++

User-Agent:       Mozilla/5.0 (Windows NT 6.0; rv:2.0b11pre) Gecko/20110129 Firefox/4.0b11pre
Build Identifier: 

Web plugins (e.g. Flash) are not currently used by Firefox for Mobile (Fennec).
Flash is available on Android 2.2 and later (a large proportion of Android Smartphones). If it is available on the phone it is installed on, it should be made use of - enable/disable-able in the add-ons preferences like desktop plugins.

Reproducible: Always

Steps to Reproduce:
1. Go to a webpage that uses a plugin installed on the device
2. Attempt to view/use content
Actual Results:  
Plugin not used, and webpage not displayed as the designer intended as a relult

Expected Results:  
Plugin is used by Fennec to optimize User eXperience.
Comment 1 Mark Finkle (:mfinkle) (use needinfo?) 2011-09-21 13:04:20 PDT
Created attachment 561551 [details] [diff] [review]
WIP 1

WIP of patch originally from Brad and Doug
* Removes any method of activating Flash except besides "click to play"
* Uses "click to play" terminology

Needs:
* Add "click" handler to the entire XML binding, not just the label
* Pass session history when opening the tab in parent process
Comment 2 Mark Finkle (:mfinkle) (use needinfo?) 2011-09-27 22:51:42 PDT
Created attachment 562975 [details] [diff] [review]
WIP 2

This patch adds a visible preference toggle to enable "on-demand" / "click to play" plugins. After you restart, you are able to tap a flas plugin and have the page reopen, with flash active.

Still need to use session history. I t would be nice if we didn't need to restart too. Something in the platform side could listen for a pref change.
Comment 3 Mark Finkle (:mfinkle) (use needinfo?) 2011-09-28 14:20:02 PDT
Created attachment 563179 [details] [diff] [review]
patch

This patch turns on plugins on Android and allows click-to-play. When re load the tab into the parent process, the patch uses the session history to "re-create" the new tab with the same session as the original tab.

Tested on http://people.mozilla.com/~mfinkle/gordon/flash_test.html

(go to the parent folder and click on the flash_test file so you get to test session history)
Comment 4 Mark Finkle (:mfinkle) (use needinfo?) 2011-09-28 14:21:09 PDT
Comment on attachment 563179 [details] [diff] [review]
patch

Brad can check some of the other bits
Comment 5 Matt Brubeck (:mbrubeck) 2011-09-28 14:28:33 PDT
Comment on attachment 563179 [details] [diff] [review]
patch

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

r=mbrubeck for the Fennec parts.  The Android and toolkit parts look good to me also, though I'm not the best person to review all of them.
Comment 6 Brad Lassey [:blassey] (use needinfo?) 2011-09-28 16:12:16 PDT
Comment on attachment 563179 [details] [diff] [review]
patch

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

::: toolkit/themes/pinstripe/mozapps/plugins/pluginProblem.css
@@ +34,5 @@
>  :-moz-type-unsupported .icon[status="ready"] {
>    background-image: url(chrome://mozapps/skin/plugins/contentPluginDownload.png);
>  }
> +:-moz-handler-clicktoplay .icon {
> +  background-image: url(chrome://mozapps/skin/plugins/contentPluginMissing.png);

do we want to get a different icon for click to play?
Comment 7 Mark Finkle (:mfinkle) (use needinfo?) 2011-09-28 21:12:52 PDT
(In reply to Brad Lassey [:blassey] from comment #6)

> > +:-moz-handler-clicktoplay .icon {
> > +  background-image: url(chrome://mozapps/skin/plugins/contentPluginMissing.png);
> 
> do we want to get a different icon for click to play?

I'm sure we will. CC'ing Madhava and Ian.
Comment 8 Dão Gottwald [:dao] 2011-09-29 08:03:23 PDT
(In reply to Mark Finkle (:mfinkle) from comment #7)
> (In reply to Brad Lassey [:blassey] from comment #6)
> 
> > > +:-moz-handler-clicktoplay .icon {
> > > +  background-image: url(chrome://mozapps/skin/plugins/contentPluginMissing.png);
> > 
> > do we want to get a different icon for click to play?
> 
> I'm sure we will. CC'ing Madhava and Ian.

And get dolske or me to review it, if you add an icon. Essentially everything in toolkit/mozapps/plugins should have been reviewed by dolske.
Comment 9 Dão Gottwald [:dao] 2011-09-29 08:05:03 PDT
Comment on attachment 563179 [details] [diff] [review]
patch

>--- a/toolkit/locales/en-US/chrome/mozapps/plugins/plugins.dtd
>+++ b/toolkit/locales/en-US/chrome/mozapps/plugins/plugins.dtd

>+<!ENTITY clickToPlayPlugin                                   "Tap here to activate plugin.">

This string isn't going to make sense for desktop, where we'll presumably want to use the same technique?
Comment 10 Mark Finkle (:mfinkle) (use needinfo?) 2011-09-29 08:13:59 PDT
(In reply to Dão Gottwald [:dao] from comment #9)
> Comment on attachment 563179 [details] [diff] [review] [diff] [details] [review]
> patch
> 
> >--- a/toolkit/locales/en-US/chrome/mozapps/plugins/plugins.dtd
> >+++ b/toolkit/locales/en-US/chrome/mozapps/plugins/plugins.dtd
> 
> >+<!ENTITY clickToPlayPlugin                                   "Tap here to activate plugin.">
> 
> This string isn't going to make sense for desktop, where we'll presumably
> want to use the same technique?

When we get to that place, we can discuss moving the string to a mobile override.
Comment 11 Matt Brubeck (:mbrubeck) 2011-09-29 13:54:22 PDT
Landed on inbound, but backed out because of reftest failures:

https://hg.mozilla.org/integration/mozilla-inbound/rev/39d5bb6c6d4d
https://hg.mozilla.org/integration/mozilla-inbound/rev/555c11e2c459
Comment 12 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-29 14:30:33 PDT
https://hg.mozilla.org/mozilla-central/rev/f8e26a743569

I accidentally pushed this patch to mozilla-central while it was living on inbound.  On the next merge, it will be merged.  Sorry for the mess!
Comment 13 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-29 15:50:56 PDT
Merged from inbound: https://hg.mozilla.org/mozilla-central/rev/39d5bb6c6d4d and also merged the backout: https://hg.mozilla.org/mozilla-central/rev/555c11e2c459
Comment 14 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-29 17:18:21 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #12)
> https://hg.mozilla.org/mozilla-central/rev/f8e26a743569
> 
> I accidentally pushed this patch to mozilla-central while it was living on
> inbound.  On the next merge, it will be merged.  Sorry for the mess!

I had merged this to mozilla-inbound, but didn't realize that the net effect would be relanding this bug.  I backed it out of inbound again:

https://hg.mozilla.org/integration/mozilla-inbound/rev/637bac44ba15

The next inbound merge to central should leave this backed out, since now it's backed out of both branches.
Comment 15 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-29 17:31:03 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #13)
> Merged from inbound: https://hg.mozilla.org/mozilla-central/rev/39d5bb6c6d4d
> and also merged the backout:
> https://hg.mozilla.org/mozilla-central/rev/555c11e2c459

For some reason, merging the backout had not worked as I expected.  I backed out explicitly from mozilla-central again: https://hg.mozilla.org/mozilla-central/rev/dbb129f069b1
Comment 16 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-30 07:38:27 PDT
https://hg.mozilla.org/mozilla-central/rev/637bac44ba15
Comment 17 Justin Dolske [:Dolske] 2011-09-30 10:15:25 PDT
(In reply to Matt Brubeck (:mbrubeck) from comment #11)
> Landed on inbound, but backed out because of reftest failures:

Why did this land on inbound? It lacks review from an appropriate content and toolkit reviewer.
Comment 18 Mark Finkle (:mfinkle) (use needinfo?) 2011-09-30 10:39:56 PDT
(In reply to Justin Dolske [:Dolske] from comment #17)
> (In reply to Matt Brubeck (:mbrubeck) from comment #11)
> > Landed on inbound, but backed out because of reftest failures:
> 
> Why did this land on inbound? It lacks review from an appropriate content
> and toolkit reviewer.

I guess we thought it was Android only so we went through Android reviewers. When doing "shared code, but not used by desktop" we could just land overrides in /mobile like we used too.

For this stuff, when we get the reftest issues figured out, we'll put it up for review with you.
Comment 19 Justin Dolske [:Dolske] 2011-10-04 02:41:22 PDT
Comment on attachment 563179 [details] [diff] [review]
patch

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

I went ahead and did a review of the existing patch. Some additional general comments:

* We need to coordinate what mobile wants to do here with was desktop's doing over in bug 549697. Having two (or more!) people duplicate effort isn't good. I'm not sure where communications broke down, but we should get this sorted out quickly. I'm _guessing_ that mobile is basically looking for something quick and minimal to help with initial testing of Flash/Android... Yes? Is the plan to ship this enabled in 10? Long term need for other plugins, whitelisting, etc? Perhaps have mbrubeck finish up with this patch, and use blair's work in the other bug as the longer-term fix for this?

* Does mobile want to support the case of a click-to-play plugin either being created by script (ie, not in DOM) or being too small for our UI to fit? There's already some code in Firefox's browser.js :: gPluginHandler to deal with showing a notification bar instead of in-content UI for this case (for crashed plugins).

* What, no tests? Perhaps you just forgot to hg commit them. ;) This really should have some testing coverage. If there are serious hurdles to testing with flash on mobile, at least something desktop-only.

::: toolkit/locales/en-US/chrome/mozapps/plugins/plugins.dtd
@@ +20,5 @@
>  <!ENTITY pluginWizard.finalPage.moreInfo.label               "Find out more about Plugins or manually find missing plugins.">
>  <!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.">
> +<!ENTITY clickToPlayPlugin                                   "Tap here to activate plugin.">

As noted by Dao, this string doesn't make sense on desktop.

::: toolkit/mozapps/plugins/content/pluginProblem.xml
@@ +57,5 @@
>          <xul:vbox class="mainBox" flex="1" chromedir="&locale.dir;">
>              <xul:spacer flex="1"/>
>              <xul:box class="icon"/>
>              <html:div class="msg msgUnsupported">&missingPlugin;</html:div>
> +            <html:div class="msg msgClickToPlay" onclick="var evt = document.createEvent('Events'); evt.initEvent('PluginClickToPlay', true, false); this.dispatchEvent(evt);">

The onclick code should go in the frontend's plugin handler. Does mobile have the equivalent of browser.js's gPluginHandler? (We should probably spin that off into a shared jsm anyway).

The notable reason for this is that the user has JS disabled, this code never runs (and this click-to-play doesn't work). By having chrome attach event listeners, it always works.

I suspect that the JS which runs here will need to to adjust the UI to show some kind of feedback to indicate "yes, you tapped this, hold on while we start flash and reload in the chrome process". Possibly as a followup, but let's at least file that now if so.

::: toolkit/themes/pinstripe/mozapps/plugins/pluginProblem.css
@@ +34,5 @@
>  :-moz-type-unsupported .icon[status="ready"] {
>    background-image: url(chrome://mozapps/skin/plugins/contentPluginDownload.png);
>  }
> +:-moz-handler-clicktoplay .icon {
> +  background-image: url(chrome://mozapps/skin/plugins/contentPluginMissing.png);

Definitely want a different icon, and quite possibly a different UI entirely. Fine for initially bringing this up, but let's get a followup filed for this and loop in the relevant visual designers (shorlander did the other icons).
Comment 20 James Willcox (:snorp) (jwillcox@mozilla.com) 2011-10-05 06:52:35 PDT
I can't seem to navigate to other pages from the awesomebar after loading a flash-enabled page.  I think this should block landing the patch until it is resolved.
Comment 21 Brad Lassey [:blassey] (use needinfo?) 2011-10-05 17:55:24 PDT
(In reply to Justin Dolske [:Dolske] from comment #19)
> Comment on attachment 563179 [details] [diff] [review] [diff] [details] [review]
> patch
> 
> Review of attachment 563179 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> I went ahead and did a review of the existing patch. Some additional general
> comments:
> 
> * We need to coordinate what mobile wants to do here with was desktop's
> doing over in bug 549697. Having two (or more!) people duplicate effort
> isn't good. I'm not sure where communications broke down, but we should get
> this sorted out quickly. I'm _guessing_ that mobile is basically looking for
> something quick and minimal to help with initial testing of Flash/Android...
> Yes? Is the plan to ship this enabled in 10? Long term need for other
> plugins, whitelisting, etc? Perhaps have mbrubeck finish up with this patch,
> and use blair's work in the other bug as the longer-term fix for this?

Yes we're looking to get something in sooner rather than later so we can get nightly users testing flash for firefox 10
> 
> * Does mobile want to support the case of a click-to-play plugin either
> being created by script (ie, not in DOM) or being too small for our UI to
> fit? There's already some code in Firefox's browser.js :: gPluginHandler to
> deal with showing a notification bar instead of in-content UI for this case
> (for crashed plugins).
Yes we do, but I figure we can do that in a follow up bug

> 
> * What, no tests? Perhaps you just forgot to hg commit them. ;) This really
> should have some testing coverage. If there are serious hurdles to testing
> with flash on mobile, at least something desktop-only.
I have no idea how we'd test this and I really hope we wouldn't hold up getting this into nightly users hands while we figure that out.
> 
> ::: toolkit/locales/en-US/chrome/mozapps/plugins/plugins.dtd
> @@ +20,5 @@
> >  <!ENTITY pluginWizard.finalPage.moreInfo.label               "Find out more about Plugins or manually find missing plugins.">
> >  <!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.">
> > +<!ENTITY clickToPlayPlugin                                   "Tap here to activate plugin.">
> 
> As noted by Dao, this string doesn't make sense on desktop.
That sounds like something that can be rectified when desktop is ready to land

> 
> ::: toolkit/mozapps/plugins/content/pluginProblem.xml
> @@ +57,5 @@
> >          <xul:vbox class="mainBox" flex="1" chromedir="&locale.dir;">
> >              <xul:spacer flex="1"/>
> >              <xul:box class="icon"/>
> >              <html:div class="msg msgUnsupported">&missingPlugin;</html:div>
> > +            <html:div class="msg msgClickToPlay" onclick="var evt = document.createEvent('Events'); evt.initEvent('PluginClickToPlay', true, false); this.dispatchEvent(evt);">
> 
> The onclick code should go in the frontend's plugin handler. Does mobile
> have the equivalent of browser.js's gPluginHandler? (We should probably spin
> that off into a shared jsm anyway).
we don't have that as far as I know
> 
> The notable reason for this is that the user has JS disabled, this code
> never runs (and this click-to-play doesn't work). By having chrome attach
> event listeners, it always works.
sounds reasonalbe... but, not sure how much I care about users who disable js and I can't imagine someone who disables js wanting to enable flash
> 
> I suspect that the JS which runs here will need to to adjust the UI to show
> some kind of feedback to indicate "yes, you tapped this, hold on while we
> start flash and reload in the chrome process". Possibly as a followup, but
> let's at least file that now if so.
we reload the page when the user taps, so in practice something happens right away
> 
> ::: toolkit/themes/pinstripe/mozapps/plugins/pluginProblem.css
> @@ +34,5 @@
> >  :-moz-type-unsupported .icon[status="ready"] {
> >    background-image: url(chrome://mozapps/skin/plugins/contentPluginDownload.png);
> >  }
> > +:-moz-handler-clicktoplay .icon {
> > +  background-image: url(chrome://mozapps/skin/plugins/contentPluginMissing.png);
> 
> Definitely want a different icon, and quite possibly a different UI
> entirely. Fine for initially bringing this up, but let's get a followup
> filed for this and loop in the relevant visual designers (shorlander did the
> other icons).
yup, I noted needing a new icon in comment 6
Comment 22 Brad Lassey [:blassey] (use needinfo?) 2011-10-05 22:18:01 PDT
Created attachment 565136 [details] [diff] [review]
patch that passes reftests

this patch passed the reftests on android 
https://tbpl.mozilla.org/?tree=Try&rev=c1b093473f19
Comment 23 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-05 22:23:04 PDT
Comment on attachment 565136 [details] [diff] [review]
patch that passes reftests


>diff --git a/embedding/android/GeckoApp.java b/embedding/android/GeckoApp.java
>--- a/embedding/android/GeckoApp.java
>+++ b/embedding/android/GeckoApp.java
>@@ -149,6 +149,7 @@ abstract public class GeckoApp
>     public ArrayList<PackageInfo> mPackageInfoCache = new ArrayList<PackageInfo>();
> 
>     String[] getPluginDirectories() {
>+        long start_time = System.currentTimeMillis();
> 
>         ArrayList<String> directories = new ArrayList<String>();
>         PackageManager pm = this.mAppContext.getPackageManager();
>@@ -264,6 +265,8 @@ abstract public class GeckoApp
>                 directories.add(directory);
>             }
>         }
>+        long end_time = System.currentTimeMillis();
>+        Log.i(LOG_FILE_NAME, "plugin folder: " + (end_time - start_time) + "ms");

Debugging cruft I forgot to remove. Feel free to remove it.
Comment 24 Brad Lassey [:blassey] (use needinfo?) 2011-10-07 10:56:26 PDT
Created attachment 565583 [details] [diff] [review]
patch
Comment 25 Justin Dolske [:Dolske] 2011-10-07 20:14:43 PDT
Comment on attachment 565583 [details] [diff] [review]
patch

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

You should grab one of the content peers to give the /content and /dom stuff a quick once-over, should be trivial.

[Thanks for talking with Blair on IRC, sounds like there's minimal overlap, and the parts that are don't really stop on each other. We're looking at getting other parts of Blair's patch landed sooner so that addon-mananger support might be done by the time you need them.]

::: mobile/chrome/content/browser.js
@@ +1286,5 @@
> +
> +        // Remove the browser from the DOM, effectively killing it's content
> +        parent.removeChild(browser);
> +
> +        // Re-create the browser as non-remote, so plugins work

Do we even push the tab back into the child process, eg after a navigation?

Presumably sessionstore can't save state of tabs while they're running in the parent process, too.

Followups, with comments pointing to bugs?

::: toolkit/themes/pinstripe/mozapps/plugins/pluginProblem.css
@@ +34,5 @@
>  :-moz-type-unsupported .icon[status="ready"] {
>    background-image: url(chrome://mozapps/skin/plugins/contentPluginDownload.png);
>  }
> +:-moz-handler-clicktoplay .icon {
> +  background-image: url(chrome://mozapps/skin/plugins/contentPluginMissing.png);

A noted before, this should have a different glyph. Fine for now, but file followup and comment here.

::: toolkit/themes/winstripe/mozapps/plugins/pluginProblem.css
@@ +34,5 @@
>  :-moz-type-unsupported .icon[status="ready"] {
>    background-image: url(chrome://mozapps/skin/plugins/contentPluginDownload.png);
>  }
> +:-moz-handler-clicktoplay .icon {
> +  background-image: url(chrome://mozapps/skin/plugins/contentPluginMissing.png);

Ditto.
Comment 26 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-07 20:31:30 PDT
(In reply to Justin Dolske [:Dolske] from comment #25)
> > +        // Remove the browser from the DOM, effectively killing it's content
> > +        parent.removeChild(browser);
> > +
> > +        // Re-create the browser as non-remote, so plugins work
> 
> Do we even push the tab back into the child process, eg after a navigation?

Never. Once a tab is local (parent process) or remote (child process) we can't move it to the other side without killing it and re-creating it.

The current state of Flash requires it must run in the parent process, so we need to move the child process tab into the parent. We load it with the session hisroty so the new tab has the history of the old tab.

> Presumably sessionstore can't save state of tabs while they're running in
> the parent process, too.

Not true. Session is saved no matter where the tab lives.
Comment 27 Johnny Stenback (:jst, jst@mozilla.com) 2011-10-10 13:07:18 PDT
Comment on attachment 565583 [details] [diff] [review]
patch

r=jst, but I want Josh to look at this too as he is changing related code in pretty significant ways.
Comment 28 Brad Lassey [:blassey] (use needinfo?) 2011-10-12 14:12:39 PDT
Comment on attachment 565583 [details] [diff] [review]
patch

Talked with jst, since Josh is on PTO we'll just go a head and land this
Comment 29 Brad Lassey [:blassey] (use needinfo?) 2011-10-12 14:25:00 PDT
pushed to inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/658653c5836d
Comment 30 Marco Bonardo [::mak] 2011-10-13 07:27:49 PDT
https://hg.mozilla.org/mozilla-central/rev/658653c5836d

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