633684 - Set browser.reuse_window to 1 aka tabs (and consider renaming the pref to browser.link.open_external, which uses 3 for tabs)

Status: RESOLVED FIXED

(Camino Graveyard :: Preferences, defect)

Description

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

In bug 520555, we defaulted Cmd-click to tabs but left the other two tab prefs alone.

It's been a real annoyance to me working on bug 629850 (since I keep forgetting to set that pref every new profile), but it also seems backwards that we don't default to opening external links in new tabs when we default command-clicks to new tabs.

(Note that in the great SWM rewrite of 2004 and subsequent Aviary landing, browser.reuse_window [values: 0-window, 1-tab, 2-reuse] got renamed browser.link.open_external [values: 1-reuse, 2-window, 3-tab] in the Suite and Firefox; it's a slightly better name and pairs fairly nicely with browser.link.open_newwindow, the actual SWM pref.  We might consider switching to the new name, too.)

Comment 1

[Stuart Morgan]

SGTM

Comment 2

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

Since no-one else has commented (or complained), let's do this.

We should switch the pref for b1 (even if we don't have renaming-migration).

Comment 3

[Chris Peterson [:cpeterson]]

btw, FF3.5 removed the browser.link.open_external pref in Bug 324164. FF uses the browser.link.open_newwindow pref to configure both target links (open_newwindow) and external links (open_external). Needless to say, this change was highly unpopular and Bug 509664 was opened in protest.

Comment 4

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

Yeah, we're not going to remove the separate UI, just switch to the post-Aviary-landing pref name to match SeaMonkey and old Firefox (and switch this one to tabs).

Comment 5

[Chris Peterson [:cpeterson]]

Attachment: open_external-v1.patch

open_external-v1.patch does the following:
1. Renames browser.reuse_window pref to browser.link.open_external.
2. Migrates old reuse_window pref values to new open_external values.
3. Defaults new profiles' open_external to "open new tab" (3 in all-camino.js.in).
4. Increments camino.prefs_version to 4.
5. Marks some global variables static (in MainController.mm).

TODO!
* Update Camino doc references from reuse_window to open_external:
http://caminobrowser.org/documentation/hiddenprefs/

QUESTION:
* Do we still want the browser.link.open_newwindow pref to default to "open new window" (2)? Firefox has defaulted to "open new tab" (3) since FF2:
http://kb.mozillazine.org/Browser.link.open_newwindow

Comment 6

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

Thanks for taking this, Chris. Some drive-by comments, as always ;)

(In reply to comment #5)
> 2. Migrates old reuse_window pref values to new open_external values.
> 3. Defaults new profiles' open_external to "open new tab" (3 in
> all-camino.js.in).

When we made tabs the default for Cmd-click in bug 520555, we made them the default for everyone (simply by switching the pref value, because there was no name migration then); we should make the migration code have the same effect as just switching the old pref would have given us.  (In other words, migrate people who have reuse set to the new reuse value, and set everyone else to tabs.)

> 4. Increments camino.prefs_version to 4.

This is probably going to be 5, since bug 626579 is most likely going to land first.

> TODO!
> * Update Camino doc references from reuse_window to open_external:
> http://caminobrowser.org/documentation/hiddenprefs/

That just needs to go here once this has landed: http://wiki.caminobrowser.org/Website:Documentation_Changes_for_2.1#Hidden_Prefs

Probably also need to make a note in the "Tabs" section to audit the Tabs documentation to make sure any references to windows as default are cleaned up.

> QUESTION:
> * Do we still want the browser.link.open_newwindow pref to default to "open new
> window" (2)? Firefox has defaulted to "open new tab" (3) since FF2:
> http://kb.mozillazine.org/Browser.link.open_newwindow

I'm on the fence about this one; I considered doing that when filing the bug, but considered the behavior more annoying.  That is, if you're reading a page somewhere in the middle of your tabs, normal-click a link (meaning you expect to not change tabs, but rather to reuse the current tab for the content of the new link), you'll end up at the far end of your tabs, away from where you were.  A window never changes your tab position; you just close it.  Maybe jumpback is good enough in this case, though; dunno.

> // Note that Firefox no longer supports browser.link.open_external as of FF 3.5.
> // http://kb.mozillazine.org/Browser.link.open_external

I'm not sure this comment adds any value here; the fact Firefox has merged all of their tab-vs-window behaviors into a single pref really doesn't affect our |migrateOldExternalLoadBehaviorPref| code ;)

Comment 7

[Chris Peterson [:cpeterson]]

Attachment: open_external-v2.patch

Thanks for the feedback, Smokey.

Patch v2 incorporates your suggestions:
* Migrate "reuse current window" pref. Everyone else ("new window", "new tab", or some unexpected value) will get the new default ("new tab").
* Also, don't set open_external pref unless it has a non-default value.
* Increment camino.prefs_version to 5.
* Remove comment about Firefox no longer supporting browser.link.open_external.

Comment 8

[Stuart Morgan]

Comment on attachment 514058 [details] [diff] [review]
open_external-v2.patch

>+#pragma mark Obsolete Tab Behavior
>+
>+const char* const kOldGeckoPrefExternalLoadBehavior = "browser.reuse_window";
>+

Move this down to next to the other obsolete prefs.

>+  // Camino 2.1 migrated browser.reuse_window pref to browser.link.open_external.
>+  if (mLastRunPrefsVersion && mLastRunPrefsVersion < 4)
>+    [self migrateOldExternalLoadBehaviorPref];

I think you meant this to be 5, but I'm actually planning to reconcile my rev patch with this one and land them right next to each other, so don't worry about it.

>+// Camino 2.1b1 default is "open new tab". Previous default was "open new window".

Just say 2.1, like you did above.

>+  BOOL gotPref;
>+  int oldExternalLoadPref = [self getIntPref:kOldGeckoPrefExternalLoadBehavior withSuccess:&gotPref];
>+
>+  // Migrate "reuse current window" pref. Everyone else ("new window", "new
>+  // tab", or some unexpected value) will get the new default ("new tab").
>+  if (gotPref) {
>+    if (oldExternalLoadPref == kOldExternalLoadReusesWindow)
>+      [self setPref:kGeckoPrefExternalLoadBehavior toInt:kExternalLoadReusesWindow];
>+
>+    [self clearPref:kOldGeckoPrefExternalLoadBehavior];
>+  }
>+}

All the gotPref logic can be removed here. If we don't get a pref, the return value will be 0, which isn't kOldExternalLoadReusesWindow.

Comment 9

[Chris Peterson [:cpeterson]]

Attachment: open_external-v3.patch

Patch v3 incorporates Stuart's feedback:
* Remove unnecessary gotPref logic
* Consolidate obsolete kOldGecko values
* kCurrentPrefsVersion = 4 (assuming version 4 will combine Java and open_external pref changes).

Comment 10

[Stuart Morgan]

Comment on attachment 514138 [details] [diff] [review]
open_external-v3.patch

>+    else // kExternalLoadOpensNewTab (or unexpected value)

One more space between code and comment.

>+#pragma mark Obsolete Tab Behavior
>+
>+// Controls the load behavior of URLs loaded from external applications.
>+extern const char* const kOldGeckoPrefExternalLoadBehavior;            // int
>+// Possible values:
>+extern const int kOldExternalLoadReusesWindow;

Should be down with the other obsolete prefs. Sorry, I meant to comment on both the header and the implementation last time.

>+// Obsolete kOldGeckoPrefExternalLoadBehavior values
>+const char* const kOldGeckoPrefExternalLoadBehavior = "browser.reuse_window";
>+const int kOldExternalLoadReusesWindow   = 2;

The obsolete pref should be by the other obsolete prefs. The obsolete value should be by the other obsolete values.


sr=smorgan; this is all trivial stuff and I'll need to merge the patch anyway to handle the version checking code, so no need to re-spin.

Comment 11

[Stuart Morgan]

Landed as http://hg.mozilla.org/camino/rev/13dd19af5f87 with the above fixed, reconciled with the other two v4 patches, and with a couple of other comment tweaks.

Comment 12

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

(In reply to comment #6)
> > TODO!
> > * Update Camino doc references from reuse_window to open_external:
> > http://caminobrowser.org/documentation/hiddenprefs/
> 
> That just needs to go here once this has landed:
> http://wiki.caminobrowser.org/Website:Documentation_Changes_for_2.1#Hidden_Prefs
> 
> Probably also need to make a note in the "Tabs" section to audit the Tabs
> documentation to make sure any references to windows as default are cleaned up.

Updated the wiki, too.