755062 - It is too easy to enable flash for sites

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Kevin Brosnan [Ex-Mozilla]]

From the release sign off enabling Flash is too easy. The door hanger tends to get the user to enable flash for the whole site.

Comment 1

[Matt Brubeck (:mbrubeck)]

Click-to-play in desktop Firefox just puts an icon in the urlbar.  The doorhanger doesn't appear unless you click the icon.  We could do the same in Fennec, especially if we implemented a general method for collapsing and re-showing doorhangers (bug 700437).

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Margaret - What do you think about hiding the doorhanger if any flash plugin is ivisible on the page. So we only show the doorhanger if the plugins on the page are not visible or too small to show the placeholder.

Comment 3

[:Margaret Leibovic]

This is a tricky issue. You'll want to see bug 734323.

Previously, we would wait until the load event to decide whether or not to show the doorhanger, but the problem was that we wouldn't end up showing the doorhanger if an invisible plugin was added to the DOM after the load event had already fired. The solution to this was to decide whether or not to show the doorhanger for each PluginClickToPlay event.

We could add some logic to see whether or not we've already processed a visible plugin, and not show the doorhanger in that case, but if an invisible plugin is added before a visible one, we'll still end up showing the doorhanger. I think this is probably as good of a solution as we'll be able to get.

Adding Jared in case he has ideas.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Warning: Hack Ahead

What if we delayed sending the doorhanger request in the PluginClickToPlay event. If we saw an invisible plugin and delayed 500ms, and then a visible plugin was found - we could cancel the timeout on the doorhanger request. We would only do the delay while the page was loading, by using "oad" or "pageshow" or something.

I did warn you!

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Yeah, we could do a hack like that. On another note, we could remove the checked-by-default site-wide enabling of Flash.

Comment 6

[Kevin Brosnan [Ex-Mozilla]]

UX had issue with that as the user would get this notification every time they went to a site (with flash).

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Then it looks like we will just have to bump up the priority for bug 700437.

Comment 8

[:Margaret Leibovic]

If we're going to need bug 700437 to fix this completely, I don't think this should be a beta blocker. I think we could try to land mfinkle's hack idea for beta, but bug 700437 feels to risky at this point, especially since the UX for it isn't spec-ed out yet.

Comment 9

[:Margaret Leibovic]

Attachment: only show the doorhanger if visible plugins don't get added in the near future

The logic is a little tricksy here, so I tried to make sure the comments explained things well enough.

To test this, I made a test page that includes an invisible plugin, then adds a visible plugin after the page has finished loading: http://people.mozilla.com/~mleibovic/test/invisible_and_visible_embeds.html.

I also checked some of Martijn's many test cases to make sure this didn't regress other behavior: http://people.mozilla.org/~mwargers/tests/plugins/flash/.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 626094 [details] [diff] [review]
only show the doorhanger if visible plugins don't get added in the near future

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

::: mobile/android/chrome/content/browser.js
@@ +2120,5 @@
> +          if (!this.pluginDoorhangerTimeout) {
> +            this.pluginDoorhangerTimeout = setTimeout(function() {
> +              if (!this.clickToPlayPluginDoorhangerShown && !this.pluginOverlayVisible)
> +                PluginHelper.showDoorHanger(this);
> +            }.bind(this), 500);

We're going to need to clearTimeout on this on pagehide.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 626094 [details] [diff] [review]
only show the doorhanger if visible plugins don't get added in the near future

Agreed about the clearTimeout comment. Also, could we somehow combine clickToPlayPluginDoorhangerShown and pluginOverlayVisible into a single flag? I cringe at adding one-off boolean flags.

Comment 12

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #11)
> Comment on attachment 626094 [details] [diff] [review]
> only show the doorhanger if visible plugins don't get added in the near
> future
> 
> Agreed about the clearTimeout comment. Also, could we somehow combine
> clickToPlayPluginDoorhangerShown and pluginOverlayVisible into a single
> flag? I cringe at adding one-off boolean flags.

We use clickToPlayPluginDoorhangerShown to decide if we need to make a NativeWindow.doorhanger.hide() call. If we combine the flags, we'd end up making that call a lot more, which means a message to Java, and I'm not sure if it's worth it.

Actually, I just discovered that we can run into a NPE in Tab.removeDoorHanger() if we try to remove a non-existent doorhanger, and that's pretty bad, especially since that's theoretically possible in our current code. I'll file a bug to fix that ASAP.

Comment 13

[:Margaret Leibovic]

(In reply to Margaret Leibovic [:margaret] from comment #12)

> Actually, I just discovered that we can run into a NPE in
> Tab.removeDoorHanger() if we try to remove a non-existent doorhanger, and
> that's pretty bad, especially since that's theoretically possible in our
> current code. I'll file a bug to fix that ASAP.

Nevermind, I'm dumb. HashMap's remove method won't throw, so this isn't an issue.

Comment 14

[:Margaret Leibovic]

Attachment: updated patch

This patch combines the flags into a single shouldShowPluginDoorhanger flag. However, this has the negative quality that we'll always be making a call to hide the doorhanger when the user clicks on an overlay to activate a plugin (keeping a check for !shouldShowPluginDoorhanger doesn't really make sense, since that should always be true if the user is clicking on a visible plugin).

Up to you, I can switch back to the two flags, or keep this as is

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 626281 [details] [diff] [review]
updated patch

I'm OK with sending the message to hide the doorhanger unconditionally. It shouldn't be a performance impact. The Tab class is becoming large (lots of data memebrs) and I worry about maintainability. I can file a new bug about re-organizing it a bit.

Comment 16

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e017a0a0f6b

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/8e017a0a0f6b

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 626281 [details] [diff] [review]
updated patch

[Triage Comment]

Comment 19

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6a19f69f6f95

Comment 20

[Cristian Nicolae (:xti)]

This works exactly like it is described in comment #15 on all branches.
Closing bug as verified fixed on:

Firefox 15.0a1 (2012-05-31)
Firefox 14.0a2 (2012-05-31)
Firefox 14.0b4 (2012-05-31)

Device: Samsung Galaxy Nexus
OS: Android 4.0.2