Last Comment Bug 708464 - Create click to play UI for fennec native
: Create click to play UI for fennec native
Status: VERIFIED FIXED
[QA!]
: uiwanted
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All Android
: P1 normal (vote)
: ---
Assigned To: :Margaret Leibovic
:
Mentors:
Depends on: 707886 710240 718983 719875 735833
Blocks: 709300
  Show dependency treegraph
 
Reported: 2011-12-07 16:26 PST by :Margaret Leibovic
Modified: 2016-07-29 14:21 PDT (History)
8 users (show)
nhirata.bugzilla: in‑litmus+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
WIP (6.33 KB, patch)
2011-12-08 13:02 PST, :Margaret Leibovic
no flags Details | Diff | Splinter Review
patch (7.13 KB, patch)
2011-12-08 15:39 PST, :Margaret Leibovic
blassey.bugs: review-
mark.finkle: review-
Details | Diff | Splinter Review
patch v2 (7.40 KB, patch)
2011-12-08 20:37 PST, :Margaret Leibovic
no flags Details | Diff | Splinter Review
patch v3 (9.80 KB, patch)
2011-12-09 10:28 PST, :Margaret Leibovic
no flags Details | Diff | Splinter Review
patch v3 (fixed) (9.50 KB, patch)
2011-12-09 10:49 PST, :Margaret Leibovic
mark.finkle: review+
Details | Diff | Splinter Review

Description :Margaret Leibovic 2011-12-07 16:26:49 PST
There are two parts to this UI:

1) A placeholder to fill larger plugin containers in content with some "Tap to play message". This already exists in toolkit for XUL fennec and just needs some listeners hooked up to work right.

2) A doorhanger message for plugins that have no visible container or whose container is too small to display a message. I need help coming up with strings here. Also, it seems like it would be annoying to have multiple doorhanger messages, so perhaps we have one message that can activate all hidden plugins?

It also came up in bug 549697 comment 24 that we may want to play all hidden plugins when a visible plugin is activated, since there may be dependencies on these hidden plugin objects.

Cc'ing Madhava and flagging uiwanted mostly to figure out what we want the doorhanger strings/interaction to be like.
Comment 1 :Margaret Leibovic 2011-12-08 13:02:57 PST
Created attachment 580160 [details] [diff] [review]
WIP

Following a discussion Madhava and I had on IRC, this patch will play all plugin objects if you click on one of them. Also, it will show a doorhanger if there are no visible plugin objects to click on. I need to localize this and figure out an issue with hiding overlays when they're too small, but it works (apply it on top of the patch in bug 707886).
Comment 2 :Margaret Leibovic 2011-12-08 15:39:11 PST
Created attachment 580214 [details] [diff] [review]
patch

We'll need follow-up bugs if we want to change/polish the behavior here, but this feels like a good starting point.
Comment 3 Brad Lassey [:blassey] (use needinfo?) 2011-12-08 18:51:44 PST
Comment on attachment 580214 [details] [diff] [review]
patch

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

r- because I think this is broken when you have multiple tabs open

::: mobile/android/chrome/content/browser.js
@@ +2914,5 @@
>  };
> +
> +var PluginHandler = {
> +  init: function() {
> +    BrowserApp.deck.addEventListener("PluginClickToPlay", this, true);

I'd rather if BrowserApp had a getter for its container element, rather than getting deck itself. I see this being done elsewhere though, so this is just me griping really.

In fact, now that I think of this, I think you really want to keep track of plugins per-tab or per-page.

@@ +2953,5 @@
> +        BrowserApp.deck.removeEventListener("DOMContentLoaded", self, false);
> +
> +        // Only show a doorhanger if there are no clickable overlays showing
> +        if (self._pluginsToPlay.length && !self._showingOverlay)
> +          self.showDoorHanger();

what happens if there is one visible plugin and one hidden or too small one? Does clicking the visible one start them all?
Comment 4 Mark Finkle (:mfinkle) (use needinfo?) 2011-12-08 19:48:29 PST
> r- because I think this is broken when you have multiple tabs open

Agreed

> ::: mobile/android/chrome/content/browser.js
> @@ +2914,5 @@
> >  };
> > +
> > +var PluginHandler = {
> > +  init: function() {
> > +    BrowserApp.deck.addEventListener("PluginClickToPlay", this, true);
> 
> I'd rather if BrowserApp had a getter for its container element, rather than
> getting deck itself. I see this being done elsewhere though, so this is just
> me griping really.

BrowserApp.deck _is_ the getter, but this should be reworked anyway. You should consider holding the page's plugins in an array in the Tab object. Also consider using Tab to listen for the "PluginClickToPlay" event. You could create a PluginHelper to hold the isTooSmall and showDoorhanger methods, since those don't need to be in the Tab object itself.

> In fact, now that I think of this, I think you really want to keep track of
> plugins per-tab or per-page.

Yep and Tab seems like a good place.

> what happens if there is one visible plugin and one hidden or too small one?
> Does clicking the visible one start them all?

I think so.
Comment 5 :Margaret Leibovic 2011-12-08 20:37:01 PST
Created attachment 580302 [details] [diff] [review]
patch v2

Oops, I wrote this after talking to Brad on IRC but before reading review comments. I can re-work it to store the plugins on the tab instead of the browser if you guys think that's better, but I figured I'd post my patch now in case you have more feedback. I'm going to go take a break to get some dinner!

Also, to answer Brad's question, yes, clicking on any plugin starts them all, as per discussion with Madhava.
Comment 6 :Margaret Leibovic 2011-12-09 10:28:14 PST
Created attachment 580458 [details] [diff] [review]
patch v3

This moves the event handling and plugin tracking to Tab. I accidentally clobbered my objdir, so I'm rebuilding now to test.

(Ignore the dump statements, or use them yourself if you apply this to test)
Comment 7 :Margaret Leibovic 2011-12-09 10:49:42 PST
Created attachment 580469 [details] [diff] [review]
patch v3 (fixed)

I needed to switch event to aEvent in the event handler. I also removed the dump statements.

This seems to work, but I'm definitely going to test more.
Comment 8 Mark Finkle (:mfinkle) (use needinfo?) 2011-12-09 11:11:14 PST
Comment on attachment 580469 [details] [diff] [review]
patch v3 (fixed)


>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+var PluginHelper = {
>+  showDoorHanger: function(aTab) {

>+        label: Strings.browser.GetStringFromName("clickToPlayFlash.yes"),
>+        callback: function() {
>+          PluginHandler.playAllPlugins(aTab);

If we move the method into Tab this would be:
aTab.playAllPlugins();

>+  playAllPlugins: function(aTab) {
>+    let plugins = aTab._pluginsToPlay;
>+    for (let i = 0; i < plugins.length; i++) {
>+      let objLoadingContent = plugins[i].QueryInterface(Ci.nsIObjectLoadingContent);
>+      objLoadingContent.playPlugin();
>+    }
>+  },

Move this method into the Tab itself? I think it would be better off there.

>+  addPluginClickCallback: function (plugin, callbackName /*callbackArgs...*/) {

>+    plugin.addEventListener("click",
>+                              function(evt) {
>+                                if (!evt.isTrusted)
>+                                  return;
>+                                evt.preventDefault();
>+                                if (callbackArgs.length == 0)
>+                                  callbackArgs = [ evt ];
>+                                (self[callbackName]).apply(self, callbackArgs);
>+                              },
>+                              true);

I know this is mostly copied but can we format like:

      plugin.addEventListener("click", function(evt) {
        if (!evt.isTrusted)
          return;
        evt.preventDefault();
        if (callbackArgs.length == 0)
          callbackArgs = [ evt ];
        (self[callbackName]).apply(self, callbackArgs);
      }, true);

Just to fit the convention in this file (curse my OCD)

>+    plugin.addEventListener("keydown",

Same

r+ with nits fixed
Comment 9 :Margaret Leibovic 2011-12-09 14:10:46 PST
We talked about moving playAllPlugins to Tab in a follow-up, since that will require refactoring addPluginClickCallback. I filed bug 709300.

https://hg.mozilla.org/integration/mozilla-inbound/rev/328dad2d25dd
Comment 10 Aaron Train [:aaronmt] 2011-12-09 18:48:42 PST
*** Bug 709425 has been marked as a duplicate of this bug. ***
Comment 11 Ed Morley [:emorley] 2011-12-10 20:32:06 PST
https://hg.mozilla.org/mozilla-central/rev/328dad2d25dd
Comment 12 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-02-29 16:24:36 PST
Various litmus test cases revolving around flash and click to play:
https://docs.google.com/spreadsheet/ccc?key=0AocUyLHteCtSdHQ5Q2tIZVhMT3NNY0lPYzhHT2MyZXc#gid=22
Comment 13 Cristian Nicolae (:xti) 2012-03-02 09:02:07 PST
Verified fixed on:

Firefox 13.0a1 (2012-03-02)
20120302031112
http://hg.mozilla.org/mozilla-central/rev/3a7b9e61c263

--
Device: Samsung Galaxy S2
OS: Android 2.3.4

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