Last Comment Bug 702653 - Add prefs for Flash plugin activation: on demand/always on/off
: Add prefs for Flash plugin activation: on demand/always on/off
Status: VERIFIED FIXED
[QA!]
: uiwanted
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P1 normal with 1 vote (vote)
: ---
Assigned To: :Margaret Leibovic
:
Mentors:
: 707178 709425 (view as bug list)
Depends on: 707886
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-15 09:15 PST by Aaron Train [:aaronmt]
Modified: 2012-03-05 02:47 PST (History)
19 users (show)
camelia.urian: in‑litmus+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
verified
11+


Attachments
WIP (8.42 KB, patch)
2011-12-12 11:54 PST, :Margaret Leibovic
no flags Details | Diff | Splinter Review
screenshot (67.87 KB, image/png)
2011-12-12 11:56 PST, :Margaret Leibovic
no flags Details
patch (9.30 KB, patch)
2011-12-12 12:08 PST, :Margaret Leibovic
mark.finkle: review-
Details | Diff | Splinter Review
patch v2 (11.32 KB, patch)
2011-12-12 14:26 PST, :Margaret Leibovic
mark.finkle: review+
bnicholson: feedback+
Details | Diff | Splinter Review

Description Aaron Train [:aaronmt] 2011-11-15 09:15:47 PST

    
Comment 1 Justin Dolske [:Dolske] 2011-11-21 17:14:42 PST
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] 2011-11-30 09:49:57 PST
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 3 Aaron Train [:aaronmt] 2011-12-02 08:07:47 PST
*** Bug 707178 has been marked as a duplicate of this bug. ***
Comment 4 Justin Dolske [:Dolske] 2011-12-02 17:53:34 PST
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 2011-12-04 22:15:47 PST
(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 2011-12-05 23:30:31 PST
The part of bug 549697 that we depended on is now split off into bug 707886.
Comment 7 :Margaret Leibovic 2011-12-08 18:13:59 PST
I'm working on a patch for this.
Comment 8 :Margaret Leibovic 2011-12-10 09:48:30 PST
*** Bug 709425 has been marked as a duplicate of this bug. ***
Comment 9 :Margaret Leibovic 2011-12-12 11:54:55 PST
Created attachment 580994 [details] [diff] [review]
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 2011-12-12 11:56:12 PST
Created attachment 580996 [details]
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 2011-12-12 12:03:16 PST
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 2011-12-12 12:08:54 PST
Created attachment 581000 [details] [diff] [review]
patch

This patch does what I suggest in comment 11 :)
Comment 13 Mark Finkle (:mfinkle) (use needinfo?) 2011-12-12 12:15:09 PST
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 2011-12-12 14:26:31 PST
Created attachment 581055 [details] [diff] [review]
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) (PTO through August 1) 2011-12-12 15:44:30 PST
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 2011-12-13 14:31:31 PST
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 2011-12-13 14:36:46 PST
    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 2011-12-13 14:45:20 PST
(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) 2012-03-02 09:29:00 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
Comment 20 Camelia Urian 2012-03-05 02:47:20 PST
Testcase added under BFT - options (preferences):
https://litmus.mozilla.org/show_test.cgi?id=50496

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