Bug 1661643 - Remove browser.link.open_newwindow=1. r=preferences-reviewers,Gijs
authorTooru Fujisawa <arai_a@mac.com>
Fri, 28 Aug 2020 13:44:46 +0000 (2020-08-28)
changeset 546798 12d62b074178284b750333eaa36cc23d5ea16fda
parent 546797 16e18fb9b76dd27557d3e1205887ede63f607b7c
child 546799 0ec8df5bd92b7847098a2d5e1c784e7baf381a24
push id125219
push userarai_a@mac.com
push dateFri, 28 Aug 2020 13:53:51 +0000 (2020-08-28)
treeherderautoland@12d62b074178 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspreferences-reviewers, Gijs
bugs1661643
milestone82.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1661643 - Remove browser.link.open_newwindow=1. r=preferences-reviewers,Gijs Differential Revision: https://phabricator.services.mozilla.com/D88572
browser/app/profile/firefox.js
browser/base/content/browser.js
browser/components/preferences/main.js
dom/serviceworkers/test/test_notification_openWindow.html
dom/tests/browser/browser_test_new_window_from_content.js
toolkit/components/windowwatcher/nsWindowWatcher.cpp
toolkit/components/windowwatcher/test/browser_popup_condition.js
--- a/browser/app/profile/firefox.js
+++ b/browser/app/profile/firefox.js
@@ -458,22 +458,22 @@ pref("permissions.default.desktop-notifi
 pref("permissions.default.shortcuts", 0);
 
 pref("permissions.desktop-notification.postPrompt.enabled", true);
 pref("permissions.desktop-notification.notNow.enabled", false);
 
 pref("permissions.fullscreen.allowed", false);
 
 // handle links targeting new windows
-// 1=current window/tab, 2=new window, 3=new tab in most recent window
+// 2=new window, 3=new tab in most recent window
 pref("browser.link.open_newwindow", 3);
 
 // handle external links (i.e. links opened from a different application)
 // default: use browser.link.open_newwindow
-// 1-3: see browser.link.open_newwindow for interpretation
+// 2-3: see browser.link.open_newwindow for interpretation
 pref("browser.link.open_newwindow.override.external", -1);
 
 // 0: no restrictions - divert everything
 // 1: don't divert window.open at all
 // 2: don't divert window.open with features
 pref("browser.link.open_newwindow.restriction", 2);
 
 // If true, this pref causes windows opened by window.open to be forced into new
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -6023,28 +6023,33 @@ nsBrowserAccess.prototype = {
     }
 
     if (isExternal && aURI && aURI.schemeIs("chrome")) {
       dump("use --chrome command-line option to load external chrome urls\n");
       return null;
     }
 
     if (aWhere == Ci.nsIBrowserDOMWindow.OPEN_DEFAULTWINDOW) {
+      let pref;
       if (
         isExternal &&
         Services.prefs.prefHasUserValue(
           "browser.link.open_newwindow.override.external"
         )
       ) {
-        aWhere = Services.prefs.getIntPref(
+        pref = Services.prefs.getIntPref(
           "browser.link.open_newwindow.override.external"
         );
       } else {
-        aWhere = Services.prefs.getIntPref("browser.link.open_newwindow");
-      }
+        pref = Services.prefs.getIntPref("browser.link.open_newwindow");
+      }
+      aWhere =
+        pref == 3
+          ? Ci.nsIBrowserDOMWindow.OPEN_NEWTAB
+          : Ci.nsIBrowserDOMWindow.OPEN_NEWWINDOW;
     }
 
     let referrerInfo;
     if (aFlags & Ci.nsIBrowserDOMWindow.OPEN_NO_REFERRER) {
       referrerInfo = new ReferrerInfo(Ci.nsIReferrerInfo.EMPTY, false, null);
     } else if (
       aOpenWindowInfo &&
       aOpenWindowInfo.parent &&
--- a/browser/components/preferences/main.js
+++ b/browser/components/preferences/main.js
@@ -114,17 +114,16 @@ Preferences.addAll([
   { id: "browser.download.useDownloadDir", type: "bool" },
   { id: "browser.download.folderList", type: "int" },
   { id: "browser.download.dir", type: "file" },
 
   /* Tab preferences
   Preferences:
 
   browser.link.open_newwindow
-      1 opens such links in the most recent window or tab,
       2 opens such links in a new window,
       3 opens such links in a new tab
   browser.tabs.loadInBackground
   - true if display should switch to a new tab which has been opened from a
     link, false if display shouldn't switch
   browser.tabs.warnOnClose
   - true if when closing a window with multiple tabs the user is warned and
     allowed to cancel the action, false to just close the window
@@ -1215,17 +1214,16 @@ var gMainPane = {
   // TABS
 
   /*
    * Preferences:
    *
    * browser.link.open_newwindow - int
    *   Determines where links targeting new windows should open.
    *   Values:
-   *     1 - Open in the current window or tab.
    *     2 - Open in a new window.
    *     3 - Open in a new tab in the most recent window.
    * browser.tabs.loadInBackground - bool
    *   True - Whether browser should switch to a new tab opened from a link.
    * browser.tabs.warnOnClose - bool
    *   True - If when closing a window with multiple tabs the user is warned and
    *          allowed to cancel the action, false to just close the window.
    * browser.tabs.warnOnOpen - bool
--- a/dom/serviceworkers/test/test_notification_openWindow.html
+++ b/dom/serviceworkers/test/test_notification_openWindow.html
@@ -39,27 +39,19 @@ add_task(async function test() {
   await waitForState(swr.installing, "activated");
 
   SimpleTest.registerCleanupFunction(async () => {
     await swr.unregister();
     navigator.serviceWorker.onmessage = null;
   });
 
   for (let prefValue of [
-    SpecialPowers.Ci.nsIBrowserDOMWindow.OPEN_CURRENTWINDOW,
     SpecialPowers.Ci.nsIBrowserDOMWindow.OPEN_NEWWINDOW,
     SpecialPowers.Ci.nsIBrowserDOMWindow.OPEN_NEWTAB,
   ]) {
-    if (prefValue == SpecialPowers.Ci.nsIBrowserDOMWindow.OPEN_CURRENTWINDOW) {
-      // Let's open a new tab and focus on it. When the service
-      // worker notification is shown, the document will open in the focused tab.
-      // If we don't open a new tab, the document will be opened in the
-      // current test-runner tab and mess up the test setup.
-      window.open("");
-    }
     info(`Setting browser.link.open_newwindow to ${prefValue}.`);
     await SpecialPowers.pushPrefEnv({
       set: [["browser.link.open_newwindow", prefValue]],
     });
 
     // The onclicknotification handler uses Clients.openWindow() to open a new
     // window. This newly created window will attempt to open another window with
     // Window.open() and some arbitrary URL. We crash before the second window
--- a/dom/tests/browser/browser_test_new_window_from_content.js
+++ b/dom/tests/browser/browser_test_new_window_from_content.js
@@ -34,50 +34,46 @@
      This file attempts to test each window opening technique against all possible settings for
      each preference.
 */
 
 const kContentDoc =
   "http://www.example.com/browser/dom/tests/browser/test_new_window_from_content_child.html";
 const kNewWindowPrefKey = "browser.link.open_newwindow";
 const kNewWindowRestrictionPrefKey = "browser.link.open_newwindow.restriction";
-const kSameTab = "same tab";
 const kNewWin = "new window";
 const kNewTab = "new tab";
 
 SpecialPowers.pushPrefEnv({
   set: [["dom.require_user_interaction_for_beforeunload", false]],
 });
 
 requestLongerTimeout(3);
 
 // The following "matrices" represent the result of content attempting to
 // open a window with window.open with the default feature set. The key of
 // the kWinOpenDefault object represents the value of browser.link.open_newwindow.
 // The value for each key is an array that represents the result (either opening
-// the link in the same tab, a new window, or a new tab), where the index of each
-// result maps to the browser.link.open_newwindow.restriction pref. I've tried
-// to illustrate this more clearly in the kWinOpenDefault object.
+// the link in a new window or a new tab), where the index of each result maps
+// to the browser.link.open_newwindow.restriction pref. I've tried to
+// illustrate this more clearly in the kWinOpenDefault object.
 const kWinOpenDefault = {
   //                    open_newwindow.restriction
   //                    0         1        2
   // open_newwindow
-  1: [kSameTab, kNewWin, kSameTab],
   2: [kNewWin, kNewWin, kNewWin],
   3: [kNewTab, kNewWin, kNewTab],
 };
 
 const kWinOpenNonDefault = {
-  1: [kSameTab, kNewWin, kNewWin],
   2: [kNewWin, kNewWin, kNewWin],
   3: [kNewTab, kNewWin, kNewWin],
 };
 
 const kTargetBlank = {
-  1: [kSameTab, kSameTab, kSameTab],
   2: [kNewWin, kNewWin, kNewWin],
   3: [kNewTab, kNewTab, kNewTab],
 };
 
 // We'll be changing these preferences a lot, so we'll stash their original
 // values and make sure we restore them at the end of the test.
 var originalNewWindowPref = Services.prefs.getIntPref(kNewWindowPrefKey);
 var originalNewWindowRestrictionPref = Services.prefs.getIntPref(
@@ -90,40 +86,32 @@ registerCleanupFunction(function() {
     kNewWindowRestrictionPrefKey,
     originalNewWindowRestrictionPref
   );
 });
 
 /**
  * For some expectation when a link is clicked, creates and
  * returns a Promise that resolves when that expectation is
- * fulfilled. For example, aExpectation might be kSameTab, which
+ * fulfilled. For example, aExpectation might be kNewTab, which
  * will cause this function to return a Promise that resolves when
- * the current tab attempts to browse to about:blank.
+ * the new tab is created.
  *
  * This function also takes care of cleaning up once the result has
  * occurred - for example, if a new window was opened, this function
  * closes it before resolving.
  *
  * @param aBrowser the <xul:browser> with the test document
- * @param aExpectation one of kSameTab, kNewWin, or kNewTab.
+ * @param aExpectation either kNewWin or kNewTab.
  * @return a Promise that resolves when the expectation is fulfilled,
  *         and cleaned up after.
  */
 function prepareForResult(aBrowser, aExpectation) {
   let expectedSpec = kContentDoc.replace(/[^\/]*$/, "dummy.html");
   switch (aExpectation) {
-    case kSameTab:
-      return (async function() {
-        await BrowserTestUtils.browserLoaded(aBrowser);
-        is(aBrowser.currentURI.spec, expectedSpec, "Should be at dummy.html");
-        // Now put the browser back where it came from
-        await BrowserTestUtils.loadURI(aBrowser, kContentDoc);
-        await BrowserTestUtils.browserLoaded(aBrowser);
-      })();
     case kNewWin:
       return (async function() {
         let newWin = await BrowserTestUtils.waitForNewWindow({
           url: expectedSpec,
         });
         let newBrowser = newWin.gBrowser.selectedBrowser;
         is(newBrowser.currentURI.spec, expectedSpec, "Should be at dummy.html");
         await BrowserTestUtils.closeWindow(newWin);
@@ -165,17 +153,17 @@ function testLinkWithMatrix(aLinkSelecto
     },
     async function(browser) {
       // This nested for-loop is unravelling the matrix const
       // we set up, and gives us three things through each tick
       // of the inner loop:
       // 1) newWindowPref: a browser.link.open_newwindow pref to try
       // 2) newWindowRestPref: a browser.link.open_newwindow.restriction pref to try
       // 3) expectation: what we expect the click outcome on this link to be,
-      //                 which will either be kSameTab, kNewWin or kNewTab.
+      //                 which will either be kNewWin or kNewTab.
       for (let newWindowPref in aMatrix) {
         let expectations = aMatrix[newWindowPref];
         for (let i = 0; i < expectations.length; ++i) {
           let newWindowRestPref = i;
           let expectation = expectations[i];
 
           Services.prefs.setIntPref(
             "browser.link.open_newwindow",
--- a/toolkit/components/windowwatcher/nsWindowWatcher.cpp
+++ b/toolkit/components/windowwatcher/nsWindowWatcher.cpp
@@ -2450,28 +2450,31 @@ int32_t nsWindowWatcher::GetWindowOpenLo
   // Where should we open this?
   int32_t containerPref;
   if (NS_FAILED(
           Preferences::GetInt("browser.link.open_newwindow", &containerPref))) {
     // We couldn't read the user preference, so fall back on the default.
     return nsIBrowserDOMWindow::OPEN_NEWTAB;
   }
 
+  if (containerPref != nsIBrowserDOMWindow::OPEN_NEWTAB) {
+    containerPref = nsIBrowserDOMWindow::OPEN_NEWWINDOW;
+  }
+
   bool isDisabledOpenNewWindow =
       aParent->GetFullScreen() &&
       Preferences::GetBool(
           "browser.link.open_newwindow.disabled_in_fullscreen");
 
   if (isDisabledOpenNewWindow &&
       (containerPref == nsIBrowserDOMWindow::OPEN_NEWWINDOW)) {
     containerPref = nsIBrowserDOMWindow::OPEN_NEWTAB;
   }
 
-  if (containerPref != nsIBrowserDOMWindow::OPEN_NEWTAB &&
-      containerPref != nsIBrowserDOMWindow::OPEN_CURRENTWINDOW) {
+  if (containerPref == nsIBrowserDOMWindow::OPEN_NEWWINDOW) {
     // Just open a window normally
     return nsIBrowserDOMWindow::OPEN_NEWWINDOW;
   }
 
   if (aCalledFromJS) {
     /* Now check our restriction pref.  The restriction pref is a power-user's
        fine-tuning pref. values:
        0: no restrictions - divert everything
--- a/toolkit/components/windowwatcher/test/browser_popup_condition.js
+++ b/toolkit/components/windowwatcher/test/browser_popup_condition.js
@@ -286,22 +286,15 @@ add_task(async function test_popup_condi
 
   // Non-popup is opened in a new tab (default behavior).
   await SpecialPowers.pushPrefEnv({
     set: [["browser.link.open_newwindow", 3]],
   });
   await test_patterns({ nonPopup: "tab" });
   await SpecialPowers.popPrefEnv();
 
-  // Non-popup is opened in a current tab.
-  await SpecialPowers.pushPrefEnv({
-    set: [["browser.link.open_newwindow", 1]],
-  });
-  await test_patterns({ nonPopup: "current" });
-  await SpecialPowers.popPrefEnv();
-
   // Non-popup is opened in a new window.
   await SpecialPowers.pushPrefEnv({
     set: [["browser.link.open_newwindow", 2]],
   });
   await test_patterns({ nonPopup: "window" });
   await SpecialPowers.popPrefEnv();
 });