Bug 1318414 - Default to empty strings for titles and parent titles if not set. r=tcsc,rnewman draft
authorKit Cambridge <kit@yakshaving.ninja>
Sat, 19 Nov 2016 08:29:26 -0800
changeset 441591 f179f4d9f723d710e0bed021d16e23100c5016d6
parent 441455 f09e137ead39230eaa94f47988ccce2cfcda4195
child 537586 aa20ff619e96590807fc505c974d3c4c1b9953e8
push id36459
push userbmo:kcambridge@mozilla.com
push dateSat, 19 Nov 2016 18:00:11 +0000
reviewerstcsc, rnewman
bugs1318414
milestone53.0a1
Bug 1318414 - Default to empty strings for titles and parent titles if not set. r=tcsc,rnewman MozReview-Commit-ID: DppxJuVrbAM
services/sync/modules/engines/bookmarks.js
services/sync/tests/unit/test_bookmark_store.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
@@ -342,33 +342,33 @@ BookmarksEngine.prototype = {
           let query = null;
           if (node.annos && node.uri.startsWith("place:")) {
             query = node.annos.find(({name}) =>
               name === PlacesSyncUtils.bookmarks.SMART_BOOKMARKS_ANNO);
           }
           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
           key = "s" + node.index;
           break;
         default:
           this._log.error("Unknown place type: '"+placeType+"'");
           continue;
       }
 
-      let parentName = parent.title;
+      let parentName = parent.title || "";
       if (guidMap[parentName] == null)
         guidMap[parentName] = {};
 
       // If the entry already exists, remember that there are explicit dupes.
       let entry = new String(guid);
       entry.hasDupe = guidMap[parentName][key] != null;
 
       // Remember this item's GUID for its parent-name/key pair.
@@ -386,42 +386,43 @@ BookmarksEngine.prototype = {
     let altKey;
     switch (item.type) {
       case "query":
         // Prior to Bug 610501, records didn't carry their Smart Bookmark
         // anno, so we won't be able to dupe them correctly. This altKey
         // 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;
         break;
       default:
         return;
     }
 
     // Figure out if we have a map to use!
     // This will throw in some circumstances. That's fine.
     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.");
       return undefined;
     }
 
     let dupe = parent[key];
 
--- a/services/sync/tests/unit/test_bookmark_store.js
+++ b/services/sync/tests/unit/test_bookmark_store.js
@@ -162,17 +162,17 @@ add_test(function test_bookmark_createRe
     _("Create a bookmark without a description or title.");
     let bmk1_id = PlacesUtils.bookmarks.insertBookmark(
       PlacesUtils.bookmarks.toolbarFolder, fxuri,
       PlacesUtils.bookmarks.DEFAULT_INDEX, null);
     let bmk1_guid = store.GUIDForId(bmk1_id);
 
     _("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);
 
   } finally {
     _("Clean up.");
     store.wipe();
     run_next_test();
   }
--- a/toolkit/components/places/PlacesSyncUtils.jsm
+++ b/toolkit/components/places/PlacesSyncUtils.jsm
@@ -274,17 +274,19 @@ const BookmarkSyncUtils = PlacesSyncUtil
   fetch: Task.async(function* (syncId) {
     let guid = BookmarkSyncUtils.syncIdToGuid(syncId);
     let bookmarkItem = yield PlacesUtils.bookmarks.fetch(guid);
     if (!bookmarkItem) {
       return null;
     }
 
     // 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) {
       case BookmarkSyncUtils.KINDS.BOOKMARK:
       case BookmarkSyncUtils.KINDS.MICROSUMMARY:
         item = yield fetchBookmarkItem(bookmarkItem);
         break;
 
@@ -304,22 +306,21 @@ const BookmarkSyncUtils = PlacesSyncUtil
         item = yield placesBookmarkToSyncBookmark(bookmarkItem);
         item.index = bookmarkItem.index;
         break;
 
       default:
         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;
   }),
 
   /**
    * Get the sync record kind for the record with provided sync id.
    *
@@ -1035,16 +1036,20 @@ function syncBookmarkToPlacesBookmark(in
   return bookmarkInfo;
 }
 
 // Creates and returns a Sync bookmark object containing the bookmark's
 // tags, keyword, description, and whether it loads in the sidebar.
 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), {});
 
   let keywordEntry = yield PlacesUtils.keywords.fetch({
     url: bookmarkItem.url,
   });
   if (keywordEntry) {
     item.keyword = keywordEntry.keyword;
@@ -1062,16 +1067,20 @@ var fetchBookmarkItem = Task.async(funct
   return item;
 });
 
 // Creates and returns a Sync bookmark object containing the folder's
 // description and children.
 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) {
     item.description = description;
   }
 
   let db = yield PlacesUtils.promiseDBConnection();
   let children = yield fetchAllChildren(db, bookmarkItem.guid);
@@ -1082,16 +1091,20 @@ var fetchFolderItem = Task.async(functio
   return item;
 });
 
 // Creates and returns a Sync bookmark object containing the livemark's
 // description, children (none), feed URI, and site URI.
 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) {
     item.description = description;
   }
 
   let feedAnno = yield getAnno(bookmarkItem.guid, PlacesUtils.LMANNO_FEEDURI);
   item.feed = new URL(feedAnno);
--- a/toolkit/components/places/tests/unit/test_sync_utils.js
+++ b/toolkit/components/places/tests/unit/test_sync_utils.js
@@ -1053,43 +1053,47 @@ add_task(function* test_fetch() {
     let item = yield PlacesSyncUtils.bookmarks.fetch(folder.syncId);
     deepEqual(item, {
       syncId: folder.syncId,
       kind: "folder",
       parentSyncId: "menu",
       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");
     equal(item.parentSyncId, "menu", "Should return parent sync ID");
     deepEqual(item.tags, ["taggy"], "Should return tags");
     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");
   {
     let item = yield PlacesSyncUtils.bookmarks.fetch(folderSep.syncId);
     strictEqual(item.index, 1, "Should return separator position");
   }
 
@@ -1127,19 +1131,20 @@ add_task(function* test_fetch_livemark()
       index: PlacesUtils.bookmarks.DEFAULT_INDEX,
     });
     PlacesUtils.annotations.setItemAnnotation(livemark.id, DESCRIPTION_ANNO,
       "Livemark description", 0, PlacesUtils.annotations.EXPIRE_NEVER);
 
     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();
   }
 
   yield PlacesUtils.bookmarks.eraseEverything();
 });