371895 - Implement a flashblock whitelist for Camino

Status: RESOLVED FIXED

(Camino Graveyard :: Annoyance Blocking, enhancement, P3)

Description

[Kurtis Shaner]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en; rv:1.8.1.2pre) Gecko/20070223 Camino/1.1b
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en; rv:1.8.1.2pre) Gecko/20070223 Camino/1.1b

Implement a flashblock whitelist for sites that you obviously want flash content to be loaded (e.g. youtube, google analytics) while still blocking other flash content (i.e. ads)

Reproducible: Always

Steps to Reproduce:
Visit site with flash content
Actual Results:  
All flash content is blocked, even consistently desired content.

Expected Results:  
Load flash content for sites determined by the user.

Comment 1

[Chris Lawson (gone)]

Confirming per the discussion in the Flashblock bug. We should do this, but it's gonna take some creative UI wrangling since that pref pane is getting huge.

Comment 2

[Bryan Atwood]

1) Quick and dirty method until this feature is implemented

add this to ~/Library/Application Support/Camino/chrome/userContent.css:

++++ snip ++++
@-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; }
}
++++ snip ++++

Check out http://developer.mozilla.org/en/docs/CSS:@-moz-document
for more information about specifying host and domain formats

2) I think the first step towards integration should be using Flashblock's built-in whitelist check, which sends a custom "flashblockCheckLoad" DOM event to the window and listens for the response.  Then users could add sites and domains to flashblock.whitelist in about:config.  Format is like: google.com,youtube.com

On the Camino side, their would be a listener that holds the list of whitelisted sites that it reads from flashblock.whitelist pref.  I tried setting up a test listener in Camino but it didn't work even though it gets normal events like mouse clicks and blurs:

  target->AddEventListener(NS_LITERAL_STRING("flashblockCheckLoad"),
                           NS_STATIC_CAST(nsIDOMEventListener *, mListener),
                           PR_TRUE);

Philip, I'm adding you to the CC list to see if you have any thoughts about DOM listeners and whether this should work.

3) Final step would be to add a user interface for adding/removing sites from flashblock.whitelist.  But yeah we may need to take that pref pane to the doctor, it may be malignant.

Comment 3

[Philip Chee]

> Philip, I'm adding you to the CC list to see if you have any thoughts about DOM
> listeners and whether this should work.

AddEventListener takes four parameters, the fourth should be PR_TRUE if you want to listen for untrusted user defined events. Since you left it out, it defaults to PR_FALSE.

Comment 4

[Bryan Atwood]

Thank you Philip, I finally found the code that does that.  My error was that I had 'target' as an nsIDOMEventTarget object.  To get the fourth parameter, it has to be a nsIDOMNSEventTarget.

I should be able to whip step 2 together pretty quickly now, just need to figure out where everything goes (split between CHBrowserView, CHBrowserListener, and BrowserWrapper?)

Comment 5

[Bryan Atwood]

Attachment: Allows flash to play if current host matches whitelisted site

This patch adds code that checks 'flashblock.whitelist' preference for whitelisted sites, then allows flash to play if there is a match.  It implements the code that is the counterpart to the built in whitelist check in Flashblock.  Patch assumes manual edit of about:config or prefs.js and does not include user interface to add/remove sites.  That will need to be added in later patch.

Comment 6

[Bryan Atwood]

Attachment: Updated patch with better pattern matching

Uploading a patch with the same functionality but a little better pattern match handling.

Comment 8

[Bryan Atwood]

I removed the review flag for this bug and will wait until the release of 1.1 before continuing.  I have the UI part ready as well (an "Edit Flashblock Whitelist" button and associated panel), but we'll need to discuss how to re-organize the pref pane since it's getting complicated.

Comment 9

[Oskar L-B]

I need flash enabled when I visit iLike.com since it doesn't work with flash off. The play buttons have the flash part hidden in a weird way so you can't click to activate them.

iLike.com is a really nice site (competitor to Last.fm) and I hate to switch to firefox just for that. Almost every site I visit now works fine in Camino. That's great! (Can we have some firefox-plugins now, please? :)

Comment 10

[Stuart Morgan]

Please don't comment in bugs just to say that you want them fixed (and especially not to make completely unrelated feature requests); that's what the vote feature is for. There is development work being done in this bug; unless you are contributing to that work, commenting isn't necessary or appropriate.

Comment 11

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

Setting priority per 1.6 roadmap.

Comment 12

[Bryan Atwood]

OK, this is ready to go, I'm just waiting for a decision about the Web Features pref pane.  It's getting ugly and I don't want to add a separate button.  Has anyone looked into splitting it up or at least rearranging it?

Comment 13

[Bryan Atwood]

Attachment: Updated patch that includes user interface

On second thought, it's probably better to add the Flashblock whitelist first, then we can decide which features should be removed.  I'm uploading a patch that handles the whitelisting and user interface along with the WebFeatures nib.

I need to jump on IRC (it's been a while!) but if someone wants to start reviewing this, let me know.

Comment 14

[Bryan Atwood]

Attachment: WebFeatures nib to go along with the patch

Comment 15

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

Comment on attachment 269475 [details] [diff] [review]
Updated patch that includes user interface

This was slightly bitrotted by the static cast cleanup (just the context of the 2nd CHBV hunk), but it should be a trivial merge, meaning this patch is still OK for review.

Let's get the patch in, if it's ready, and then fix the UI, rather than let the whole thing bitrot.

Comment 16

[Sean Murphy]

Comment on attachment 269475 [details] [diff] [review]
Updated patch that includes user interface

Nice work here Bryan.  I found the whilelist searching algorithm to work as advertised.  We might want to just double check and ensure page load performance is not greatly affected by a large whitelist, as it does have to do a linear search and compare.

I wasn't quite sure what will happen with the UI, but I did review the WebFeatures code as well.

r=me with the following list of comments addressed (they're all minor):

>+- (void)onFlashblockCheck:(nsIDOMEvent*)inEvent
>+{
  
We might want to give a quick overview comment above the implementation like the other listener methods.

+    if ([currentHost hasSuffix:site])
+      inEvent->PreventDefault();
+  }

The enumeration loop should end when a match has been found.

>+
>+    // Register the CHBrowserListener to listen for flashblock whitelist checks.
>+    // Need to use an nsIDOMNSEventTarget since flashblockCheckLoad is untrusted
>+    nsCOMPtr<nsIDOMNSEventTarget> nsEventTarget = do_QueryInterface(eventTarget);
>+    if (nsEventTarget)
>+    {
>+      rv = nsEventTarget->AddEventListener(NS_LITERAL_STRING("flashblockCheckLoad"),
>+                                            NS_STATIC_CAST(nsIDOMEventListener *, _listener),                             +                                            PR_TRUE, PR_TRUE);
>+      NS_ASSERTION(NS_SUCCEEDED(rv), "AddEventListener failed: flashblockCheckLoad");
>+    }

As Smokey said, the NS_STATIC_CAST() will need to be changed to: static_cast<nsIDOMEventListener*>(_listener).

>+  IBOutlet id mFlashblockWhitelistPanel;
>+  IBOutlet ExtendedTableView*   mFlashblockWhitelistTable;
>+  IBOutlet NSTextField*         mAddFlashblockField;
>+  IBOutlet NSButton*            mAddFlashblockButton;
>+  NSMutableArray*               mFlashblockSites;
>+  NSCharacterSet*               mFlashblockSiteCharSet;

Indent mFlashblockWhitelistPanel to match the rest.

>+-(IBAction) editFlashblockwhitelistDone:(id)aSender;

Capitalization.

>+-(IBAction) addFlshblockWhitelistSite:(id)Sender;

Spelling.

>+-(void) editFlashblockWhitelistSheetDidEnd:(NSWindow *)sheetreturnCode:(int)returnCode contextInfo:(void  *)contextInfo;

We shouldn't need to declare this method, but if we do, insert a space after "sheet" and remove extra between (void  *) and move it private.

>+-(void)populateFlashblockCache;
>+-(void)saveFlashblockWhitelist;

I'd actually move these two methods over to the private category, as they're only called from within WebFeatures.mm.  If staying public, add a space after the close paren to match the style in the rest of the header.

> // data source informal protocol (NSTableDataSource)
> - (int)numberOfRowsInTableView:(NSTableView *)aTableView
> {
>   PRUint32 numRows = 0;
>-  if ( mCachedPermissions )
>+  if ( aTableView == mWhitelistTable && mCachedPermissions )
>     mCachedPermissions->Count(&numRows);
>+  else if ( aTableView == mFlashblockWhitelistTable && mFlashblockSites )
>+    numRows = [mFlashblockSites count];
> 
>   return (int) numRows;
> }

I know it's not generating a warning, but should the return value of [mFlashblockSites count] be explicitly cast into a PRUint32?  If a cast is necessary, we might want to rework the method a bit and avoid reusing/recasting the same variable so much.

> - (void) dealloc
> {
>   [[NSNotificationCenter defaultCenter] removeObserver:self];
> 
>+  if (mFlashblockSites)
>+    [mFlashblockSites release];
>+    
>+  if (mFlashblockSiteCharSet)
>+    [mFlashblockSiteCharSet release];
>+    
>   NS_IF_RELEASE(mManager);
>   [super dealloc];
> }

Not a big deal, but the two if() statements aren't absolutely necessary as messages to nil are safe here.  Also note that the PreferencePanes are not actually deallocated until Camino is quitting (but these two objects should be lightweight enough).

>+  while (NSString* site = [enumerator nextObject]) {

To match Camino style, throw an extra set of parens around (Note: this line appears twice).

>+  [NSApp beginSheet:mFlashblockWhitelistPanel
>+         modalForWindow:[mEditFlashblockWhitelist window]   // any old window accessor
>+         modalDelegate:self
>+         didEndSelector:@selector(editFlashblockWhitelistSheetDidEnd:returnCode:contextInfo:)
>+         contextInfo:NULL];

>+-(void) editFlashblockWhitelistSheetDidEnd:(NSWindow *)sheet returnCode:(int)returnCode contextInfo:(void  *)contextInfo
>+{
>+}

The didEndSelector is optional, and since it is unused you might want to just supply NULL and leave it out.

>+-(void)populateFlashblockCache
>+{
> ...
>+  NSEnumerator *enumerator = [whitelistArray objectEnumerator];
>+  while (NSString* site = [enumerator nextObject]) {
>+    site = [site stringByRemovingCharactersInSet:[NSCharacterSet whitespaceAndNewlineCharacterSet]];
>+    if ([site length] > 0 && [self isValidFlashblockSite:site])

Fix up the enumerator pointer style here and on the same variable in BrowserWrapper.
I think |isValidFlashblockSite:| should take care of checking [site length] > 0 as well, making it the central place for all validation logic.

>+-(BOOL) isValidFlashblockSite:(NSString*)aSite
>+{
>+  // Reuse character string for hostname validation since it's expensive to make
>+  if (!mFlashblockSiteCharSet) {
>+    NSMutableCharacterSet *workingSet;
>+    workingSet = [[NSCharacterSet alphanumericCharacterSet] mutableCopy];
>+    [workingSet addCharactersInString:@".-_*"];

Concerning workingSet, should URLs like 'http://site.com/someArea/' be allowed as well, or is this just a hostname thing?

>+-(IBAction) removeFlashblockWhitelistSite:(id)aSender
>+{
>+  if (mFlashblockSites) {
>+    [mFlashblockSites removeObjectAtIndex:[mFlashblockWhitelistTable selectedRow]];
>+    [self saveFlashblockWhitelist];
>+    [mFlashblockWhitelistTable reloadData];
>+  }
>+}

We'll need to either disable the remove button when nothing is selected or do some error checking in this method because [mFlashblockWhitelistTable selectedRow] hands back '-1' in that case.

And a few other nib items:

The "Edit Flash Block Exceptions List" button is not in the key-view loop (can't tab to focus it)

Hitting return after typing text into the site text field will flicker the "www.example.com" placeholder for a split second when adding.

Comment 17

[Bryan Atwood]

(In reply to comment #16)
Sean, thanks for the great comments.  I've fixed the code up as you suggested, with a few additional comments below.

> > // data source informal protocol (NSTableDataSource)
> > - (int)numberOfRowsInTableView:(NSTableView *)aTableView
> > {
> >   PRUint32 numRows = 0;
> >-  if ( mCachedPermissions )
> >+  if ( aTableView == mWhitelistTable && mCachedPermissions )
> >     mCachedPermissions->Count(&numRows);
> >+  else if ( aTableView == mFlashblockWhitelistTable && mFlashblockSites )
> >+    numRows = [mFlashblockSites count];
> > 
> >   return (int) numRows;
> > }
> 
> I know it's not generating a warning, but should the return value of
> [mFlashblockSites count] be explicitly cast into a PRUint32?  If a cast is
> necessary, we might want to rework the method a bit and avoid reusing/recasting
> the same variable so much.

Ok, I declared numRows as int and cast it to PRUint32 only when Count() is called.

> >+  [NSApp beginSheet:mFlashblockWhitelistPanel
> >+         modalForWindow:[mEditFlashblockWhitelist window]   // any old window accessor
> >+         modalDelegate:self
> >+         didEndSelector:@selector(editFlashblockWhitelistSheetDidEnd:returnCode:contextInfo:)
> >+         contextInfo:NULL];
> 
> >+-(void) editFlashblockWhitelistSheetDidEnd:(NSWindow *)sheet returnCode:(int)returnCode contextInfo:(void  *)contextInfo
> >+{
> >+}
> 
> The didEndSelector is optional, and since it is unused you might want to just
> supply NULL and leave it out.

Sounds good, I assigned the didEndSelector to nil and removed the method.

> 
> >+-(void)populateFlashblockCache
> >+{
> > ...
> >+  NSEnumerator *enumerator = [whitelistArray objectEnumerator];
> >+  while (NSString* site = [enumerator nextObject]) {
> >+    site = [site stringByRemovingCharactersInSet:[NSCharacterSet whitespaceAndNewlineCharacterSet]];
> >+    if ([site length] > 0 && [self isValidFlashblockSite:site])
> 
> Fix up the enumerator pointer style here and on the same variable in
> BrowserWrapper.

Done, but you might want to double check that I understood you correctly.

> >+-(BOOL) isValidFlashblockSite:(NSString*)aSite
> >+{
> >+  // Reuse character string for hostname validation since it's expensive to make
> >+  if (!mFlashblockSiteCharSet) {
> >+    NSMutableCharacterSet *workingSet;
> >+    workingSet = [[NSCharacterSet alphanumericCharacterSet] mutableCopy];
> >+    [workingSet addCharactersInString:@".-_*"];
> 
> Concerning workingSet, should URLs like 'http://site.com/someArea/' be allowed
> as well, or is this just a hostname thing?

I followed Flashblock on Firefox, which only checks the hostname.
 
> And a few other nib items:
> 
> The "Edit Flash Block Exceptions List" button is not in the key-view loop
> (can't tab to focus it)
> 
> Hitting return after typing text into the site text field will flicker the
> "www.example.com" placeholder for a split second when adding.

OK, but the www.example.com flicker happens on the popup whitelist too.  I decided to hold off on making these two changes until we decide if it's going in the UI or not (I still vote no, it's more of a power-user about:config feature).  If it goes in, these changes are easy to make.

Thanks again!

Comment 18

[Bryan Atwood]

Attachment: Updated patch with murph's edits

Comment 19

[Bryan Atwood]

Attachment: Updated patch with murph's edits

Comment 20

[Chris Lawson (gone)]

Hey, how close are we to being able to get this in for 1.6? I haven't seen any talk about this in ages and the patch has just been sitting there since August.

Comment 21

[Chris Lawson (gone)]

Oh, and I guarantee the nib is rotted. Bryan, any chance of posting an up-to-date nib (and patch, if necessary) in the next day or so, now that multiple accounts is no longer eating all your spare cycles? If all this needs is sr, we ought to at least try for 1.6 on it. Nominating for 1.6? and 1.6b3? just to get it in the queues.

Comment 22

[Stuart Morgan]

Removing 1.6?; it touches a nib, so unless it makes b3, it has to wait.

Comment 23

[Chris Lawson (gone)]

Per today's meeting, this is waiting on a new nib now that Web Features has gone on a diet. We're not sure if it was enough of a diet ;)

Comment 24

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

We won't block b3 on this, though we will take a patch.

(In reply to comment #15)
> Let's get the patch in, if it's ready, and then fix the UI, rather than let the
> whole thing bitrot.

This was true months ago, but if it's going to land before 1.6, given our schedule, it needs to have (basically) perfect UI.

Comment 25

[Bryan Atwood]

I'm having a hard time finding a block of time to work on the nib and am out all next week, so it probably won't make b3, for reasons that Sam mentioned.

I'll get a nib ready in a couple of weeks though.

Comment 26

[Bryan Atwood]

Attachment: Fresh nib

Oh, found some time, so uploading fresh nib file based on the leaner and meaner WebFeatures nib

Comment 27

[Bryan Atwood]

Attachment: Fresh patch

The patch itself was a little bit stale.  I cleaned it up so that it would merge correctly, so it probably needs a re-review.

Comment 28

[Bryan Atwood]

Attachment: Fresher patch

Oops, the "Edit Flash Block Exception List" button didn't toggle state when the checkbox was checked.

Comment 29

[Stuart Morgan]

Comment on attachment 299102 [details]
Fresh nib

Did you post the right file? This isn't based on the new nib.

Comment 30

[Bryan Atwood]

Attachment: Fresher nib

Oops, it's not my day.  Thanks for catching that Stuart.  This one should be right.

Comment 31

[Philip Chee]

I don't know anything about nibs but looking at the .zip, I don't think the CVS metadata directory is necessary.

Comment 32

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

Comment on attachment 299108 [details]
Fresher nib

I'm usually a good victim for this review type ;)

Most of our for-review nibs get uploaded to Bugzilla with the hidden CVS dir; it gets sorted out at checkin ;)

Comment 33

[Stuart Morgan]

If we go with this layout, I think we'll want to indent the hint text to match the button, switch the order of popups and Flashblock, and probably make all three large controls (the two push buttons and the popup button) the same size.

Comment 34

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

Attachment: screenshot of a comment 33-ish arrangement

(In reply to comment #33)
> If we go with this layout, I think we'll want to indent the hint text to match
> the button, switch the order of popups and Flashblock, and probably make all
> three large controls (the two push buttons and the popup button) the same size.

Yeah.  If we use "Edit Flash Exceptions List…" it doesn't look too bad.

Also, as you see from this screenshot, the "Edit Flash" button doesn't initialize to the correct state when the nib is loaded (nor does it respect the state of the "Enable JavaScript" checkbox like the "Block Flash" checkbox does).

Comment 35

[Bryan Atwood]

Attachment: patch for comment #34

Fixed.  Button should now be initialized correctly.

Comment 36

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

Attachment: Nib implementing comment 33-ish arrangement

Here's a nib (fixed up in IB from Xcode 1.5) that implements what Stuart and I suggested previously in comment 33 and comment 34 (and looks identical to the screenshot in comment 34). I also made a couple of minor size/alignment changes in the Flash Exceptions sheet to make it identical (warts and all, such a lovely sheet!) to the Pop-ups Exceptions sheet.  Sean, can you sanity-check the nib?

I tested the patch a little bit with the nib, and it seems to work nicely.  If we like this pref UI and the patch is solid, I'd like to see us get this in for b3.

Comment 37

[Sean Murphy]

Comment on attachment 302482 [details]
Nib implementing comment 33-ish arrangement

One minor point: should we worry about disabling the remove button when there is no site selected?  (None of the other exception sheets do this either, though.)

Otherwise, works great.
r=murph.

Comment 38

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

I thought about that, but it's a pre-existing problem with those sheets (and one that could go away if we bring UI parity between Privacy and WebFeatures implementations, so I commented in that bug about it).

Comment 39

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

Attachment: Nib implementing comment 33-ish arrangement plus the fix for bug 416727

Sean, tagging you to re-review this with the fix for bug 416727.

Comment 40

[Sean Murphy]

Comment on attachment 302979 [details]
Nib implementing comment 33-ish arrangement plus the fix for bug 416727

r=murph.

Comment 41

[Stuart Morgan]

Comment on attachment 299117 [details] [diff] [review]
patch for comment #34

+-(void)populateFlashblockCache

The method signature spacing should all be in the form:
- (foo)bar
(I know the file is inconsistent already, but we are trying to standardize going forward.)

>+  // Always reload pref since user may have manually edited list
>+  // The whitelist pref is a string with format:  
>+  //  siteA.com, www.siteB.com, *.siteC.com

As generated by this code, it's actually siteA.com,www.siteB.com,*.siteC.com, so it should be documented without the spaces.

>+  NSArray* whitelistArray = [[self getStringPref:"flashblock.whitelist" withSuccess:NULL]
>+                              componentsSeparatedByString:@","];

The second line should be indented two more spaces.

>+  [self populateFlashblockCache];
>+
>+  [NSApp beginSheet:mFlashblockWhitelistPanel
>+         modalForWindow:[mEditFlashblockWhitelist window]   // any old window accessor
>+         modalDelegate:self
>+         didEndSelector:nil
>+         contextInfo:NULL];

Align these lines on the ':'

>+ -(IBAction) editFlashblockWhitelistDone:(id)aSender

This method should also clean up mFlashblockSites and the character set, so that they aren't sticking around past when we need the. It should also clear the add field.

>+  if (!mFlashblockSites)
>+    return;

No need for this check everywhere; this can't realistically happen while the sheet is open.

>+  NSString* site = [[mAddFlashblockField stringValue] stringByRemovingCharactersInSet:
>+                    [NSCharacterSet whitespaceAndNewlineCharacterSet]];

Two more spaces.

+  // Only add a Flashblock whitelist site if it's properly formatted and not already added
+  if ([self isValidFlashblockSite:site] && ![mFlashblockSites containsObject:site]) {
+    [mFlashblockSites addObject:site];
+    [self saveFlashblockWhitelist];
+    [mFlashblockWhitelistTable reloadData];      
+  }
+
+  [mAddFlashblockField setStringValue:@""];      
+  [mAddFlashblockButton setEnabled:NO];

If someone enters an invalid character by accident, this will silently discard it, which they might not even notice if there are a lot of exceptions already. At a minimum, the !isValidFlashblockSite case should leave the string intact and NSBeep().

>+  if ( aTableView == mWhitelistTable )

No internal spaces.

>+  if ( aTableView == mWhitelistTable ) {

Same.

>+  }
>+  else // aTableView == mFlashblockWhitelistTable
>+    retVal = [mFlashblockSites objectAtIndex:rowIndex];

The |else| needs {} since it's paired with an |if| that has them.

>+-(BOOL) isValidFlashblockSite:(NSString*)aSite
>+{
>+  if ([aSite length] == 0)
>+    return FALSE;

BOOL must be either YES or NO.

>+    NSMutableCharacterSet *workingSet;
>+    workingSet = [[NSCharacterSet alphanumericCharacterSet] mutableCopy];
>+    [workingSet addCharactersInString:@".-_*"];
>+    mFlashblockSiteCharSet = [workingSet copy];
>+    [workingSet release];

No need for the extra copy here, since NSMutableCharacterSet is a subclass of NSCharacterSet. Just 

>+- (void)onFlashblockCheck:(nsIDOMEvent*)inEvent

I'm somewhat concerned by the amount of duplicate string parsing work this is doing on every Flash element load. Could you keep this list, with all the fixup already done, in memory in a central location that watches for changes to the pref and updates itself only at that point?

Comment 42

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

Bryan said he was going to try to get to this in the next couple weeks, so nominating for 2.0a1 to keep it on that radar; if this is ready/close, it'll be a nice UI feature for a1.

Comment 43

[Bryan Atwood]

Attachment: patch for comment #41

Stuart, I finally got time to look at your comments.  Sorry for being so late.  I implemented all of your changes, but wanted to comment on a couple of things.

>>+ -(IBAction) editFlashblockWhitelistDone:(id)aSender
>This method should also clean up mFlashblockSites and the character set, so
>that they aren't sticking around past when we need the. It should also clear
>the add field.

What do you mean by character set?  I stored it in a member variable so that it doesn't need to be re-computed every time.

>>+- (void)onFlashblockCheck:(nsIDOMEvent*)inEvent
> I'm somewhat concerned by the amount of duplicate string parsing work this is
> doing on every Flash element load. Could you keep this list, with all the fixup
> already done, in memory in a central location that watches for changes to the
> pref and updates itself only at that point?

OK, I added member variables to track the array and it only recreates the array if the preference string changes.  It checks whether the string has changed each time onFlashblockCheck is called instead of having some sort of callback function be invoked whenever the preference is changed.  It seemed easier, and should be minimal overhead while still addressing your concern.

Comment 44

[Bryan Atwood]

Attachment: Updated patch for comment #41

Same as the previous patch, but has some formatting fixes.  There were some white space characters that I "took care of".

Comment 45

[Stuart Morgan]

Comment on attachment 324516 [details] [diff] [review]
Updated  patch for comment #41

First off, sorry for the long delay (and I'll turn the next version around much faster). It's very close!

(In reply to comment #43)
> What do you mean by character set?  I stored it in a member variable so that it
> doesn't need to be re-computed every time.

Caching it during the lifetime of the sheet session is good, but there's no need to keep it it memory for the entire lifetime of the app just because the sheet was shown once; building the set once each time the sheet is shown is not a big deal. So leave all the caching you currently do, but release and nil the member variable when the sheet is dismissed.



>+  if (!mFlashblockSites)
>+    mFlashblockSites = [[NSMutableArray alloc] init];

So that this method is correct even if called repeatedly, toss in an else that empties the array.

>+  while ((site = [enumerator nextObject])) {
>+    site = [site stringByRemovingCharactersInSet:[NSCharacterSet whitespaceAndNewlineCharacterSet]];
>+    if ([self isValidFlashblockSite:site])
>+      [mFlashblockSites addObject:site];
>+  }
>+
>+  // Write correctly formatted sites to prefs so that we stay in sync
>+  [self saveFlashblockWhitelist];
>+}

There's only a synchronization problem if a site was invalid right (since whitespace is ignored no matter what)? If so, let's save on unnecessary I/O by only doing the save if [whitelistArray count] != [mFlashblockSites count] after loading.

>+-(void)saveFlashblockWhitelist
[...]
>+-(IBAction) editFlashblockWhitelist:(id)aSender
[...]
>+ -(IBAction) editFlashblockWhitelistDone:(id)aSender
[...]
>+-(IBAction) removeFlashblockWhitelistSite:(id)aSender
[..]
>+-(IBAction) addFlashblockWhitelistSite:(id)sender

Since unlike the header the (wrong) format isn't consistent in the implementation file, go ahead and do all of these according to the correct Camino style:
- (foo)bar:baz:

>+  [mAddButton setEnabled:NO];
>+}
>+
>+ -(IBAction) editFlashblockWhitelistDone:(id)aSender
>+{
[...]
>+  [mAddFlashblockField setStringValue:@""];

Since these two things are tightly coupled, do them both in the same place (which will have to be when showing the sheet, so the button is correct the first time).

>+-(BOOL) isValidFlashblockSite:(NSString*)aSite
>+{
>+  if ([aSite length] == 0)
>+    return NO;
>+
>+  // Reuse character string for hostname validation since it's expensive to make
>+  if (!mFlashblockSiteCharSet) {
>+    mFlashblockSiteCharSet = [[NSCharacterSet alphanumericCharacterSet] mutableCopy];
>+    [mFlashblockSiteCharSet addCharactersInString:@".-_*"];
>+  }

There are quite a few invalid domains this will let through: "...", for example, or "a*.com", or ".foo.com" (I'm assuming the latter two aren't supposed to work based on the match logic later on). How about splitting on '.' (after removing an initial dot if .foo.com is allowed), and checking each component for being a valid, non-zero segment?.

>+  NSString*                 mFlashblockWhitelistPref;      // STRONG
>+  NSMutableArray*           mFlashblockWhitelistSites;     // STRONG

Having one of these per tab is pretty wasteful. How about a simple little singleton class that manages this in one place, and has an isAllowed:(NSString*)domain (or similar) for client to use? And if you don't want to set up a pref listener in it, you could have the exceptions UI send a notification that it listens for to re-create the list, rather than re-checking the pref each time (that will miss live edits to about:config, but we don't care since there's a real UI).

>+nsresult
>+CHBrowserListener::HandleFlashblockCheckEvent(nsIDOMEvent* inEvent)
>+{
>+   [mContainer onFlashblockCheck:inEvent];
>+
>+   return NS_OK;
>+}

Indentation is off by one.

Comment 46

[Stuart Morgan]

Missed a1, but assuming Bryan has time to get to this again soon we'd really like it for b1.

Comment 47

[Bryan Atwood]

Attachment: Updated patch for comment #45

Stuart,

I'm sorry for the delay.  I've implemented all of your changes.  In particular, please check the following:

>>+  NSString*                 mFlashblockWhitelistPref;      // STRONG
>>+  NSMutableArray*           mFlashblockWhitelistSites;     // STRONG

>Having one of these per tab is pretty wasteful. How about a simple little
>singleton class that manages this in one place, and has an
>isAllowed:(NSString*)domain (or similar) for client to use? And if you don't
>want to set up a pref listener in it, you could have the exceptions UI send a
>notification that it listens for to re-create the list, rather than re-checking
>the pref each time (that will miss live edits to about:config, but we don't
>care since there's a real UI).

I did end up creating a singleton class in BrowserWrapper.mm and set up an NSNotification that is sent by the exceptions UI when a new pref is saved.  Please check this code since it's got the most changes.

I also changed the matching code a bit.  All sites in the whitelist have '.' added to them in the singleton class (eg. 'youtube.com' becomes '.youtube.com').  When a site is checked against the list, a '.' is also appended (www.youtube.com becomes .www.youtube.com).  The whitelist sites are matched against the end of the current host site.  This prevents 'www.somesite.com' from matching a whitelist setting of 'site.com'.

Everything else should be pretty straightforward.

Thanks for the great review, it's educational as always.  And apologies once again to the team for the tardiness.

Comment 48

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

BTW, the nib on this bug is slightly bitrotted, so it will have to be respinned when the patch is ready.

Comment 49

[Bryan Atwood]

Attachment: Updated nib, old one was bit-rotted

Comment 50

[Sean Murphy]

Comment on attachment 350882 [details]
Updated nib, old one was bit-rotted

The popup exceptions sheet doesn't do it either, but we might want to think about disabling the remove button if no site is selected in the table view.

Other than that, everything looks great.  Key view loop works, and the button is disabled without Javascript, etc.

r=murph.

Comment 51

[Stuart Morgan]

Comment on attachment 350739 [details] [diff] [review]
 Updated patch for comment #45

This looks great! Thanks for making those changes. r=me on the code with the two minor tweaks below.

I don't have time to do thorough testing at the moment, so I'm marking this for a second review, but it should just be a behavioral test.


>+// Function called when application shuts down to dealloc instance
>+- (void)shutdown;

Nothing particularly interesting happens in dealloc, so there's no need to jump through hoops to make sure that it's called. Remove |shutdown| and the XPCOM notification listener (leave dealloc for completeness, but add a note that it won't actually be called).

>+  if (!mFlashblockSiteCharSet) {
>+    mFlashblockSiteCharSet = [[NSCharacterSet alphanumericCharacterSet] mutableCopy];
>+    [mFlashblockSiteCharSet addCharactersInString:@"-_"];
>+  }

The docs note that mutable character sets are less efficient to use, so it would be better to do:
  NSMutableCharacterSet* charSet = [[[NSCharacterSet alphanumericCharacterSet] mutableCopy] autorelease];
  [charSet addCharactersInString:@"-_"];
  mFlashblockSiteCharSet = [charSet copy];
(and change the type of the member variable, of course)

Comment 52

[Stuart Morgan]

Comment on attachment 350739 [details] [diff] [review]
 Updated patch for comment #45

Giving the testing to murph, since it sounds like he's already done most of it ;)

Comment 53

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

Per discussion today, assuming no serious issues arise from murph's review or from sr, and assuming that this makes it through sr at the same time as the other b1 blockers this week, we'll block b1 on this.

Comment 54

[Bryan Atwood]

Attachment: Updated patch for comment #51

Done and done.  Thanks for the fast response, let's try to keep on this one until we push it through.

Comment 55

[Sean Murphy]

Comment on attachment 351991 [details] [diff] [review]
Updated patch for comment #51

Behavior looks great to me, I do not see any issues.  Nice code Bryan!

r=murph.

Comment 56

[Mike Pinkerton (not reading bugmail)]

++ (FlashblockWhitelistManager*)flashblockWhitelistManager

Maybe instead call this |sharedManager| or something to align with the convention in AppKit?

+  NSMutableArray*               mFlashblockSites;
+  NSCharacterSet*               mFlashblockSiteCharSet;

if these are strong references, you should say so.

+    if (!mFlashblockWhitelistSites)
+      mFlashblockWhitelistSites = [[NSMutableArray alloc] init];
+
+    [mFlashblockWhitelistSites removeAllObjects];

elsewhere in the code, there's an |else| here to avoid clearing an empty array. Be consistent? 

+  if (aTableView == mWhitelistTable)
+    return [mCachedPermissions count];
+  else // aTableView == mFlashblockWhitelistTable
+    return [mFlashblockSites count];

no need for an else after a return. Omit it.

sr=pink with these fixed up.

Comment 57

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 351991 [details] [diff] [review]
Updated patch for comment #51

sr=pink (so you don't have to come back to me later)

Comment 58

[Bryan Atwood]

Attachment: Updated patch for comment #56

Thanks Mike, I made your changes.

> ++ (FlashblockWhitelistManager*)flashblockWhitelistManager

> Maybe instead call this |sharedManager| or something to align with the
> convention in AppKit?

ok, I called it |sharedInstance| to follow other places in the code (eg. PreferenceManager)

Hopefully we are almost done!

Comment 59

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

(In reply to comment #58)
> Hopefully we are almost done!

Done!  Checked in on cvs trunk in advance of 2.0b1!  Thanks for sticking with us through this, Bryan.

Can someone please file a bug on comment 37/comment 50?  Bug 370119 might obsolete it, but good to have it on file regardless.

Comment 60

[Christopher Henderson]

Bug 469926 filed.