diff --git a/browser/components/customizableui/CustomizableWidgets.jsm b/browser/components/customizableui/CustomizableWidgets.jsm index ddc18bdbb5d3..47d7a7afd741 100644 --- a/browser/components/customizableui/CustomizableWidgets.jsm +++ b/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", diff --git a/browser/components/syncedtabs/SyncedTabsListStore.js b/browser/components/syncedtabs/SyncedTabsListStore.js index 6e3f00911b42..dd36ba9aa4fe 100644 --- a/browser/components/syncedtabs/SyncedTabsListStore.js +++ b/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); }) diff --git a/browser/components/syncedtabs/TabListView.js b/browser/components/syncedtabs/TabListView.js index 3fc2da966727..b04999d22bcc 100644 --- a/browser/components/syncedtabs/TabListView.js +++ b/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() { diff --git a/browser/components/syncedtabs/test/browser/browser_sidebar_syncedtabslist.js b/browser/components/syncedtabs/test/browser/browser_sidebar_syncedtabslist.js index 2c4aa1b3a4a5..eebb3d44a61a 100644 --- a/browser/components/syncedtabs/test/browser/browser_sidebar_syncedtabslist.js +++ b/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); diff --git a/services/sync/modules/SyncedTabs.jsm b/services/sync/modules/SyncedTabs.jsm index 9c790711c904..3b3791d59dde 100644 --- a/services/sync/modules/SyncedTabs.jsm +++ b/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; + }); + }, };