Bug 841096 - Part 1: speed up about:sync-tabs. r=rnewman
authorValery Yundin <yuvaleriy@gmail.com>
Mon, 24 Mar 2014 16:11:37 -0700
changeset 175135 aa32872101fe464016e84ac392187d8130db1beb
parent 175134 a593f2a9a80c73d0441c110dd7986b52f2caa8a6
child 175136 6321fef01f690e47ebd289a244a865b47116cc31
push id1
push userroot
push dateMon, 20 Oct 2014 17:29:22 +0000
reviewersrnewman
bugs841096
milestone31.0a1
Bug 841096 - Part 1: speed up about:sync-tabs. r=rnewman
browser/base/content/sync/aboutSyncTabs.js
browser/metro/base/content/startui/RemoteTabsView.js
services/sync/modules/engines/tabs.js
services/sync/tests/unit/test_tab_engine.js
--- a/browser/base/content/sync/aboutSyncTabs.js
+++ b/browser/base/content/sync/aboutSyncTabs.js
@@ -135,26 +135,29 @@ let RemoteTabViewer = {
 
     // clear out existing richlistitems
     let count = list.getRowCount();
     if (count > 0) {
       for (let i = count - 1; i >= 0; i--)
         list.removeItemAt(i);
     }
 
+    let seenURLs = new Set();
+    let localURLs = engine.getOpenURLs();
+
     for (let [guid, client] in Iterator(engine.getAllClients())) {
       // Create the client node, but don't add it in-case we don't show any tabs
       let appendClient = true;
-      let seenURLs = {};
+
       client.tabs.forEach(function({title, urlHistory, icon}) {
         let url = urlHistory[0];
-        if (engine.locallyOpenTabMatchesURL(url) || url in seenURLs)
+        if (!url || localURLs.has(url) || seenURLs.has(url)) {
           return;
-
-        seenURLs[url] = null;
+        }
+        seenURLs.add(url);
 
         if (appendClient) {
           let attrs = {
             type: "client",
             clientName: client.clientName,
             class: Weave.Service.clientsEngine.isMobile(client.id) ? "mobile" : "desktop"
           };
           let clientEnt = this.createItem(attrs);
--- a/browser/metro/base/content/startui/RemoteTabsView.js
+++ b/browser/metro/base/content/startui/RemoteTabsView.js
@@ -63,25 +63,26 @@ RemoteTabsView.prototype = Util.extend(O
     }
   },
 
   populateGrid: function populateGrid() {
 
     let tabsEngine = Weave.Service.engineManager.get("tabs");
     let list = this._set;
     let seenURLs = new Set();
+    let localURLs = tabsEngine.getOpenURLs();
 
     // Clear grid, We don't know what has happened to tabs since last sync
     // Also can result in duplicate tabs(bug 864614)
     this._set.clearAll();
     let show = false;
     for (let [guid, client] in Iterator(tabsEngine.getAllClients())) {
       client.tabs.forEach(function({title, urlHistory, icon}) {
         let url = urlHistory[0];
-        if (tabsEngine.locallyOpenTabMatchesURL(url) || seenURLs.has(url)) {
+        if (!url || getOpenURLs.has(url) || seenURLs.has(url)) {
           return;
         }
         seenURLs.add(url);
         show = true;
 
         // If we wish to group tabs by client, we should be looking for records
         //  of {type:client, clientName, class:{mobile, desktop}} and will
         //  need to readd logic to reset seenURLs for each client.
--- a/services/sync/modules/engines/tabs.js
+++ b/services/sync/modules/engines/tabs.js
@@ -68,29 +68,25 @@ TabEngine.prototype = {
     this._tracker.modified = true;
   },
 
   removeClientData: function removeClientData() {
     let url = this.engineURL + "/" + this.service.clientsEngine.localID;
     this.service.resource(url).delete();
   },
 
-  /* The intent is not to show tabs in the menu if they're already
-   * open locally.  There are a couple ways to interpret this: for
-   * instance, we could do it by removing a tab from the list when
-   * you open it -- but then if you close it, you can't get back to
-   * it.  So the way I'm doing it here is to not show a tab in the menu
-   * if you have a tab open to the same URL, even though this means
-   * that as soon as you navigate anywhere, the original tab will
-   * reappear in the menu.
+  /**
+   * Return a Set of open URLs.
    */
-  locallyOpenTabMatchesURL: function TabEngine_localTabMatches(url) {
-    return this._store.getAllTabs().some(function (tab) {
-      return tab.urlHistory[0] == url;
-    });
+  getOpenURLs: function () {
+    let urls = new Set();
+    for (let entry of this._store.getAllTabs()) {
+      urls.add(entry.urlHistory[0]);
+    }
+    return urls;
   }
 };
 
 
 function TabStore(name, engine) {
   Store.call(this, name, engine);
 }
 TabStore.prototype = {
--- a/services/sync/tests/unit/test_tab_engine.js
+++ b/services/sync/tests/unit/test_tab_engine.js
@@ -25,24 +25,25 @@ function fakeSessionSvc() {
   delete Svc.Session;
   Svc.Session = {
     getBrowserState: function() JSON.stringify(obj)
   };
 }
 
 function run_test() {
 
-  _("test locallyOpenTabMatchesURL");
+  _("test getOpenURLs");
   let engine = new TabEngine(Service);
 
   // 3 tabs
   fakeSessionSvc("http://bar.com", "http://foo.com", "http://foobar.com");
 
   let matches;
 
   _("  test matching works (true)");
-  matches = engine.locallyOpenTabMatchesURL("http://foo.com");
+  let openurlsset = engine.getOpenURLs();
+  matches = openurlsset.has("http://foo.com");
   do_check_true(matches);
 
   _("  test matching works (false)");
-  matches = engine.locallyOpenTabMatchesURL("http://barfoo.com");
+  matches = openurlsset.has("http://barfoo.com");
   do_check_false(matches);
 }