320668 - Dragging a bookmark into the tab bar creates a background tab, regardless of pref

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[Stuart Morgan]

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.

Comment 1

[Chris Lawson (gone)]

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

Comment 2

[Stuart Morgan]

Agreed.

Comment 3

[froodian (Ian Leue)]

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

Comment 4

[Chris Lawson (gone)]

Attachment: fix

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

Comment 5

[Nick Kreeger]

Comment on attachment 221999 [details] [diff] [review]
fix

Looks good to me, i didn't test it.

Comment 6

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

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.

Comment 7

[Chris Lawson (gone)]

Attachment: respects shift toggle now

Smokey, feel free to try this out too.

cl

Comment 8

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

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 9

[Stuart Morgan]

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.)

Comment 10

[Chris Lawson (gone)]

Attachment: uses shared implementation, fixes looped caller

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

cl

Comment 11

[Chris Lawson (gone)]

Attachment: whoops, missed one

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

Comment 12

[Nick Kreeger]

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.

Comment 13

[Stuart Morgan]

(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 14

[Chris Lawson (gone)]

Comment on attachment 223527 [details] [diff] [review]
whoops, missed one

Bitrotted. New version coming soon-ish.

Comment 15

[Chris Lawson (gone)]

Attachment: un-bitrotted

This should do it.

Comment 16

[Stuart Morgan]

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 17

[Chris Lawson (gone)]

Attachment: fixed

Comment 18

[Stuart Morgan]

Comment on attachment 243863 [details] [diff] [review]
fixed

Change |inBG| to |inBackground|, and r=me.

Comment 19

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 243863 [details] [diff] [review]
fixed

sr=pink

Comment 20

[froodian (Ian Leue)]

Checked in on trunk and 1.8branch.