Bug 1336282 - Moved bookmark frecency query into `PlacesSyncUtils`. r?kitcambridge
MozReview-Commit-ID: 4DDXOUQ2C8N
--- 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",