475201 - add "allow flash from current site" to contextual menu - or other shortcut to whitelist flash

Status: RESOLVED FIXED

(Camino Graveyard :: Annoyance Blocking, enhancement)

Description

[dan pritts]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

Hi -

Thanks for getting flashblock whitelisting working!

it would be great (and hopefully easy to implement) to have "allow flash from current site" in the contextual menu when you right-click on a blocked flash.  If this isn't clear, right-click on a blocked flash in firefox to see what i mean.

Alternately, having some other way to easily add the current site to the whitelist would be great.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Chris Lawson (gone)]

If we can limit this so that it *only* appears in the CM when clicking on a blocked Flash animation, I think we should definitely do this.

Pre-filling (and selecting, so it could easily be typed over) the foreground tab's domain in the Add field of the whitelist sheet might also be a good idea.

cl

Comment 2

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

(In reply to comment #0)
> User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US;
> rv:1.8) Gecko/20051111 Firefox/1.5
Um...you probably don't want to be spoofing as a three-plus-year-old version of another browser. ;)

(In reply to comment #1)
> If we can limit this so that it *only* appears in the CM when clicking on a
> blocked Flash animation, I think we should definitely do this.

There are probably perf penalties with doing this, although it seems reasonably fast in the full Flashblock on my fast MBP.

I pretty much agree we ought to come up with some good UI for easy adding to the whitelist, though.

Comment 3

[Philip Chee]

> There are probably perf penalties with doing this, although it seems reasonably
> fast in the full Flashblock on my fast MBP.

The Firefox context menu is XUL of course, and the context menu code already contains functionality for turning on/off various menu items based on the target so I would guess that the XUL popup code is optimized to reduce any pref impact.

> I pretty much agree we ought to come up with some good UI for easy adding to
> the whitelist, though.

The full Flashblock has an optional toolbar button with a dropdown menu with one of the options being "Allow flash from this site".

Comment 4

[Christopher Henderson]

Taking. I have the context menu/flashblock detection stuff done, I just need to figure out how to hook it up to the whitelist sheet.

Comment 5

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

(In reply to comment #3)
> The Firefox context menu is XUL of course, and the context menu code already
> contains functionality for turning on/off various menu items based on the
> target

I had forgotten we already had some of this code, too.

> The full Flashblock has an optional toolbar button with a dropdown menu with
> one of the options being "Allow flash from this site".

If not for bug 448992, people could probably do this pretty easily with toolbar scripts.

Comment 6

[Christopher Henderson]

As a note for myself, there's not really a way for BWC to open the WebFeatures prefpane. It can be done by hacking MVPreferencesController, but we don't want to do that. The better strategy is to move whitelist management to its own class, which both BWC and WebFeatures can use. I might investigate to see if I can make a class that all four whitelists can use. Once that is done, I can make a sheet for the browser window to add a domain to the flashblock whitelist.

Comment 7

[Christopher Henderson]

Attachment: Fix v1.0

This patch implements one method of fixing this bug. I don't know if this is how we want to do it, and if it's not please speak up.

The main context menu now has an 'Allow Flash From This Site' item in it. The |contextMenu| method in BWC hides this item and the separator above it if there is no Flashblock placeholder under the mouse. It does this by inspecting the current context menu node element for a background image that is unique to the Flashblock placeholder.

If the item is selected, a sheet is presented to the user with the textfield pre-filled witht the current site's domain. The user then has the option to edit the domain (for example, to change nz.youtube.com to youtube.com) before clicking Allow.

FlashblockWhitelistManager has been moved to its own files. This allows us to access its whitelist from the Web Features prefpane, obviating much of the duplication in the latter's source. The prefpane has the same validation as the sheet, though by keeping a reference to the shared manager and querying it (BWC calls |sharedInstance| each time the text changes). The shared instance shouldn't change throughout the app's lifetime, so is this safe enough?

When an entry is added or removed from the prefpane table, the FlashblockWhitelistManager is called; it changes its array of sites, saves the new array; this sets the gecko pref flashblock.whitelist, which posts a gekco pref change notification, which alerts the manager, which loads the pref, which causes another notification to be picked up by the prefpane, which reloads the table data (an array from the manager). Convoluted, but it works. And it ensures that any change (from a browser sheet, prefpane, or about:config) is immediately reflected in the UI.

A similar process happens when the prefpane sheet is opened. The prefpane calls |loadWhitelistSites|, which gets the gecko pref and posts a notification, which the prefpane receives and reloads the table data (an array from the manager).

The prefpane gets the table data from the manager using the latter's |whitelistArray| method. The manager loads and stores the sites with a full stop prepended to match subdomains properly. We strip those before giving them to the prefpane (and for saving the pref), so they look sensible to the user. We are doing comparisons with the full stop prefix more often, so it makes sense to keep the manager's array of domains in that format.

Questions:
1. Is the browser window sheet overkill? Do we just want to add the current domain without any further user input? They can always refine the whitelist via the prefpane.
2. Do we want to reload the page after the user clicks Allow? The standard arguments re: inputted form data would apply.
3. Do we want to have the continual validation of the domain? To me, it adds slickness. The main thing to worry about would be performance; my C2D has no problem, but I don't know how a G3 would handle it. If we want to keep it, is keeping a reference to the shared manager preferable to always calling |sharedInstance|?
4. Is the memory management stuff OK? I'm pretty sure the retains and releases match, but would still appreciate an assessment.

Comment 8

[Christopher Henderson]

Attachment: BrowserWindow.nib for Fix v1.0

BrowserWindow.nib to go along with the patch. Adds the CM items, and a Window for the Flashblock sheet. Created with IB 2.5 on Leopard.

Comment 9

[Stuart Morgan]

> 1. Is the browser window sheet overkill? Do we just want to add the current
> domain without any further user input?

Yes (as discussed on IRC)

> 2. Do we want to reload the page after the user clicks Allow? The standard
> arguments re: inputted form data would apply.

We probably need to, or the feature will appear broken.

> 3. Do we want to have the continual validation of the domain?

Seems reasonable; the strings are short, and the list itself is likely to be short as well, so I wouldn't expect probems. We can decide based on your test on an old machine though.

> is keeping a reference to the shared manager preferable to always
> calling |sharedInstance|?

No, since one message dispatch is essentially free as compared to all the string walking and comparing that will happen.

Comment 10

[Christopher Henderson]

Attachment: Fix v1.1

New patch which incorporates the suggestions from comment 9. Also fixes an infinite loop that would happen if the prefpane sheet was the first object to created the shared manager. The performance of the site validation was fine on a 800 MHz G3 iBook, with only a little slow down when the delete was held down.

Comment 11

[Christopher Henderson]

Attachment: BrowserWindow.nib for Fix v1.1

New BrowserWindow.nib without the browser window sheet. Only has the two CM items. Modified with IB 2.5 on 10.5.

Comment 12

[Eiichi]

Attachment: BrowserWindow.nib (if needed)

Opened and saved id=367000 with Interface Builder (Version 2.5.6) on 10.4.11.

Comment 13

[dan pritts]

after some dorking around I got the trunk built with this patch applied.  Works for me.

a couple thoughts - 

The flash plugin's contextual menu probably isn't modifiable, but if it is, having "blacklist flash from this site" ("remove from flash whitelist"?) available would be great.

for that matter, I'm not sure this shouldn't be a toggle, always available anywhere in a page if flashblock is enabled.  

finally, a random experience that is probably a red herring.  once, and only once, while experimenting with this patch applied, i was unable to add a site to the whitelist from the whitelist preferences pane.  

I can't replicate it with the patch, nor with the latest nightly.

I don't know if this patch touches the code for the whitelist pref pane at all (I'd guess not), but the one time this happened, it was with the patch applied.  

sequence of events, roughly, from memory:
whack camino prefs directory
start camino
turn on flashblock
visit espn.com 
use menu to add to whitelist, watch page reload with flash on
go to prefpane
remove espn.go.com from whitelist in prefpane
try to add go.com or espn.go.com to whitelist, the "add" button is always grayed out.

Comment 14

[Christopher Henderson]

Thanks for testing, Dan.

I have no idea whether we can modify plugin context menus, but I'm guessing we can't.

The context menu behaviour was modeled on that of the Flashblock Firefox extension, but that doesn't mean we can't do what you suggest. That would mean having the item present for all pages, since walking the DOM to find Flashblock elements would probably be too resource intensive.

This patch removes duplication from the pref pane's code. I tried to reproduce the problem with the Add button, but couldn't.

Comment 15

[Chris Lawson (gone)]

(In reply to comment #14)
> Thanks for testing, Dan.
> 
> I have no idea whether we can modify plugin context menus, but I'm guessing we
> can't.

I don't know what, exactly, is doing the modifying, but a right-click on the player at

http://www.cartalk.com/piplayer/cartalkplayer.html?play=showAllsmil.xml


(which is Flash-based) shows an item that isn't in the standard Flash CM. If we can duplicate the process of how it got there, we might be able to do this part of the bug.

Comment 16

[philippe (part-time)]

(In reply to comment #15)
 
> I don't know what, exactly, is doing the modifying, but a right-click on the
> player at
> 
> http://www.cartalk.com/piplayer/cartalkplayer.html?play=showAllsmil.xml
> 
> 
> (which is Flash-based) shows an item that isn't in the standard Flash CM. If we
> can duplicate the process of how it got there, we might be able to do this part
> of the bug.

Those are additional CM items added _within_ the flash player itself.

Comment 17

[philippe (part-time)]

I could (easily) repro Dan's problem with the Add button (comment 13).

STR
1. turn FlashBlock on
2. load http://www.elpais.com/global
3. scroll down, right click on a Flash movie and whitelist it.
4. open the prefpane, Edit FlashBlock Exception list
5. Try to add another domain: fails, button remains greyed out.

I have ad-blocking turned on as well, of course.

Also reproduced with guardian.co.uk/world.
The whole thing otherwise works pretty well.

Build is made using GCC 4.2.1 and 10.5 SDK.

Comment 18

[dan pritts]

> The context menu behaviour was modeled on that of the Flashblock Firefox
> extension, but that doesn't mean we can't do what you suggest. That would mean
> having the item present for all pages, since walking the DOM to find Flashblock
> elements would probably be too resource intensive.

Just to be clear, I am mostly just brainstorming as I make these suggestions.  Having the firefox-equivalent behavior is definitely the 95% solution.  

I'm ok with having it available all the time (I am a knob-turner), but i'm not sure it's the right thing to do vs. keeping the UI uncluttered.  I also, e.g., like the idea of having cookie-blocking stuff in the context menu.

re: add button:  I am NOT using the generic ad-blocking, just flashblock.

Comment 19

[Christopher Henderson]

Does the Add button remain greyed out even when there is a valid domain in the text field (eg: guardian.co.uk)? There is live validation of the string going on that means that the Add button is inactive until a valid-looking string is entered. In this case, the button is inactive until guardian.co is the string; it looks for >= 1 full stop and no invalid characters.

If there is too much user confusion we should probably go back to the previous behaviour of validation on Add.

Comment 20

[philippe (part-time)]

(In reply to comment #19)
> Does the Add button remain greyed out even when there is a valid domain in the
> text field (eg: guardian.co.uk)?

Yes. I entered a full domain (www.example.com) and the button remained greyed out.

But a puzzling thing: I tested this now under Troubleshoot Camino. Turned Ad-blocking and Flash Block ON. Loading ElPais.com, white-list it.

Under that configuration, after entering www.exa the Add button became active.

Comment 21

[Christopher Henderson]

Attachment: Fix v1.2

That's the damnedest thing; editing the whitelist, then disabling ad blocking, then trying to edit the whitelist, results in the always-disabled Add button. What's happening is the delegate method isn't getting called when the text field changes; this new version of the patch fixes that by always setting the delegate when opening the sheet.

Thanks Dan and Phileppe for bringing this to my attention.

Comment 22

[philippe (part-time)]

(In reply to comment #21)
> Created an attachment (id=369203) [details]
> Fix v1.2
> 
> That's the damnedest thing; editing the whitelist, then disabling ad blocking,
> then trying to edit the whitelist, results in the always-disabled Add button.
> What's happening is the delegate method isn't getting called when the text
> field changes; this new version of the patch fixes that by always setting the
> delegate when opening the sheet.

Yup, that worked fine now, with my usual profile.

> 
> Thanks Dan and Phileppe for bringing this to my attention.
s/Phileppe/Philippe
:-)

Comment 23

[Chris Lawson (gone)]

Comment on attachment 369203 [details] [diff] [review]
Fix v1.2

Today's landings rotted this; hendy's going to spin a new patch with some review comments (from a read-through) I gave on IRC. I'll test it once the new patch is spun.

Comment 24

[Christopher Henderson]

Attachment: Fix v1.3

Unrotted version with a few changes suggested by Chris on IRC.

Comment 25

[Chris Lawson (gone)]

Comment on attachment 369427 [details] [diff] [review]
Fix v1.3

Tested on

http://www.adobe.com/shockwave/welcome/

and it works great. I don't get the menu item on anything unless Flashblock is on, and then I only get it on blocked Flash animations. Adding sites to the whitelist seems to work just fine; the page reloads and the Flash appears. Behaviour seems to be what we want, so that's good.

Code looks good and conforms to style.

On IRC, hendy briefly mentioned the possibility of moving the Flashblock placeholder detection into GeckoUtils, just to avoid making BWC any bigger than it already is. In the interest of keeping our classes self-contained where it makes sense, though, neither of us could come up with a reason to do that other than size (we can't think of any other classes that might want to use that functionality).

Tagging smorgan for sr.

Comment 26

[Stuart Morgan]

Comment on attachment 369427 [details] [diff] [review]
Fix v1.3

>+      // We need to identify the Flashblock placeholder somehow. The most
>+      // direct way is to look for chrome://flashblock/content/*
>+      // as the background image, as this is what Flashblock uses.
>+      nsCOMPtr<nsIDOMElement> flashElement(do_QueryInterface(mDataOwner->mContextMenuNode));
>+      if(flashElement) {
>+        nsCOMPtr<nsIDOMDocument> document;
>+        flashElement->GetOwnerDocument(getter_AddRefs(document));
>+        nsCOMPtr<nsIDOMDocumentView> docView(do_QueryInterface(document));
>+
>+        nsCOMPtr<nsIDOMAbstractView> defaultView;
>+        docView->GetDefaultView(getter_AddRefs(defaultView));
>+        nsCOMPtr<nsIDOMViewCSS> defaultCSSView(do_QueryInterface(defaultView));
>+
>+        nsCOMPtr<nsIDOMCSSStyleDeclaration> computedStyle;
>+        nsCOMPtr<nsIDOMCSSPrimitiveValue> primitiveValue;
>+        defaultCSSView->GetComputedStyle(flashElement, EmptyString(),
>+                                         getter_AddRefs(computedStyle));
>+        if (computedStyle) {
>+          nsCOMPtr<nsIDOMCSSValue> cssValue;
>+          computedStyle->GetPropertyCSSValue(NS_LITERAL_STRING("background-image"),
>+                                             getter_AddRefs(cssValue));
>+          nsAutoString bgStringValue;
>+          primitiveValue = do_QueryInterface(cssValue);
>+          if (primitiveValue) {
>+            primitiveValue->GetStringValue(bgStringValue);
>+            if ([[NSString stringWith_nsAString:bgStringValue]
>+                 hasPrefix:@"chrome://flashblock/content/"]) {
>+              isFlashblock = YES;
>+            }
>+          }
>+        }
>+      }
>+    }    

Move all of this to a private helper method that takes mDataOwner->mContextMenuNode as an argument. Once it's a separate method, invert the NULL checks to be early-return NOs rather than nesting.

>+- (IBAction)allowFlashFromSite:(id)sender

Maybe s/allow/unblock/, since allow doesn't really suggest that it will reload immediately. Also, FromSite: sounds like it should take an argument, so perhaps unblockFlashFromCurrentDomain.

>+// Loads whitelisted sites from preference
>+- (void)loadWhitelistSites;
>+- (void)saveFlashblockWhitelist;
>+
>+- (NSString*)addFlashblockWhitelistSite:(NSString*)aSite;
>+- (void)removeFlashblockWhitelistSite:(NSString*)aSite;
>+
>+// Checks if Flash is allowed for the site
>+- (BOOL)isFlashAllowedForSite:(NSString*)site;
>+
>+- (BOOL)canAddToWhitelist:(NSString*)site;
>+
>+- (BOOL)isValidFlashblockSite:(NSString*)aSite;
>+
>+- (NSArray*)whitelistArray;

All of these should have comments documenting them.

>+- (id)init
>+{
>+  [self loadWhitelistSites];
>+
>+  [[PreferenceManager sharedInstance] addObserver:self forPref:kGeckoPrefFlashblockWhitelist];
>+  [[NSNotificationCenter defaultCenter] addObserver:self
>+                                           selector:@selector(loadWhitelistSites)
>+                                               name:kPrefChangedNotificationName
>+                                             object:self]; // since we added ourself as the Gecko pref observer
>+  return self;
>+}

Can you wrap the guts of this in the usual |if ((self = [super init])) {| block while you are moving it? I just noticed we don't have it.

>+- (NSString*)addFlashblockWhitelistSite:(NSString*)aSite

Given than nobody looks at the return value, why make this return a string?

>+  else {
>+    NSBeep();

Beeping is UI feedback, so it doesn't belong here. This method should return a BOOL (or nil/non-nil if it really has to be an NSString for some reason) and clients should use that to decide if it worked and give whatever feedback is appropriate in their case.

>+  // Sometimes the delegate gets lost.
>+  [mAddFlashblockField setDelegate:self];

This worries me; I don't like hacking around regressions that we don't understand. Have you tried setting a breakpoint on setDelegate: and seeing if you can find out where it is being unset?

removeFlashblockWhitelistSite:(NSString*)[mFlashblockSites objectAtIndex:row]];

Why the cast here?

Comment 27

[Christopher Henderson]

Attachment: Fix v1.4

Updated patch to address sr comments.

>All of these should have comments documenting them.

Done, but tell me if I've gone overboard with the method comments.

>Given than nobody looks at the return value, why make this return a string?

Changed to return a BOOL.

>This worries me; I don't like hacking around regressions that we don't
understand.

This regression was due to my removing the pref pane as an observer in |editFlashblockWhitelistDone|, which also removed it as an observer of text field string changes. I have modified that call to make it more specific.

>Why the cast here?

Beats me :). I have removed it.

Comment 28

[Christopher Henderson]

Attachment: BrowserWindow.nib for Fix v1.1

New BrowserWindow.nib, due to the renaming of |allowFlashFromSite| to |unblockFlashFromCurrentDomain|. Will need to be resaved in 10.4, but not until the patch passes sr.

Comment 29

[Stuart Morgan]

Comment on attachment 371584 [details] [diff] [review]
Fix v1.4

Looks good; just a few more small things.

>+    if ([[PreferenceManager sharedInstance] getBooleanPref:kGeckoPrefBlockFlash withSuccess:NULL]
>+            && [self isFlashblockElement:(mDataOwner->mContextMenuNode)]) {

The second line should align just inside the (, so 4 spaces left.

>+  nsCOMPtr<nsIDOMDocumentView> docView(do_QueryInterface(document));
>+
>+  nsCOMPtr<nsIDOMAbstractView> defaultView;
>+  docView->GetDefaultView(getter_AddRefs(defaultView));
>+  nsCOMPtr<nsIDOMViewCSS> defaultCSSView(do_QueryInterface(defaultView));
>+
>+  nsCOMPtr<nsIDOMCSSStyleDeclaration> computedStyle;
>+  defaultCSSView->GetComputedStyle(flashElement, EmptyString(),
>+                                   getter_AddRefs(computedStyle));

Add null-check-with-returns before using docView and defaultCSSView.

>+// Add and remove a site from the whitelist.
>+- (BOOL)addFlashblockWhitelistSite:(NSString*)aSite;
>+- (void)removeFlashblockWhitelistSite:(NSString*)aSite;

The return value of addFlashblockWhitelistSite: is non-obvious, so should be documented. The comment you have for this method in the implementation file is actually perfect for the header, since it describes what it does and what it returns; I'd suggest just moving it there. (Also, you should say what aSite can be (just a domain? full URL?), or make the parameter name more clear.)

Several of your comments should actually be moved from the implementation to the header. The rule is basically the header comment should tell you everything you need to know to use it, and the implementation comment should just have any implementation details worth noting (your implementation comment for loadWhitelistSites is great example of the latter).

>+// Checks if Flash is allowed for the site.
>+- (BOOL)isFlashAllowedForSite:(NSString*)site;
>+
>+// Validation methods for a proposed whitelist site.
>+- (BOOL)canAddToWhitelist:(NSString*)site;
>+- (BOOL)isValidFlashblockSite:(NSString*)aSite;

Same comment about site applies here.

>+  [[NSNotificationCenter defaultCenter] removeObserver:self 
>+                                                  name:kFlashblockWhitelistChangedNotificationName 
>+                                                object:nil];

For dealloc, just use the [... removeObserver:self] form.

>+  [mFlashblockSites release];
>+  mFlashblockSites = nil;

This should be done after closing the sheet.

Comment 30

[Christopher Henderson]

Attachment: Fix v1.5

Patch updated to address Stuart's comments, and with a Smokey-friendly filename.

>For dealloc, just use the [... removeObserver:self] form.

Seeing we remove ourselves as an observer in |dealloc|, I have removed that line in |editFlashblockWhitelistDone|.

Comment 31

[Stuart Morgan]

Comment on attachment 374209 [details] [diff] [review]
Fix v1.5

>+//
>+// -saveFlashblockWhitelist:
>+//
>+// Sets the Flashblock whitelist Gecko preference to the current whitelist array.
>+//

For header comments don't repeat the method name or add blank comment lines (I've never particularly liked it in our implementation files files, but in the header it's extra distracting).

(In reply to comment #30)
> >For dealloc, just use the [... removeObserver:self] form.
> 
> Seeing we remove ourselves as an observer in |dealloc|, I have removed that
> line in |editFlashblockWhitelistDone|.

Oops, I wasn't paying enough attention to the method. Please restore that line the way you had it, which was right in the first place :) No need to keep observing once the sheet is gone.

sr=smorgan with those changes

Comment 32

[Christopher Henderson]

Attachment: Fix, for checkin

Patch, with sr+ comments addressed, for checkin. The nib (attachment 371585 [details]) will need resaving in 10.4 first, though.

Comment 33

[Eiichi]

Attachment: BrowserWindow.nib

(In reply to comment #32)

>The nib (attachment 371585 [details])
> will need resaving in 10.4 first, though.

Opened and resaved id=371585 with Interface Builder (Version 2.5.6) on 10.4.11.

Comment 34

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

Checked in on cvs trunk.  Thanks for nib, Eiichi!

Comment 35

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

Backed out due to our old friend the Xcode 2.x ld suckage.

Comment 36

[Christopher Henderson]

Attachment: Fix for checkin, take 2

Adds a no_dead_strip line to prevent bustage. Hopefully this will be enough to get the tree green again.

Comment 37

[Stuart Morgan]

That's weird. The Camino binary uses FlashblockWhitelistManager, so I don't see why/how this could be necessary.

Comment 38

[Stuart Morgan]

Nevermind, that's true of KeychainDenyList too. I was thinking of the similar problem with unused constants.

Comment 39

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

And back in again, with green!

Comment 40

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

Turns out this was the cause of bug 490278, so I backed out the existing files that the patch changed (I was tired of removing and re-adding the new FlashblockWhitelistManager files, so they're still in the tree, just not built).

We'll need Sam to get us build logs from when boxset failed (or me to generate new ones when he can look at them) to see what's wrong (I suspect another 10.4/Xcode 2.x toolchain bug, offhand).

Comment 41

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

Comment on attachment 374651 [details] [diff] [review]
Fix for checkin, take 2

>Index: camino/src/browser/FlashblockWhitelistManager.h
>===================================================================
[...]
>+ * The Initial Developer of the Original Code is
>+ * Bryan Atwood
>+ * Portions created by the Initial Developer are Copyright (C) 2002
>+ * the Initial Developer. All Rights Reserved.

I forgot to fix the copyright dates on the FlashblockWhitelistManager files when I landed them (either time :P), so when the rest of the patch re-lands, we need to remember to update those dates, too.

Comment 42

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

Attachment: boxset compile failure

The story here (after much tribulation; who knew Mac decoder apps only want to decode base64 if there's a filename header? :P) is that boxset is experiencing an ICE segfault trying to compile BWC here (see attached log snippet): http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/camino/src/browser/BrowserWindowController.mm&rev=HEAD&mark=4142#4142  (If anyone would like the full log, I can send it to them--in a sane format--but there are no other errors or warnings.)

Just to make sure it wasn't some spurious problem like bug 431910, I issued a CLOBBER, which didn't seem to help.

hendy had his trusty iBook build with boxset's .mozconfig (to rule out the 10.4/Xcode 2.5/PPC/shared/opt/release scenario) and had no problems, so that more-or-less says this is a local issue.

I've backed the patch out again so that we can reopen the tree once boxset starts reporting to the graph-sever again.  The next course of action is to have Sam remove and re-install Xcode 2.5, which he says he might be able to do tonight.  Beyond that, it's probably a wipe-and-reinstall, which will suck and take forever, and if that doesn't work, well, I don't want to think about that.

Comment 43

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

Flagging this so we remember to check on it after the boxset/perf scenario is resolved but before b3.

Comment 44

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

Relanded, with the copyright date fixes from comment 41, and none of the working boxen went red.

FIXED.

Comment 45

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

And, uh, boxset reappeared :P

/me knocks on wood