More inconsistent tab background/foreground handling: if you drag a single bookmark from the bookmark bar into the tab bar background, you get a background tab regardless of the state of the 'load in background' pref.

Right now we load in the foreground IFF dragged to the active tab, so maybe the patch for this should also bring to the foreground any background tab which is the target of a bookmark drag, per the pref. I hope that made sense ;) cl

Agreed.

This will probably get fixed by the fix for bug 183401. Marking dependent.

This does *not* fix the other bug (and the two fixes turned out not to be dependent at all).

Comment on attachment 221999 [details] [diff] [review] fix Looks good to me, i didn't test it.

Comment on attachment 221999 [details] [diff] [review] fix I tested the patch, and it now honors the pref, but it ignores the shift toggle. It should honor the toggle, too. r- based on that.

Smokey, feel free to try this out too. cl

Comment on attachment 222003 [details] [diff] [review] respects shift toggle now This now works as I'd expect, with the exception of bug 337958. r=me, FWIW Stuart's better at checking all the possible implications to other behaviors, of course :)

Comment on attachment 222003 [details] [diff] [review] respects shift toggle now This should use a shared implementation of the logic (see my comment in bug 337958). Also, shouldn't the logic be in handleDropOnTab:... to address your comment 1? Lastly, there's going to have to be an override or some restructuring somewhere; there's a looped caller to handleDropOnTab:..., and having each call pull a new tab to the front is going to be unpleasant. (It looks like that call point should probably be changed to use openURLArray:tabOpenPolicy:allowPopups: for a number of reasons anyway.)

This should do it. Works here, at least, with all the data I can think of to throw at it. cl

Also fixes bug 183401, which the previous patch didn't fix properly. Sorry for the bugspam.

Comment on attachment 223527 [details] [diff] [review] whoops, missed one - if (overTabViewItem) - { - [[overTabViewItem view] loadURI: url referrer:nil flags: NSLoadFlagsNone activate:NO allowPopups:NO]; + if (overTabViewItem) { + [[overTabViewItem view] loadURI:url referrer:nil flags:NSLoadFlagsNone activate:NO allowPopups:NO]; + First, lets keep the braces the way they were, not change them. + // only select drag target if loading in BG and shift is down, or loading in FG and shift isn't down + if ((loadInBG && shiftIsDown) || !(loadInBG || shiftIsDown)) In your comment you state "or loading in FG and shift isn't down", but your logic seems strange here. Should it be (!loadInBG && !shiftIsDown), or is your comment wrong? + // if we're over the content area, just load the first one + if (overContentArea) + return [self handleDropOnTab:overTabViewItem overContent:YES withURL:[urls objectAtIndex:1]]; + // otherwise load the first in the tab, and keep going + else + [[[self window] windowController] openURLArray:urls tabOpenPolicy:(overTabViewItem ? eReplaceTabs : eAppendTabs) allowPopups:NO]; If you are returning the first tab, shouldn't it be the item at index 0? Can you be a little clearer with the comments here? For instance, what is the first one, the tab or the url? Also, a little nit, to make things easier to read, put a blank space between your if and else statment.

(In reply to comment #12) > + // only select drag target if loading in BG and shift is down, or loading > in FG and shift isn't down > + if ((loadInBG && shiftIsDown) || !(loadInBG || shiftIsDown)) > > In your comment you state "or loading in FG and shift isn't down", but your > logic seems strange here. > Should it be (!loadInBG && !shiftIsDown), or is your comment wrong? !(loadInBG || shiftIsDown) is logically equivalent to (!loadInBG && !shiftIsDown) > Also, a little nit, to make things easier to read, put a blank space between > your if and else statment. Putting blank lines between if and else often makes it less apparent that there is an else, which can reduce readibility.

Comment on attachment 223527 [details] [diff] [review] whoops, missed one Bitrotted. New version coming soon-ish.

This should do it.

Comment on attachment 242372 [details] [diff] [review] un-bitrotted >+ // don't use |shouldLoadInBackground| here because we have to compare the two parameters separately >+ BOOL loadInBG = [[PreferenceManager sharedInstance] getBooleanPref:"browser.tabs.loadInBackground" withSuccess:NULL]; >+ BOOL shiftIsDown = ([[NSApp currentEvent] modifierFlags] & NSShiftKeyMask); >+ >+ // only select drag target if loading in BG and shift is down, or loading in FG and shift isn't down >+ if ((loadInBG && shiftIsDown) || (!loadInBG && !shiftIsDown)) This is all just |if (![BWC shouldLoadInBackground])|. >- else if (overContentArea) >- { >+ else if (overContentArea) { > [[[self selectedTabViewItem] view] loadURI:url referrer:nil flags:NSLoadFlagsNone focusContent:YES allowPopups:NO]; > return YES; > } > else > { Fix that last brace too so the method is consistent. >--(void)addTabForURL:(NSString*)aURL referrer:(NSString*)aReferrer >+-(void)addTabForURL:(NSString*)aURL referrer:(NSString*)aReferrer inBackground:(BOOL)inBG This patch doesn't change any call sites for this method, which is going to break stuff.

Comment on attachment 243863 [details] [diff] [review] fixed Change |inBG| to |inBackground|, and r=me.

Comment on attachment 243863 [details] [diff] [review] fixed sr=pink

Checked in on trunk and 1.8branch.