Bug 1787565 - update recently closed tab entries in Firefox View based on delayed tab navigation information from session history, r=sclements

I don't love this solution but it appears to work and I can't think of anything better.

Differential Revision: https://phabricator.services.mozilla.com/D155920

browser/components/firefoxview/recently-closed-tabs.mjs

@@ -33,7 +33,7 @@ class RecentlyClosedTabsList extends HTMLElement {
   constructor() {
     super();
     this.maxTabsLength = 25;
-    this.closedTabsData = [];
+    this.closedTabsData = new Map();
 
     // The recency timestamp update period is stored in a pref to allow tests to easily change it
     XPCOMUtils.defineLazyPreferenceGetter(
@@ -106,7 +106,7 @@ class RecentlyClosedTabsList extends HTMLElement {
   getTabStateValue(tab, key) {
     let value = "";
     const tabEntries = tab.state.entries;
-    const activeIndex = tabEntries.length - 1;
+    const activeIndex = tab.state.index - 1;
 
     if (activeIndex >= 0 && tabEntries[activeIndex]) {
       value = tabEntries[activeIndex][key];
@@ -142,49 +142,53 @@ class RecentlyClosedTabsList extends HTMLElement {
     );
   }
 
-  initiateTabsList() {
-    let closedTabs = lazy.SessionStore.getClosedTabData(getWindow());
-    closedTabs = closedTabs.slice(0, this.maxTabsLength);
-    this.closedTabsData = closedTabs;
-
-    for (const tab of closedTabs) {
-      const li = this.generateListItem(tab);
-      this.tabsList.append(li);
-    }
-    this.tabsList.hidden = false;
-  }
-
   updateTabsList() {
     let newClosedTabs = lazy.SessionStore.getClosedTabData(getWindow());
     newClosedTabs = newClosedTabs.slice(0, this.maxTabsLength);
 
-    if (this.closedTabsData.length && !newClosedTabs.length) {
+    if (this.closedTabsData.size && !newClosedTabs.length) {
       // if a user purges history, clear the list
-      [...this.tabsList.children].forEach(node =>
-        this.tabsList.removeChild(node)
-      );
+      while (this.tabsList.lastElementChild) {
+        this.tabsList.lastElementChild.remove();
+      }
       document
         .getElementById("recently-closed-tabs-container")
         .togglePlaceholderVisibility(true);
       this.tabsList.hidden = true;
-      this.closedTabsData = [];
+      this.closedTabsData = new Map();
       return;
     }
 
-    const tabsToAdd = newClosedTabs.filter(
-      newTab =>
-        !this.closedTabsData.some(tab => {
-          return (
-            this.getTabStateValue(tab, "ID") ==
-            this.getTabStateValue(newTab, "ID")
-          );
-        })
-    );
+    // First purge obsolete items out of the map so we don't leak them forever:
+    for (let id of this.closedTabsData.keys()) {
+      if (!newClosedTabs.some(t => t.closedId == id)) {
+        this.closedTabsData.delete(id);
+      }
+    }
 
-    if (!tabsToAdd.length) {
+    // Then work out which of the new closed tabs are additions and which update
+    // existing items:
+    let tabsToAdd = [];
+    let tabsToUpdate = [];
+    for (let newTab of newClosedTabs) {
+      let oldTab = this.closedTabsData.get(newTab.closedId);
+      this.closedTabsData.set(newTab.closedId, newTab);
+      if (!oldTab) {
+        tabsToAdd.push(newTab);
+      } else if (
+        this.getTabStateValue(oldTab, "url") !=
+        this.getTabStateValue(newTab, "url")
+      ) {
+        tabsToUpdate.push(newTab);
+      }
+    }
+
+    // If there's nothing to add/update, return.
+    if (!tabsToAdd.length && !tabsToUpdate.length) {
       return;
     }
 
+    // Add new tabs.
     for (let tab of tabsToAdd.reverse()) {
       if (this.tabsList.children.length == this.maxTabsLength) {
         this.tabsList.lastChild.remove();
@@ -193,11 +197,16 @@ class RecentlyClosedTabsList extends HTMLElement {
       this.tabsList.prepend(li);
     }
 
-    this.closedTabsData = newClosedTabs;
+    // Update any recently closed tabs that now have different URLs:
+    for (let tab of tabsToUpdate) {
+      let tabElement = this.querySelector(
+        `.closed-tab-li[data-tabid="${tab.closedId}"]`
+      );
+      let url = this.getTabStateValue(tab, "url");
+      this.updateURLForListItem(tabElement, url);
+    }
 
-    // for situations where the tab list will initially be empty (such as
-    // with new profiles or automatic session restore is disabled) and
-    // this.initiateTabsList won't be called
+    // Now unhide the list if necessary:
     if (this.tabsList.hidden) {
       this.tabsList.hidden = false;
       document
@@ -209,6 +218,7 @@ class RecentlyClosedTabsList extends HTMLElement {
   generateListItem(tab) {
     const li = document.createElement("li");
     li.classList.add("closed-tab-li");
+    li.dataset.tabid = tab.closedId;
     li.setAttribute("tabindex", 0);
     li.setAttribute("role", "button");
 
@@ -220,27 +230,36 @@ class RecentlyClosedTabsList extends HTMLElement {
     li.append(favicon);
 
     const targetURI = this.getTabStateValue(tab, "url");
-    li.dataset.targetURI = targetURI;
-    document.l10n.setAttributes(li, "firefoxview-tabs-list-tab-button", {
-      targetURI,
-    });
 
-    const url = document.createElement("span");
-
-    if (targetURI) {
-      url.textContent = formatURIForDisplay(targetURI);
-      url.title = targetURI;
-      url.classList.add("closed-tab-li-url");
-    }
+    const urlElement = document.createElement("span");
+    urlElement.classList.add("closed-tab-li-url");
 
     const time = document.createElement("span");
     time.textContent = convertTimestamp(tab.closedAt, this.fluentStrings);
     time.setAttribute("data-timestamp", tab.closedAt);
     time.classList.add("closed-tab-li-time");
 
-    li.append(title, url, time);
+    li.append(title, urlElement, time);
+    this.updateURLForListItem(li, targetURI);
     return li;
   }
+
+  // Update the URL for a new or previously-populated list item.
+  // This is needed because when tabs get closed we don't necessarily
+  // have all the requisite information for them immediately.
+  updateURLForListItem(li, targetURI) {
+    li.dataset.targetURI = targetURI;
+    document.l10n.setAttributes(li, "firefoxview-tabs-list-tab-button", {
+      targetURI,
+    });
+    let urlElement = li.querySelector(".closed-tab-li-url");
+    if (targetURI) {
+      urlElement.textContent = formatURIForDisplay(targetURI);
+      urlElement.title = targetURI;
+    } else {
+      urlElement.textContent = urlElement.title = "";
+    }
+  }
 }
 customElements.define("recently-closed-tabs-list", RecentlyClosedTabsList);
 
@@ -306,7 +325,7 @@ class RecentlyClosedTabsContainer extends HTMLElement {
     if (this.getClosedTabCount() == 0) {
       this.togglePlaceholderVisibility(true);
     } else {
-      this.list.initiateTabsList();
+      this.list.updateTabsList();
     }
     Services.obs.addObserver(
       this.boundObserve,

browser/components/firefoxview/tests/browser/browser_recently_closed_tabs.js

@@ -64,14 +64,9 @@ add_task(async function test_empty_list() {
     },
     async browser => {
       const { document } = browser.contentWindow;
-      const closedObjectsChanged = TestUtils.topicObserved(
-        "sessionstore-closed-objects-changed"
-      );
-
+      let container = document.querySelector("#collapsible-tabs-container");
       ok(
-        document
-          .querySelector("#collapsible-tabs-container")
-          .classList.contains("empty-container"),
+        container.classList.contains("empty-container"),
         "collapsible container should have correct styling when the list is empty"
       );
 
@@ -85,12 +80,15 @@ add_task(async function test_empty_list() {
       const tab1 = await add_new_tab(URLs[0]);
 
       await close_tab(tab1);
-      await closedObjectsChanged;
 
+      // The UI update happens asynchronously as we learn of the new closed tab.
+      await BrowserTestUtils.waitForMutationCondition(
+        container,
+        { attributeFilter: ["class"] },
+        () => !container.classList.contains("empty-container")
+      );
       ok(
-        !document
-          .querySelector("#collapsible-tabs-container")
-          .classList.contains("empty-container"),
+        !container.classList.contains("empty-container"),
         "collapsible container should have correct styling when the list is not empty"
       );
 
@@ -387,3 +385,55 @@ add_task(async function test_arrow_keys() {
     }
   );
 });
+
+add_task(async function test_switch_before_closing() {
+  clearHistory();
+
+  const INITIAL_URL = "https://example.org/iwilldisappear";
+  const FINAL_URL = "https://example.com/ishouldappear";
+  await withFirefoxView({}, async function(browser) {
+    let gBrowser = browser.getTabBrowser();
+    let newTab = await BrowserTestUtils.openNewForegroundTab(
+      gBrowser,
+      INITIAL_URL
+    );
+
+    // Switch back to FxView:
+    await BrowserTestUtils.switchTab(
+      gBrowser,
+      gBrowser.getTabForBrowser(browser)
+    );
+
+    // Update the tab we opened to a different site:
+    let loadPromise = BrowserTestUtils.browserLoaded(
+      newTab.linkedBrowser,
+      null,
+      FINAL_URL
+    );
+    BrowserTestUtils.loadURI(newTab.linkedBrowser, FINAL_URL);
+    await loadPromise;
+
+    // Close the added tab
+    BrowserTestUtils.removeTab(newTab);
+
+    const { document } = browser.contentWindow;
+    const tabsList = document.querySelector("ol.closed-tabs-list");
+    await BrowserTestUtils.waitForMutationCondition(
+      tabsList,
+      { childList: true },
+      () => !!tabsList.children.length
+    );
+    info("A tab appeared in the list, ensure it has the right URL.");
+    let urlBit = tabsList.firstElementChild.querySelector(".closed-tab-li-url");
+    await BrowserTestUtils.waitForMutationCondition(
+      urlBit,
+      { characterData: true, attributeFilter: ["title"] },
+      () => urlBit.textContent.includes(".com")
+    );
+    is(
+      urlBit.textContent,
+      "example.com",
+      "Item should end up with the correct URL."
+    );
+  });
+});

browser/components/sessionstore/SessionStore.jsm

@@ -1304,6 +1304,8 @@ var SessionStoreInternal = {
       // Update the closed tab's state. This will be reflected in its
       // window's list of closed tabs as that refers to the same object.
       lazy.TabState.copyFromCache(permanentKey, closedTab.tabData.state);
+      this._closedObjectsChanged = true;
+      this._notifyOfClosedObjectsChange();
     }
   },