Last Comment Bug 320668 - Dragging a bookmark into the tab bar creates a background tab, regardless of pref
: Dragging a bookmark into the tab bar creates a background tab, regardless of ...
Status: RESOLVED FIXED
: fixed1.8.1.1
Product: Camino Graveyard
Classification: Graveyard
Component: Bookmarks (show other bugs)
: 1.8 Branch
: PowerPC Mac OS X
-- minor (vote)
: ---
Assigned To: Chris Lawson (gone)
:
:
Mentors:
Depends on: 337958
Blocks: 183401
  Show dependency treegraph
 
Reported: 2005-12-17 10:40 PST by Stuart Morgan
Modified: 2006-11-06 09:11 PST (History)
5 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
fix (1.61 KB, patch)
2006-05-14 20:11 PDT, Chris Lawson (gone)
nick.kreeger: review+
alqahira: review-
Details | Diff | Splinter Review
respects shift toggle now (1.73 KB, patch)
2006-05-14 22:02 PDT, Chris Lawson (gone)
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
uses shared implementation, fixes looped caller (5.47 KB, patch)
2006-05-26 20:47 PDT, Chris Lawson (gone)
no flags Details | Diff | Splinter Review
whoops, missed one (5.67 KB, patch)
2006-05-26 21:45 PDT, Chris Lawson (gone)
nick.kreeger: review-
Details | Diff | Splinter Review
un-bitrotted (4.56 KB, patch)
2006-10-15 20:29 PDT, Chris Lawson (gone)
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
fixed (4.36 KB, patch)
2006-10-27 17:47 PDT, Chris Lawson (gone)
stuart.morgan+bugzilla: review+
mikepinkerton: superreview+
Details | Diff | Splinter Review

Description User image Stuart Morgan 2005-12-17 10:40:40 PST
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 User image Chris Lawson (gone) 2005-12-17 20:31:49 PST
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 User image Stuart Morgan 2005-12-20 21:29:40 PST
Agreed.
Comment 3 User image froodian (Ian Leue) 2006-05-14 19:44:51 PDT
This will probably get fixed by the fix for bug 183401.  Marking dependent.
Comment 4 User image Chris Lawson (gone) 2006-05-14 20:11:00 PDT
Created attachment 221999 [details] [diff] [review]
fix

This does *not* fix the other bug (and the two fixes turned out not to be dependent at all).
Comment 5 User image Nick Kreeger 2006-05-14 20:59:47 PDT
Comment on attachment 221999 [details] [diff] [review]
fix

Looks good to me, i didn't test it.
Comment 6 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-05-14 21:43:20 PDT
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 User image Chris Lawson (gone) 2006-05-14 22:02:21 PDT
Created attachment 222003 [details] [diff] [review]
respects shift toggle now

Smokey, feel free to try this out too.

cl
Comment 8 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-05-14 22:39:47 PDT
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 User image Stuart Morgan 2006-05-16 22:19:56 PDT
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 User image Chris Lawson (gone) 2006-05-26 20:47:48 PDT
Created attachment 223524 [details] [diff] [review]
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 User image Chris Lawson (gone) 2006-05-26 21:45:18 PDT
Created attachment 223527 [details] [diff] [review]
whoops, missed one

Also fixes bug 183401, which the previous patch didn't fix properly. Sorry for the bugspam.
Comment 12 User image Nick Kreeger 2006-06-13 21:08:35 PDT
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 User image Stuart Morgan 2006-06-24 09:26:20 PDT
(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 User image Chris Lawson (gone) 2006-10-13 22:26:16 PDT
Comment on attachment 223527 [details] [diff] [review]
whoops, missed one

Bitrotted. New version coming soon-ish.
Comment 15 User image Chris Lawson (gone) 2006-10-15 20:29:41 PDT
Created attachment 242372 [details] [diff] [review]
un-bitrotted

This should do it.
Comment 16 User image Stuart Morgan 2006-10-20 23:33:56 PDT
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 User image Chris Lawson (gone) 2006-10-27 17:47:56 PDT
Created attachment 243863 [details] [diff] [review]
fixed
Comment 18 User image Stuart Morgan 2006-10-30 07:09:28 PST
Comment on attachment 243863 [details] [diff] [review]
fixed

Change |inBG| to |inBackground|, and r=me.
Comment 19 User image Mike Pinkerton (not reading bugmail) 2006-11-06 07:03:59 PST
Comment on attachment 243863 [details] [diff] [review]
fixed

sr=pink
Comment 20 User image froodian (Ian Leue) 2006-11-06 09:11:04 PST
Checked in on trunk and 1.8branch.

Note You need to log in before you can comment on or make changes to this bug.