Bug 615410 - Have bookmarks generate new-style GUIDs. r=mconnor

services/sync/modules/engines/bookmarks.js

@@ -21,6 +21,7 @@
  *  Dan Mills <thunder@mozilla.com>
  *  Jono DiCarlo <jdicarlo@mozilla.org>
  *  Anant Narayanan <anant@kix.in>
+ *  Philipp von Weitershausen <philipp@weitershausen.de>
  *
  * Alternatively, the contents of this file may be used under the terms of
  * either the GNU General Public License Version 2 or later (the "GPL"), or
@@ -42,6 +43,7 @@ const Cc = Components.classes;
 const Ci = Components.interfaces;
 const Cu = Components.utils;
 
+const GUID_ANNO = "sync/guid";
 const PARENT_ANNO = "weave/parent";
 const PREDECESSOR_ANNO = "weave/predecessor";
 const SERVICE_NOT_SUPPORTED = "Service not supported on this platform";
@@ -91,19 +93,6 @@ Utils.lazy2(kSpecialIds, "mobile", function() {
   return mobile;
 });
 
-// Create some helper functions to convert GUID/ids
-function idForGUID(guid) {
-  if (guid in kSpecialIds)
-    return kSpecialIds[guid];
-  return Svc.Bookmark.getItemIdForGUID(guid);
-}
-function GUIDForId(placeId) {
-  for (let [guid, id] in Iterator(kSpecialIds))
-    if (placeId == id)
-      return guid;
-  return Svc.Bookmark.getItemGUID(placeId);
-}
-
 function BookmarksEngine() {
   SyncEngine.call(this, "Bookmarks");
   this._handleImport();
@@ -113,6 +102,7 @@ BookmarksEngine.prototype = {
   _recordObj: PlacesItem,
   _storeObj: BookmarksStore,
   _trackerObj: BookmarksTracker,
+  version: 2,
 
   _handleImport: function _handleImport() {
     Svc.Obs.add("bookmarks-restore-begin", function() {
@@ -151,7 +141,7 @@ BookmarksEngine.prototype = {
       for (let guid in this._store.getAllIDs()) {
         // Figure out what key to store the mapping
         let key;
-        let id = idForGUID(guid);
+        let id = this._store.idForGUID(guid);
         switch (Svc.Bookmark.getItemType(id)) {
           case Svc.Bookmark.TYPE_BOOKMARK:
             key = "b" + Svc.Bookmark.getBookmarkURI(id).spec + ":" +
@@ -257,8 +247,8 @@ function BookmarksStore(name) {
     this.__ls = null;
     this.__ms = null;
     this.__ts = null;
-    if (this.__frecencyStm)
+    for each ([query, stmt] in Iterator(this._stmts))
-      this.__frecencyStm.finalize();
+      stmt.finalize();
   }, this);
 }
 BookmarksStore.prototype = {
@@ -276,7 +266,8 @@ BookmarksStore.prototype = {
   get _hsvc() {
     if (!this.__hsvc)
       this.__hsvc = Cc["@mozilla.org/browser/nav-history-service;1"].
-                    getService(Ci.nsINavHistoryService);
+                    getService(Ci.nsINavHistoryService).
+                    QueryInterface(Ci.nsPIPlacesDatabase);
     return this.__hsvc;
   },
 
@@ -314,7 +305,7 @@ BookmarksStore.prototype = {
 
 
   itemExists: function BStore_itemExists(id) {
-    return idForGUID(id) > 0;
+    return this.idForGUID(id) > 0;
   },
 
   // Hash of old GUIDs to the new renamed GUIDs
@@ -370,7 +361,7 @@ BookmarksStore.prototype = {
     let parentGUID = record.parentid;
     record._orphan = false;
     if (parentGUID != null) {
-      let parentId = idForGUID(parentGUID);
+      let parentId = this.idForGUID(parentGUID);
 
       // Default to unfiled if we don't have the parent yet
       if (parentId <= 0) {
@@ -392,7 +383,7 @@ BookmarksStore.prototype = {
         record._insertPos = 0;
       else {
         // The insert position is one after the predecessor of the same parent
-        let predId = idForGUID(predGUID);
+        let predId = this.idForGUID(predGUID);
         if (predId != -1 && this._getParentGUIDForId(predId) == parentGUID) {
           record._insertPos = Svc.Bookmark.getItemIndex(predId) + 1;
           record._predId = predId;
@@ -406,7 +397,7 @@ BookmarksStore.prototype = {
     Store.prototype.applyIncoming.apply(this, arguments);
 
     // Do some post-processing if we have an item
-    let itemId = idForGUID(record.id);
+    let itemId = this.idForGUID(record.id);
     if (itemId > 0) {
       // Move any children that are looking for this folder as a parent
       if (record.type == "folder")
@@ -447,7 +438,7 @@ BookmarksStore.prototype = {
    */
   _reparentOrphans: function _reparentOrphans(parentId) {
     // Find orphans and reunite with this folder parent
-    let parentGUID = GUIDForId(parentId);
+    let parentGUID = this.GUIDForId(parentId);
     let orphans = this._findAnnoItems(PARENT_ANNO, parentGUID);
 
     this._log.debug("Reparenting orphans " + orphans + " to " + parentId);
@@ -496,7 +487,7 @@ BookmarksStore.prototype = {
    * For the provided predecessor item, attach its followers to it
    */
   _attachFollowers: function BStore__attachFollowers(predId) {
-    let predGUID = GUIDForId(predId);
+    let predGUID = this.GUIDForId(predId);
     let followers = this._findAnnoItems(PREDECESSOR_ANNO, predGUID);
     if (followers.length > 1)
       this._log.warn(predId + " has more than one followers: " + followers);
@@ -592,7 +583,7 @@ BookmarksStore.prototype = {
   },
 
   remove: function BStore_remove(record) {
-    let itemId = idForGUID(record.id);
+    let itemId = this.idForGUID(record.id);
     if (itemId <= 0) {
       this._log.debug("Item " + record.id + " already removed");
       return;
@@ -620,7 +611,7 @@ BookmarksStore.prototype = {
   },
 
   update: function BStore_update(record) {
-    let itemId = idForGUID(record.id);
+    let itemId = this.idForGUID(record.id);
 
     if (itemId <= 0) {
       this._log.debug("Skipping update for unknown item: " + record.id);
@@ -703,7 +694,7 @@ BookmarksStore.prototype = {
     }, this);
 
     // Make sure there's an item to change GUIDs
-    let itemId = idForGUID(oldID);
+    let itemId = this.idForGUID(oldID);
     if (itemId <= 0)
       return;
 
@@ -711,15 +702,6 @@ BookmarksStore.prototype = {
     this._setGUID(itemId, newID);
   },
 
-  _setGUID: function BStore__setGUID(itemId, guid) {
-    let collision = idForGUID(guid);
-    if (collision != -1) {
-      this._log.warn("Freeing up GUID " + guid  + " used by " + collision);
-      Svc.Annos.removeItemAnnotation(collision, "placesInternal/GUID");
-    }
-    Svc.Bookmark.setItemGUID(itemId, guid);
-  },
-
   _getNode: function BStore__getNode(folder) {
     let query = this._hsvc.getNewQuery();
     query.setFolders([folder], 1);
@@ -758,7 +740,7 @@ BookmarksStore.prototype = {
 
   // Create a record starting from the weave id (places guid)
   createRecord: function createRecord(id, collection) {
-    let placeId = idForGUID(id);
+    let placeId = this.idForGUID(id);
     let record;
     if (placeId <= 0) { // deleted item
       record = new PlacesItem(collection, id);
@@ -848,18 +830,141 @@ BookmarksStore.prototype = {
     return record;
   },
 
-  __frecencyStm: null,
+  _stmts: {},
+  _getStmt: function(query) {
+    if (query in this._stmts)
+      return this._stmts[query];
+
+    this._log.trace("Creating SQL statement: " + query);
+    return this._stmts[query] = Utils.createStatement(this._hsvc.DBConnection,
+                                                      query);
+  },
+
   get _frecencyStm() {
-    if (!this.__frecencyStm) {
+    return this._getStmt(
-      this._log.trace("Creating SQL statement: _frecencyStm");
-      this.__frecencyStm = Utils.createStatement(
-        Svc.History.DBConnection,
         "SELECT frecency " +
         "FROM moz_places " +
         "WHERE url = :url " +
         "LIMIT 1");
+  },
+
+  get _addGUIDAnnotationNameStm() {
+    let stmt = this._getStmt(
+      "INSERT OR IGNORE INTO moz_anno_attributes (name) VALUES (:anno_name)");
+    stmt.params.anno_name = GUID_ANNO;
+    return stmt;
+  },
+
+  get _checkGUIDItemAnnotationStm() {
+    let stmt = this._getStmt(
+      "SELECT b.id AS item_id, " +
+        "(SELECT id FROM moz_anno_attributes WHERE name = :anno_name) AS name_id, " +
+        "a.id AS anno_id, a.dateAdded AS anno_date " +
+      "FROM moz_bookmarks b " +
+      "LEFT JOIN moz_items_annos a ON a.item_id = b.id " +
+                                 "AND a.anno_attribute_id = name_id " +
+      "WHERE b.id = :item_id");
+    stmt.params.anno_name = GUID_ANNO;
+    return stmt;
+  },
+
+  get _addItemAnnotationStm() {
+    return this._getStmt(
+    "INSERT OR REPLACE INTO moz_items_annos " +
+      "(id, item_id, anno_attribute_id, mime_type, content, flags, " +
+       "expiration, type, dateAdded, lastModified) " +
+    "VALUES (:id, :item_id, :name_id, :mime_type, :content, :flags, " +
+            ":expiration, :type, :date_added, :last_modified)");
+  },
+
+  // Some helper functions to handle GUIDs
+  _setGUID: function _setGUID(id, guid) {
+    if (arguments.length == 1)
+      guid = Utils.makeGUID();
+
+    // Ensure annotation name exists
+    Utils.queryAsync(this._addGUIDAnnotationNameStm);
+
+    let stmt = this._checkGUIDItemAnnotationStm;
+    stmt.params.item_id = id;
+    let result = Utils.queryAsync(stmt, ["item_id", "name_id", "anno_id",
+                                         "anno_date"])[0];
+    if (!result) {
+      let log = Log4Moz.repository.getLogger("Engine.Bookmarks");
+      log.warn("Couldn't annotate bookmark id " + id);
+      return guid;
     }
-    return this.__frecencyStm;
+
+    stmt = this._addItemAnnotationStm;
+    if (result.anno_id) {
+      stmt.params.id = result.anno_id;
+      stmt.params.date_added = result.anno_date;
+    } else {
+      stmt.params.id = null;
+      stmt.params.date_added = Date.now() * 1000;
+    }
+    stmt.params.item_id = result.item_id;
+    stmt.params.name_id = result.name_id;
+    stmt.params.content = guid;
+    stmt.params.flags = 0;
+    stmt.params.expiration = Ci.nsIAnnotationService.EXPIRE_NEVER;
+    stmt.params.type = Ci.nsIAnnotationService.TYPE_STRING;
+    stmt.params.last_modified = Date.now() * 1000;
+    Utils.queryAsync(stmt);
+
+    return guid;
+  },
+
+  get _guidForIdStm() {
+    let stmt = this._getStmt(
+      "SELECT a.content AS guid " +
+      "FROM moz_items_annos a " +
+      "JOIN moz_anno_attributes n ON n.id = a.anno_attribute_id " +
+      "JOIN moz_bookmarks b ON b.id = a.item_id " +
+      "WHERE n.name = :anno_name AND b.id = :item_id");
+    stmt.params.anno_name = GUID_ANNO;
+    return stmt;
+  },
+
+  GUIDForId: function GUIDForId(id) {
+    for (let [guid, specialId] in Iterator(kSpecialIds))
+      if (id == specialId)
+         return guid;
+
+    let stmt = this._guidForIdStm;
+    stmt.params.item_id = id;
+
+    // Use the existing GUID if it exists
+    let result = Utils.queryAsync(stmt, ["guid"])[0];
+    if (result)
+      return result.guid;
+
+    // Give the uri a GUID if it doesn't have one
+    return this._setGUID(id);
+  },
+
+  get _idForGUIDStm() {
+    let stmt = this._getStmt(
+      "SELECT a.item_id AS item_id " +
+      "FROM moz_items_annos a " +
+      "JOIN moz_anno_attributes n ON n.id = a.anno_attribute_id " +
+      "WHERE n.name = :anno_name AND a.content = :guid");
+    stmt.params.anno_name = GUID_ANNO;
+    return stmt;
+  },
+
+  idForGUID: function idForGUID(guid) {
+    if (guid in kSpecialIds)
+      return kSpecialIds[guid];
+
+    let stmt = this._idForGUIDStm;
+    // guid might be a String object rather than a string.
+    stmt.params.guid = guid.toString();
+
+    let result = Utils.queryAsync(stmt, ["item_id"])[0];
+    if (result)
+      return result.item_id;
+    return -1;
   },
 
   _calculateIndex: function _calculateIndex(record) {
@@ -898,7 +1003,7 @@ BookmarksStore.prototype = {
       parentid = this._bms.unfiledBookmarksFolder;
       this._bms.moveItem(itemId, parentid, -1);
     }
-    return GUIDForId(parentid);
+    return this.GUIDForId(parentid);
   },
 
   _getPredecessorGUIDForId: function BStore__getPredecessorGUIDForId(itemId) {
@@ -942,13 +1047,13 @@ BookmarksStore.prototype = {
         return;
     }
 
-    return GUIDForId(predecessorId);
+    return this.GUIDForId(predecessorId);
   },
 
   _getChildren: function BStore_getChildren(guid, items) {
     let node = guid; // the recursion case
     if (typeof(node) == "string") // callers will give us the guid as the first arg
-      node = this._getNode(idForGUID(guid));
+      node = this._getNode(this.idForGUID(guid));
 
     if (node.type == node.RESULT_TYPE_FOLDER &&
         !this._ls.isLivemark(node.itemId)) {
@@ -958,7 +1063,7 @@ BookmarksStore.prototype = {
       // Remember all the children GUIDs and recursively get more
       for (var i = 0; i < node.childCount; i++) {
         let child = node.getChild(i);
-        items[GUIDForId(child.itemId)] = true;
+        items[this.GUIDForId(child.itemId)] = true;
         this._getChildren(child, items);
       }
     }
@@ -1056,6 +1161,11 @@ BookmarksTracker.prototype = {
     Ci.nsISupportsWeakReference
   ]),
 
+  _GUIDForId: function _GUIDForId(item_id) {
+    // Isn't indirection fun...
+    return Engines.get("bookmarks")._store.GUIDForId(item_id);
+  },
+
   /**
    * Add a bookmark (places) id to be uploaded and bump up the sync score
    *
@@ -1063,7 +1173,7 @@ BookmarksTracker.prototype = {
    *        Places internal id of the bookmark to upload
    */
   _addId: function BMT__addId(itemId) {
-    if (this.addChangedID(GUIDForId(itemId)))
+    if (this.addChangedID(this._GUIDForId(itemId, true)))
       this._upScore();
   },
 
@@ -1102,7 +1212,7 @@ BookmarksTracker.prototype = {
 
     // Make sure to remove items that have the exclude annotation
     if (Svc.Annos.itemHasAnnotation(itemId, "places/excludeFromBackup")) {
-      this.removeChangedID(GUIDForId(itemId));
+      this.removeChangedID(this._GUIDForId(itemId, true));
       return true;
     }
 

services/sync/tests/unit/test_bookmark_predecessor.js

@@ -3,8 +3,6 @@ Cu.import("resource://services-sync/engines/bookmarks.js");
 Cu.import("resource://services-sync/util.js");
 
 function run_test() {
-  let baseuri = "http://fake/uri/";
-
   _("Starting with a clean slate of no bookmarks");
   let store = new (new BookmarksEngine())._storeObj();
   store.wipe();
@@ -14,7 +12,7 @@ function run_test() {
     folder = folder || Svc.Bookmark.toolbarFolder;
     let name = "pos" + pos;
     let bmk = Svc.Bookmark.insertBookmark(folder, uri, pos, name);
-    Svc.Bookmark.setItemGUID(bmk, name);
+    store._setGUID(bmk, name);
     return bmk;
   }
 

services/sync/tests/unit/test_bookmark_store.js

@@ -13,8 +13,7 @@ function test_bookmark_create() {
     do_check_eq(ids.length, 0);
 
     _("Let's create a new record.");
-    let fxrecord = new Bookmark("bookmarks",
+    let fxrecord = new Bookmark("bookmarks", "get-firefox1");
-                                "{5d81b87c-d5fc-42d9-a114-d69b7342f10e}0");
     fxrecord.bmkUri        = fxuri.spec;
     fxrecord.description   = "Firefox is awesome.";
     fxrecord.title         = "Get Firefox!";
@@ -26,8 +25,8 @@ function test_bookmark_create() {
     store.applyIncoming(fxrecord);
 
     _("Verify it has been created correctly.");
-    let id = Svc.Bookmark.getItemIdForGUID(fxrecord.id);
+    let id = store.idForGUID(fxrecord.id);
-    do_check_eq(Svc.Bookmark.getItemGUID(id), fxrecord.id);
+    do_check_eq(store.GUIDForId(id), fxrecord.id);
     do_check_eq(Svc.Bookmark.getItemType(id), Svc.Bookmark.TYPE_BOOKMARK);
     do_check_eq(Svc.Bookmark.getItemTitle(id), fxrecord.title);
     do_check_eq(Svc.Bookmark.getFolderIdForItem(id),
@@ -54,15 +53,14 @@ function test_bookmark_create() {
 function test_folder_create() {
   try {
     _("Create a folder.");
-    let folder = new BookmarkFolder("bookmarks",
+    let folder = new BookmarkFolder("bookmarks", "testfolder-1");
-                                    "{5d81b87c-d5fc-42d9-a114-d69b7342f10e}0");
     folder.parentName = "Bookmarks Toolbar";
     folder.parentid   = "toolbar";
     folder.title      = "Test Folder";
     store.applyIncoming(folder);
 
     _("Verify it has been created correctly.");
-    let id = Svc.Bookmark.getItemIdForGUID(folder.id);
+    let id = store.idForGUID(folder.id);
     do_check_eq(Svc.Bookmark.getItemType(id), Svc.Bookmark.TYPE_FOLDER);
     do_check_eq(Svc.Bookmark.getItemTitle(id), folder.title);
     do_check_eq(Svc.Bookmark.getFolderIdForItem(id),
@@ -87,13 +85,13 @@ function test_move_folder() {
     _("Create two folders and a bookmark in one of them.");
     let folder1_id = Svc.Bookmark.createFolder(
       Svc.Bookmark.toolbarFolder, "Folder1", 0);
-    let folder1_guid = Svc.Bookmark.getItemGUID(folder1_id);
+    let folder1_guid = store.GUIDForId(folder1_id);
     let folder2_id = Svc.Bookmark.createFolder(
       Svc.Bookmark.toolbarFolder, "Folder2", 0);
-    let folder2_guid = Svc.Bookmark.getItemGUID(folder2_id);
+    let folder2_guid = store.GUIDForId(folder2_id);
     let bmk_id = Svc.Bookmark.insertBookmark(
       folder1_id, fxuri, Svc.Bookmark.DEFAULT_INDEX, "Get Firefox!");
-    let bmk_guid = Svc.Bookmark.getItemGUID(bmk_id);
+    let bmk_guid = store.GUIDForId(bmk_id);
 
     _("Get a record, reparent it and apply it to the store.");
     let record = store.createRecord(bmk_guid);
@@ -104,7 +102,7 @@ function test_move_folder() {
 
     _("Verify the new parent.");
     let new_folder_id = Svc.Bookmark.getFolderIdForItem(bmk_id);
-    do_check_eq(Svc.Bookmark.getItemGUID(new_folder_id), folder2_guid);
+    do_check_eq(store.GUIDForId(new_folder_id), folder2_guid);
   } finally {
     _("Clean up.");
     store.wipe();
@@ -117,11 +115,11 @@ function test_move_order() {
     let bmk1_id = Svc.Bookmark.insertBookmark(
       Svc.Bookmark.toolbarFolder, fxuri, Svc.Bookmark.DEFAULT_INDEX,
       "Get Firefox!");
-    let bmk1_guid = Svc.Bookmark.getItemGUID(bmk1_id);
+    let bmk1_guid = store.GUIDForId(bmk1_id);
     let bmk2_id = Svc.Bookmark.insertBookmark(
       Svc.Bookmark.toolbarFolder, tburi, Svc.Bookmark.DEFAULT_INDEX,
       "Get Thunderbird!");
-    let bmk2_guid = Svc.Bookmark.getItemGUID(bmk2_id);
+    let bmk2_guid = store.GUIDForId(bmk2_id);
 
     _("Verify order.");
     do_check_eq(Svc.Bookmark.getItemIndex(bmk1_id), 0);

services/sync/tests/unit/test_bookmark_tracker.js

@@ -1,8 +1,10 @@
+Cu.import("resource://services-sync/engines.js");
 Cu.import("resource://services-sync/engines/bookmarks.js");
 Cu.import("resource://services-sync/util.js");
 
 function run_test() {
-  let engine = new BookmarksEngine();
+  Engines.register(BookmarksEngine);
+  let engine = Engines.get("bookmarks");
   engine._store.wipe();
 
   _("Verify we've got an empty tracker to work with.");