Bug 320668 will get dragging indiv. bookmarks to respect not only the "Load in background" pref but also the toggle. Folders/tab groups already respect the pref, but they do not honor the toggle.

Also fixes some whitespace goofs in the file. The meat of the patch is the last 15 lines or so. cl

Comment on attachment 222007 [details] [diff] [review] fix This also works as expected, but I didn't test extensively for possible side-effects. r=me, FWIW

Comment on attachment 222007 [details] [diff] [review] fix >- int numItems = (int)[urlArray count]; >+ int numItems = (int)[urlArray count]; Let's go crazy here, and remove both tabs from this line. > for (int i = 0; i < numItems; i++) > { ... >- else >- { >+ else { Why doesn't the |for| get any love? > BOOL loadInBackground = [[PreferenceManager sharedInstance] getBooleanPref:"browser.tabs.loadInBackground" withSuccess:NULL]; >- >+ >+ // respect the standard shift toggle >+ if ([[NSApp currentEvent] modifierFlags] & NSShiftKeyMask) >+ loadInBackground = !loadInBackground; >+ This block of code is starting to crop up in more and more places. I think it's time for a method to encapsulate it and cut down on the wear and tear on copy and paste shortcut keys. Something like (BOOL)shouldLoadInBackgroundIgnoringShift:(BOOL)ignoreShift (That's a tad long, but I'm having trouble shortening it without sacrificing clarity.)

How's this?

Comment on attachment 223522 [details] [diff] [review] shared implementation What about all the other places currently using getBooleanPref:"browser.tabs.loadInBackground"? LXR shows around 15, and this only changes 2. >- [[result itemWithTarget:self andAction:@selector(copyAddressToClipboard:)] setTitle:NSLocalizedString(@"Copy Addresses", @"")]; >+ [[result itemAtIndex:[result indexOfItemWithTarget:self andAction:@selector(copyAddressToClipboard:)]] setTitle:NSLocalizedString(@"Copy Addresses", @"")]; Why are you obfuscating this? >+// -shouldLoadInBackground:ignoringShift Why is the colon in the wrong place? That's not the name of the method. >+// uses shift key during event to toggle foreground/background tab loading ... >+ // respect the standard shift toggle This method is very short; no need to explain the shift key thing twice. >+// Look for the shift key and use it to toggle the load-in-background pref >+- (BOOL)shouldLoadInBackgroundIgnoringShift:(BOOL)ignoreShift; The primary purpose of this method is to get the pref, not to look for the shift key. The summary should be something more like "Get the load-in-background pref, optionally reversing it based on the shift key"

(In reply to comment #5) > (From update of attachment 223522 [details] [diff] [review] [edit]) > What about all the other places currently using > getBooleanPref:"browser.tabs.loadInBackground"? LXR shows around 15, and this > only changes 2. I hadn't thought of them? :-p > >- [[result itemWithTarget:self andAction:@selector(copyAddressToClipboard:)] setTitle:NSLocalizedString(@"Copy Addresses", @"")]; > >+ [[result itemAtIndex:[result indexOfItemWithTarget:self andAction:@selector(copyAddressToClipboard:)]] setTitle:NSLocalizedString(@"Copy Addresses", @"")]; > > Why are you obfuscating this? Required to fix a compiler warning, unless someone has a better idea. > >+// -shouldLoadInBackground:ignoringShift > > Why is the colon in the wrong place? That's not the name of the method. Because I'm an idiot? :) I'll fix the comments too. cl

Fixes comments above, ignores the protocol warning mentioned in the last comment, and touches all files formerly using the PreferenceManager call directly. Also fixes several miscellaneous compiler warnings, mostly due to missing casts, in some other files. cl

All the call site changes look good from a desired behavior standpoint. However, I'm not convinced that BWC is the right place for the new method, since it has nothing BWC-specific about it (and at the very least it seems like it should be a class method of where ever it ends up). I'm not sure where it fits best--maybe PreferenceManager? See what one of the sr folks think. (Also for the next round: GoMenu.mm, and possibly others since I didn't check them all, have consistent bracing style--do not change just one function in a file like that. If you want to change a file like that, file a cleanup bug with a whole-file-at-once patch.)

Comment on attachment 223559 [details] [diff] [review] fixed (Forgot the r-; sorry for bugspam)

CCing Mike, Simon, and Mark per comment 8. cl

i don't think the -shouldLoadInBackground... method should go on the pref mgr, it really is a browser-ish thing and the prefMgr doesn't have much specific to the browser (correct me if i'm wrong). I do agree it makes sense as a class method since it doesn't use any instance vars, or it could even be in a new namespace with other functions that do similar things. i hesitate to say it might be a a candidate for a category on PrefMgr, but if PrefMgr were apple's we'd probably go that route.

Lets just get a version of this patch with the function as a class method on BWC. We need to get the bugs that depend on this moving.

cl, any ETA on getting a patch in the style of comment 17 ready? This really is blocking new/toggle fixup work on a number of bugs.

Comment on attachment 223559 [details] [diff] [review] fixed Here are some very late comments on this patch. Just saw the bug ref'd from the meeting log. * Please name it |shouldLoadInBackground| and skip the "IgnoringShift" suffix. (We can add a new version if it's ever gonna be much needed.) Making it a class method as previously suggested also sounds good.

Something like this, then? cl

BTW, the above patch was -uw. I'll have one without the -w flag for checkin. cl

Comment on attachment 241283 [details] [diff] [review] uses renamed class method Sorry, what I meant in the previous comment was to use |shouldLoadInBackground| as name *and* not have the ignoreShift parameter at all, unless we plan to use it now. Isn't it simpler to leave handleCommandReturn like it is with its current param, and skip it for all shouldLoadInBackground callers (since all of them pass NO anyway).

Comment on attachment 241283 [details] [diff] [review] uses renamed class method Yeah, axe the ignoreShift parameter. I thought there was a call site that would need it back when I suggested it, but apparently not.

No more ignoreShift parameter, and no changes to handleCommandReturn: either.

Comment on attachment 242145 [details] [diff] [review] no more ignoreShift parameter >Index: src/browser/ContentClickListener.mm >=================================================================== >RCS file: /cvsroot/mozilla/camino/src/browser/ContentClickListener.mm,v >retrieving revision 1.28 >diff -u -w -r1.28 ContentClickListener.mm >--- src/browser/ContentClickListener.mm 23 Jun 2006 03:10:23 -0000 1.28 >+++ src/browser/ContentClickListener.mm 5 Oct 2006 03:58:35 -0000 >@@ -100,7 +100,7 @@ > if ((metaKey && button == 0) || button == 1) { > // The command key is down or we got a middle click. Open the link in a new window or tab. > BOOL useTab = [[PreferenceManager sharedInstance] getBooleanPref:"browser.tabs.opentabfor.middleclick" withSuccess:NULL]; >- BOOL loadInBackground = [[PreferenceManager sharedInstance] getBooleanPref:"browser.tabs.loadInBackground" withSuccess:NULL]; >+ BOOL loadInBackground = [BrowserWindowController shouldLoadInBackground]; Making this change without removing the following section that toggles it with the shift key breaks the use of shift in the content area. There's also a lot of style problems in BWC beyond what you fix, so I'll be doing a large BWC cleanup very soon either way; I'd prefer all the cleanup be done in one cleanup patch rather than tacking parts of it onto this change.

OK, I think this should do it.

Comment on attachment 242371 [details] [diff] [review] whew Looks very clean to me!

Comment on attachment 242371 [details] [diff] [review] whew It looks so good I have to steal it from smorgan.

Mike, any idea when you can get to this--or can smorgan take a stab at sr? This has been sitting for a week or so now, and it's blocking a number of other bugs....

Comment on attachment 242371 [details] [diff] [review] whew sr=pink when we clean up BWC, sLIB should probably live elsewhere, but i guess it's ok where it is now.

Checked in on 1.8branch and trunk.