Bug 1252054 - Sort "Synced Tabs" sidebar entries by last used date. r=markh

MozReview-Commit-ID: BvmQfQMHlMw

--HG--
extra : rebase_source : 167fecace1c6d45f300f14abade3986caeff2c6a
extra : histedit_source : 7c29e76072f8ea7dc64e16055187df783ba8a2c7

browser/components/customizableui/CustomizableWidgets.jsm

@@ -414,7 +414,7 @@ const CustomizableWidgets = [
 
         this.setDeckIndex(this.deckIndices.DECKINDEX_TABS);
         this._clearTabList();
-        this._sortFilterClientsAndTabs(clients);
+        SyncedTabs.sortTabClientsByLastUsed(clients, 50 /* maxTabs */);
         let fragment = doc.createDocumentFragment();
 
         for (let client of clients) {
@@ -488,29 +488,6 @@ const CustomizableWidgets = [
       });
       return item;
     },
-    _sortFilterClientsAndTabs(clients) {
-      // First sort and filter the list of tabs for each client. Note that the
-      // SyncedTabs module promises that the objects it returns are never
-      // shared, so we are free to mutate those objects directly.
-      const maxTabs = 50;
-      for (let client of clients) {
-        let tabs = client.tabs;
-        tabs.sort((a, b) => b.lastUsed - a.lastUsed);
-        client.tabs = tabs.slice(0, maxTabs);
-      }
-      // Now sort the clients - the clients are sorted in the order of the
-      // most recent tab for that client (ie, it is important the tabs for
-      // each client are already sorted.)
-      clients.sort((a, b) => {
-        if (a.tabs.length == 0) {
-          return 1; // b comes first.
-        }
-        if (b.tabs.length == 0) {
-          return -1; // a comes first.
-        }
-        return b.tabs[0].lastUsed - a.tabs[0].lastUsed;
-      });
-    },
   }, {
     id: "privatebrowsing-button",
     shortcutId: "key_privatebrowsing",

browser/components/syncedtabs/SyncedTabsListStore.js

@@ -210,7 +210,8 @@ Object.assign(SyncedTabsListStore.prototype, EventEmitter.prototype, {
   // and update
   getData(filter) {
     let updateType;
-    if (typeof filter !== "undefined") {
+    let hasFilter = typeof filter !== "undefined";
+    if (hasFilter) {
       this.filter = filter;
       this._selectedRow = [-1, -1];
 
@@ -223,6 +224,10 @@ Object.assign(SyncedTabsListStore.prototype, EventEmitter.prototype, {
     // return promise for tests
     return this._SyncedTabs.getTabClients(this.filter)
       .then(result => {
+        if (!hasFilter) {
+          // Only sort clients and tabs if we're rendering the whole list.
+          this._SyncedTabs.sortTabClientsByLastUsed(result);
+        }
         this.data = result;
         this._change(updateType);
       })

browser/components/syncedtabs/TabListView.js

@@ -310,7 +310,11 @@ TabListView.prototype = {
 
   onFilter(event) {
     let query = event.target.value;
-    this.props.onFilter(query);
+    if (query) {
+      this.props.onFilter(query);
+    } else {
+      this.props.onClearFilter();
+    }
   },
 
   onClearFilter() {

browser/components/syncedtabs/test/browser/browser_sidebar_syncedtabslist.js

@@ -38,6 +38,15 @@ const FIXTURE = [
         "icon": "moz-anno:favicon:https://www.mozilla.org/media/img/firefox/favicon-nightly.560395bbb2e1.png",
         "client": "2xU5h-4bkWqA",
         "lastUsed": 1451519420
+      },
+      {
+        // Should appear first for this client.
+        "type": "tab",
+        "title": "Mozilla Developer Network",
+        "url": "https://developer.mozilla.org/en-US/",
+        "icon": "moz-anno:favicon:https://developer.cdn.mozilla.net/static/img/favicon32.e02854fdcf73.png",
+        "client": "2xU5h-4bkWqA",
+        "lastUsed": 1451519725
       }
     ]
   },
@@ -87,7 +96,7 @@ add_task(function* testSyncedTabsSidebarList() {
   };
 
   sinon.stub(syncedTabsDeckComponent, "_accountStatus", ()=> Promise.resolve(true));
-  sinon.stub(SyncedTabs._internal, "getTabClients", ()=> Promise.resolve(FIXTURE));
+  sinon.stub(SyncedTabs._internal, "getTabClients", ()=> Promise.resolve(Cu.cloneInto(FIXTURE, {})));
 
   yield syncedTabsDeckComponent.updatePanel();
   // This is a hacky way of waiting for the view to render. The view renders
@@ -103,17 +112,20 @@ add_task(function* testSyncedTabsSidebarList() {
   Assert.ok(selectedPanel.classList.contains("tabs-container"),
     "tabs panel is selected");
 
-  Assert.equal(selectedPanel.querySelectorAll(".tab").length, 3,
-    "three tabs listed");
+  Assert.equal(selectedPanel.querySelectorAll(".tab").length, 4,
+    "four tabs listed");
   Assert.equal(selectedPanel.querySelectorAll(".client").length, 3,
     "three clients listed");
   Assert.equal(selectedPanel.querySelectorAll(".client")[2].querySelectorAll(".empty").length, 1,
     "third client is empty");
 
+  // Verify that the tabs are sorted by last used time.
+  var expectedTabIndices = [[0], [2, 0, 1]];
   Array.prototype.forEach.call(selectedPanel.querySelectorAll(".client"), (clientNode, i) => {
     checkItem(clientNode, FIXTURE[i]);
     Array.prototype.forEach.call(clientNode.querySelectorAll(".tab"), (tabNode, j) => {
-      checkItem(tabNode, FIXTURE[i].tabs[j]);
+      let tabIndex = expectedTabIndices[i][j];
+      checkItem(tabNode, FIXTURE[i].tabs[tabIndex]);
     });
   });
 
@@ -137,7 +149,7 @@ add_task(function* testSyncedTabsSidebarFilteredList() {
   };
 
   sinon.stub(syncedTabsDeckComponent, "_accountStatus", ()=> Promise.resolve(true));
-  sinon.stub(SyncedTabs._internal, "getTabClients", ()=> Promise.resolve(FIXTURE));
+  sinon.stub(SyncedTabs._internal, "getTabClients", ()=> Promise.resolve(Cu.cloneInto(FIXTURE, {})));
 
   yield syncedTabsDeckComponent.updatePanel();
   // This is a hacky way of waiting for the view to render. The view renders
@@ -155,20 +167,27 @@ add_task(function* testSyncedTabsSidebarFilteredList() {
   Assert.ok(selectedPanel.classList.contains("tabs-container"),
     "tabs panel is selected");
 
-  Assert.equal(selectedPanel.querySelectorAll(".tab").length, 3,
-    "three tabs listed");
+  Assert.equal(selectedPanel.querySelectorAll(".tab").length, 4,
+    "four tabs listed");
   Assert.equal(selectedPanel.querySelectorAll(".client").length, 0,
     "no clients are listed");
 
   Assert.equal(filterInput.value, "filter text",
     "filter text box has correct value");
 
+  // Tabs should not be sorted when filter is active.
   let FIXTURE_TABS = FIXTURE.reduce((prev, client) => prev.concat(client.tabs), []);
 
   Array.prototype.forEach.call(selectedPanel.querySelectorAll(".tab"), (tabNode, i) => {
     checkItem(tabNode, FIXTURE_TABS[i]);
   });
 
+  // Removing the filter should resort tabs.
+  FIXTURE_TABS.sort((a, b) => b.lastUsed - a.lastUsed);
+  yield syncedTabsDeckComponent.tabListComponent._store.getData();
+  Array.prototype.forEach.call(selectedPanel.querySelectorAll(".tab"), (tabNode, i) => {
+    checkItem(tabNode, FIXTURE_TABS[i]);
+  });
 });
 
 add_task(testClean);

services/sync/modules/SyncedTabs.jsm

@@ -276,5 +276,30 @@ this.SyncedTabs = {
   syncTabs(force) {
     return this._internal.syncTabs(force);
   },
+
+  sortTabClientsByLastUsed(clients, maxTabs = Infinity) {
+    // First sort and filter the list of tabs for each client. Note that
+    // this module promises that the objects it returns are never
+    // shared, so we are free to mutate those objects directly.
+    for (let client of clients) {
+      let tabs = client.tabs;
+      tabs.sort((a, b) => b.lastUsed - a.lastUsed);
+      if (Number.isFinite(maxTabs)) {
+        client.tabs = tabs.slice(0, maxTabs);
+      }
+    }
+    // Now sort the clients - the clients are sorted in the order of the
+    // most recent tab for that client (ie, it is important the tabs for
+    // each client are already sorted.)
+    clients.sort((a, b) => {
+      if (a.tabs.length == 0) {
+        return 1; // b comes first.
+      }
+      if (b.tabs.length == 0) {
+        return -1; // a comes first.
+      }
+      return b.tabs[0].lastUsed - a.tabs[0].lastUsed;
+    });
+  },
 };