295755 - Implement Flashblock as a default Camino preference

Status: VERIFIED FIXED

(Camino Graveyard :: Preferences, defect)

Description

[Samuel Sidler (old account; do not CC)]

This bug would add CamiFlash (or it's functionality at least) to the Web
Features preference panel by default. This would add *one* button to the panel,
from what I can tell.

Comment 1

[Mike Pinkerton (not reading bugmail)]

tell me how to implement it and i'll add it in a heartbeat.

Comment 2

[Jasper]

Take the XPI file from http://flashblock.mozdev.org/, and only put the jar file
inside that in Camino's chrome folder. I also think the crhome.rdf file needs
some info but maybe that is done by camino on first launch. Note that this will
need another restart.

Comment 3

[Samuel Sidler (old account; do not CC)]

(Since I don't currently use it), does CamiFlash require a restart?

Comment 4

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

(In reply to comment #3)
> (Since I don't currently use it), does CamiFlash require a restart?

Yes.  And both chrome.rdf and userContent.css have to be modified in order for
it to work (the CamiFlash installer does this, unless you have an existing
userContent.css, in which case you have to do that part manually)

Comment 5

[Echidnae]

Someone should take a look at how CamiFlash and the new adblocking that was
added into the 20050528 nightly coexist.  It seems that disabling adblocking in
the Web Features prefpane also disables CamiFlash, even though the CamiFlash
prefpane still says it's on.  It also looks like when I opened the Web Features
prefpane for the first time today, "Block web advertising" was checked, even
though it was not on - I'm guessing because CamiFlash was enabled.

Comment 6

[Karlheinz Dobler]

When I disable the new feature then you *delete* the whole usercontent.css file?
Even if there are tons of other stuff in it?

Comment 7

[Echidnae]

Ok, correct me if I'm wrong here, but after taking a quick look, I think what
CamiFlash does is that it installs flashblock like you would manually (see
http://www.macosxhints.com/article.php?story=20041217163514494 for the
instructions).  Once that is done, to disable flashblock, CamiFlash removes the
flashblock.jar file in ~/Library/Application Support/Camino/chrome and then adds
it back to enable it again.

If that's all, then I don't think it'd be too much trouble to get this built
into Camino.

Comment 8

[M-Rick]

Add compatibility with the Firefox Adblock extension.

Comment 9

[Samuel Sidler (old account; do not CC)]

(In reply to comment #8)
> Add compatibility with the Firefox Adblock extension.
> 

File a bug about it. This bug is about Flash blocking in Camino.

Mike, since this will require a restart, this probably isn't something we want. However, is there any way to do this *without* requiring a restart? We were able to do it with adblocking, it seems like we should be able to for flash blocking as well.

Afaict, we can place the appropriate files in their appropriate chrome directories, but just not enable them via userContent.css (or a similar more flexible file) until the user checks the box. So, to summarize...

   1) By default include flashblock.jar
   2) By default include the appropriate changes to chrome.rdf
   3) Create a new adblocking.css-like file which can be enabled and disabled in the same manner that adblocking can.

Simon, is this feasible?

Comment 10

[Simon Fraser [no longer active]]

What does FlashBlock do that our ad_blocking.css doesn't? i.e. is it just a list of wildcards that are added to a blocking file, or are there more smarts to it? Also, does it have any of its own UI, and if so, is that XUL? Does it work in Camino?

Comment 11

[Samuel Sidler (old account; do not CC)]

(In reply to comment #10)
> What does FlashBlock do that our ad_blocking.css doesn't? i.e. is it just a
> list of wildcards that are added to a blocking file, or are there more smarts
> to it? Also, does it have any of its own UI, and if so, is that XUL? Does it
> work in Camino?
> 

Flash block adds a UI to the page for every Flash item that allows the user to "click" to play the Flash animation. Afaict, the flashblock entries in userContent.css are just simple CSS entries (duh) but Flashblock adds other files to the mix including a jar file and an entry in chrome.rdf. It works flawlessly in Camino when implemented right (ala CamiFlash). Also, see the link in comment 7 for the actual manual implementation which CamiFlash does (and we'd have to do).

Comment 12

[Samuel Sidler (old account; do not CC)]

And add the following to comment 9:

   4) Add a nice "Block flash animations" checkbox to the Web Features pref panel. We should probably add a description underneath or something. Like... "All Flash animations will be blocked until clicking on them."

This all looks like a job for... Ian! :) I'd really love this in 1.1, but then, I filed the bug... 

Comment 13

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

More fun for Ian: FlashBlock requires JavaScript to be enabled (bug 236839 comment 16/<http://flashblock.mozdev.org/faq.html#fbNojavascript>), so he gets to do some more conditional enabling fu when fixing this :)

Since no one's mentioned license yet, I checked and it's tri-licensed <http://flashblock.mozdev.org/licences.html>; at some point we should probably cc the FlashBlock folks and let them know we're doing this.

Comment 14

[Philip Chee]

> at some point we should probably cc the FlashBlock folks and
> let them know we're doing this.

Flashblock developer here. I'm adding myself to the cc list.

Does Camino ship with nsIStyleSheetService? If so you don't need userContent.css anymore and can use this bit of code from Flashblock:

  var sss = Components.classes["@mozilla.org/content/style-sheet-service;1"]
            .getService(Components.interfaces.nsIStyleSheetService);
  var ios = Components.classes["@mozilla.org/network/io-service;1"]
            .getService(Components.interfaces.nsIIOService);
  var u = ios.newURI("chrome://flashblock/content/flashblock.css", null, null);
  if(!sss.sheetRegistered(u, sss.USER_SHEET)) {
    sss.loadAndRegisterSheet(u, sss.USER_SHEET);
    dump("installing flashblock\n");
  }

Comment 15

[Bryan Atwood]

I've taken a shot at this bug, implementing Philip's dynamic load method in Objective-C (in WebFeatures.mm).  To turn flash off, you click the button in the pref pane and the click-to-play-Flash icon show immediately.  Turning it back on requires a page reload.

Here is the diff for the 3 files (WebFeatures.mm/h, PreferenceManger.mm):
 http://pastebin.mozilla.org/3291

I'll post the updated WebFeature nib file and the flashblock.jar file.  To get this to work, you will need the flashblock.jar file in the global chrome file (Camino.app/Contents/MacOS/chrome) and update the chrome.rdf file there as follows:

  <RDF:Description RDF:about="urn:mozilla:package:flashblock"
    c:baseURL="jar:resource:/chrome/flashblock.jar!/content/flashblock/"
    c:locType="profile"
    c:displayName="Flashblock"
    c:author="The Flashblock team"
    c:authorURL="http://flashblock.mozdev.org/"
    c:name="flashblock"    c:extension="true"
    c:description="Replaces Flash objects with a button you can click to view th
em." />

Comment 16

[Bryan Atwood]

Attachment: Updated WebFeatures nib file

Updated WebFeatures nib file by Bryan Atwood

Comment 17

[Bryan Atwood]

Attachment: flashblock.jar from current flashblock.xpi

flashblock.jar taken from Flashblock Firefox extension

Comment 18

[Bryan Atwood]

By the way, you can whitelist sites by adding something like this to your userContent.css file:

@-moz-document 
domain(youtube.com),
domain(google.com)
{
    object[classid*=":D27CDB6E-AE6D-11cf-96B8-444553540000"],
    object[codebase*="swflash.cab"],
    object[data*=".swf"],
    embed[type="application/x-shockwave-flash"],
    embed[src*=".swf"],
    object[type="application/x-shockwave-flash"],
    object[src*=".swf"]
    { -moz-binding: none !important; }
}

Comment 19

[Chris Lawson (gone)]

Comments on the patch:

>Index: PreferencePanes/WebFeatures/WebFeatures.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/PreferencePanes/WebFeatures/WebFeatures.mm,v
>retrieving revision 1.26
>diff -u -8 -r1.26 WebFeatures.mm
>--- PreferencePanes/WebFeatures/WebFeatures.mm  26 Jan 2007 20:05:29 -0000        1.26
>+++ PreferencePanes/WebFeatures/WebFeatures.mm  29 Jan 2007 01:27:57 -0000
>@@ -53,16 +53,17 @@
> #include "nsNetUtil.h"
> 
> // we should really get this from "CHBrowserService.h",                         
> // but that requires linkage and extra search paths.                           
> static NSString* XPCOMShutDownNotificationName = @"XPCOMShutDown";             
> 
> // needs to match the string in PreferenceManager.mm
> static NSString* const AdBlockingChangedNotificationName = @"AdBlockingChanged";
>+static NSString* const FlashBlockChangedNotificationName = @"FlashBlockChanged";

With the mixed style in here, it's an easy mistake to make, but prefix this with a "k" since it's a constant, please. Don't feel obligated to fix the other occurrences as that'll make the patch a lot more complicated and confusing (and I think we have a bug for fixing those anyway).

>+  [self setPref:"camino.enable_flashblock" toBoolean:[sender state] == NSOnState];

Put parentheses around the bool, like this: ([sender state] == NSOnState)

It also wouldn't kill us to put a little more of a description in the method comment, like we did for the ad-blocking method.

>   [[NSNotificationCenter defaultCenter] addObserver:self
>                                            selector:@selector(adBlockingPrefChanged:)
>                                                name:AdBlockingChangedNotificationName
>                                              object:nil];
>-
>+                                                                             

No whitespace on blank lines, please.

>+  [[NSNotificationCenter defaultCenter] addObserver:self
>+                                           selector:@selector(flashBlockPrefChanged:)
>+                                               name:FlashBlockChangedNotificationName
>+                                             object:nil];

>@@ -822,16 +828,21 @@
>     BOOL enableAdBlocking = [self getBooleanPref:"camino.enable_ad_blocking" withSuccess:&prefExists];
>     if (!prefExists) {
>       enableAdBlocking = [self cleanupUserContentCSS];
>       [self setPref:"camino.enable_ad_blocking" toBoolean:enableAdBlocking];
>     }
> 
>     if (enableAdBlocking)
>       [self refreshAdBlockingStyleSheet:YES];
>+       
>+       // Load flashblock if enabled
>+       BOOL enableFlashBlock = [self getBooleanPref:"camino.enable_flashblock" withSuccess:&prefExists];
>+       if (enableFlashBlock)
>+         [self refreshFlashBlockStyleSheet:YES];
> }

Make the indentation of this added code match the |if (enableAdBlocking)| above it, so it doesn't look like you're missing curly braces in a multi-line if.

I don't know the answer to this, but I'll throw this question out there -- is there a reason we should treat Flash-blocking different from ad-blocking from a filesystem perspective? That is, why wouldn't we just have a flashblock.css file like we do for ad-blocking? Seems like that would be better than making modifications to the chrome.rdf file.

Also, next time, feel free to just post the diff as an attachment here. It's a lot easier to deal with that way. :)

cl

Comment 20

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

(In reply to comment #15)
> To turn flash off, you click the button in
> the pref pane and the click-to-play-Flash icon show immediately.  Turning it
> back on requires a page reload.

Is this because it's using a binding here rather than a pure stylesheet? (i.e., on the trunk, stylesheets like ad_blocking.css take effect or are removed on toggle of the pref, no reload required).

(In reply to comment #19)
> I don't know the answer to this, but I'll throw this question out there -- is
> there a reason we should treat Flash-blocking different from ad-blocking from a
> filesystem perspective? That is, why wouldn't we just have a flashblock.css
> file like we do for ad-blocking? Seems like that would be better than making
> modifications to the chrome.rdf file.
Flashblock relies on the XBL bindings it ships to do some of its work, and needs that chrome.rdf to let the guts of Gecko know about the stuff it ships.

Incidentally, properly modifying the chrome.rdf file when enabling flashblock was one of the major issues holding up this bug before.  We'll need a solution for that (as well as some sort of build system love, or profile-creation love, or something like that, for the .jar) before we can land this feature.  (One of the complaints about the initial implementation of ad-blocking is that we overwrote userContent.css rather than appending to it or using another method.)

Thanks for working on this, Bryan!  I've assigned the bug to you :)

Comment 21

[Bryan Atwood]

(In reply to comment #20)
> Is this because it's using a binding here rather than a pure stylesheet? (i.e.,
> on the trunk, stylesheets like ad_blocking.css take effect or are removed on
> toggle of the pref, no reload required).

Right, it uses moz-binding and loads an XBL file.  Maybe this is why the refresh is needed on flashblock disable.

> (In reply to comment #19)

> Flashblock relies on the XBL bindings it ships to do some of its work, and
> needs that chrome.rdf to let the guts of Gecko know about the stuff it ships.
> Incidentally, properly modifying the chrome.rdf file when enabling flashblock
> was one of the major issues holding up this bug before.  

I've been tinkering with this to try to get around the chrome registration but it seems loading XBL from a local URL (file://) has been disabled for security reasons.  Loading from a chrome:// URL may be the only way to load the XBL. The chrome.rdf file will need to be modified to add flashblock after all, as you mentioned.  

> Thanks for working on this, Bryan!  I've assigned the bug to you :)

Uh oh, is this a case of "fools rushing in..." :)

Comment 22

[Philip Chee]

>  <RDF:Description RDF:about="urn:mozilla:package:flashblock"
>    c:baseURL="jar:resource:/chrome/flashblock.jar!/content/flashblock/"
>    c:locType="profile"

Nit: If you are going to put this in global chrome, you should change the locType to "install"

Comment 23

[Stuart Morgan]

(In reply to comment #20)
> Incidentally, properly modifying the chrome.rdf file when enabling flashblock
> was one of the major issues holding up this bug before.  We'll need a solution
> for that

Does having the chrome definitions there all the time cause any problems? Could we just have the definition in the app's chrome.rdf and do all the enabling and disabling with nsIStyleSheetService?

Comment 24

[Bryan Atwood]

OK, round two.  I've got something that works I think.  Since we need the XBL, I installed Flashblock in the app's chrome directory.  It's permanently enabled, I don't see anything wrong with that.  Then we can turn it on and off with nsIStyleSheetService as Stuart and Philip mentioned.

A manifest file would be the easiest solution, but Camino doesn't seem to like them.  We have to install Flashblock in the chrome directory of the app by adding a line to installed-chrome.txt and adding a contents.rdf file in flashblock.jar. 

Here's a summary of the fix:

1. Edit the source files WebFeatures.mm/.h PreferenceManager.mm

2. Update the WebFeatures nib

3. Have Makefile (or something) add a line to Camino.app/Contents/MacOS/chrome/installed-chrome.txt that installs flashblock.jar:

 echo "content,install,url,jar:resource:/chrome/flashblock.jar!/content/flashblock/" >> chrome/installed-chrome.txt

4. Add the modified flashblock.jar file to the chrome directory (ie. like embed.jar)

Those steps seem to work for me and should require minimal changes to how Camino works.  I'll attach the new flashblock.jar file (with contents.rdf added and stuff we don't use removed) and a patch of the source that includes Chris' changes (Thanks for all the help, Chris!).  The nib file I attached earlier still works.

Comment 25

[Bryan Atwood]

Attachment: Patched source files WebFeatures.mm/.h, PreferenceManager.mm

Comment 26

[Bryan Atwood]

Attachment: Updated flashblock.jar file (includes contents.rdf, removes XUL stuff)

Comment 27

[Philip Chee]

In the flashblock XBL you could remove the code that calls the Flashblock whitelist since it doesn't work in Camino. Or you could reimplement the Flashblock whitelist listener in Objective C (probably in a followup bug).

Comment 28

[Bryan Atwood]

Nice catch.  I left it in because I worried about maintenance from the Camino side.  One idea is to just copy the files Camino needs straight from the Flashblock source tree as is.  But like you say, it does leave in the whitelist call that won't work in Camino.

As for whitelisting, I couldn't figure out an easy method for Camino to intercept DOM events and that's why I gave the userContent.css solution above.  Hopefully someone who knows what they are doing takes over from here :)

Comment 29

[Bryan Atwood]

I wanted to update this from a conversation in #camino:

Smokey recommended using the stock flashblock.jar, then adding a contents.rdf file into it that would tell Camino how to install flashblock in the chrome path.  I'll attach the contents.rdf file in case we go that route.  We can then update the stock jar to a Camino-friendly one with the following command in a Makefile:

 jar uf flashblock.jar content/flashblock/contents.rdf

It was also suggested that we add the jar command and changing the installed-chrome.txt file to the part of the Makefile where the embed.jar adjustments take place.

The reasons for this route stem from maintainability.  Camino devs wouldn't have to look at the source of flashblock.jar every time it is released.  Just copy the jar file.  One drawback is that the jar file would be 70k (since it includes the XUL stuff) instead of about 18k for a Camino-only version of the jar file.

By the way, it looks like an older version of flashblock (1.3.5) includes contents.rdf but I imagine we'll want to use the latest version we can.

Comment 30

[Bryan Atwood]

Attachment: contents.rdf file to be added to a stock flashblock.jar for Camino

Comment 31

[Philip Chee]

> By the way, it looks like an older version of flashblock (1.3.5)
> includes contents.rdf

FYI Flashblock 1.3 branch is developed in parallel with the Flashblock 1.5 branch and is feature equivalent. The difference is that 1.3.x supports older toolkit browsers and XPFE based browsers (e.g. SeaMonkey) as well. There are some additional try/catch blocks to workaround missing functionality in older browsers but shouldn't affect browsers using recent Gecko versions.

Comment 32

[Philip Chee]

> Camino devs wouldn't have to look at the source of flashblock.jar
> every time it is released.  Just copy the jar file.  One drawback
> is that the jar file would be 70k (since it includes the XUL stuff)

Is there anything stopping you from using the jar utility to remove all the unneeded files? An alternative is to pull the necessary files from our CVS directly and then pack them in your own custom .jar.

Comment 33

[Bryan Atwood]

No, those are both fine.  I think it's a choice between blindly copying the jar file versus making sure that we pull the right source files (in case a future version of flashblock adds things we need, like image files).  btw, I'm not the one making the decision, just trying to present the arguments for both sides. 

Comment 34

[Bryan Atwood]

Also, I just confirmed that using the stock flashblock.jar file from version 1.3.5 works without adding the contents.rdf file.  Thanks for the input Philip!

Comment 35

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

Flagging b1? since there's a patch here that could conceivably make 1.1 but which also requires UI changes.

Philip, thanks for all the input here!  We're going to try to talk about integration strategies at our meeting on Weds.  I have one more question at the moment: how long are you guys planning on maintaining the 1.3 branch (and/or maintaining it at feature equivalence with the 1.5 branch)?  That might have some bearing on what route we choose.

Comment 36

[Philip Chee]

Lorenzo only cares about the latest and greatest Firefox version and the Flashblock 1.5 branch will always track that. On the other hand I've promised the rest of our users to maintain the 1.3 branch for those on other browsers until they have a proper migration path. I'll definitely maintain the 1.3 branch at least until SeaMonkey becomes a toolkit app sometime in the 1.5 timeframe. And possibly beyond (there are some diehard Netscape 7.2 users who just won't give it up until its pried from their cold dead fingers).

Comment 37

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

We talked about this at today's meeting and we do want this for 1.1.  

mento prefers that we land the necessary flashblock files in mozilla/camino somewhere and build the jar ourselves (which he'll set up if I keep bugging him) ;)

Comment 38

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

Comment on attachment 253147 [details] [diff] [review]
Patched source files WebFeatures.mm/.h, PreferenceManager.mm

cl, can you do another pass on this patch?

Comment 39

[Bryan Atwood]

btw, here are the files we need from Flashblock (1.3 branch):

content/flashblock/authorware.png
content/flashblock/contents.rdf
content/flashblock/director.png
content/flashblock/flash.png
content/flashblock/flashplay.png
content/flashblock/flashblock.css
content/flashblock/flashblock.xml

As Philip says, we can strip out the whitelist function call in flashblock.xml (or implement a listener for it in Objective C in a separate feature request)

Comment 40

[Bryan Atwood]

Attachment: Update patch - handles case when javascript/plugins disabled

Comment 41

[Bryan Atwood]

Attachment: Updated nib file - new text, location of flashblock

Comment 42

[froodian (Ian Leue)]

Comment on attachment 253580 [details] [diff] [review]
Update patch - handles case when javascript/plugins disabled

Looks pretty good in general.  Mostly just nits:

>+  // Set Flashblock on or off.  Flashblock requires javascript and plugins enabled  
>+  // We check the dependencies here to make sure there are no conflicts in the preference file
>+  [self resolveFlashBlockDependencies];
>+  BOOL enableFlashBlock = [self getBooleanPref:"camino.enable_flashblock" withSuccess:&gotPref];
>+  [mEnableFlashBlock setState:(enableFlashBlock)];

Set checkbox state to NSOnState/NSOffSate instead of a boolean, so something like

[mEnableFlashBlock setState:(enableFlashBlock ? NSOnState : NSOffState)];

> -(IBAction) clickEnableJS:(id)sender
> {
>   [self setPref:"javascript.enabled" toBoolean:([sender state] == NSOnState)];
>+
>+  // Flashblock requires javascript and plugins to be enabled
>+  [self resolveFlashBlockDependencies];

I wouldn't mention "plugins" in the comment here, since it's only in the JS code.  Same thing (but other way around) in |clickEnablePlugins|

>+-(void) resolveFlashBlockDependencies
>+{
>+  if ([mEnableJS state] == NSOnState && [mEnablePlugins state] == NSOnState) {
>+    [mEnableFlashBlock setEnabled:TRUE];
>+  } else {
>+    [self setPref:"camino.enable_flashblock" toBoolean:false];
>+    [[NSNotificationCenter defaultCenter] postNotificationName:kFlashBlockChangedNotificationName object:nil];
>+    [mEnableFlashBlock setState:NSOffState];
>+    [mEnableFlashBlock setEnabled:FALSE];
>+  }
>+}

I would check the actual JS/Plugin prefs "camino.enable_plugins" and "camino.enable_flashblock" rather than relying on the checkboxes to be right.

Also, Camino style is

}
else {

instead of

} else {

Also, it's an ugly thing to think about, but there could be cases where the flashblock pref is on and then its dependencies get resolved. (1. Turn off JS. 2. Leave web features open 3. Enable flashblock via about:config 4. Turn on JS)  In this case, we would want the checkbox back on again.  Setting the checkbox's state to the pref value when enabling should do the trick.

>+- (void)flashBlockPrefChanged:(NSNotification*)inNotification
...

Is there any way that some of this method (basically the "reload if it's yes, unload if it's no" logic) can get wrapped back up into a shared method between this and |refreshAdBlockingStyleSheet| to minimize duplicated code?

Also, a couple nib things I noticed while I was testing this:
- The checkbox is still in its previous place in the tab loop
- Can you lowercase-ize "plug-ins" in the enable plugins checkbox while you're in this nib? (bug 368798)

Comment 43

[froodian (Ian Leue)]

That last quotation should be

+- (void)refreshFlashBlockStyleSheet:(BOOL)inLoad

Instead of for |flashBlockPrefChanged|, sorry.

Comment 44

[Stuart Morgan]

I have a couple of concerns with the nib:
1) The pref is phrased as "Block" but grouped with the "Enable" prefs, meaning that its effect is opposite that of the nearby checkboxes.
2) Nothing in the nib suggests a dependency on the JavaScript pref, so someone with that off might never be able to figure out why flash-block is disabled.

Perhaps this should be in "Annoyance Blocking" with explanatory text about the dependencies? Or where it is, but with an explanation of the JS dependency? I hate to add yet more text to this pane, but it needs to not be mysterious.

Comment 45

[Bryan Atwood]

Attachment: Updated patch with froodian's comments

Comment 46

[Bryan Atwood]

Thanks for the comments.  I'll wait to upload the nib until we agree on where to put the "block flash" checkbox: in Annoyance blocking or Content control.  I'm leaning towards Annoyance blocking since it fits the "block" theme there.  Also might look better there if we add a whitelist button later.

Comment 47

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

We discussed the nib and agreed to put the pref in Annoyance Blocking, between ad-blocking and pop-up blocking, with the hint text "JavaScript and plug-ins must be enabled to block Flash animations."

Please do fix froodian's nits from comment 42 as well :)

Comment 48

[Bryan Atwood]

Attachment: nib file update for comment #43

Comment 49

[Bryan Atwood]

Thanks Smokey.  Also, I posted a patch that includes fixes from #42.  However, I don't like the solution that checks for changes to about:config while the web preferences pane is open.  It causes the flashblock stylesheet to be reloaded every time the pref pane is redisplayed.  It also makes the code a little uglier.  

I don't think we need that check since we currently don't check any of the other preferences in about:config while the pane is open.  (ie. (1) open pref pane, (2) change javascript enabled setting in about:config (3) notice pref pane is not updated)  If we want to be that particular, we should set up a listener for changes in about:config.  So I'd like to remove that addition to the patch.  Without it, the worst that will happen is that flashblock won't be turned on if you follow the 4 steps in comment #42.

Comment 50

[froodian (Ian Leue)]

(In reply to comment #49)
> I don't like the solution that checks for changes to about:config while the web
> preferences pane is open.  It causes the flashblock stylesheet to be reloaded
> every time the pref pane is redisplayed.  It also makes the code a little
> uglier.  

~snip

>Without it, the worst that will happen is that flashblock won't be
> turned on if you follow the 4 steps in comment #42.
 
I think there are some other ways of getting into a state like that (ie, it's not really related to about:config at all), but I'm ok with you axing it.  The code for it doesn't need to be that ugly, btw, and "else if" would do the trick quite gracefully.

Comment 51

[Bryan Atwood]

> I don't like the solution that checks for changes...

Argh, what a difference a word makes.  I wanted to say "I don't like _my_ solution".  Sorry froodian, I didn't mean to diss your idea (which is good), just to diss my implementation.  Can I call a "do over"?

I'm still going to remove the check since (my) code caused the flashblock stylesheet to reload every time the web pref pane is opened.  If someone takes a crack at it and finds a good way to do it, I'll certainly add the change.

Comment 52

[Bryan Atwood]

Attachment: Updated patch for comments #50/51

Comment 53

[Bryan Atwood]

Attachment: Updated patch - re-checks Flashblock when javascript re-enabled

As discussed on #camino, we decided to re-check Flashblock when javascript/plug-ins are re-enabled if Flashblock was previously set.  This patch does that.

The solution was to let the preference (camino.enable_flashblock) be true even if the Flashblock stylesheet is not loaded.  So when javascript/plugins are re-enabled, the check box for Flashblock is set by the state of the flashblock preference and the stylesheet is reloaded.

One drawback is that the javascript/plug-in setting is checked in two places: once in WebManager to set the checkbox state, and once in PreferenceManager to set the actual stylesheet.  There is a replicated function (checkFlashBlockAllowed) in these two files.

Comment 54

[Bryan Atwood]

Attachment: Updated nib - fine tuning

Also on #camino, ardissone had some nits to pick (j/k):
 - fix bottom margin
 - min-height should be actual height
 - hint text should be left-aligned with check box

Comment 55

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

There's one other issue we need to think about how to fix (it's on the meeting agenda for tomorrow): what to do about chrome.rdf files in $profile/chrome that were modified by CamiFlash.  

Built-in Flashblock won't work if the profile's chrome.rdf file was modified by CamiFlash, and presumably all those people will move to using built-in Flashblock.

Comment 56

[Chris Lawson (gone)]

(In reply to comment #55)
> There's one other issue we need to think about how to fix (it's on the meeting
> agenda for tomorrow): what to do about chrome.rdf files in $profile/chrome that
> were modified by CamiFlash.

Probably outside the scope of this bug, but there's no hope of, say, detecting and "repairing" (or simply replacing with an un-modified file) this situation, is there?

cl

Comment 57

[Bryan Atwood]

One way is just to delete chrome.rdf.  It gets automatically regenerated with a default if it's not there.  A user would lose anything else that was added, but I don't know anything else that would go there for Camino other than Flashblock.

Philip, doesn't the Flashblock extension have an uninstaller?  Before manifests, did it do anything special to remove the definition from chrome.rdf?

Comment 58

[Philip Chee]

> Philip, doesn't the Flashblock extension have an uninstaller?  Before
> manifests, did it do anything special to remove the definition from chrome.rdf?

No we never had an installer. We rely on extension uninstaller type extensions for  MozillaSuite/SeaMonkey and the EM in toolkit applications.

Do you have a convenient wrapper to the chrome rdf:datasource? You could remove the flashblock entries from chrome.rdf by simply unasserting them.

Comment 59

[froodian (Ian Leue)]

Comment on attachment 254273 [details] [diff] [review]
Updated patch - re-checks Flashblock when javascript re-enabled

Behavior is great.  Style/Codepath comments:

>+  // Flashblock depends on Javascript so make sure to update it

>+  // Flash depends on plugins so make sure to update the Flashblock settings

Sorry, but can these comments follow the same format as eachother?

>+//
>+// checkFlashBlockAllowed
>+//
>+// Checks whether Flashblock can be enabled
>+// Flashblock only allowed if javascript and plug-ins enabled
>+// Also sets whether checkbox can be checked

Remove the last line, since it doesn't set whether the checkbox can be enabled.  Elsewhere, this method is used for that, but the method doesn't touch the checkbox.

>+//
>+-(BOOL) checkFlashBlockAllowed
>+{
>+  BOOL gotPref = NO;
>+  BOOL jsEnabled = [self getBooleanPref:"javascript.enabled" withSuccess:nil];
>+  BOOL pluginsEnabled = [self getBooleanPref:"camino.enable_plugins" withSuccess:&gotPref] || !gotPref;
>+
>+  return jsEnabled && pluginsEnabled;
>+}

How about |isFlashBlockAllowed|?  It's a more cocoa-y name.

We should get both of these prefs the same way, in relation to gotPref.  I tend to think that if somehow the plugins pref didn't exist, we'd want flashblock off, not on, but the important part here is having a consistent getter pattern.

>+// Update FlashBlock state, called when dependencies are changed

The method doesn't need to know when it's called, and a comment to that effect is likely to become wrong.  How about "Update the state of the FlashBlock checkbox"

>+//
>+-(void) updateFlashBlock
>+{
>+  BOOL allowed = [self checkFlashBlockAllowed];
>+  [mEnableFlashBlock setEnabled:allowed];
>+
>+  // Flashblock state can only change if it's already enabled 
>+  // since changing dependencies won't have affect on disabled Flashblock
>+  if (![self getBooleanPref:"camino.enable_flashblock" withSuccess:nil])
>+    return;
>+
>+  // Flashblock is enabled.  Checkbox is on if FlashBlock also allowed

This is a very confusing comment.  How about something like s/Flashblock is/Flashblock pref

>+//
>+// checkFlashBlockAllowed
>+//
>+// Checks whether Flashblock can be enabled
>+// Flashblock only allowed if javascript and plug-ins enabled
>+// Also sets whether checkbox can be checked
>+//
>+-(BOOL) checkFlashBlockAllowed
>+{
>+  BOOL gotPref = NO;
>+  BOOL jsEnabled = [self getBooleanPref:"javascript.enabled" withSuccess:nil];
>+  BOOL pluginsEnabled = [self getBooleanPref:"camino.enable_plugins" withSuccess:&gotPref] || !gotPref;
>+
>+  return jsEnabled && pluginsEnabled;
>+}
>+
> @end

Man, this code duplication is ugly.  I'm not immediately sure of the best way to get rid of it, but let's try to find a way.

Comment 60

[Bryan Atwood]

froodian, thanks for taking a look.  I've made all of your changes except for the code duplication.  One question: do you think we should change "FlashBlock" to "Flashblock" (even in functions) since that's the official name of the extension?

About the getPref on plug-ins, I took the code from the initialization function (WebFeatures mainViewDidLoad) where plug-ins are enabled by default and only turned off if there is an explicit pref setting.  We should keep these two checks consistent.

>>+-(BOOL) checkFlashBlockAllowed
>>
>Man, this code duplication is ugly.  I'm not immediately sure of 
>the best way to get rid of it, but let's try to find a way.

The only thing to fix before I submit a new patch is the duplicate function.  As you know, the redundant code is because we decided to have the Flashblock enabled pref be a "suggestion" and so we need to check the dependencies before actually turning on the check box and stylesheet itself.  These happen in two separate objects that only interact with each other through (one-way) message passing.  So we have to check twice :(  Yes, it's hideous.

I do like having Flashblock automatically enabled if you re-enable one of its dependencies.  Here are some solutions to get rid of the extra dependency check (isFlashblockAllowed) in Preference Manager:

1) Have a separate pref for past state of Flashblock.  So we have one pref for "suggested" value, and one for actual value.  We also talked about putting this "suggested" setting in the plist file.

2) Use an integer pref setting instead of binary.  So '0' is flashblock off, '1' is flashblock preferred, and '2' is Flashblock on.  

I'm leaning towards option two because it keeps all states of flashblock in one place.

Both of these open us up to problems in some cases where users manually change the pref.  But in that case, it won't break anything, it just won't load flashblock. And we can have WebFeatures fix it the next time the pref pane is opened.

Comment 61

[froodian (Ian Leue)]

FlashBlock or Flashblock, whichever you think is better.  If the actual extension is Flashblock, maybe we should go with that.

We talked about the code duplication some in channel, and here's what we can do.  Rather than just sending a "pref changed" notification, we either

a) send the notification with an NSValue containing a bool saying whether we're turning flashblock on or off or
b) send two notifications, one for turning it on, and one for turning it off

That way the pane decides (based on the pref and dependencies) whether flashblock is enabled and allowed, and the pref manager just trusts it to be right.

Comment 62

[Bryan Atwood]

Attachment: Updated patch for comments #59/#60

We discussed the boolean message passing idea on #camino.  The problem is that on startup, the Pref Manager must load the flashblock stylesheet.  Since the WebFeatures pref pane is not activated, no messages are sent to Pref Manager.  Therefore, Pref Manager only has the flashblock pref value to go by.  Since the pref means 'activate flashblock *if* plug-ins and JS are enabled', Pref Manager has to do the dependency check, at least on startup.  So we can't get rid of the copied function unless we go to tri-state pref or dual prefs or some other magic.

smorgan made the executive decision to leave the duplicate code in.  This patch fixes the other issues in #59.

Comment 63

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

Comment on attachment 254274 [details]
Updated nib - fine tuning

r=ardissone; this looks (and works) as expected.

Comment 64

[froodian (Ian Leue)]

Comment on attachment 254559 [details] [diff] [review]
Updated patch for comments #59/#60

Looks good. r=me

Comment 65

[Stuart Morgan]

Comment on attachment 254559 [details] [diff] [review]
Updated patch for comments #59/#60

Just a few nits:

> // needs to match the string in PreferenceManager.mm

s/needs to match the string/need to match the strings/

>+  

No whitespace on empty lines (there are a couple of these).

>+    [mEnableFlashBlock setState:enableFlashBlock];

    [mEnableFlashBlock setState:(enableFlashBlock ? NSOnState : NSOffState)];

>+// Enable and disable FlashBlock.  When enabled, an icon is displayed and the 
>+//  Flash animation plays when the user clicks it.  When disabled, Flash plays automatically

Get rid of the extra initial space before "Flash"

sr=smorgan with those changes.

Comment 66

[Bryan Atwood]

Attachment: Patch including #65 comments from smorgan

Comment 67

[Bryan Atwood]

Thanks for the comments guys. I've updated the patch and hopefully assigned you both correctly.  If not, let me know.

Comment 68

[Stuart Morgan]

Comment on attachment 255172 [details] [diff] [review]
Patch including #65 comments from smorgan

No need to re-request reviews once it's been +'d.

This has one blank space on each of the lines that used to have two, rather than none. That can be fixed on checkin though.

Comment 69

[Mark Mentovai]

"JavaScript and plug-ins must be enabled to block Flash."

If plug-ins are disabled, won't all Flash be blocked anyway?

Comment 70

[Stuart Morgan]

Per irc, we'll go with nesting it under Enable Plug-ins, with the hint text "JavaScript must be enabled to block Flash."

Comment 71

[Mark Mentovai]

Attachment: Consolidated patch with build stuff

OK, here's the build stuff.  We also decided to tweak the nib a little bit à la http://escapedthoughts.com/camino/webfeatures.png, but I haven't done that, as I'm working over VNC today and IB over VNC is a huge pain.

Comment 72

[Stuart Morgan]

Comment on attachment 255830 [details] [diff] [review]
Consolidated patch with build stuff

r=me. I haven't tested, since I don't have all the new binary files, but it looks as sane as a makefile can ;)

Comment 73

[Mark Mentovai]

Attachment: WebFeatures.nib

This is the nib with the modification discussed above.  The nib is in sync on the trunk and 1.8 branch, so we can use this for both.

Comment 74

[Stuart Morgan]

Note that it doesn't matter that the tab chain is off in this nib because I'll almost immediately be stomping it with my nib for the blacklist stuff.

Comment 75

[Mark Mentovai]

Checked in on the trunk.

Comment 76

[Mark Mentovai]

Whoops, sorry about the tab order.

Comment 77

[Mark Mentovai]

Attachment: WebFeatures.nib, fixed tab order

I'm sticking this here so we don't have to check the one with the braindead tab order in on the branch.

Comment 78

[Mark Mentovai]

Attachment: Branch patch

The only change we make to core here is the addition of a file to /mozilla/allmakefiles.sh, since we're, uh, adding a Makefile.  The added file is in a Camino-only section (MAKEFILES_macbrowser).

Comment 79

[Daniel Veditz [:dveditz]]

Comment on attachment 255937 [details] [diff] [review]
Branch patch

a=dveditz for 1.8 branch

Comment 80

[Mark Mentovai]

Checked in on MOZILLA_1_8_BRANCH before Camino 1.1b[1].

Comment 81

[Simon Fraser [no longer active]]

(In reply to comment #71)
> Created an attachment (id=255830) [details]
> Consolidated patch with build stuff
> 
> OK, here's the build stuff.  We also decided to tweak the nib a little bit à
> la http://escapedthoughts.com/camino/webfeatures.png

11 checkboxes in one panel. Man that looks complex now.