Bug 1336282 - Moved bookmark frecency query into `PlacesSyncUtils`. r=kitcambridge draft
authorAhsan <ahsan.r.kazmi@gmail.com>
Thu, 09 Feb 2017 12:28:26 -0800
changeset 481466 b4d1785e126b87672fc064dee329a791977b1372
parent 481376 eea9995ed14c07675c972400e8ce36b3608c01b1
child 545190 e1eccb618aa31d9cca14e6166c6659d1756a5eca
push id44806
push userbmo:kit@mozilla.com
push dateThu, 09 Feb 2017 21:10:35 +0000
reviewerskitcambridge
bugs1336282
milestone54.0a1
Bug 1336282 - Moved bookmark frecency query into `PlacesSyncUtils`. r=kitcambridge MozReview-Commit-ID: 4DDXOUQ2C8N
services/sync/modules/engines/bookmarks.js
toolkit/components/places/PlacesSyncUtils.jsm
toolkit/components/places/tests/unit/test_sync_utils.js
--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -272,44 +272,16 @@ BookmarksEngine.prototype = {
   _storeObj: BookmarksStore,
   _trackerObj: BookmarksTracker,
   version: 2,
   _defaultSort: "index",
 
   syncPriority: 4,
   allowSkippedRecord: false,
 
-  // A diagnostic helper to get the string value for a bookmark's URL given
-  // its ID. Always returns a string - on error will return a string in the
-  // form of "<description of error>" as this is purely for, eg, logging.
-  // (This means hitting the DB directly and we don't bother using a cached
-  // statement - we should rarely hit this.)
-  _getStringUrlForId(id) {
-    let url;
-    try {
-      let stmt = this._store._getStmt(`
-            SELECT h.url
-            FROM moz_places h
-            JOIN moz_bookmarks b ON h.id = b.fk
-            WHERE b.id = :id`);
-      stmt.params.id = id;
-      let rows = Async.querySpinningly(stmt, ["url"]);
-      url = rows.length == 0 ? "<not found>" : rows[0].url;
-    } catch (ex) {
-      if (Async.isShutdownException(ex)) {
-        throw ex;
-      }
-      if (ex instanceof Ci.mozIStorageError) {
-        url = `<failed: Storage error: ${ex.message} (${ex.result})>`;
-      } else {
-        url = `<failed: ${ex.toString()}>`;
-      }
-    }
-    return url;
-  },
 
   _guidMapFailed: false,
   _buildGUIDMap: function _buildGUIDMap() {
     let store = this._store;
     let guidMap = {};
     let tree = Async.promiseSpinningly(PlacesUtils.promiseBookmarksTree(""));
 
     function* walkBookmarksTree(tree, parent = null) {
@@ -618,24 +590,16 @@ BookmarksEngine.prototype = {
   getValidator() {
     return new BookmarkValidator();
   }
 };
 
 function BookmarksStore(name, engine) {
   Store.call(this, name, engine);
   this._itemsToDelete = new Set();
-  // Explicitly nullify our references to our cached services so we don't leak
-  Svc.Obs.add("places-shutdown", function() {
-    for (let query in this._stmts) {
-      let stmt = this._stmts[query];
-      stmt.finalize();
-    }
-    this._stmts = {};
-  }, this);
 }
 BookmarksStore.prototype = {
   __proto__: Store.prototype,
 
   itemExists: function BStore_itemExists(id) {
     return this.idForGUID(id) > 0;
   },
 
@@ -779,36 +743,16 @@ BookmarksStore.prototype = {
     let record = new recordObj(collection, id);
     record.fromSyncBookmark(item);
 
     record.sortindex = this._calculateIndex(record);
 
     return record;
   },
 
-  _stmts: {},
-  _getStmt(query) {
-    if (query in this._stmts) {
-      return this._stmts[query];
-    }
-
-    this._log.trace("Creating SQL statement: " + query);
-    let db = PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase)
-                        .DBConnection;
-    return this._stmts[query] = db.createAsyncStatement(query);
-  },
-
-  get _frecencyStm() {
-    return this._getStmt(
-        "SELECT frecency " +
-        "FROM moz_places " +
-        "WHERE url_hash = hash(:url) AND url = :url " +
-        "LIMIT 1");
-  },
-  _frecencyCols: ["frecency"],
 
   GUIDForId: function GUIDForId(id) {
     let guid = Async.promiseSpinningly(PlacesUtils.promiseItemGuid(id));
     return PlacesSyncUtils.bookmarks.guidToSyncId(guid);
   },
 
   idForGUID: function idForGUID(guid) {
     // guid might be a String object rather than a string.
@@ -826,20 +770,19 @@ BookmarksStore.prototype = {
     // For anything directly under the toolbar, give it a boost of more than an
     // unvisited bookmark
     let index = 0;
     if (record.parentid == "toolbar")
       index += 150;
 
     // Add in the bookmark's frecency if we have something.
     if (record.bmkUri != null) {
-      this._frecencyStm.params.url = record.bmkUri;
-      let result = Async.querySpinningly(this._frecencyStm, this._frecencyCols);
-      if (result.length)
-        index += result[0].frecency;
+      let frecency = Async.promiseSpinningly(PlacesSyncUtils.history.fetchURLFrecency(record.bmkUri));
+      if (frecency != -1)
+        index += frecency;
     }
 
     return index;
   },
 
   wipe: function BStore_wipe() {
     this.clearPendingDeletions();
     Async.promiseSpinningly(Task.spawn(function* () {
@@ -855,18 +798,16 @@ BookmarksStore.prototype = {
 // changed since the last sync. Because it's a "pull-based" tracker, it ignores
 // all concepts of "add a changed ID." However, it still registers an observer
 // to bump the score, so that changed bookmarks are synced immediately.
 function BookmarksTracker(name, engine) {
   this._batchDepth = 0;
   this._batchSawScoreIncrement = false;
   this._migratedOldEntries = false;
   Tracker.call(this, name, engine);
-
-  Svc.Obs.add("places-shutdown", this);
 }
 BookmarksTracker.prototype = {
   __proto__: Tracker.prototype,
 
   // `_ignore` checks the change source for each observer notification, so we
   // don't want to let the engine ignore all changes during a sync.
   get ignoreAll() {
     return false;
--- a/toolkit/components/places/PlacesSyncUtils.jsm
+++ b/toolkit/components/places/PlacesSyncUtils.jsm
@@ -52,16 +52,32 @@ XPCOMUtils.defineLazyGetter(this, "ROOT_
   [PlacesUtils.bookmarks.unfiledGuid]: "unfiled",
   [PlacesUtils.bookmarks.mobileGuid]: "mobile",
 }));
 
 XPCOMUtils.defineLazyGetter(this, "ROOTS", () =>
   Object.keys(ROOT_SYNC_ID_TO_GUID)
 );
 
+const HistorySyncUtils = PlacesSyncUtils.history = Object.freeze({
+  fetchURLFrecency: Task.async(function* (url) {
+    let canonicalURL = PlacesUtils.SYNC_BOOKMARK_VALIDATORS.url(url);
+
+    let db = yield PlacesUtils.promiseDBConnection();
+    let rows = yield db.executeCached(`
+      SELECT frecency
+      FROM moz_places
+      WHERE url_hash = hash(:url) AND url = :url
+      LIMIT 1`,
+      { url:canonicalURL.href }
+    );
+    return rows.length ? rows[0].getResultByName("frecency") : -1;
+  }),
+});
+
 const BookmarkSyncUtils = PlacesSyncUtils.bookmarks = Object.freeze({
   SMART_BOOKMARKS_ANNO: "Places/SmartBookmark",
   DESCRIPTION_ANNO: "bookmarkProperties/description",
   SIDEBAR_ANNO: "bookmarkProperties/loadInSidebar",
   SYNC_PARENT_ANNO: "sync/parent",
   SYNC_MOBILE_ROOT_ANNO: "mobile/bookmarksRoot",
 
   KINDS: {
--- a/toolkit/components/places/tests/unit/test_sync_utils.js
+++ b/toolkit/components/places/tests/unit/test_sync_utils.js
@@ -166,16 +166,35 @@ var ignoreChangedRoots = Task.async(func
   if (!ObjectUtils.deepEqual(Object.keys(changes).sort(), expectedRoots)) {
     // Make sure the previous test cleaned up.
     throw new Error(`Unexpected changes at start of test: ${
       JSON.stringify(changes)}`);
   }
   yield setChangesSynced(changes);
 });
 
+add_task(function* test_fetchURLFrecency() {
+  // Add visits to the following URLs and then check if frecency for those URLs is not -1.
+  let arrayOfURLsToVisit = ["https://www.mozilla.org/en-US/", "http://getfirefox.com", "http://getthunderbird.com"];
+  for (let url of arrayOfURLsToVisit) {
+    yield PlacesTestUtils.addVisits(url);
+  }
+  for (let url of arrayOfURLsToVisit) {
+    let frecency = yield PlacesSyncUtils.history.fetchURLFrecency(url);
+    equal(typeof frecency, "number");
+    notEqual(frecency, -1);
+  }
+  // Do not add visits to the following URLs, and then check if frecency for those URLs is -1.
+  let arrayOfURLsNotVisited = ["https://bugzilla.org", "https://example.org"];
+  for (let url of arrayOfURLsNotVisited) {
+    let frecency = yield PlacesSyncUtils.history.fetchURLFrecency(url);
+    equal(frecency, -1);
+  }
+});
+
 add_task(function* test_order() {
   do_print("Insert some bookmarks");
   let guids = yield populateTree(PlacesUtils.bookmarks.menuGuid, {
     kind: "bookmark",
     title: "childBmk",
     url: "http://getfirefox.com",
   }, {
     kind: "bookmark",