Bug 1085774 - Part 1: add more history URLs to tab records. r=rnewman
authorEdouard Oger <edouard.oger@gmail.com>
Mon, 24 Nov 2014 14:40:25 +0100
changeset 218147 9dffef3538c3c683e1804ce14a5713e334bcff90
parent 218146 b0e899a4f71749531e8e4cf9f344d955bb043ccf
child 218148 5994d92115accb8c5478b5dea15d90f7a6341a6e
push id27917
push usercbook@mozilla.com
push dateMon, 01 Dec 2014 11:03:31 +0000
treeherdermozilla-central@707a34b55e44 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrnewman
bugs1085774
milestone37.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1085774 - Part 1: add more history URLs to tab records. r=rnewman
services/sync/modules/engines/tabs.js
services/sync/tests/unit/head_helpers.js
services/sync/tests/unit/test_tab_store.js
--- a/services/sync/modules/engines/tabs.js
+++ b/services/sync/modules/engines/tabs.js
@@ -2,16 +2,17 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 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");
 Cu.import("resource://services-sync/engines.js");
 Cu.import("resource://services-sync/engines/clients.js");
 Cu.import("resource://services-sync/record.js");
 Cu.import("resource://services-sync/util.js");
 Cu.import("resource://services-sync/constants.js");
@@ -124,33 +125,50 @@ TabStore.prototype = {
       for (let tab of win.gBrowser.tabs) {
         tabState = this.getTabState(tab);
 
         // Make sure there are history entries to look at.
         if (!tabState || !tabState.entries.length) {
           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),
         });
       }
     }
 
     return allTabs;
   },
 
   createRecord: function (id, collection) {
--- a/services/sync/tests/unit/head_helpers.js
+++ b/services/sync/tests/unit/head_helpers.js
@@ -133,36 +133,41 @@ function mockShouldSkipWindow (win) {
   return win.closed ||
          win.mockIsPrivate;
 }
 
 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 = {
       closed: false,
       mockIsPrivate: false,
       gBrowser: {
         tabs: tabs,
       },
     };
     elements.push(win);
 
     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"
         },
         lastAccessed: 1499
       }));
     }
   }
 
--- a/services/sync/tests/unit/test_tab_store.js
+++ b/services/sync/tests/unit/test_tab_store.js
@@ -47,33 +47,53 @@ function test_create() {
   // reset the notifyTabState
   Svc.Prefs.reset("notifyTabState");
 }
 
 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() {
   let store = getMockStore();
   let record;
 
   store.getTabState = mockGetTabState;
   store.shouldSkipWindow = mockShouldSkipWindow;