Bug 1201977 - Replace usage of nsINavHistoryQuery with a string query to nsPIPlacesDatabase.asyncStatement r=oyiptong draft
authorMaxim Zhilyaev <mzhilyaev@mozilla.com>
Wed, 17 Feb 2016 09:28:55 -0800
changeset 331580 2ae61ccd803979b95168f9d05d494386357bee14
parent 331578 4f1701beb4ec0db8ccf400fa8ec8fa4f275b76cc
child 514413 682870dda6d9897542849b4ac514d0405b13a45f
push id11017
push usermzhilyaev@mozilla.com
push dateWed, 17 Feb 2016 17:29:15 +0000
reviewersoyiptong
bugs1201977
milestone47.0a1
Bug 1201977 - Replace usage of nsINavHistoryQuery with a string query to nsPIPlacesDatabase.asyncStatement r=oyiptong MozReview-Commit-ID: 6nsqU2PO0YB
browser/components/newtab/PlacesProvider.jsm
browser/components/newtab/tests/xpcshell/test_PlacesProvider.js
--- a/browser/components/newtab/PlacesProvider.jsm
+++ b/browser/components/newtab/PlacesProvider.jsm
@@ -25,16 +25,19 @@ XPCOMUtils.defineLazyGetter(this, "Event
   return EventEmitter;
 });
 
 XPCOMUtils.defineLazyGetter(this, "gPrincipal", function() {
   let uri = Services.io.newURI("about:newtab", null, null);
   return Services.scriptSecurityManager.getNoAppCodebasePrincipal(uri);
 });
 
+XPCOMUtils.defineLazyModuleGetter(this, "Task",
+                                  "resource://gre/modules/Task.jsm");
+
 // The maximum number of results PlacesProvider retrieves from history.
 const HISTORY_RESULTS_LIMIT = 100;
 
 /**
  * Singleton that checks if a given link should be displayed on about:newtab
  * or if we should rather not do it for security reasons. URIs that inherit
  * their caller's principal will be filtered.
  */
@@ -63,56 +66,16 @@ let LinkChecker = {
     } catch (e) {
       // We got a weird URI or one that would inherit the caller's principal.
       Cu.reportError(e);
     }
     return result;
   }
 };
 
-/**
- * Singleton that provides utility functions for links.
- * A link is a plain object that looks like this:
- *
- * {
- *   url: "http://www.mozilla.org/",
- *   title: "Mozilla",
- *   frecency: 1337,
- *   lastVisitDate: 1394678824766431,
- * }
- */
-const LinkUtils = {
-  _sortProperties: [
-    "frecency",
-    "lastVisitDate",
-    "url",
-  ],
-
-  /**
-   * Compares two links.
-   *
-   * @param {String} aLink1 The first link.
-   * @param {String} aLink2 The second link.
-   * @return {Number} A negative number if aLink1 is ordered before aLink2, zero if
-   *         aLink1 and aLink2 have the same ordering, or a positive number if
-   *         aLink1 is ordered after aLink2.
-   *         Order is ascending.
-   */
-  compareLinks: function LinkUtils_compareLinks(aLink1, aLink2) {
-    for (let prop of LinkUtils._sortProperties) {
-      if (!aLink1.hasOwnProperty(prop) || !aLink2.hasOwnProperty(prop)) {
-        throw new Error("Comparable link missing required property: " + prop);
-      }
-    }
-    return aLink2.frecency - aLink1.frecency ||
-           aLink2.lastVisitDate - aLink1.lastVisitDate ||
-           aLink1.url.localeCompare(aLink2.url);
-  },
-};
-
 /* Queries history to retrieve the most visited sites. Emits events when the
  * history changes.
  * Implements the EventEmitter interface.
  */
 let Links = function Links() {
   EventEmitter.decorate(this);
 };
 
@@ -187,85 +150,99 @@ Links.prototype = {
     PlacesUtils.history.removeObserver(this.historyObserver);
   },
 
   /**
    * Gets the current set of links delivered by this provider.
    *
    * @returns {Promise} Returns a promise with the array of links as payload.
    */
-  getLinks: function PlacesProvider_getLinks() {
-    let getLinksPromise = new Promise((resolve, reject) => {
-      let options = PlacesUtils.history.getNewQueryOptions();
-      options.maxResults = this.maxNumLinks;
+  getLinks: Task.async(function*() {
+    // Select a single page per host with highest frecency, highest recency.
+    // Choose N top such pages. Note +rev_host, to turn off optimizer per :mak
+    // suggestion.
+    let sqlQuery = `SELECT url, title, frecency,
+                          last_visit_date as lastVisitDate,
+                          "history" as type
+                   FROM moz_places
+                   WHERE frecency in (
+                     SELECT MAX(frecency) as frecency
+                     FROM moz_places
+                     WHERE hidden = 0 AND last_visit_date NOTNULL
+                     GROUP BY +rev_host
+                     ORDER BY frecency DESC
+                     LIMIT :limit
+                   )
+                   GROUP BY rev_host HAVING MAX(lastVisitDate)
+                   ORDER BY frecency DESC, lastVisitDate DESC, url`;
 
-      // Sort by frecency, descending.
-      options.sortingMode = Ci.nsINavHistoryQueryOptions
-        .SORT_BY_FRECENCY_DESCENDING;
+    let links = yield this.executePlacesQuery(sqlQuery, {
+                  columns: ["url", "title", "lastVisitDate", "frecency", "type"],
+                  params: {limit: this.maxNumLinks}
+                });
 
-      let links = [];
+    return links.filter(link => LinkChecker.checkLoadURI(link.url));
+  }),
 
-      let queryHandlers = {
-        handleResult: function(aResultSet) {
-          for (let row = aResultSet.getNextRow(); row; row = aResultSet.getNextRow()) {
-            let url = row.getResultByIndex(1);
-            if (LinkChecker.checkLoadURI(url)) {
-              let link = {
-                url: url,
-                title: row.getResultByIndex(2),
-                frecency: row.getResultByIndex(12),
-                lastVisitDate: row.getResultByIndex(5),
-                type: "history",
-              };
-              links.push(link);
+  /**
+   * Executes arbitrary query against places database
+   *
+   * @param {String} aSql
+   *        SQL query to execute
+   * @param {Object} [optional] aOptions
+   *        aOptions.columns - an array of column names. if supplied the returned
+   *        items will consist of objects keyed on column names. Otherwise
+   *        an array of raw values is returned in the select order
+   *        aOptions.param - an object of SQL binding parameters
+   *        aOptions.callback - a callback to handle query rows
+   *
+   * @returns {Promise} Returns a promise with the array of retrieved items
+   */
+  executePlacesQuery: Task.async(function*(aSql, aOptions={}) {
+    let {columns, params, callback} = aOptions;
+    let items = [];
+    let queryError = null;
+    let conn = yield PlacesUtils.promiseDBConnection();
+    yield conn.executeCached(aSql, params, aRow => {
+      try {
+        // check if caller wants to handle query raws
+        if (callback) {
+          callback(aRow);
+        }
+        // otherwise fill in the item and add items array
+        else {
+          let item = null;
+          // if columns array is given construct an object
+          if (columns && Array.isArray(columns)) {
+            item = {};
+            columns.forEach(column => {
+              item[column] = aRow.getResultByName(column);
+            });
+          } else {
+            // if no columns - make an array of raw values
+            item = [];
+            for (let i = 0; i < aRow.numEntries; i++) {
+              item.push(aRow.getResultByIndex(i));
             }
           }
-        },
-
-        handleError: function(aError) {
-          reject(aError);
-        },
-
-        handleCompletion: function(aReason) { // jshint ignore:line
-          // The Places query breaks ties in frecency by place ID descending, but
-          // that's different from how Links.compareLinks breaks ties, because
-          // compareLinks doesn't have access to place IDs.  It's very important
-          // that the initial list of links is sorted in the same order imposed by
-          // compareLinks, because Links uses compareLinks to perform binary
-          // searches on the list.  So, ensure the list is so ordered.
-          let i = 1;
-          let outOfOrder = [];
-          while (i < links.length) {
-            if (LinkUtils.compareLinks(links[i - 1], links[i]) > 0) {
-              outOfOrder.push(links.splice(i, 1)[0]);
-            } else {
-              i++;
-            }
-          }
-          for (let link of outOfOrder) {
-            i = BinarySearch.insertionIndexOf(LinkUtils.compareLinks, links, link);
-            links.splice(i, 0, link);
-          }
-
-          resolve(links);
+          items.push(item);
         }
-      };
-
-      // Execute the query.
-      let query = PlacesUtils.history.getNewQuery();
-      let db = PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase);
-      db.asyncExecuteLegacyQueries([query], 1, options, queryHandlers);
+      } catch (e) {
+        queryError = e;
+        throw StopIteration;
+      }
     });
-
-    return getLinksPromise;
-  }
+    if (queryError) {
+      throw new Error(queryError);
+    }
+    return items;
+  }),
 };
 
 /**
  * Singleton that serves as the default link provider for the grid.
  */
 const gLinks = new Links(); // jshint ignore:line
 
 let PlacesProvider = {
   LinkChecker: LinkChecker,
-  LinkUtils: LinkUtils,
   links: gLinks,
 };
--- a/browser/components/newtab/tests/xpcshell/test_PlacesProvider.js
+++ b/browser/components/newtab/tests/xpcshell/test_PlacesProvider.js
@@ -26,16 +26,40 @@ XPCOMUtils.defineLazyModuleGetter(this, 
 
 // ensure a profile exists
 do_get_profile();
 
 function run_test() {
   run_next_test();
 }
 
+// url prefix for test history population
+const TEST_URL = "https://mozilla.com/";
+// time when the test starts execution
+const TIME_NOW = (new Date()).getTime();
+
+// utility function to compute past timestap
+function timeDaysAgo(numDays) {
+  return TIME_NOW - (numDays * 24 * 60 * 60 * 1000);
+}
+
+// utility function to make a visit for insetion into places db
+function makeVisit(index, daysAgo, isTyped, domain=TEST_URL) {
+  let {
+    TRANSITION_TYPED,
+    TRANSITION_LINK
+  } = PlacesUtils.history;
+
+  return {
+    uri: NetUtil.newURI(`${domain}${index}`),
+    visitDate: timeDaysAgo(daysAgo),
+    transition: (isTyped) ? TRANSITION_TYPED : TRANSITION_LINK,
+  };
+}
+
 /** Test LinkChecker **/
 
 add_task(function test_LinkChecker_securityCheck() {
 
   let urls = [
     {url: "file://home/file/image.png", expected: false},
     {url: "resource:///modules/PlacesProvider.jsm", expected: false},
     {url: "javascript:alert('hello')", expected: false}, // jshint ignore:line
@@ -45,107 +69,16 @@ add_task(function test_LinkChecker_secur
     {url: "ftp://example.com", expected: true},
   ];
   for (let {url, expected} of urls) {
     let observed = PlacesProvider.LinkChecker.checkLoadURI(url);
     equal(observed , expected, `can load "${url}"?`);
   }
 });
 
-/** Test LinkUtils **/
-
-add_task(function test_LinkUtils_compareLinks() {
-
-  let fixtures = {
-    firstOlder: {
-      url: "http://www.mozilla.org/firstolder",
-      title: "Mozilla",
-      frecency: 1337,
-      lastVisitDate: 1394678824766431,
-    },
-    older: {
-      url: "http://www.mozilla.org/older",
-      title: "Mozilla",
-      frecency: 1337,
-      lastVisitDate: 1394678824766431,
-    },
-    newer: {
-      url: "http://www.mozilla.org/newer",
-      title: "Mozilla",
-      frecency: 1337,
-      lastVisitDate: 1494678824766431,
-    },
-    moreFrecent: {
-      url: "http://www.mozilla.org/moreFrecent",
-      title: "Mozilla",
-      frecency: 1337357,
-      lastVisitDate: 1394678824766431,
-    }
-  };
-
-  let links = [
-    // tests string ordering, f is before o
-    {link1: fixtures.firstOlder, link2: fixtures.older, expected: false},
-
-    // test identity
-    {link1: fixtures.older, link2: fixtures.older, expected: false},
-
-    // test ordering by date
-    {link1: fixtures.older, link2: fixtures.newer, expected: true},
-    {link1: fixtures.newer, link2: fixtures.older, expected: false},
-
-    // test frecency
-    {link1: fixtures.moreFrecent, link2: fixtures.older, expected: false},
-  ];
-
-  for (let {link1, link2, expected} of links) {
-    let observed = PlacesProvider.LinkUtils.compareLinks(link1, link2) > 0;
-    equal(observed , expected, `comparing ${link1.url} and ${link2.url}`);
-  }
-
-  // test error scenarios
-
-  let errorFixtures = {
-    missingFrecency: {
-      url: "http://www.mozilla.org/firstolder",
-      title: "Mozilla",
-      lastVisitDate: 1394678824766431,
-    },
-    missingVisitDate: {
-      url: "http://www.mozilla.org/firstolder",
-      title: "Mozilla",
-      frecency: 1337,
-    },
-    missingURL: {
-      title: "Mozilla",
-      frecency: 1337,
-      lastVisitDate: 1394678824766431,
-    }
-  };
-
-  let errorLinks = [
-    {link1: fixtures.older, link2: errorFixtures.missingFrecency},
-    {link2: fixtures.older, link1: errorFixtures.missingFrecency},
-    {link1: fixtures.older, link2: errorFixtures.missingVisitDate},
-    {link1: fixtures.older, link2: errorFixtures.missingURL},
-    {link1: errorFixtures.missingFrecency, link2: errorFixtures.missingVisitDate}
-  ];
-
-  let errorCount = 0;
-  for (let {link1, link2} of errorLinks) {
-    try {
-      let observed = PlacesProvider.LinkUtils.compareLinks(link1, link2) > 0; // jshint ignore:line
-    } catch (e) {
-      ok(true, `exception for comparison of ${link1.url} and ${link2.url}`);
-      errorCount += 1;
-    }
-  }
-  equal(errorCount, errorLinks.length);
-});
-
 /** Test Provider **/
 
 add_task(function* test_Links_getLinks() {
   yield PlacesTestUtils.clearHistory();
   let provider = PlacesProvider.links;
 
   let links = yield provider.getLinks();
   equal(links.length, 0, "empty history yields empty links");
@@ -157,51 +90,58 @@ add_task(function* test_Links_getLinks()
   links = yield provider.getLinks();
   equal(links.length, 1, "adding a visit yields a link");
   equal(links[0].url, testURI.spec, "added visit corresponds to added url");
 });
 
 add_task(function* test_Links_getLinks_Order() {
   yield PlacesTestUtils.clearHistory();
   let provider = PlacesProvider.links;
-  let {
-    TRANSITION_TYPED,
-    TRANSITION_LINK
-  } = PlacesUtils.history;
 
-  function timeDaysAgo(numDays) {
-    let now = new Date();
-    return now.getTime() - (numDays * 24 * 60 * 60 * 1000);
-  }
-
-  let timeEarlier = timeDaysAgo(0);
-  let timeLater = timeDaysAgo(2);
-
+  // all four visits must come from different domains to avoid deduplication
   let visits = [
-    // frecency 200
-    {uri: NetUtil.newURI("https://mozilla.com/0"), visitDate: timeEarlier, transition: TRANSITION_TYPED},
-    // sort by url, frecency 200
-    {uri: NetUtil.newURI("https://mozilla.com/1"), visitDate: timeEarlier, transition: TRANSITION_TYPED},
-    // sort by last visit date, frecency 200
-    {uri: NetUtil.newURI("https://mozilla.com/2"), visitDate: timeLater, transition: TRANSITION_TYPED},
-    // sort by frecency, frecency 10
-    {uri: NetUtil.newURI("https://mozilla.com/3"), visitDate: timeLater, transition: TRANSITION_LINK},
+    makeVisit(0, 0, true, "http://bar.com/"), // frecency 200, today
+    makeVisit(1, 0, true, "http://foo.com/"), // frecency 200, today
+    makeVisit(2, 2, true, "http://buz.com/"), // frecency 200, 2 days ago
+    makeVisit(3, 2, false, "http://aaa.com/"), // frecency 10, 2 days ago, transition
   ];
 
   let links = yield provider.getLinks();
   equal(links.length, 0, "empty history yields empty links");
   yield PlacesTestUtils.addVisits(visits);
 
   links = yield provider.getLinks();
   equal(links.length, visits.length, "number of links added is the same as obtain by getLinks");
   for (let i = 0; i < links.length; i++) {
     equal(links[i].url, visits[i].uri.spec, "links are obtained in the expected order");
   }
 });
 
+add_task(function* test_Links_getLinks_Deduplication() {
+  yield PlacesTestUtils.clearHistory();
+  let provider = PlacesProvider.links;
+
+  // all for visits must come from different domains to avoid deduplication
+  let visits = [
+    makeVisit(0, 2, true, "http://bar.com/"), // frecency 200, 2 days ago
+    makeVisit(1, 0, true, "http://bar.com/"), // frecency 200, today
+    makeVisit(2, 0, false, "http://foo.com/"), // frecency 10, today
+    makeVisit(3, 0, true, "http://foo.com/"), // frecency 200, today
+  ];
+
+  let links = yield provider.getLinks();
+  equal(links.length, 0, "empty history yields empty links");
+  yield PlacesTestUtils.addVisits(visits);
+
+  links = yield provider.getLinks();
+  equal(links.length, 2, "only two links must be left after deduplication");
+  equal(links[0].url, visits[1].uri.spec, "earliest link is present");
+  equal(links[1].url, visits[3].uri.spec, "most fresent link is present");
+});
+
 add_task(function* test_Links_onLinkChanged() {
   let provider = PlacesProvider.links;
   provider.init();
 
   let url = "https://example.com/onFrecencyChanged1";
   let linkChangedMsgCount = 0;
 
   let linkChangedPromise = new Promise(resolve => {
@@ -300,8 +240,127 @@ add_task(function* test_Links_onManyLink
 
   // trigger DecayFrecency
   PlacesUtils.history.QueryInterface(Ci.nsIObserver).
     observe(null, "idle-daily", "");
 
   yield promise;
   provider.destroy();
 });
+
+add_task(function* test_Links_execute_query() {
+  yield PlacesTestUtils.clearHistory();
+  let provider = PlacesProvider.links;
+
+  let visits = [
+    makeVisit(0, 0, true), // frecency 200, today
+    makeVisit(1, 0, true), // frecency 200, today
+    makeVisit(2, 2, true), // frecency 200, 2 days ago
+    makeVisit(3, 2, false), // frecency 10, 2 days ago, transition
+  ];
+
+  yield PlacesTestUtils.addVisits(visits);
+
+  function testItemValue(results, index, value) {
+    equal(results[index][0], `${TEST_URL}${value}`, "raw url");
+    equal(results[index][1], `test visit for ${TEST_URL}${value}`, "raw title");
+  }
+
+  function testItemObject(results, index, columnValues) {
+    Object.keys(columnValues).forEach(name => {
+      equal(results[index][name], columnValues[name], "object name " + name);
+    });
+  }
+
+  // select all 4 records
+  let results = yield provider.executePlacesQuery("select url, title from moz_places");
+  equal(results.length, 4, "expect 4 items");
+  // check for insert order sequence
+  for (let i = 0; i < results.length; i++) {
+    testItemValue(results, i, i);
+  }
+
+  // test parameter passing
+  results = yield provider.executePlacesQuery(
+              "select url, title from moz_places limit :limit",
+              {params: {limit: 2}}
+            );
+  equal(results.length, 2, "expect 2 items");
+  for (let i = 0; i < results.length; i++) {
+    testItemValue(results, i, i);
+  }
+
+  // test extracting items by name
+  results = yield provider.executePlacesQuery(
+              "select url, title from moz_places limit :limit",
+              {columns: ["url", "title"], params: {limit: 4}}
+            );
+  equal(results.length, 4, "expect 4 items");
+  for (let i = 0; i < results.length; i++) {
+    testItemObject(results, i, {
+      "url": `${TEST_URL}${i}`,
+      "title": `test visit for ${TEST_URL}${i}`,
+    });
+  }
+
+  // test ordering
+  results = yield provider.executePlacesQuery(
+              "select url, title, last_visit_date, frecency from moz_places " +
+              "order by frecency DESC, last_visit_date DESC, url DESC limit :limit",
+              {columns: ["url", "title", "last_visit_date", "frecency"], params: {limit: 4}}
+            );
+  equal(results.length, 4, "expect 4 items");
+  testItemObject(results, 0, {url: `${TEST_URL}1`});
+  testItemObject(results, 1, {url: `${TEST_URL}0`});
+  testItemObject(results, 2, {url: `${TEST_URL}2`});
+  testItemObject(results, 3, {url: `${TEST_URL}3`});
+
+  // test callback passing
+  results = [];
+  function handleRow(aRow) {
+    results.push({
+      url:              aRow.getResultByName("url"),
+      title:            aRow.getResultByName("title"),
+      last_visit_date:  aRow.getResultByName("last_visit_date"),
+      frecency:         aRow.getResultByName("frecency")
+    });
+  };
+  yield provider.executePlacesQuery(
+        "select url, title, last_visit_date, frecency from moz_places " +
+        "order by frecency DESC, last_visit_date DESC, url DESC",
+        {callback: handleRow}
+      );
+  equal(results.length, 4, "expect 4 items");
+  testItemObject(results, 0, {url: `${TEST_URL}1`});
+  testItemObject(results, 1, {url: `${TEST_URL}0`});
+  testItemObject(results, 2, {url: `${TEST_URL}2`});
+  testItemObject(results, 3, {url: `${TEST_URL}3`});
+
+  // negative test cases
+  // bad sql
+  try {
+    yield provider.executePlacesQuery("select from moz");
+    do_throw("bad sql should've thrown");
+  }
+  catch (e) {
+    do_check_true("expected failure - bad sql");
+  }
+  // missing bindings
+  try {
+    yield provider.executePlacesQuery("select * from moz_places limit :limit");
+    do_throw("bad sql should've thrown");
+  }
+  catch (e) {
+    do_check_true("expected failure - missing bidning");
+  }
+  // non-existent column name
+  try {
+    yield provider.executePlacesQuery("select * from moz_places limit :limit",
+                                     {columns: ["no-such-column"], params: {limit: 4}});
+    do_throw("bad sql should've thrown");
+  }
+  catch (e) {
+    do_check_true("expected failure - wrong column name");
+  }
+
+  // cleanup
+  yield PlacesTestUtils.clearHistory();
+});