708464 - Create click to play UI for fennec native

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[:Margaret Leibovic]

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]

Attachment: 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]

Attachment: 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?)]

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?)]

> 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]

Attachment: 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]

Attachment: 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]

Attachment: 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?)]

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]

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 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/328dad2d25dd

Comment 12

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

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)]

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