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

RESOLVED FIXED

Status

Camino Graveyard
Bookmarks
--
minor
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Stuart Morgan, Assigned: Chris Lawson (gone))

Tracking

({fixed1.8.1.1})

1.8 Branch
PowerPC
Mac OS X
fixed1.8.1.1
Dependency tree / graph

Details

Attachments

(1 attachment, 5 obsolete attachments)

4.36 KB, patch
Stuart Morgan
: review+
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

12 years ago
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.
(Assignee)

Comment 1

12 years ago
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
(Reporter)

Comment 2

12 years ago
Agreed.

Comment 3

11 years ago
This will probably get fixed by the fix for bug 183401.  Marking dependent.
Depends on: 183401
(Assignee)

Comment 4

11 years ago
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).
Assignee: mikepinkerton → bugzilla
Status: NEW → ASSIGNED
Attachment #221999 - Flags: review?(stuart.morgan)
(Assignee)

Updated

11 years ago
No longer depends on: 183401

Comment 5

11 years ago
Comment on attachment 221999 [details] [diff] [review]
fix

Looks good to me, i didn't test it.
Attachment #221999 - Flags: review?(stuart.morgan) → review+
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.
Attachment #221999 - Flags: review-
(Assignee)

Comment 7

11 years ago
Created attachment 222003 [details] [diff] [review]
respects shift toggle now

Smokey, feel free to try this out too.

cl
Attachment #221999 - Attachment is obsolete: true
Attachment #222003 - Flags: review?(stuart.morgan)
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 :)
(Reporter)

Comment 9

11 years ago
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.)
Attachment #222003 - Flags: review?(stuart.morgan) → review-
(Assignee)

Comment 10

11 years ago
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
Attachment #222003 - Attachment is obsolete: true
Attachment #223524 - Flags: review?(stuart.morgan)
(Assignee)

Comment 11

11 years ago
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.
Attachment #223524 - Attachment is obsolete: true
Attachment #223527 - Flags: review?(stuart.morgan)
Attachment #223524 - Flags: review?(stuart.morgan)
(Assignee)

Updated

11 years ago
Depends on: 337958
(Assignee)

Updated

11 years ago
Blocks: 183401

Comment 12

11 years ago
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.
Attachment #223527 - Flags: review?(stuart.morgan) → review-
(Reporter)

Comment 13

11 years ago
(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.
(Assignee)

Comment 14

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

Bitrotted. New version coming soon-ish.
Attachment #223527 - Attachment is obsolete: true
QA Contact: bookmarks
(Assignee)

Comment 15

11 years ago
Created attachment 242372 [details] [diff] [review]
un-bitrotted

This should do it.
Attachment #242372 - Flags: review?(stuart.morgan)
(Reporter)

Comment 16

11 years ago
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.
Attachment #242372 - Flags: review?(stuart.morgan) → review-
(Assignee)

Comment 17

11 years ago
Created attachment 243863 [details] [diff] [review]
fixed
Attachment #242372 - Attachment is obsolete: true
Attachment #243863 - Flags: review?(stuart.morgan)
(Reporter)

Comment 18

11 years ago
Comment on attachment 243863 [details] [diff] [review]
fixed

Change |inBG| to |inBackground|, and r=me.
Attachment #243863 - Flags: superreview?(mikepinkerton)
Attachment #243863 - Flags: review?(stuart.morgan)
Attachment #243863 - Flags: review+
Comment on attachment 243863 [details] [diff] [review]
fixed

sr=pink
Attachment #243863 - Flags: superreview?(mikepinkerton) → superreview+

Comment 20

11 years ago
Checked in on trunk and 1.8branch.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1.1
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.