702653 - Add prefs for Flash plugin activation: on demand/always on/off

Status: VERIFIED FIXED

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

Comment 1

[Justin Dolske [:Dolske]]

We'll be wanting this on desktop for sure, maybe Madhava and Shorlander can have a draw-off (or dance-off) for the visual designs. ;-)

Comment 2

[Madhava Enros [:madhava]]

Pending actual mockups (if required here), this is a tri-state pref where, on android, we'll pop up a radio dialog with the three options:


Enable Flash
(o) Yes
( ) Tap to Enable
( ) No

Comment 4

[Justin Dolske [:Dolske]]

Oh, I misunderstood this. Bug 549697 is doing the click-to-play stuff, this bug is just some basic prefs so doesn't really matter for Desktop. Though, fwiw, 549697 is implementing support in this for addons-mgr, I'm not sure how much of that Fennec gets for free. And since plugins are a special case on Fennec anyway (just Flash, right), that might not even be useful.

Comment 5

[:Margaret Leibovic]

(In reply to Justin Dolske [:Dolske] from comment #4)
> Oh, I misunderstood this. Bug 549697 is doing the click-to-play stuff, this
> bug is just some basic prefs so doesn't really matter for Desktop. Though,
> fwiw, 549697 is implementing support in this for addons-mgr, I'm not sure
> how much of that Fennec gets for free. And since plugins are a special case
> on Fennec anyway (just Flash, right), that might not even be useful.

I've been trying to sort out the dependencies on bug 549697, and we definitely need the basic platform support that's in progress in that bug in order to get this working on mobile. However, the WIP platform patch in bug 549697 is trying to do more than we need on mobile (and more than I think we really need in a first step for desktop), so I think I will try to file separate smaller bugs that can break up that work. Stay tuned!

(AIUI, the addons manager part of bug 549697 wouldn't apply to mobile, since it would be support for per-plugin prefs, and we only have one plugin on mobile right now)

Comment 6

[:Margaret Leibovic]

The part of bug 549697 that we depended on is now split off into bug 707886.

Comment 7

[:Margaret Leibovic]

I'm working on a patch for this.

Comment 9

[:Margaret Leibovic]

Attachment: WIP

This works, but after changing the pref, I'm seeing the integer pref value appear below "Enable Flash" in the main preference UI. When I come back to the Preferences interface, it's gone, so it only happens right after editing the value. I'll attach a screenshot to show what's happening.

Also, I ended up using a string type for this fake "plugin.enable" pref because that's what made the UI work for me, but maybe there's a way to get it to work using an integer type?

Comment 10

[:Margaret Leibovic]

Attachment: screenshot

See the "1" under Enable Flash? That appears after I change the value, but I don't want it to ever be there.

Comment 11

[:Margaret Leibovic]

Oh, I see, it's because we do this .setSummary() when the pref changes:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoPreferences.java#135

Since this is going to be the only ListPreference, I think we could just get rid of this line. Even if we wanted that summary to appear, we should probably set it up when we initialize the pref UI, not just after a pref changes.

Comment 12

[:Margaret Leibovic]

Attachment: patch

This patch does what I suggest in comment 11 :)

Comment 13

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

Comment on attachment 581000 [details] [diff] [review]
patch

* Can we move setPluginPreference/getPluginPreference to the PluginHelper? I don't like adding more and more to BrowserApp if possible.
* We really do need the List Summary. Can we map the values to the display string?
r- for the list summary bit

Comment 14

[:Margaret Leibovic]

Attachment: patch v2

Getting the string for the pref entry in onPreferenceChange is kinda gross, but that's the only way I found to get the entry that corresponds to newValue (we can't use .getEntry() because the pref hasn't changed yet when the event is fired).

Comment 15

[Brian Nicholson (:bnicholson)]

Comment on attachment 581055 [details] [diff] [review]
patch v2

Looks fine to me. I just wish we had better adapter logic for Fennec prefs -> UI prefs; it seems like every preference we add is a special case that requires more code fudging.

Comment 16

[:Margaret Leibovic]

https://hg.mozilla.org/mozilla-central/rev/41ceaff2e5f1

Now all we need is bug 707886 to land, and this feature should be pretty good to go (modulo any bugs that pop up).

Comment 17

[Jim Jeffery not reading bug-mail 1/2/11]

    Is this going to be on the desktop or is mobile only ?  If its mobile only, is there a separate bug for desktop ?

Comment 18

[:Margaret Leibovic]

(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #17)
>     Is this going to be on the desktop or is mobile only ?  If its mobile
> only, is there a separate bug for desktop ?

Right now it is only a mobile feature, but the platform work helps with the desktop feature. The desktop work is being tracked in bug 549697.

Comment 19

[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

Comment 20

[Camelia Urian]

Testcase added under BFT - options (preferences):
https://litmus.mozilla.org/show_test.cgi?id=50496