All users were logged out of Bugzilla on October 13th, 2018

Flashblock pref shouldn't be enabled if Flash is not installed

VERIFIED FIXED

Status

--
minor
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: bugzilla-graveyard, Assigned: alqahira)

Tracking

({polish})

unspecified
polish
Bug Flags:
camino2.1.1 +

Details

(Whiteboard: [camino-2.1.1])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 years ago
Should we fix the Flashblock-enabling logic to take into account whether the Flash plugin is even installed? Right now, the plugin could be missing and we'll still allow Flashblock to be enabled, which seems sort of silly.
I'm OK with confirming this, though I'm not sure how hard it would be to do.  

PreferenceManager already has code for getting installed plug-ins (as part of |updatePluginEnableState|); we'd just have to see if that array contains "Shockwave Flash" (or kPluginNameFlash, as PluginBlocklistService uses), and then weave that into PreferenceManager's |isFlashblockAllowed|.
Keywords: polish
Summary: Flashblock doesn't know whether Flash is installed → Flashblock pref shouldn't be enabled if Flash is not installed
Created attachment 540678 [details] [diff] [review]
Incomplete hack

(In reply to comment #1)
> I'm OK with confirming this, though I'm not sure how hard it would be to do.

Harder than it looks at first glance ;-)

This incomplete PoC hack implements the part where PreferenceManager checks to see if Flash is an installed plug-in (in probably the least-efficient way possible!).

However, I didn't read the comment in |isFlashblockAllowed| carefully enough; that method is duplicated in WebFeatures.mm because WebFeatures controls the actual UI *and*, for purposes of this bug, WebFeatures is blissfully ignorant of PreferenceManager (and, of course, Gecko).

So to make this work, |isFlashPluginInstalled| would have to be made public and WebFeatures would have to learn about PreferenceManager, or there'd need to be another way of passing that BOOL around.
(Reporter)

Comment 3

7 years ago
Let's go ahead and confirm this, at least, especially since we have a proof of concept hack to do it.
Status: UNCONFIRMED → NEW
Ever confirmed: true
We ought to be able to look at Stuart's code to disable the Java checkbox if there's no Java plug-in installed to see how he worked around the issue in comment 2.  Not sure if that was written after I hacked on this or if I just forgot about it then.
(Reporter)

Comment 5

7 years ago
That(In reply to Smokey Ardisson (back-ish; no bugmail - do not email) from comment #4)
> We ought to be able to look at Stuart's code to disable the Java checkbox if
> there's no Java plug-in installed

...which would be bug 671246, for the terminally lazy.
Yeah, bug 671246 shows how we need to do the BOOL stuff, and it wasn't in the tree until July (so I definitely didn't miss it before, yay!).  This ought to be a cinch to finish up.
Assignee: nobody → alqahira
Flags: camino2.1.1?
Created attachment 572972 [details] [diff] [review]
Fix

This is a trivial merge of Stuart's swell method of checking to see if a Java plug-in is installed with the remaining parts of my original hack.  It has the same "it's not really a preference" aftertaste as bug 671246, but that's life.  

Also fixes spacing in the method "header"(?) to match Camino style for the method in WebFeatures and the one that came right after it.
Attachment #540678 - Attachment is obsolete: true
Attachment #572972 - Flags: superreview?(stuart.morgan+bugzilla)
Status: NEW → ASSIGNED

Comment 8

7 years ago
Comment on attachment 572972 [details] [diff] [review]
Fix

Since the code is identical, instead of duplicating it make a private helper called isPluginInstalledForType:(const char*)mimeType, and then change the hard-coded string to mimeType. That way each helper can just be a one-line call with the mime type as a param.

Also, rename the method you added isFlashInstalled.

(I should really make a CHPluginList or similar that wraps this so it's not all stuck in the prefs code; I've been playing with some refactoring for post-2.1, so I'll add it to my list ;) )
Attachment #572972 - Flags: superreview?(stuart.morgan+bugzilla) → superreview-
Created attachment 575969 [details] [diff] [review]
Fix, v.1.1

This should address all the prior sr comments.
Attachment #572972 - Attachment is obsolete: true
Attachment #575969 - Flags: superreview?(stuart.morgan+bugzilla)

Comment 10

7 years ago
Comment on attachment 575969 [details] [diff] [review]
Fix, v.1.1

Sr=smorgan
Attachment #575969 - Flags: superreview?(stuart.morgan+bugzilla) → superreview+
http://hg.mozilla.org/camino/rev/a93567bd3943
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: camino2.1.1? → camino2.1.1+
Resolution: --- → FIXED
Whiteboard: [camino-2.1.1]
(Reporter)

Updated

7 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.