Bug 1013051 - Fix "Comparable link missing required property: frecency" error. r=ttaubert, a=sledru
authorDrew Willcoxon <adw@mozilla.com>
Tue, 10 Jun 2014 10:34:20 -0700
changeset 207055 9c69f87096d4c6a5fa2faae5351d6c332f123a31
parent 207054 4da73f25390a7f1dac00a710eb04631970bb5c45
child 207056 ee9221a6cc71db21897e48ba5b61847d73b4b700
push id3741
push userasasaki@mozilla.com
push dateMon, 21 Jul 2014 20:25:18 +0000
treeherdermozilla-beta@4d6f46f5af68 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersttaubert, sledru
bugs1013051
milestone32.0a2
Bug 1013051 - Fix "Comparable link missing required property: frecency" error. r=ttaubert, a=sledru
toolkit/modules/NewTabUtils.jsm
toolkit/modules/tests/xpcshell/test_NewTabUtils.js
--- a/toolkit/modules/NewTabUtils.jsm
+++ b/toolkit/modules/NewTabUtils.jsm
@@ -912,68 +912,65 @@ let Links = {
     let links = this._providerLinks.get(aProvider);
     if (!links)
       // This is not an error, it just means that between the time the provider
       // was added and the future time we call getLinks on it, it notified us of
       // a change.
       return;
 
     let { sortedLinks, linkMap } = links;
-
-    // Nothing to do if the list is full and the link isn't in it and shouldn't
-    // be in it.
-    if (!linkMap.has(aLink.url) &&
-        sortedLinks.length &&
-        sortedLinks.length == aProvider.maxNumLinks) {
-      let lastLink = sortedLinks[sortedLinks.length - 1];
-      if (this.compareLinks(lastLink, aLink) < 0)
-        return;
-    }
-
+    let existingLink = linkMap.get(aLink.url);
+    let insertionLink = null;
     let updatePages = false;
 
-    // Update the title in O(1).
-    if ("title" in aLink) {
-      let link = linkMap.get(aLink.url);
-      if (link && link.title != aLink.title) {
-        link.title = aLink.title;
+    if (existingLink) {
+      // Update our copy's position in O(lg n) by first removing it from its
+      // list.  It's important to do this before modifying its properties.
+      if (this._sortProperties.some(prop => prop in aLink)) {
+        let idx = this._indexOf(sortedLinks, existingLink);
+        if (idx < 0) {
+          throw new Error("Link should be in _sortedLinks if in _linkMap");
+        }
+        sortedLinks.splice(idx, 1);
+        // Update our copy's properties.
+        for (let prop of this._sortProperties) {
+          if (prop in aLink) {
+            existingLink[prop] = aLink[prop];
+          }
+        }
+        // Finally, reinsert our copy below.
+        insertionLink = existingLink;
+      }
+      // Update our copy's title in O(1).
+      if ("title" in aLink && aLink.title != existingLink.title) {
+        existingLink.title = aLink.title;
         updatePages = true;
       }
     }
-
-    // Update the link's position in O(lg n).
-    if (this._sortProperties.some((prop) => prop in aLink)) {
-      let link = linkMap.get(aLink.url);
-      if (link) {
-        // The link is already in the list.
-        let idx = this._indexOf(sortedLinks, link);
-        if (idx < 0)
-          throw new Error("Link should be in _sortedLinks if in _linkMap");
-        sortedLinks.splice(idx, 1);
-        for (let prop of this._sortProperties) {
-          if (prop in aLink)
-            link[prop] = aLink[prop];
+    else if (this._sortProperties.every(prop => prop in aLink)) {
+      // Before doing the O(lg n) insertion below, do an O(1) check for the
+      // common case where the new link is too low-ranked to be in the list.
+      if (sortedLinks.length && sortedLinks.length == aProvider.maxNumLinks) {
+        let lastLink = sortedLinks[sortedLinks.length - 1];
+        if (this.compareLinks(lastLink, aLink) < 0) {
+          return;
         }
       }
-      else {
-        // The link is new.
-        for (let prop of this._sortProperties) {
-          if (!(prop in aLink))
-            throw new Error("New link missing required sort property: " + prop);
-        }
-        // Copy the link object so that if the caller changes it, it doesn't
-        // screw up our bookkeeping.
-        link = {};
-        for (let [prop, val] of Iterator(aLink)) {
-          link[prop] = val;
-        }
-        linkMap.set(link.url, link);
+      // Copy the link object so that changes later made to it by the caller
+      // don't affect our copy.
+      insertionLink = {};
+      for (let prop in aLink) {
+        insertionLink[prop] = aLink[prop];
       }
-      let idx = this._insertionIndexOf(sortedLinks, link);
-      sortedLinks.splice(idx, 0, link);
+      linkMap.set(aLink.url, insertionLink);
+    }
+
+    if (insertionLink) {
+      let idx = this._insertionIndexOf(sortedLinks, insertionLink);
+      sortedLinks.splice(idx, 0, insertionLink);
       if (sortedLinks.length > aProvider.maxNumLinks) {
         let lastLink = sortedLinks.pop();
         linkMap.delete(lastLink.url);
       }
       updatePages = true;
     }
 
     if (updatePages)
--- a/toolkit/modules/tests/xpcshell/test_NewTabUtils.js
+++ b/toolkit/modules/tests/xpcshell/test_NewTabUtils.js
@@ -46,22 +46,17 @@ add_test(function changeLinks() {
   NewTabUtils.links.addProvider(provider);
 
   // This is sync since the provider's getLinks is sync.
   NewTabUtils.links.populateCache(function () {}, false);
 
   do_check_links(NewTabUtils.links.getLinks(), expectedLinks);
 
   // Notify of a new link.
-  let newLink = {
-    url: "http://example.com/19",
-    title: "My frecency is 19",
-    frecency: 19,
-    lastVisitDate: 0,
-  };
+  let newLink = makeLink(19);
   expectedLinks.splice(1, 0, newLink);
   provider.notifyLinkChanged(newLink);
   do_check_links(NewTabUtils.links.getLinks(), expectedLinks);
 
   // Notify of a link that's changed sort criteria.
   newLink.frecency = 17;
   expectedLinks.splice(1, 1);
   expectedLinks.splice(2, 0, newLink);
@@ -76,21 +71,17 @@ add_test(function changeLinks() {
   provider.notifyLinkChanged({
     url: newLink.url,
     title: newLink.title,
   });
   do_check_links(NewTabUtils.links.getLinks(), expectedLinks);
 
   // Notify of a new link again, but this time make it overflow maxNumLinks.
   provider.maxNumLinks = expectedLinks.length;
-  newLink = {
-    url: "http://example.com/21",
-    frecency: 21,
-    lastVisitDate: 0,
-  };
+  newLink = makeLink(21);
   expectedLinks.unshift(newLink);
   expectedLinks.pop();
   do_check_eq(expectedLinks.length, provider.maxNumLinks); // Sanity check.
   provider.notifyLinkChanged(newLink);
   do_check_links(NewTabUtils.links.getLinks(), expectedLinks);
 
   // Notify of many links changed.
   expectedLinks = makeLinks(0, 3, 1);
@@ -120,16 +111,44 @@ add_task(function oneProviderAlreadyCach
 
   NewTabUtils.links.populateCache(function () {}, false);
   do_check_links(NewTabUtils.links.getLinks(), links2.concat(links1));
 
   NewTabUtils.links.removeProvider(provider1);
   NewTabUtils.links.removeProvider(provider2);
 });
 
+add_task(function newLowRankedLink() {
+  // Init a provider with 10 links and make its maximum number also 10.
+  let links = makeLinks(0, 10, 1);
+  let provider = new TestProvider(done => done(links));
+  provider.maxNumLinks = links.length;
+
+  NewTabUtils.initWithoutProviders();
+  NewTabUtils.links.addProvider(provider);
+
+  // This is sync since the provider's getLinks is sync.
+  NewTabUtils.links.populateCache(function () {}, false);
+  do_check_links(NewTabUtils.links.getLinks(), links);
+
+  // Notify of a new link that's low-ranked enough not to make the list.
+  let newLink = makeLink(0);
+  provider.notifyLinkChanged(newLink);
+  do_check_links(NewTabUtils.links.getLinks(), links);
+
+  // Notify about the new link's title change.
+  provider.notifyLinkChanged({
+    url: newLink.url,
+    title: "a new title",
+  });
+  do_check_links(NewTabUtils.links.getLinks(), links);
+
+  NewTabUtils.links.removeProvider(provider);
+});
+
 function TestProvider(getLinksFn) {
   this.getLinks = getLinksFn;
   this._observers = new Set();
 }
 
 TestProvider.prototype = {
   addObserver: function (observer) {
     this._observers.add(observer);
@@ -160,17 +179,21 @@ function do_check_links(actualLinks, exp
     do_check_eq(actual.lastVisitDate, expected.lastVisitDate);
   }
 }
 
 function makeLinks(frecRangeStart, frecRangeEnd, step) {
   let links = [];
   // Remember, links are ordered by frecency descending.
   for (let i = frecRangeEnd; i > frecRangeStart; i -= step) {
-    links.push({
-      url: "http://example.com/" + i,
-      title: "My frecency is " + i,
-      frecency: i,
-      lastVisitDate: 0,
-    });
+    links.push(makeLink(i));
   }
   return links;
 }
+
+function makeLink(frecency) {
+  return {
+    url: "http://example.com/" + frecency,
+    title: "My frecency is " + frecency,
+    frecency: frecency,
+    lastVisitDate: 0,
+  };
+}