Bug 1252054 - Sort "Synced Tabs" sidebar entries by last used date. r?markh draft
authorKit Cambridge <kcambridge@mozilla.com>
Mon, 07 Mar 2016 14:13:41 -0800
changeset 337532 4c4cdb56f4e871b57e6d26cf094261bc988cbab5
parent 337531 5ac431e075e823abd6492e71e70d327800a48b4e
child 338234 a9fb6427018a37f89648dbcb5bccd00804e82e62
push id12384
push userkcambridge@mozilla.com
push dateMon, 07 Mar 2016 23:11:36 +0000
reviewersmarkh
bugs1252054
milestone47.0a1
Bug 1252054 - Sort "Synced Tabs" sidebar entries by last used date. r?markh MozReview-Commit-ID: BvmQfQMHlMw
browser/components/customizableui/CustomizableWidgets.jsm
browser/components/syncedtabs/SyncedTabsListStore.js
browser/components/syncedtabs/TabListView.js
browser/components/syncedtabs/test/browser/browser_sidebar_syncedtabslist.js
services/sync/modules/SyncedTabs.jsm
--- a/browser/components/customizableui/CustomizableWidgets.jsm
+++ b/browser/components/customizableui/CustomizableWidgets.jsm
@@ -409,17 +409,17 @@ const CustomizableWidgets = [
 
         if (clients.length === 0) {
           this.setDeckIndex(this.deckIndices.DECKINDEX_NOCLIENTS);
           return;
         }
 
         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) {
           // add a menu separator for all clients other than the first.
           if (fragment.lastChild) {
             let separator = doc.createElementNS(kNSXUL, "menuseparator");
             fragment.appendChild(separator);
           }
@@ -483,39 +483,16 @@ const CustomizableWidgets = [
       // We need to use "click" instead of "command" here so openUILink
       // respects different buttons (eg, to open in a new tab).
       item.addEventListener("click", e => {
         doc.defaultView.openUILink(tabInfo.url, e);
         CustomizableUI.hidePanelForNode(item);
       });
       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",
     defaultArea: CustomizableUI.AREA_PANEL,
     onCommand: function(e) {
       if (e.target && e.target.ownerDocument && e.target.ownerDocument.defaultView) {
         let win = e.target.ownerDocument.defaultView;
         if (typeof win.OpenBrowserWindow == "function") {
--- a/browser/components/syncedtabs/SyncedTabsListStore.js
+++ b/browser/components/syncedtabs/SyncedTabsListStore.js
@@ -205,28 +205,33 @@ Object.assign(SyncedTabsListStore.protot
     this._selectedRow = [-1, -1];
     return this.getData();
   },
 
   // Fetches data from the SyncedTabs module and triggers
   // and update
   getData(filter) {
     let updateType;
-    if (typeof filter !== "undefined") {
+    let hasFilter = typeof filter !== "undefined";
+    if (hasFilter) {
       this.filter = filter;
       this._selectedRow = [-1, -1];
 
       // When a filter is specified we tell the view that only the list
       // needs to be rerendered so that it doesn't disrupt the input
       // field's focus.
       updateType = "searchbox";
     }
 
     // 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);
       })
       .catch(Cu.reportError);
   }
 });
 
--- a/browser/components/syncedtabs/TabListView.js
+++ b/browser/components/syncedtabs/TabListView.js
@@ -305,17 +305,21 @@ TabListView.prototype = {
     let item = this.container.querySelector('.item.selected');
     if (this._isTab(item) && item.dataset.url) {
       this.props.onOpenTab(item.dataset.url, event);
     }
   },
 
   onFilter(event) {
     let query = event.target.value;
-    this.props.onFilter(query);
+    if (query) {
+      this.props.onFilter(query);
+    } else {
+      this.props.onClearFilter();
+    }
   },
 
   onClearFilter() {
     this.props.onClearFilter();
   },
 
   onFilterFocus() {
     this.props.onFilterFocus();
--- a/browser/components/syncedtabs/test/browser/browser_sidebar_syncedtabslist.js
+++ b/browser/components/syncedtabs/test/browser/browser_sidebar_syncedtabslist.js
@@ -33,16 +33,25 @@ const FIXTURE = [
       },
       {
         "type": "tab",
         "title": "Firefox Nightly First Run Page",
         "url": "https://www.mozilla.org/en-US/firefox/nightly/firstrun/?oldversion=45.0a1",
         "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
       }
     ]
   },
   {
     "id": "OL3EJCsdb2JD",
     "type": "client",
     "name": "desktop",
     "icon": "chrome://browser/skin/sync-desktopIcon.png",
@@ -82,43 +91,46 @@ add_task(function* testSyncedTabsSidebar
   SyncedTabs._internal = {
     isConfiguredToSyncTabs: true,
     hasSyncedThisSession: true,
     getTabClients() { return Promise.resolve([])},
     syncTabs() {return Promise.resolve();},
   };
 
   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
   // after the following promise (a different instance of which is triggered
   // in updatePanel) resolves, so we wait for it here as well
   yield syncedTabsDeckComponent.tabListComponent._store.getData();
 
   Assert.ok(SyncedTabs._internal.getTabClients.called, "get clients called");
 
   let selectedPanel = syncedTabsDeckComponent.container.querySelector(".sync-state.selected");
 
 
   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]);
     });
   });
 
 });
 
 add_task(testClean);
 
 add_task(function* testSyncedTabsSidebarFilteredList() {
@@ -132,17 +144,17 @@ add_task(function* testSyncedTabsSidebar
   SyncedTabs._internal = {
     isConfiguredToSyncTabs: true,
     hasSyncedThisSession: true,
     getTabClients() { return Promise.resolve([])},
     syncTabs() {return Promise.resolve();},
   };
 
   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
   // after the following promise (a different instance of which is triggered
   // in updatePanel) resolves, so we wait for it here as well
   yield syncedTabsDeckComponent.tabListComponent._store.getData();
 
   let filterInput = syncedTabsDeckComponent.container.querySelector(".tabsFilter");
@@ -150,30 +162,37 @@ add_task(function* testSyncedTabsSidebar
   filterInput.blur();
 
   yield syncedTabsDeckComponent.tabListComponent._store.getData("filter text");
 
   let selectedPanel = syncedTabsDeckComponent.container.querySelector(".sync-state.selected");
   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);
 
 add_task(function* testSyncedTabsSidebarStatus() {
   let accountExists = false;
 
   yield SidebarUI.show('viewTabsSidebar');
--- a/services/sync/modules/SyncedTabs.jsm
+++ b/services/sync/modules/SyncedTabs.jsm
@@ -271,10 +271,35 @@ this.SyncedTabs = {
   // Starts a background request to start syncing tabs. Returns a promise that
   // resolves when the sync is complete, but there's no resolved value -
   // callers should be listening for TOPIC_TABS_CHANGED.
   // If |force| is true we always sync. If false, we only sync if the most
   // recent sync wasn't "recently".
   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;
+    });
+  },
 };