Bug 1252054 - Sort "Synced Tabs" sidebar entries by last used date. r?markh
MozReview-Commit-ID: BvmQfQMHlMw
--- 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;
+ });
+ },
};