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

VERIFIED FIXED

Status

()

Firefox for Android
General
P1
normal
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: aaronmt, Assigned: Margaret)

Tracking

({uiwanted})

unspecified
ARM
Android
uiwanted
Points:
---
Bug Flags:
in-litmus +

Firefox Tracking Flags

(firefox11 fixed, firefox13 verified, fennec11+)

Details

(Whiteboard: [QA!])

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
Keywords: uiwanted
Assignee: nobody → madhava
Depends on: 549697
Priority: -- → P1
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. ;-)
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
(Reporter)

Updated

6 years ago
Duplicate of this bug: 707178
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.
(Assignee)

Comment 5

6 years ago
(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)
(Assignee)

Updated

6 years ago
Depends on: 707886
(Assignee)

Comment 6

6 years ago
The part of bug 549697 that we depended on is now split off into bug 707886.
No longer depends on: 549697
(Assignee)

Comment 7

6 years ago
I'm working on a patch for this.
Assignee: madhava → margaret.leibovic
(Assignee)

Updated

6 years ago
Duplicate of this bug: 709425
(Assignee)

Comment 9

5 years ago
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?
Attachment #580994 - Flags: feedback?(bnicholson)
(Assignee)

Comment 10

5 years ago
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.
(Assignee)

Comment 11

5 years ago
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.
(Assignee)

Comment 12

5 years ago
Created attachment 581000 [details] [diff] [review]
patch

This patch does what I suggest in comment 11 :)
Attachment #580994 - Attachment is obsolete: true
Attachment #580996 - Attachment is obsolete: true
Attachment #580994 - Flags: feedback?(bnicholson)
Attachment #581000 - Flags: review?(mark.finkle)
Attachment #581000 - Flags: review?(bnicholson)
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
Attachment #581000 - Flags: review?(mark.finkle) → review-
(Assignee)

Comment 14

5 years ago
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).
Attachment #581000 - Attachment is obsolete: true
Attachment #581000 - Flags: review?(bnicholson)
Attachment #581055 - Flags: review?(mark.finkle)
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.
Attachment #581055 - Flags: feedback+
Attachment #581055 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 16

5 years ago
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).
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Updated

5 years ago
Flags: in-litmus?(fennec)
Whiteboard: [QA+]
    Is this going to be on the desktop or is mobile only ?  If its mobile only, is there a separate bug for desktop ?
(Assignee)

Comment 18

5 years ago
(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.

Updated

5 years ago
Summary: Add prefs for flash activation on demand/always on/off → Add prefs for Flash plugin activation: on demand/always on/off
tracking-fennec: --- → 11+
status-firefox11: --- → fixed
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
Status: RESOLVED → VERIFIED
status-firefox13: --- → verified
Whiteboard: [QA+] → [QA!]

Comment 20

5 years ago
Testcase added under BFT - options (preferences):
https://litmus.mozilla.org/show_test.cgi?id=50496
Flags: in-litmus?(fennec) → in-litmus+
You need to log in before you can comment on or make changes to this bug.