Bug 1376088 - make tab active immediately in tabs.duplicate r=robwu,Gijs
authorMélanie Chauvel (ariasuni) <perso@hack-libre.org>
Thu, 25 Jun 2020 11:45:12 +0000
changeset 537391 8358b0f93e052320b436aad60a52e46fba5cd8e1
parent 537390 3e4854c6b5885ff319e841c3187aed5f6bc8f166
child 537392 18a94269c57c8e26320012010464e37e80323f19
push id119960
push usercbrindusan@mozilla.com
push dateThu, 25 Jun 2020 12:04:17 +0000
treeherderautoland@8358b0f93e05 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrobwu, Gijs
bugs1376088
milestone79.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 1376088 - make tab active immediately in tabs.duplicate r=robwu,Gijs Differential Revision: https://phabricator.services.mozilla.com/D73732
browser/components/extensions/parent/ext-tabs.js
browser/components/extensions/test/browser/browser_ext_tabs_duplicate.js
browser/components/sessionstore/SessionStore.jsm
--- a/browser/components/extensions/parent/ext-tabs.js
+++ b/browser/components/extensions/parent/ext-tabs.js
@@ -1073,35 +1073,34 @@ this.tabs = class extends ExtensionAPI {
             tabsMoved.push(nativeTab);
           }
 
           return tabsMoved.map(nativeTab => tabManager.convert(nativeTab));
         },
 
         duplicate(tabId, duplicateProperties) {
           const { active, index } = duplicateProperties || {};
+          const inBackground = active === undefined ? false : !active;
+
           // Schema requires tab id.
           let nativeTab = getTabOrActive(tabId);
 
           let gBrowser = nativeTab.ownerGlobal.gBrowser;
-          let newTab = gBrowser.duplicateTab(nativeTab, true, { index });
+          let newTab = gBrowser.duplicateTab(nativeTab, true, {
+            inBackground,
+            index,
+          });
 
           tabListener.blockTabUntilRestored(newTab);
-
           return new Promise(resolve => {
-            // We need to use SSTabRestoring because any attributes set before
-            // are ignored.
+            // Use SSTabRestoring to ensure that the tab's URL is ready before
+            // resolving the promise.
             newTab.addEventListener(
               "SSTabRestoring",
-              function() {
-                if (active !== false) {
-                  gBrowser.selectedTab = newTab;
-                }
-                resolve(tabManager.convert(newTab));
-              },
+              () => resolve(tabManager.convert(newTab)),
               { once: true }
             );
           });
         },
 
         getZoom(tabId) {
           let nativeTab = getTabOrActive(tabId);
 
--- a/browser/components/extensions/test/browser/browser_ext_tabs_duplicate.js
+++ b/browser/components/extensions/test/browser/browser_ext_tabs_duplicate.js
@@ -5,39 +5,41 @@
 add_task(async function testDuplicateTab() {
   await BrowserTestUtils.openNewForegroundTab(gBrowser, "http://example.net/");
 
   let extension = ExtensionTestUtils.loadExtension({
     manifest: {
       permissions: ["tabs"],
     },
 
-    background: function() {
-      browser.tabs.query(
-        {
-          lastFocusedWindow: true,
-        },
-        function(tabs) {
-          let source = tabs[1];
-          // By moving it 0, we check that the new tab is created next
-          // to the existing one.
-          browser.tabs.move(source.id, { index: 0 }, () => {
-            browser.tabs.duplicate(source.id, tab => {
-              browser.test.assertEq("http://example.net/", tab.url);
-              // Should be the second tab, next to the one duplicated.
-              browser.test.assertEq(1, tab.index);
-              // Should be active by default.
-              browser.test.assertTrue(tab.active);
+    background: async function() {
+      let [source] = await browser.tabs.query({
+        lastFocusedWindow: true,
+        active: true,
+      });
+
+      let tab = await browser.tabs.duplicate(source.id);
 
-              browser.tabs.remove([tab.id, source.id]);
-              browser.test.notifyPass("tabs.duplicate");
-            });
-          });
-        }
+      browser.test.assertEq(
+        "http://example.net/",
+        tab.url,
+        "duplicated tab should have the same URL as the source tab"
       );
+      browser.test.assertEq(
+        source.index + 1,
+        tab.index,
+        "duplicated tab should open next to the source tab"
+      );
+      browser.test.assertTrue(
+        tab.active,
+        "duplicated tab should be active by default"
+      );
+
+      await browser.tabs.remove([source.id, tab.id]);
+      browser.test.notifyPass("tabs.duplicate");
     },
   });
 
   await extension.startup();
   await extension.awaitFinish("tabs.duplicate");
   await extension.unload();
 });
 
@@ -141,35 +143,35 @@ add_task(async function testDuplicatePin
   );
   gBrowser.pinTab(tab);
 
   let extension = ExtensionTestUtils.loadExtension({
     manifest: {
       permissions: ["tabs"],
     },
 
-    background: function() {
-      browser.tabs.query(
-        {
-          lastFocusedWindow: true,
-        },
-        function(tabs) {
-          // Duplicate the pinned tab, example.net.
-          browser.tabs.duplicate(tabs[0].id, tab => {
-            browser.test.assertEq("http://example.net/", tab.url);
-            // Should be the second tab, next to the original.
-            browser.test.assertEq(1, tab.index);
-            // Duplicated tab is not pinned, even if the original tab is.
-            browser.test.assertFalse(tab.pinned);
+    background: async function() {
+      let [source] = await browser.tabs.query({
+        lastFocusedWindow: true,
+        active: true,
+      });
+      let tab = await browser.tabs.duplicate(source.id);
 
-            browser.tabs.remove([tabs[0].id, tab.id]);
-            browser.test.notifyPass("tabs.duplicate.pinned");
-          });
-        }
+      browser.test.assertEq(
+        source.index + 1,
+        tab.index,
+        "duplicated tab should open next to the source tab"
       );
+      browser.test.assertFalse(
+        tab.pinned,
+        "duplicated tab should not be pinned by default, even if source tab is"
+      );
+
+      await browser.tabs.remove([source.id, tab.id]);
+      browser.test.notifyPass("tabs.duplicate.pinned");
     },
   });
 
   await extension.startup();
   await extension.awaitFinish("tabs.duplicate.pinned");
   await extension.unload();
 });
 
@@ -256,65 +258,59 @@ add_task(async function testDuplicatePin
   });
 
   await extension.startup();
   await extension.awaitFinish("tabs.duplicate.incorrect_index");
   await extension.unload();
 });
 
 add_task(async function testDuplicateResolvePromiseRightAway() {
-  const BASE =
-    "http://mochi.test:8888/browser/browser/components/extensions/test/browser/";
-  const URL = BASE + "file_slowed_document.sjs";
-
-  await BrowserTestUtils.openNewForegroundTab(gBrowser, URL);
+  await BrowserTestUtils.openNewForegroundTab(
+    gBrowser,
+    "http://mochi.test:8888/browser/browser/components/extensions/test/browser/file_slowed_document.sjs"
+  );
 
   let extension = ExtensionTestUtils.loadExtension({
     manifest: {
       // The host permission matches the above URL. No :8888 due to bug 1468162.
       permissions: ["tabs", "http://mochi.test/"],
     },
 
     background: async function() {
-      browser.tabs.query(
-        {
-          lastFocusedWindow: true,
+      let [source] = await browser.tabs.query({
+        lastFocusedWindow: true,
+        active: true,
+      });
+
+      let resolvedRightAway = true;
+      browser.tabs.onUpdated.addListener(
+        (tabId, changeInfo, tab) => {
+          resolvedRightAway = false;
         },
-        async tabs => {
-          let resolvedRightAway = null;
-          browser.tabs.onUpdated.addListener(
-            (tabId, changeInfo, tab) => {
-              if (resolvedRightAway === null) {
-                resolvedRightAway = false;
-              }
-            },
-            { urls: [tabs[1].url] }
-          );
+        { urls: [source.url] }
+      );
 
-          browser.tabs.duplicate(tabs[1].id, async tab => {
-            // if the promise is resolved before any onUpdated event has been fired,
-            // then the promise has been resolved before waiting for the tab to load
-            if (resolvedRightAway === null) {
-              resolvedRightAway = true;
-            }
+      let tab = await browser.tabs.duplicate(source.id);
+      // if the promise is resolved before any onUpdated event has been fired,
+      // then the promise has been resolved before waiting for the tab to load
+      browser.test.assertTrue(
+        resolvedRightAway,
+        "tabs.duplicate() should resolve as soon as possible"
+      );
 
-            // Regression test for bug 1559216: APIs such as tabs.executeScript
-            // should be queued until tabs.duplicate has restored the tab.
-            let code = "document.URL";
-            let [result] = await browser.tabs.executeScript(tab.id, { code });
-            browser.test.assertEq(tab.url, result, "executeScript result");
+      // Regression test for bug 1559216
+      let code = "document.URL";
+      let [result] = await browser.tabs.executeScript(tab.id, { code });
+      browser.test.assertEq(
+        source.url,
+        result,
+        "APIs such as tabs.executeScript should be queued until tabs.duplicate has restored the tab"
+      );
 
-            await browser.tabs.remove([tabs[1].id, tab.id]);
-            if (resolvedRightAway) {
-              browser.test.notifyPass("tabs.duplicate.resolvePromiseRightAway");
-            } else {
-              browser.test.notifyFail("tabs.duplicate.resolvePromiseRightAway");
-            }
-          });
-        }
-      );
+      await browser.tabs.remove([source.id, tab.id]);
+      browser.test.notifyPass("tabs.duplicate.resolvePromiseRightAway");
     },
   });
 
   await extension.startup();
   await extension.awaitFinish("tabs.duplicate.resolvePromiseRightAway");
   await extension.unload();
 });
--- a/browser/components/sessionstore/SessionStore.jsm
+++ b/browser/components/sessionstore/SessionStore.jsm
@@ -3107,17 +3107,17 @@ var SessionStoreInternal = {
       : TAB_CUSTOM_VALUES.get(obj);
   },
 
   duplicateTab: function ssi_duplicateTab(
     aWindow,
     aTab,
     aDelta = 0,
     aRestoreImmediately = true,
-    { index } = {}
+    { inBackground, index } = {}
   ) {
     if (!aTab || !aTab.ownerGlobal) {
       throw Components.Exception("Need a valid tab", Cr.NS_ERROR_INVALID_ARG);
     }
     if (!aTab.ownerGlobal.__SSi) {
       throw Components.Exception(
         "Default view is not tracked",
         Cr.NS_ERROR_INVALID_ARG
@@ -3181,16 +3181,20 @@ var SessionStoreInternal = {
 
       tabState.index += aDelta;
       tabState.index = Math.max(
         1,
         Math.min(tabState.index, tabState.entries.length)
       );
       tabState.pinned = false;
 
+      if (inBackground === false) {
+        aWindow.gBrowser.selectedTab = newTab;
+      }
+
       // Restore the state into the new tab.
       this.restoreTab(newTab, tabState, {
         restoreImmediately: aRestoreImmediately,
       });
     });
 
     return newTab;
   },