Bug 1318414 - Default to empty strings for titles and parent titles if not set. r=rnewman,tcsc

MozReview-Commit-ID: DppxJuVrbAM

--HG--
extra : rebase_source : a214d54fda7d25798039fc9844c34cabd3165a27

services/sync/modules/engines/bookmarks.js

@@ -349,12 +349,12 @@ BookmarksEngine.prototype = {
           if (query && query.value) {
             key = "q" + query.value;
           } else {
-            key = "b" + node.uri + ":" + node.title;
+            key = "b" + node.uri + ":" + (node.title || "");
           }
           break;
         case PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER:
           // Folder
-          key = "f" + node.title;
+          key = "f" + (node.title || "");
           break;
         case PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR:
           // Separator
@@ -365,7 +365,7 @@ BookmarksEngine.prototype = {
           continue;
       }
 
-      let parentName = parent.title;
+      let parentName = parent.title || "";
       if (guidMap[parentName] == null)
         guidMap[parentName] = {};
 
@@ -393,17 +393,17 @@ BookmarksEngine.prototype = {
         // hack should get them to dupe correctly.
         if (item.queryId) {
           key = "q" + item.queryId;
-          altKey = "b" + item.bmkUri + ":" + item.title;
+          altKey = "b" + item.bmkUri + ":" + (item.title || "");
           break;
         }
         // No queryID? Fall through to the regular bookmark case.
       case "bookmark":
       case "microsummary":
-        key = "b" + item.bmkUri + ":" + item.title;
+        key = "b" + item.bmkUri + ":" + (item.title || "");
         break;
       case "folder":
       case "livemark":
-        key = "f" + item.title;
+        key = "f" + (item.title || "");
         break;
       case "separator":
         key = "s" + item.pos;
@@ -417,8 +417,9 @@ BookmarksEngine.prototype = {
     let guidMap = this._guidMap;
 
     // Give the GUID if we have the matching pair.
-    this._log.trace("Finding mapping: " + item.parentName + ", " + key);
-    let parent = guidMap[item.parentName];
+    let parentName = item.parentName || "";
+    this._log.trace("Finding mapping: " + parentName + ", " + key);
+    let parent = guidMap[parentName];
 
     if (!parent) {
       this._log.trace("No parent => no dupe.");

services/sync/tests/unit/test_bookmark_store.js

@@ -167,7 +167,7 @@ add_test(function test_bookmark_createRecord() {
 
     _("Verify that the record is created accordingly.");
     let record = store.createRecord(bmk1_guid);
-    do_check_eq(record.title, null);
+    do_check_eq(record.title, "");
     do_check_eq(record.description, null);
     do_check_eq(record.keyword, null);
 

toolkit/components/places/PlacesSyncUtils.jsm

@@ -601,7 +601,9 @@ const BookmarkSyncUtils = PlacesSyncUtils.bookmarks = Object.freeze({
     }
 
     // Convert the Places bookmark object to a Sync bookmark and add
-    // kind-specific properties.
+    // kind-specific properties. Titles are required for bookmarks,
+    // folders, and livemarks; optional for queries, and omitted for
+    // separators.
     let kind = yield getKindForItem(bookmarkItem);
     let item;
     switch (kind) {
@@ -631,12 +633,11 @@ const BookmarkSyncUtils = PlacesSyncUtils.bookmarks = Object.freeze({
         throw new Error(`Unknown bookmark kind: ${kind}`);
     }
 
-    // Sync uses the parent title for de-duping.
+    // Sync uses the parent title for de-duping. All Sync bookmark objects
+    // except the Places root should have this property.
     if (bookmarkItem.parentGuid) {
       let parent = yield PlacesUtils.bookmarks.fetch(bookmarkItem.parentGuid);
-      if ("title" in parent) {
-        item.parentTitle = parent.title;
-      }
+      item.parentTitle = parent.title || "";
     }
 
     return item;
@@ -1418,6 +1419,10 @@ function syncBookmarkToPlacesBookmark(info) {
 var fetchBookmarkItem = Task.async(function* (bookmarkItem) {
   let item = yield placesBookmarkToSyncBookmark(bookmarkItem);
 
+  if (!item.title) {
+    item.title = "";
+  }
+
   item.tags = PlacesUtils.tagging.getTagsForURI(
     PlacesUtils.toURI(bookmarkItem.url), {});
 
@@ -1445,6 +1450,10 @@ var fetchBookmarkItem = Task.async(function* (bookmarkItem) {
 var fetchFolderItem = Task.async(function* (bookmarkItem) {
   let item = yield placesBookmarkToSyncBookmark(bookmarkItem);
 
+  if (!item.title) {
+    item.title = "";
+  }
+
   let description = yield getAnno(bookmarkItem.guid,
                                   BookmarkSyncUtils.DESCRIPTION_ANNO);
   if (description) {
@@ -1465,6 +1474,10 @@ var fetchFolderItem = Task.async(function* (bookmarkItem) {
 var fetchLivemarkItem = Task.async(function* (bookmarkItem) {
   let item = yield placesBookmarkToSyncBookmark(bookmarkItem);
 
+  if (!item.title) {
+    item.title = "";
+  }
+
   let description = yield getAnno(bookmarkItem.guid,
                                   BookmarkSyncUtils.DESCRIPTION_ANNO);
   if (description) {

toolkit/components/places/tests/unit/test_sync_utils.js

@@ -1543,14 +1543,15 @@ add_task(function* test_fetch() {
       description: "Folder description",
       childSyncIds: [folderBmk.syncId, folderSep.syncId],
       parentTitle: "Bookmarks Menu",
-    }, "Should include description, children, and parent title in folder");
+      title: "",
+    }, "Should include description, children, title, and parent title in folder");
   }
 
   do_print("Fetch bookmark with description, sidebar anno, and tags");
   {
     let item = yield PlacesSyncUtils.bookmarks.fetch(bmk.syncId);
-    deepEqual(Object.keys(item).sort(), ["syncId", "kind", "parentSyncId", "url",
-      "tags", "description", "loadInSidebar", "parentTitle"].sort(),
+    deepEqual(Object.keys(item).sort(), ["syncId", "kind", "parentSyncId",
+      "url", "tags", "description", "loadInSidebar", "parentTitle", "title"].sort(),
       "Should include bookmark-specific properties");
     equal(item.syncId, bmk.syncId, "Sync ID should match");
     equal(item.url.href, "https://example.com/", "Should return URL");
@@ -1559,17 +1560,20 @@ add_task(function* test_fetch() {
     equal(item.description, "Bookmark description", "Should return bookmark description");
     strictEqual(item.loadInSidebar, true, "Should return sidebar anno");
     equal(item.parentTitle, "Bookmarks Menu", "Should return parent title");
+    strictEqual(item.title, "", "Should return empty title");
   }
 
   do_print("Fetch bookmark with keyword; without parent title or annos");
   {
     let item = yield PlacesSyncUtils.bookmarks.fetch(folderBmk.syncId);
     deepEqual(Object.keys(item).sort(), ["syncId", "kind", "parentSyncId",
-      "url", "keyword", "tags", "loadInSidebar"].sort(),
+      "url", "keyword", "tags", "loadInSidebar", "parentTitle", "title"].sort(),
       "Should omit blank bookmark-specific properties");
     strictEqual(item.loadInSidebar, false, "Should not load bookmark in sidebar");
     deepEqual(item.tags, [], "Tags should be empty");
     equal(item.keyword, "kw", "Should return keyword");
+    strictEqual(item.parentTitle, "", "Should include parent title even if empty");
+    strictEqual(item.title, "", "Should include bookmark title even if empty");
   }
 
   do_print("Fetch separator");
@@ -1618,11 +1622,12 @@ add_task(function* test_fetch_livemark() {
     do_print("Fetch livemark");
     let item = yield PlacesSyncUtils.bookmarks.fetch(livemark.guid);
     deepEqual(Object.keys(item).sort(), ["syncId", "kind", "parentSyncId",
-      "description", "feed", "site", "parentTitle"].sort(),
+      "description", "feed", "site", "parentTitle", "title"].sort(),
       "Should include livemark-specific properties");
     equal(item.description, "Livemark description", "Should return description");
     equal(item.feed.href, site + "/feed/1", "Should return feed URL");
     equal(item.site.href, site + "/", "Should return site URL");
+    strictEqual(item.title, "", "Should include livemark title even if empty");
   } finally {
     yield stopServer();
   }