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

RESOLVED FIXED

Status

--
minor
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: stuart.morgan+bugzilla, Assigned: bugzilla-graveyard)

Tracking

({fixed1.8.1.1})

1.8 Branch
PowerPC
macOS
fixed1.8.1.1
Dependency tree / graph

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

13 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

13 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

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

Comment 4

13 years ago
Posted patch fix (obsolete) — Splinter Review
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

13 years ago
No longer depends on: 183401

Comment 5

13 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

13 years ago
Posted patch respects shift toggle now (obsolete) — Splinter Review
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

13 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

13 years ago
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

13 years ago
Posted patch whoops, missed one (obsolete) — Splinter Review
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

13 years ago
Depends on: 337958
(Assignee)

Updated

13 years ago
Blocks: 183401

Comment 12

13 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

13 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

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

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

Comment 15

13 years ago
Posted patch un-bitrotted (obsolete) — Splinter Review
This should do it.
Attachment #242372 - Flags: review?(stuart.morgan)
(Reporter)

Comment 16

13 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

13 years ago
Posted patch fixedSplinter Review
Attachment #242372 - Attachment is obsolete: true
Attachment #243863 - Flags: review?(stuart.morgan)
(Reporter)

Comment 18

13 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+
Checked in on trunk and 1.8branch.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 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.