Bug 1451176 - Preserve tab-specific data when tab is moved to another window draft
authorOriol Brufau <oriol-bugzilla@hotmail.com>
Wed, 04 Apr 2018 16:04:11 +0200
changeset 797916 a060f770925bf0d8c6daf05af828bc289d06bf9c
parent 797835 10d74a74f250ac8c1d51e285ed75237ae7069ab4
push id110620
push userbmo:oriol-bugzilla@hotmail.com
push dateMon, 21 May 2018 23:28:03 +0000
bugs1451176
milestone62.0a1
Bug 1451176 - Preserve tab-specific data when tab is moved to another window MozReview-Commit-ID: 80h4U7G3gmb
browser/components/extensions/parent/ext-browser.js
browser/components/extensions/parent/ext-browserAction.js
browser/components/extensions/parent/ext-pageAction.js
browser/components/extensions/parent/ext-sidebarAction.js
browser/components/extensions/test/browser/browser_ext_browserAction_context.js
browser/components/extensions/test/browser/browser_ext_pageAction_context.js
browser/components/extensions/test/browser/browser_ext_sidebarAction_context.js
browser/components/extensions/test/browser/head_pageAction.js
mobile/android/components/extensions/ext-browserAction.js
mobile/android/components/extensions/ext-pageAction.js
mobile/android/components/extensions/ext-utils.js
--- a/browser/components/extensions/parent/ext-browser.js
+++ b/browser/components/extensions/parent/ext-browser.js
@@ -122,45 +122,46 @@ global.replaceUrlInTab = (gBrowser, tab,
 };
 
 /**
  * Manages tab-specific and window-specific context data, and dispatches
  * tab select events across all windows.
  */
 global.TabContext = class extends EventEmitter {
   /**
-   * @param {Function} getDefaults
-   *        Provides the context value for a tab or window when there is none.
+   * @param {Function} getDefaultPrototype
+   *        Provides the prototype of the context value for a tab or window when there is none.
    *        Called with a XULElement or ChromeWindow argument.
-   *        The returned value is cached.
-   * @param {Object} extension
-   *        The extension object.
+   *        Should return an object or null.
    */
-  constructor(getDefaults, extension) {
+  constructor(getDefaultPrototype) {
     super();
 
-    this.extension = extension;
-    this.getDefaults = getDefaults;
+    this.getDefaultPrototype = getDefaultPrototype;
 
     this.tabData = new WeakMap();
 
     windowTracker.addListener("progress", this);
     windowTracker.addListener("TabSelect", this);
+
+    this.tabDetached = this.tabDetached.bind(this);
+    tabTracker.on("tab-detached", this.tabDetached);
   }
 
   /**
    * Returns the context data associated with `keyObject`.
    *
    * @param {XULElement|ChromeWindow} keyObject
    *        Browser tab or browser chrome window.
-   * @returns {any}
+   * @returns {Object}
    */
   get(keyObject) {
     if (!this.tabData.has(keyObject)) {
-      this.tabData.set(keyObject, this.getDefaults(keyObject));
+      let data = Object.create(this.getDefaultPrototype(keyObject));
+      this.tabData.set(keyObject, data);
     }
 
     return this.tabData.get(keyObject);
   }
 
   /**
    * Clears the context data associated with `keyObject`.
    *
@@ -182,22 +183,34 @@ global.TabContext = class extends EventE
   onLocationChange(browser, webProgress, request, locationURI, flags) {
     let gBrowser = browser.ownerGlobal.gBrowser;
     let tab = gBrowser.getTabForBrowser(browser);
     // fromBrowse will be false in case of e.g. a hash change or history.pushState
     let fromBrowse = !(flags & Ci.nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT);
     this.emit("location-change", tab, fromBrowse);
   }
 
+  tabDetached(eventType, {nativeTab, adoptedBy}) {
+    if (!this.tabData.has(nativeTab)) {
+      return;
+    }
+    // Create a new object (possibly with different inheritance) when a tab is moved
+    // into a new window. But then reassign own properties from the old object.
+    let newData = this.get(adoptedBy);
+    let oldData = this.tabData.get(nativeTab);
+    Object.assign(newData, oldData);
+  }
+
   /**
    * Makes the TabContext instance stop emitting events.
    */
   shutdown() {
     windowTracker.removeListener("progress", this);
     windowTracker.removeListener("TabSelect", this);
+    tabTracker.off("tab-detached", this.tabDetached);
   }
 };
 
 
 class WindowTracker extends WindowTrackerBase {
   addProgressListener(window, listener) {
     window.gBrowser.addTabsProgressListener(listener);
   }
--- a/browser/components/extensions/parent/ext-browserAction.js
+++ b/browser/components/extensions/parent/ext-browserAction.js
@@ -107,20 +107,20 @@ this.browserAction = class extends Exten
       this.defaults.icon,
       await StartupCache.get(
         extension, ["browserAction", "default_icon_data"],
         () => this.getIconData(this.defaults.icon)));
 
     this.tabContext = new TabContext(target => {
       let window = target.ownerGlobal;
       if (target === window) {
-        return Object.create(this.globals);
+        return this.globals;
       }
-      return Object.create(this.tabContext.get(window));
-    }, extension);
+      return this.tabContext.get(window);
+    });
 
     // eslint-disable-next-line mozilla/balanced-listeners
     this.tabContext.on("location-change", this.handleLocationChange.bind(this));
 
     this.build();
   }
 
   handleLocationChange(eventType, tab, fromBrowse) {
--- a/browser/components/extensions/parent/ext-pageAction.js
+++ b/browser/components/extensions/parent/ext-pageAction.js
@@ -71,18 +71,17 @@ this.pageAction = class extends Extensio
     };
 
     this.browserStyle = options.browser_style || false;
     if (options.browser_style === null) {
       this.extension.logger.warn("Please specify whether you want browser_style " +
                                  "or not in your page_action options.");
     }
 
-    this.tabContext = new TabContext(tab => Object.create(this.defaults),
-                                     extension);
+    this.tabContext = new TabContext(tab => this.defaults);
 
     this.tabContext.on("location-change", this.handleLocationChange.bind(this)); // eslint-disable-line mozilla/balanced-listeners
 
     pageActionMap.set(extension, this);
 
     this.defaults.icon = await StartupCache.get(
       extension, ["pageAction", "default_icon"],
       () => this.normalize({path: options.default_icon || ""}));
--- a/browser/components/extensions/parent/ext-sidebarAction.js
+++ b/browser/components/extensions/parent/ext-sidebarAction.js
@@ -52,20 +52,20 @@ this.sidebarAction = class extends Exten
       icon: IconDetails.normalize({path: options.default_icon}, extension),
       panel: options.default_panel || "",
     };
     this.globals = Object.create(this.defaults);
 
     this.tabContext = new TabContext(target => {
       let window = target.ownerGlobal;
       if (target === window) {
-        return Object.create(this.globals);
+        return this.globals;
       }
-      return Object.create(this.tabContext.get(window));
-    }, extension);
+      return this.tabContext.get(window);
+    });
 
     // We need to ensure our elements are available before session restore.
     this.windowOpenListener = (window) => {
       this.createMenuItem(window, this.globals);
     };
     windowTracker.addOpenListener(this.windowOpenListener);
 
     this.updateHeader = (event) => {
--- a/browser/components/extensions/test/browser/browser_ext_browserAction_context.js
+++ b/browser/components/extensions/test/browser/browser_ext_browserAction_context.js
@@ -655,20 +655,20 @@ add_task(async function testMultipleWind
           browser.browserAction.setPopup({windowId, popup: "window2.html"});
           browser.browserAction.setTitle({windowId, title: "window2"});
           browser.browserAction.setBadgeText({windowId, text: "w2"});
           browser.browserAction.setBadgeBackgroundColor({windowId, color: "#222"});
           expect(null, details[2], null, details[0]);
         },
         async expect => {
           browser.test.log("Move tab from old window to the new one. Tab-specific data"
-            + " is cleared (bug 1451176) and inheritance is from the new window");
+            + " is preserved but inheritance is from the new window");
           await browser.tabs.move(tabs[1], {windowId: windows[1], index: -1});
           await browser.tabs.update(tabs[1], {active: true});
-          expect(null, details[2], null, details[0]);
+          expect(details[3], details[2], null, details[0]);
         },
         async expect => {
           browser.test.log("Close the tab, expect window values.");
           await browser.tabs.remove(tabs[1]);
           expect(null, details[2], null, details[0]);
         },
         async expect => {
           browser.test.log("Close the new window and go back to the previous one.");
--- a/browser/components/extensions/test/browser/browser_ext_pageAction_context.js
+++ b/browser/components/extensions/test/browser/browser_ext_pageAction_context.js
@@ -194,16 +194,82 @@ add_task(async function testTabSwitchCon
 
           expect(null);
         },
       ];
     },
   });
 });
 
+add_task(async function testMultipleWindows() {
+  await runTests({
+    manifest: {
+      "page_action": {
+        "default_icon": "default.png",
+        "default_popup": "default.html",
+        "default_title": "Default Title",
+      },
+    },
+
+    "files": {
+      "default.png": imageBuffer,
+      "tab.png": imageBuffer,
+    },
+
+    getTests: function(tabs, windows) {
+      let details = [
+        {"icon": browser.runtime.getURL("default.png"),
+         "popup": browser.runtime.getURL("default.html"),
+         "title": "Default Title"},
+        {"icon": browser.runtime.getURL("tab.png"),
+         "popup": browser.runtime.getURL("tab.html"),
+         "title": "tab"},
+      ];
+
+      return [
+        async expect => {
+          browser.test.log("Create a new tab, expect hidden pageAction.");
+          let tab = await browser.tabs.create({active: true});
+          tabs.push(tab.id);
+          expect(null);
+        },
+        async expect => {
+          browser.test.log("Show the pageAction, expect default values.");
+          await browser.pageAction.show(tabs[1]);
+          expect(details[0]);
+        },
+        async expect => {
+          browser.test.log("Set tab-specific values, expect them.");
+          await browser.pageAction.setIcon({tabId: tabs[1], path: "tab.png"});
+          await browser.pageAction.setPopup({tabId: tabs[1], popup: "tab.html"});
+          await browser.pageAction.setTitle({tabId: tabs[1], title: "tab"});
+          expect(details[1]);
+        },
+        async expect => {
+          browser.test.log("Open a new window, expect hidden pageAction.");
+          let {id} = await browser.windows.create();
+          windows.push(id);
+          expect(null);
+        },
+        async expect => {
+          browser.test.log("Move tab from old window to the new one, expect old values.");
+          await browser.tabs.move(tabs[1], {windowId: windows[1], index: -1});
+          await browser.tabs.update(tabs[1], {active: true});
+          expect(details[1]);
+        },
+        async expect => {
+          browser.test.log("Close the new window and go back to the previous one.");
+          await browser.windows.remove(windows[1]);
+          expect(null);
+        },
+      ];
+    },
+  });
+});
+
 add_task(async function testNavigationClearsData() {
   let url = "http://example.com/";
   let default_title = "Default title";
   let tab_title = "Tab title";
 
   let {Management: {global: {tabTracker}}} = ChromeUtils.import("resource://gre/modules/Extension.jsm", {});
   let extension, tabs = [];
   async function addTab(...args) {
--- a/browser/components/extensions/test/browser/browser_ext_sidebarAction_context.js
+++ b/browser/components/extensions/test/browser/browser_ext_sidebarAction_context.js
@@ -554,20 +554,20 @@ add_task(async function testMultipleWind
           let windowId = windows[1];
           browser.sidebarAction.setIcon({windowId, path: "window2.png"});
           browser.sidebarAction.setPanel({windowId, panel: "window2.html"});
           browser.sidebarAction.setTitle({windowId, title: "window2"});
           expect(null, details[2], null, details[0]);
         },
         async expect => {
           browser.test.log("Move tab from old window to the new one. Tab-specific data"
-            + " is cleared (bug 1451176) and inheritance is from the new window");
+            + " is preserved but inheritance is from the new window");
           await browser.tabs.move(tabs[1], {windowId: windows[1], index: -1});
           await browser.tabs.update(tabs[1], {active: true});
-          expect(null, details[2], null, details[0]);
+          expect(details[3], details[2], null, details[0]);
         },
         async expect => {
           browser.test.log("Close the tab, expect window values.");
           await browser.tabs.remove(tabs[1]);
           expect(null, details[2], null, details[0]);
         },
         async expect => {
           browser.test.log("Close the new window and go back to the previous one.");
--- a/browser/components/extensions/test/browser/head_pageAction.js
+++ b/browser/components/extensions/test/browser/head_pageAction.js
@@ -18,72 +18,69 @@
   Services.locale.setAvailableLocales(["en-US", "es-ES"]);
   registerCleanupFunction(() => {
     Services.locale.setAvailableLocales(avLocales);
   });
 }
 
 async function runTests(options) {
   function background(getTests) {
-    let tabs;
     let tests;
 
     // Gets the current details of the page action, and returns a
     // promise that resolves to an object containing them.
-    async function getDetails() {
-      let [tab] = await browser.tabs.query({active: true, currentWindow: true});
-      let tabId = tab.id;
-
-      browser.test.log(`Get details: tab={id: ${tabId}, url: ${JSON.stringify(tab.url)}}`);
-
+    async function getDetails(tabId) {
       return {
         title: await browser.pageAction.getTitle({tabId}),
         popup: await browser.pageAction.getPopup({tabId}),
         isShown: await browser.pageAction.isShown({tabId}),
       };
     }
 
 
     // Runs the next test in the `tests` array, checks the results,
     // and passes control back to the outer test scope.
     function nextTest() {
       let test = tests.shift();
 
       test(async expecting => {
-        function finish() {
-          // Check that the actual icon has the expected values, then
-          // run the next test.
-          browser.test.sendMessage("nextTest", expecting, tests.length);
-        }
+        let [tab] = await browser.tabs.query({active: true, currentWindow: true});
+        let {id: tabId, windowId, url} = tab;
+
+        browser.test.log(`Get details: tab={id: ${tabId}, url: ${url}}`);
 
         // Check that the API returns the expected values, and then
         // run the next test.
-        let details = await getDetails();
+        let details = await getDetails(tabId);
         if (expecting) {
           browser.test.assertEq(expecting.title, details.title,
                                 "expected value from getTitle");
 
           browser.test.assertEq(expecting.popup, details.popup,
                                 "expected value from getPopup");
         }
 
         browser.test.assertEq(!!expecting, details.isShown,
                               "expected value from isShown");
 
-        finish();
+        // Check that the actual icon has the expected values, then
+        // run the next test.
+        browser.test.sendMessage("nextTest", expecting, windowId, tests.length);
       });
     }
 
     async function runTests() {
-      tabs = [];
-      tests = getTests(tabs);
+      let tabs = [];
+      let windows = [];
+      tests = getTests(tabs, windows);
 
       let resultTabs = await browser.tabs.query({active: true, currentWindow: true});
 
       tabs[0] = resultTabs[0].id;
+      windows[0] = resultTabs[0].windowId;
 
       nextTest();
     }
 
     browser.test.onMessage.addListener((msg) => {
       if (msg == "runTests") {
         runTests();
       } else if (msg == "runNextTest") {
@@ -103,18 +100,19 @@ async function runTests(options) {
 
     background: `(${background})(${options.getTests})`,
   });
 
   let pageActionId;
   let currentWindow = window;
   let windows = [];
 
-  function checkDetails(details) {
-    let image = currentWindow.document.getElementById(pageActionId);
+  function checkDetails(details, windowId) {
+    let {document} = Services.wm.getOuterWindowWithId(windowId);
+    let image = document.getElementById(pageActionId);
     if (details == null) {
       ok(image == null || image.getAttribute("disabled") == "true", "image is disabled");
     } else {
       ok(image, "image exists");
 
       is(getListStyleImage(image), details.icon, "icon URL is correct");
 
       let title = details.title || options.manifest.name;
@@ -122,24 +120,24 @@ async function runTests(options) {
       is(image.getAttribute("aria-label"), title, "image aria-label is correct");
       // TODO: Popup URL.
     }
   }
 
   let testNewWindows = 1;
 
   let awaitFinish = new Promise(resolve => {
-    extension.onMessage("nextTest", async (expecting, testsRemaining) => {
+    extension.onMessage("nextTest", async (expecting, windowId, testsRemaining) => {
       if (!pageActionId) {
         pageActionId = BrowserPageActions.urlbarButtonNodeIDForActionID(makeWidgetId(extension.id));
       }
 
       await promiseAnimationFrame(currentWindow);
 
-      checkDetails(expecting);
+      checkDetails(expecting, windowId);
 
       if (testsRemaining) {
         extension.sendMessage("runNextTest");
       } else if (testNewWindows) {
         testNewWindows--;
 
         BrowserTestUtils.openNewBrowserWindow().then(window => {
           windows.push(window);
--- a/mobile/android/components/extensions/ext-browserAction.js
+++ b/mobile/android/components/extensions/ext-browserAction.js
@@ -18,18 +18,17 @@ class BrowserAction extends EventEmitter
 
     this.uuid = `{${extension.uuid}}`;
 
     this.defaults = {
       name: options.default_title || extension.name,
       popup: options.default_popup,
     };
 
-    this.tabContext = new TabContext(tab => Object.create(this.defaults),
-                                     extension);
+    this.tabContext = new TabContext(tabId => this.defaults);
 
     this.tabManager = extension.tabManager;
 
     this.tabContext.on("tab-selected", // eslint-disable-line mozilla/balanced-listeners
                        (evt, tabId) => { this.onTabSelected(tabId); });
     this.tabContext.on("tab-closed", // eslint-disable-line mozilla/balanced-listeners
                        (evt, tabId) => { this.onTabClosed(tabId); });
 
--- a/mobile/android/components/extensions/ext-pageAction.js
+++ b/mobile/android/components/extensions/ext-pageAction.js
@@ -33,17 +33,17 @@ class PageAction extends EventEmitter {
     this.defaults = {
       icons: IconDetails.normalize({path: manifest.default_icon}, extension),
       popup: manifest.default_popup,
     };
 
     this.tabManager = extension.tabManager;
     this.context = null;
 
-    this.tabContext = new TabContext(() => Object.create(this.defaults), extension);
+    this.tabContext = new TabContext(tabId => this.defaults);
 
     this.options = {
       title: manifest.default_title || extension.name,
       id: `{${extension.uuid}}`,
       clickCallback: () => {
         let tab = tabTracker.activeTab;
 
         this.tabManager.addActiveTabPermission(tab);
--- a/mobile/android/components/extensions/ext-utils.js
+++ b/mobile/android/components/extensions/ext-utils.js
@@ -563,32 +563,32 @@ class Tab extends TabBase {
       microphone: false,
       camera: false,
     };
   }
 }
 
 // Manages tab-specific context data and dispatches tab select and close events.
 class TabContext extends EventEmitter {
-  constructor(getDefaults, extension) {
+  constructor(getDefaultPrototype) {
     super();
 
-    this.extension = extension;
-    this.getDefaults = getDefaults;
+    this.getDefaultPrototype = getDefaultPrototype;
     this.tabData = new Map();
 
     GlobalEventDispatcher.registerListener(this, [
       "Tab:Selected",
       "Tab:Closed",
     ]);
   }
 
   get(tabId) {
     if (!this.tabData.has(tabId)) {
-      this.tabData.set(tabId, this.getDefaults());
+      let data = Object.create(this.getDefaultPrototype(tabId));
+      this.tabData.set(tabId, data);
     }
 
     return this.tabData.get(tabId);
   }
 
   clear(tabId) {
     this.tabData.delete(tabId);
   }