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

Status: VERIFIED FIXED

(Camino Graveyard :: Preferences, defect)

Description

[Chris Lawson (gone)]

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.

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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|.

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: 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.

Comment 3

[Chris Lawson (gone)]

Let's go ahead and confirm this, at least, especially since we have a proof of concept hack to do it.

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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.

Comment 5

[Chris Lawson (gone)]

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.

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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.

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: 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.

Comment 8

[Stuart Morgan]

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 ;) )

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Fix, v.1.1

This should address all the prior sr comments.

Comment 10

[Stuart Morgan]

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

Sr=smorgan

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://hg.mozilla.org/camino/rev/a93567bd3943