Bug 1085774 - Part 1: add more history URLs to tab records. r=rnewman

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),
         });
       }
     }

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"
         },

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() {