320660 - When opening multiple tabs, we always focus the first new one, ignoring the pref

Status: RESOLVED FIXED

(Camino Graveyard :: Tabbed Browsing, defect)

Description

[Håkan Waara]

from bug 228840: "Currently, when command-clicking a tab group and prefs set to open in new tabs, the new tabs are *always* focused, no matter what the background pref."

Investigating, it's a regression from bug 294019.

When opening multiple URLs in openURLs:, we always select the first of those URLs, even if the user cmd-clicked or middle-clicked (to append new tabs)

Comment 1

[Håkan Waara]

Attachment: Proposed fix

Ok, so if I've understood the earlier bug correctly we need to focus the first new tab if we replace all tabs.

However, if we just append them, we ought to follow the pref, so in that case the code shouldn't apply.

This patch just makes us check whether we're replacing all tabs, or appending them - and then focusing only in the first case.

Comment 2

[Håkan Waara]

*** Bug 318626 has been marked as a duplicate of this bug. ***

Comment 3

[Håkan Waara]

I don't know if the reviewers I chose for this patch are OK, so if anyone feels like it feel free to have a look.

Comment 4

[Stuart Morgan]

Comment on attachment 206185 [details] [diff] [review]
Proposed fix

> This patch just makes us check whether we're replacing all tabs, or appending
> them - and then focusing only in the first case.

That's not the correct behavior.  There is a pref for whether or not tabs are loaded in the background or the foreground, which needs to be respected.

It's probably also a bit more complicated than that; you'll have to look at who calls into this function, and what the expected behavior would be, since some might reasonably override the pref (the open panel, for example).

Comment 5

[Stuart Morgan]

Ew. This is a tangent, but eAppendFromCurrentTab is horribly named; while you are touching this function it would be great if you could rename it to eReplaceFromCurrentTab, since that's what it actually does.

Comment 6

[Håkan Waara]

Maybe I was unclear, but this patch fixes the bug that prevents that very pref from working.

Comment 7

[Håkan Waara]

Attachment: Patch 2

Actually, you were right. My previous patch fixed a regression (that we were _always_ focusing new tabs, no matter if they were only appended). This new patch also makes us respect the focus-pref.

Comment 8

[Simon Fraser [no longer active]]

Isn't that patch missing a header change?

Comment 9

[Stuart Morgan]

Comment on attachment 206433 [details] [diff] [review]
Patch 2

Yes, I was just in the midst of reviewing/testing, and it's missing the enum declaration change in BrowserWindowController.h.
Also:
+  if ((tabPolicy == eReplaceTabs) || !loadInBackground)
should be
  if (!((tabPolicy == eAppendTabs) && loadInBackground))
so it only affects eAppendTabs.  I'm about to test with those changes though

Comment 10

[Stuart Morgan]

Looks good other than that, so r=me with the two changes above.

Comment 11

[Håkan Waara]

Attachment: Patch v3

Been testing this patch in lots of cases and it works well. Made the changes suggested by Stuart Morgan.

Comment 12

[Nick Kreeger]

Comment on attachment 206873 [details] [diff] [review]
Patch v3

Lets have Stuart actually set the flags please !

Comment 13

[Håkan Waara]

(In reply to comment #12)
> (From update of attachment 206873 [details] [diff] [review] [edit])
> Lets have Stuart actually set the flags please ! 
> 

I made the changes he proposed, and he wrote "r=me with the two changes above."?

Comment 14

[Stuart Morgan]

Comment on attachment 206873 [details] [diff] [review]
Patch v3

Now you've left this change out:
-    [browserController openURLArray:urlStringsArray tabOpenPolicy:eAppendFromCurrentTab allowPopups:YES];
+    [browserController openURLArray:urlStringsArray tabOpenPolicy:eReplaceFromCurrentTab allowPopups:YES];
so once again the patch won't compile.  You need to make sure you are generating patches from BrowserWindowController.mm,  BrowserWindowController.h, and MainController.mm.

Also, don't add the whitespace at the end of the BWC changes.

(This is why it's best not to do an r+ in someone else's name, by the way--patchs don't always get spun the way one intends.)

Comment 15

[Håkan Waara]

Attachment: Patch again

Sorry, I have so many changes in my tree it's hard to keep track of them when diffing.

Comment 16

[Håkan Waara]

Comment on attachment 206909 [details] [diff] [review]
Patch again

I'll also remove that stale whitespace.

Comment 17

[Stuart Morgan]

Comment on attachment 206909 [details] [diff] [review]
Patch again

r=me

Comment 18

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

Since this is a regression and has an r+ patch, putting on the 1.0 list.

Comment 19

[Håkan Waara]

Smfr, wanna have a look at this patch? It's got r+, and fixes a regression. Maybe we could even get it into 1.0 ?

Comment 20

[Mark Mentovai]

This has a reasonable-looking patch and is a regression.  1.0?

Comment 21

[Mark Mentovai]

Checked in everywhere, sans the bogus and obsolete NSLocalizedString diff.  Thanks, Håkan!

Comment 22

[Håkan Waara]

Thanks for checking in for me - I'm currently busy with non-Mozilla projects  :)