diff --git a/services/sync/modules/engines/tabs.js b/services/sync/modules/engines/tabs.js index dce102d8775..afc07fc82b1 100644 --- a/services/sync/modules/engines/tabs.js +++ b/services/sync/modules/engines/tabs.js @@ -7,6 +7,7 @@ this.EXPORTED_SYMBOLS = ["TabEngine", "TabSetRecord"]; const {classes: Cc, interfaces: Ci, utils: Cu} = Components; const TABS_TTL = 604800; // 7 days. +const TAB_ENTRIES_LIMIT = 25; // How many URLs to include in tab history. Cu.import("resource://gre/modules/Preferences.jsm"); Cu.import("resource://gre/modules/XPCOMUtils.jsm"); @@ -129,23 +130,40 @@ TabStore.prototype = { continue; } - // Until we store full or partial history, just grab the current entry. - // index is 1 based, so make sure we adjust. - let entry = tabState.entries[tabState.index - 1]; + let acceptable = !filter ? (url) => url : + (url) => url && !filteredUrls.test(url); - // Filter out some urls if necessary. SessionStore can return empty - // tabs in some cases - easiest thing is to just ignore them for now. - if (!entry.url || filter && filteredUrls.test(entry.url)) { + let entries = tabState.entries; + let index = tabState.index; + let current = entries[index - 1]; + + // We ignore the tab completely if the current entry url is + // not acceptable (we need something accurate to open). + if (!acceptable(current.url)) { continue; } - // I think it's also possible that attributes[.image] might not be set - // so handle that as well. + // The element at `index` is the current page. Previous URLs were + // previously visited URLs; subsequent URLs are in the 'forward' stack, + // which we can't represent in Sync, so we truncate here. + let candidates = (entries.length == index) ? + entries : + entries.slice(0, index); + + let urls = candidates.map((entry) => entry.url) + .filter(acceptable) + .reverse(); // Because Sync puts current at index 0, and history after. + + // Truncate if necessary. + if (urls.length > TAB_ENTRIES_LIMIT) { + urls.length = TAB_ENTRIES_LIMIT; + } + allTabs.push({ - title: entry.title || "", - urlHistory: [entry.url], + title: current.title || "", + urlHistory: urls, icon: tabState.attributes && tabState.attributes.image || "", - lastUsed: Math.floor((tabState.lastAccessed || 0) / 1000) + lastUsed: Math.floor((tabState.lastAccessed || 0) / 1000), }); } } diff --git a/services/sync/tests/unit/head_helpers.js b/services/sync/tests/unit/head_helpers.js index 38020348037..8f8d036a139 100644 --- a/services/sync/tests/unit/head_helpers.js +++ b/services/sync/tests/unit/head_helpers.js @@ -138,8 +138,16 @@ function mockGetTabState (tab) { return tab; } -function mockGetWindowEnumerator(url, numWindows, numTabs) { +function mockGetWindowEnumerator(url, numWindows, numTabs, indexes, moreURLs) { let elements = []; + + function url2entry(url) { + return { + url: ((typeof url == "function") ? url() : url), + title: "title" + }; + } + for (let w = 0; w < numWindows; ++w) { let tabs = []; let win = { @@ -153,11 +161,8 @@ function mockGetWindowEnumerator(url, numWindows, numTabs) { for (let t = 0; t < numTabs; ++t) { tabs.push(TestingUtils.deepCopy({ - index: 1, - entries: [{ - url: ((typeof url == "string") ? url : url()), - title: "title" - }], + index: indexes ? indexes() : 1, + entries: (moreURLs ? [url].concat(moreURLs()) : [url]).map(url2entry), attributes: { image: "image" }, diff --git a/services/sync/tests/unit/test_tab_store.js b/services/sync/tests/unit/test_tab_store.js index dc733e48f84..f8265492fbb 100644 --- a/services/sync/tests/unit/test_tab_store.js +++ b/services/sync/tests/unit/test_tab_store.js @@ -52,23 +52,43 @@ function test_getAllTabs() { let store = getMockStore(); let tabs; - store.getWindowEnumerator = mockGetWindowEnumerator.bind(this, "http://foo.com", 1, 1); + let threeUrls = ["http://foo.com", "http://fuubar.com", "http://barbar.com"]; + + store.getWindowEnumerator = mockGetWindowEnumerator.bind(this, "http://bar.com", 1, 1, () => 2, () => threeUrls); _("Get all tabs."); tabs = store.getAllTabs(); _("Tabs: " + JSON.stringify(tabs)); do_check_eq(tabs.length, 1); do_check_eq(tabs[0].title, "title"); - do_check_eq(tabs[0].urlHistory.length, 1); - do_check_eq(tabs[0].urlHistory[0], ["http://foo.com"]); + do_check_eq(tabs[0].urlHistory.length, 2); + do_check_eq(tabs[0].urlHistory[0], "http://foo.com"); + do_check_eq(tabs[0].urlHistory[1], "http://bar.com"); do_check_eq(tabs[0].icon, "image"); do_check_eq(tabs[0].lastUsed, 1); _("Get all tabs, and check that filtering works."); - store.getWindowEnumerator = mockGetWindowEnumerator.bind(this, "about:foo", 1, 1); + let twoUrls = ["about:foo", "http://fuubar.com"]; + store.getWindowEnumerator = mockGetWindowEnumerator.bind(this, "http://foo.com", 1, 1, () => 2, () => twoUrls); tabs = store.getAllTabs(true); _("Filtered: " + JSON.stringify(tabs)); do_check_eq(tabs.length, 0); + + _("Get all tabs, and check that the entries safety limit works."); + let allURLs = []; + for (let i = 0; i < 50; i++) { + allURLs.push("http://foo" + i + ".bar"); + } + allURLs.splice(35, 0, "about:foo", "about:bar", "about:foobar"); + + store.getWindowEnumerator = mockGetWindowEnumerator.bind(this, "http://bar.com", 1, 1, () => 45, () => allURLs); + tabs = store.getAllTabs((url) => url.startsWith("about")); + + _("Sliced: " + JSON.stringify(tabs)); + do_check_eq(tabs.length, 1); + do_check_eq(tabs[0].urlHistory.length, 25); + do_check_eq(tabs[0].urlHistory[0], "http://foo40.bar"); + do_check_eq(tabs[0].urlHistory[24], "http://foo16.bar"); } function test_createRecord() {