Bug 648338 - Remove Firefox 3.5/3.6 compat code in the bookmarks engine. r=rnewman
authorPhilipp von Weitershausen <philipp@weitershausen.de>
Thu, 07 Apr 2011 15:30:31 -0700
changeset 67773 f73ae964d03329cb86fae0c12004c68e425f845c
parent 67772 6795af6d25c4cc39418fcdd8e236a0deba15517c
child 67774 20ab4d32a215b9112ebe239946942eefdaea2ee7
push id1
push userroot
push dateTue, 26 Apr 2011 22:38:44 +0000
treeherdermozilla-beta@bfdb6e623a36 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrnewman
bugs648338
milestone2.2a1pre
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 648338 - Remove Firefox 3.5/3.6 compat code in the bookmarks engine. r=rnewman
services/sync/modules/engines/bookmarks.js
services/sync/tests/unit/test_bookmark_store.js
services/sync/tests/unit/test_bookmark_tracker.js
--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -988,41 +988,22 @@ BookmarksStore.prototype = {
   _getStaticTitle: function BStore__getStaticTitle(id) {
     try {
       return Svc.Annos.getItemAnnotation(id, STATICTITLE_ANNO);
     } catch (e) {
       return "";
     }
   },
 
-  __childGUIDsStm: null,
   get _childGUIDsStm() {
-    if (this.__childGUIDsStm) {
-      return this.__childGUIDsStm;
-    }
-
-    let stmt;
-    if (this._haveGUIDColumn) {
-      stmt = this._getStmt(
-        "SELECT id AS item_id, guid " +
-        "FROM moz_bookmarks " +
-        "WHERE parent = :parent " +
-        "ORDER BY position");
-    } else {
-      stmt = this._getStmt(
-        "SELECT b.id AS item_id, " +
-          "(SELECT id FROM moz_anno_attributes WHERE name = '" + GUID_ANNO + "') AS name_id," +
-          "a.content AS guid " +
-        "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.parent = :parent " +
-        "ORDER BY b.position");
-    }
-    return this.__childGUIDsStm = stmt;
+    return this._getStmt(
+      "SELECT id AS item_id, guid " +
+      "FROM moz_bookmarks " +
+      "WHERE parent = :parent " +
+      "ORDER BY position");
   },
   _childGUIDsCols: ["item_id", "guid"],
 
   _getChildGUIDsForId: function _getChildGUIDsForId(itemid) {
     let stmt = this._childGUIDsStm;
     stmt.params.parent = itemid;
     let rows = Utils.queryAsync(stmt, this._childGUIDsCols);
     return rows.map(function (row) {
@@ -1140,167 +1121,53 @@ BookmarksStore.prototype = {
   },
 
   _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);
-  },
-
-  __haveGUIDColumn: null,
-  get _haveGUIDColumn() {
-    if (this.__haveGUIDColumn !== null) {
-      return this.__haveGUIDColumn;
-    }
-    let stmt;
-    try {
-      stmt = this._hsvc.DBConnection.createStatement(
-        "SELECT guid FROM moz_places");
-      stmt.finalize();
-      return this.__haveGUIDColumn = true;
-    } catch(ex) {
-      return this.__haveGUIDColumn = false;
-    }
+    return this._stmts[query] = this._hsvc.DBConnection
+                                    .createAsyncStatement(query);
   },
 
   get _frecencyStm() {
     return this._getStmt(
         "SELECT frecency " +
         "FROM moz_places " +
         "WHERE url = :url " +
         "LIMIT 1");
   },
   _frecencyCols: ["frecency"],
 
-  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() {
-    // Gecko <2.0 only
-    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;
-  },
-  _checkGUIDItemAnnotationCols: ["item_id", "name_id", "anno_id", "anno_date"],
-
-  get _addItemAnnotationStm() {
+  get _setGUIDStm() {
     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)");
-  },
-
-  __setGUIDStm: null,
-  get _setGUIDStm() {
-    if (this.__setGUIDStm !== null) {
-      return this.__setGUIDStm;
-    }
-
-    // Obtains a statement to set the guid iff the guid column exists.
-    let stmt;
-    if (this._haveGUIDColumn) {
-      stmt = this._getStmt(
-        "UPDATE moz_bookmarks " +
-        "SET guid = :guid " +
-        "WHERE id = :item_id");
-    } else {
-      stmt = false;
-    }
-    return this.__setGUIDStm = stmt;
+      "UPDATE moz_bookmarks " +
+      "SET guid = :guid " +
+      "WHERE id = :item_id");
   },
 
   // Some helper functions to handle GUIDs
   _setGUID: function _setGUID(id, guid) {
     if (!guid)
       guid = Utils.makeGUID();
 
-    // If we can, set the GUID on moz_bookmarks and do not do any other work.
-    let (stmt = this._setGUIDStm) {
-      if (stmt) {
-        stmt.params.guid = guid;
-        stmt.params.item_id = id;
-        Utils.queryAsync(stmt);
-        return guid;
-      }
-    }
-
-    // Ensure annotation name exists
-    Utils.queryAsync(this._addGUIDAnnotationNameStm);
-
-    let stmt = this._checkGUIDItemAnnotationStm;
+    let stmt = this._setGUIDStm;
+    stmt.params.guid = guid;
     stmt.params.item_id = id;
-    let result = Utils.queryAsync(stmt, this._checkGUIDItemAnnotationCols)[0];
-    if (!result) {
-      this._log.warn("Couldn't annotate bookmark id " + id);
-      return guid;
-    }
-
-    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;
   },
 
-  __guidForIdStm: null,
   get _guidForIdStm() {
-    if (this.__guidForIdStm) {
-      return this.__guidForIdStm;
-    }
-
-    // Try to first read from moz_bookmarks.  Creating the statement will
-    // fail, however, if the guid column does not exist.  We fallback to just
-    // reading the annotation table in this case.
-    let stmt;
-    if (this._haveGUIDColumn) {
-      stmt = this._getStmt(
-        "SELECT guid " +
-        "FROM moz_bookmarks " +
-        "WHERE id = :item_id");
-    } else {
-      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 = '" + GUID_ANNO + "' " +
-        "AND b.id = :item_id");
-    }
-
-    return this.__guidForIdStm = stmt;
+    return this._getStmt(
+      "SELECT guid " +
+      "FROM moz_bookmarks " +
+      "WHERE id = :item_id");
   },
   _guidForIdCols: ["guid"],
 
   GUIDForId: function GUIDForId(id) {
     let special = kSpecialIds.specialGUIDForId(id);
     if (special)
       return special;
 
@@ -1311,47 +1178,21 @@ BookmarksStore.prototype = {
     let result = Utils.queryAsync(stmt, this._guidForIdCols)[0];
     if (result && result.guid)
       return result.guid;
 
     // Give the uri a GUID if it doesn't have one
     return this._setGUID(id);
   },
 
-  __idForGUIDStm: null,
   get _idForGUIDStm() {
-    if (this.__idForGUIDStm) {
-      return this.__idForGUIDStm;
-    }
-
-
-    // Try to first read from moz_bookmarks.  Creating the statement will
-    // fail, however, if the guid column does not exist.  We fallback to just
-    // reading the annotation table in this case.
-    let stmt;
-    if (this._haveGUIDColumn) {
-      stmt = this._getStmt(
-        "SELECT id AS item_id " +
-        "FROM moz_bookmarks " +
-        "WHERE guid = :guid");
-    } else {
-      // Order results by lastModified so we can preserve the ID of the oldest bookmark.
-      // Copying a record preserves its dateAdded, and only modifying the
-      // bookmark alters its lastModified, so we also order by its item_id --
-      // lowest wins ties. Of course, Places can still screw us by reassigning IDs...
-      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 = '" + GUID_ANNO + "' " +
-        "AND a.content = :guid " +
-        "ORDER BY a.lastModified, a.item_id");
-    }
-
-    return this.__idForGUIDStm = stmt;
+    return this._getStmt(
+      "SELECT id AS item_id " +
+      "FROM moz_bookmarks " +
+      "WHERE guid = :guid");
   },
   _idForGUIDCols: ["item_id"],
 
   // noCreate is provided as an optional argument to prevent the creation of
   // non-existent special records, such as "mobile".
   idForGUID: function idForGUID(guid, noCreate) {
     if (kSpecialIds.isSpecialGUID(guid))
       return kSpecialIds.specialIdForGUID(guid, !noCreate);
@@ -1365,32 +1206,16 @@ BookmarksStore.prototype = {
                     + results.length);
     
     // Here's the one we care about: the first.
     let result = results[0];
     
     if (!result)
       return -1;
     
-    if (!this._haveGUIDColumn) {
-      try {
-        // Assign new GUIDs to any that came later.
-        for (let i = 1; i < results.length; ++i) {
-          let surplus = results[i];
-          this._log.debug("Assigning new GUID to copied row " + surplus.item_id);
-          this._setGUID(surplus.item_id);
-        }
-      } catch (ex) {
-        // Just skip it and carry on. This shouldn't happen, but if it does we
-        // don't want to fail hard.
-        this._log.debug("Got exception assigning new GUIDs: " +
-                        Utils.exceptionStr(ex));
-      }
-    }
-    
     return result.item_id;
   },
 
   _calculateIndex: function _calculateIndex(record) {
     // Ensure folders have a very high sort index so they're not synced last.
     if (record.type == "folder")
       return FOLDER_SORTINDEX;
 
--- a/services/sync/tests/unit/test_bookmark_store.js
+++ b/services/sync/tests/unit/test_bookmark_store.js
@@ -347,80 +347,21 @@ function test_reparentOrphans() {
                 folder1_guid);
 
   } finally {
     _("Clean up.");
     store.wipe();
   }
 }
 
-// Copying a bookmark in the UI also copies its annotations, including the GUID
-// annotation in 3.x.
-function test_copying_avoid_duplicate_guids() {
-  if (store._haveGUIDColumn) {
-    _("No GUID annotation handling functionality; returning without testing.");
-    return;
-  }
-    
-  try {
-    _("Ensure the record isn't present yet.");
-    let ids = Svc.Bookmark.getBookmarkIdsForURI(fxuri, {});
-    do_check_eq(ids.length, 0);
-
-    _("Let's create a new record.");
-    let fxrecord = new Bookmark("bookmarks", "get-firefox1");
-    fxrecord.bmkUri        = fxuri.spec;
-    fxrecord.description   = "Firefox is awesome.";
-    fxrecord.title         = "Get Firefox!";
-    fxrecord.tags          = ["firefox", "awesome", "browser"];
-    fxrecord.keyword       = "awesome";
-    fxrecord.loadInSidebar = false;
-    fxrecord.parentName    = "Bookmarks Toolbar";
-    fxrecord.parentid      = "toolbar";
-    store.applyIncoming(fxrecord);
-
-    _("Verify it has been created correctly.");
-    let id = store.idForGUID(fxrecord.id);
-    do_check_eq(store.GUIDForId(id), fxrecord.id);
-    do_check_true(Svc.Bookmark.getBookmarkURI(id).equals(fxuri));
-    do_check_eq(Svc.Annos.getItemAnnotation(id, "bookmarkProperties/description"),
-                fxrecord.description);
-    
-    _("Copy the record as happens in the UI: with the same GUID.");
-    let parentID = Svc.Bookmark.getFolderIdForItem(id);
-    let copy = Svc.Bookmark.insertBookmark(parentID, fxuri, -1, fxrecord.title);
-    Svc.Bookmark.setItemLastModified(copy, Svc.Bookmark.getItemLastModified(id));
-    Svc.Bookmark.setItemDateAdded(copy, Svc.Bookmark.getItemDateAdded(id));
-    store._setGUID(copy, fxrecord.id);
-    
-    do_check_eq(store.GUIDForId(copy), store.GUIDForId(id));
-    
-    _("Calling idForGUID fixes things.");
-    _("GUID before: " + store.GUIDForId(copy));
-    do_check_eq(store.idForGUID(fxrecord.id), id);           // Oldest wins.
-    _("GUID now: " + store.GUIDForId(copy));
-    do_check_neq(store.GUIDForId(copy), store.GUIDForId(id));
-    
-    _("Verify that the anno itself has changed.");
-    do_check_neq(Svc.Annos.getItemAnnotation(copy, "sync/guid"), fxrecord.id);
-    do_check_eq(Svc.Annos.getItemAnnotation(copy, "sync/guid"),
-                store.GUIDForId(copy));
-    
-  } finally {
-    _("Clean up.");
-    store.wipe();
-  }
-}
-
 function run_test() {
   initTestLogging('Trace');
   test_bookmark_create();
   test_bookmark_createRecord();
   test_bookmark_update();
   test_folder_create();
   test_folder_createRecord();
   test_deleted();
   test_move_folder();
   test_move_order();
   test_orphan();
   test_reparentOrphans();
-  test_copying_avoid_duplicate_guids();
 }
--- a/services/sync/tests/unit/test_bookmark_tracker.js
+++ b/services/sync/tests/unit/test_bookmark_tracker.js
@@ -243,77 +243,19 @@ function test_tracking() {
   } finally {
     _("Clean up.");
     store.wipe();
     tracker.clearChangedIDs();
     Svc.Obs.notify("weave:engine:stop-tracking");
   }
 }
 
-function test_guid_stripping() {
-  // Gecko <2.0
-  if (store._haveGUIDColumn) {
-    _("We have a GUID column; not testing anno GUID fixing.");
-    return;
-  }
-
-  _("Verify we've got an empty tracker to work with.");
-  let tracker = engine._tracker;
-  let folder = Svc.Bookmark.createFolder(Svc.Bookmark.bookmarksMenuFolder,
-                                         "Test Folder",
-                                         Svc.Bookmark.DEFAULT_INDEX);
-  function createBmk() {
-    return Svc.Bookmark.insertBookmark(folder,
-                                       Utils.makeURI("http://getfirefox.com"),
-                                       Svc.Bookmark.DEFAULT_INDEX,
-                                       "Get Firefox!");
-  }
-
-  do_check_eq([id for (id in tracker.changedIDs)].length, 0);
-
-  try {
-    _("Directly testing GUID stripping.");
-
-    Svc.Obs.notify("weave:engine:start-tracking");
-    tracker.ignoreAll = false;
-    let suspect = createBmk();
-    let victim = createBmk();
-
-    _("Suspect: " + suspect + ", victim: " + victim);
-
-    let suspectGUID = store.GUIDForId(suspect);
-    let victimGUID  = store.GUIDForId(victim);
-    _("Set the GUID on one entry to be the same as another.");
-    do_check_neq(suspectGUID, victimGUID);
-    Svc.Annos.setItemAnnotation(suspect, SYNC_GUID_ANNO, store.GUIDForId(victim),
-                                0, Svc.Annos.EXPIRE_NEVER);
-
-    _("Tracker changed it to something else.");
-    let newGUID = store.GUIDForId(suspect);
-    do_check_neq(newGUID, victimGUID);
-    do_check_neq(newGUID, suspectGUID);
-
-    _("Victim GUID remains unchanged.");
-    do_check_eq(victimGUID, store.GUIDForId(victim));
-
-    _("New GUID is in the tracker.");
-    _(JSON.stringify(tracker.changedIDs));
-    do_check_neq(tracker.changedIDs[newGUID], null);
-  } finally {
-    _("Clean up.");
-    store.wipe();
-    tracker.clearChangedIDs();
-    Svc.Obs.notify("weave:engine:stop-tracking");
-  }
-}
-
 function run_test() {
   initTestLogging("Trace");
 
   Log4Moz.repository.getLogger("Engine.Bookmarks").level = Log4Moz.Level.Trace;
   Log4Moz.repository.getLogger("Store.Bookmarks").level = Log4Moz.Level.Trace;
   Log4Moz.repository.getLogger("Tracker.Bookmarks").level = Log4Moz.Level.Trace;
 
   test_copying_places();
   test_tracking();
-  test_guid_stripping();
 }