bug 970291 - Remove BookmarkJSONUtils.serializeNodeAsJSONToOutputStream. r=mano
authorMarco Bonardo <mbonardo@mozilla.com>
Mon, 14 Apr 2014 12:00:11 +0200
changeset 196849 9e92a6a293fa1738f2de7a3d16be301e75d94edc
parent 196812 4b27cfbf2f3b600ba67ad4f04aed86cf9bd8d060
child 196850 e2aff2cd9a9a1fd27390158da12f9b59a87a87e5
push id3624
push userasasaki@mozilla.com
push dateMon, 09 Jun 2014 21:49:01 +0000
treeherdermozilla-beta@b1a5da15899a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmano
bugs970291
milestone31.0a1
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 970291 - Remove BookmarkJSONUtils.serializeNodeAsJSONToOutputStream. r=mano
browser/components/places/src/PlacesUIUtils.jsm
browser/components/places/tests/unit/test_leftpane_corruption_handling.js
toolkit/components/places/BookmarkJSONUtils.jsm
--- a/browser/components/places/src/PlacesUIUtils.jsm
+++ b/browser/components/places/src/PlacesUIUtils.jsm
@@ -799,37 +799,40 @@ this.PlacesUIUtils = {
       // Build the leftPaneQueries Map.  This is used to quickly access them,
       // associating a mnemonic name to the real item ids.
       delete this.leftPaneQueries;
       this.leftPaneQueries = {};
 
       let items = as.getItemsWithAnnotation(this.ORGANIZER_QUERY_ANNO);
       // While looping through queries we will also check for their validity.
       let queriesCount = 0;
+      let corrupt = false;
       for (let i = 0; i < items.length; i++) {
         let queryName = as.getItemAnnotation(items[i], this.ORGANIZER_QUERY_ANNO);
 
         // Some extension did use our annotation to decorate their items
         // with icons, so we should check only our elements, to avoid dataloss.
         if (!(queryName in queries))
           continue;
 
         let query = queries[queryName];
         query.itemId = items[i];
 
         if (!itemExists(query.itemId)) {
           // Orphan annotation, bail out and create a new left pane root.
+          corrupt = true;
           break;
         }
 
         // Check that all queries have valid parents.
         let parentId = bs.getFolderIdForItem(query.itemId);
         if (items.indexOf(parentId) == -1 && parentId != leftPaneRoot) {
           // The parent is not part of the left pane, bail out and create a new
           // left pane root.
+          corrupt = true;
           break;
         }
 
         // Titles could have been corrupted or the user could have changed his
         // locale.  Check title and eventually fix it.
         if (bs.getItemTitle(query.itemId) != query.title)
           bs.setItemTitle(query.itemId, query.title);
         if ("concreteId" in query) {
@@ -837,17 +840,21 @@ this.PlacesUIUtils = {
             bs.setItemTitle(query.concreteId, query.concreteTitle);
         }
 
         // Add the query to our cache.
         this.leftPaneQueries[queryName] = query.itemId;
         queriesCount++;
       }
 
-      if (queriesCount != EXPECTED_QUERY_COUNT) {
+      // Note: it's not enough to just check for queriesCount, since we may
+      // find an invalid query just after accounting for a sufficient number of
+      // valid ones.  As well as we can't just rely on corrupt since we may find
+      // less valid queries than expected.
+      if (corrupt || queriesCount != EXPECTED_QUERY_COUNT) {
         // Queries number is wrong, so the left pane must be corrupt.
         // Note: we can't just remove the leftPaneRoot, because some query could
         // have a bad parent, so we have to remove all items one by one.
         items.forEach(safeRemoveItem);
         safeRemoveItem(leftPaneRoot);
       }
       else {
         // Everything is fine, return the current left pane folder.
--- a/browser/components/places/tests/unit/test_leftpane_corruption_handling.js
+++ b/browser/components/places/tests/unit/test_leftpane_corruption_handling.js
@@ -7,17 +7,17 @@
 /**
  * Tests that we build a working leftpane in various corruption situations.
  */
 
 // Used to store the original leftPaneFolderId getter.
 let gLeftPaneFolderIdGetter;
 let gAllBookmarksFolderIdGetter;
 // Used to store the original left Pane status as a JSON string.
-let gReferenceJSON;
+let gReferenceHierarchy;
 let gLeftPaneFolderId;
 // Third party annotated folder.
 let gFolderId;
 
 // Corruption cases.
 let gTests = [
 
   function test1() {
@@ -99,100 +99,68 @@ function run_test() {
                                                  PlacesUtils.bookmarks.DEFAULT_INDEX);
   PlacesUtils.annotations.setItemAnnotation(gFolderId, ORGANIZER_QUERY_ANNO,
                                             "test", 0,
                                             PlacesUtils.annotations.EXPIRE_NEVER);
 
   // Create the left pane, and store its current status, it will be used
   // as reference value.
   gLeftPaneFolderId = PlacesUIUtils.leftPaneFolderId;
-
+  gReferenceHierarchy = folderIdToHierarchy(gLeftPaneFolderId);
   do_test_pending();
-
-  Task.spawn(function() {
-    gReferenceJSON = yield folderToJSON(gLeftPaneFolderId);
-
-    // Kick-off tests.
-    do_timeout(0, run_next_test);
-  });
+  run_next_test();
 }
 
 function run_next_test() {
   if (gTests.length) {
     // Create corruption.
     let test = gTests.shift();
     test();
     // Regenerate getters.
     PlacesUIUtils.__defineGetter__("leftPaneFolderId", gLeftPaneFolderIdGetter);
     gLeftPaneFolderId = PlacesUIUtils.leftPaneFolderId;
     PlacesUIUtils.__defineGetter__("allBookmarksFolderId", gAllBookmarksFolderIdGetter);
     // Check the new left pane folder.
     Task.spawn(function() {
-      let leftPaneJSON = yield folderToJSON(gLeftPaneFolderId);
-      do_check_true(compareJSON(gReferenceJSON, leftPaneJSON));
+      let leftPaneHierarchy = folderIdToHierarchy(gLeftPaneFolderId)
+      if (gReferenceHierarchy != leftPaneHierarchy) {
+        do_throw("hierarchies differ!\n" + gReferenceHierarchy +
+                                    "\n" + leftPaneHierarchy);
+      }
       do_check_eq(PlacesUtils.bookmarks.getItemTitle(gFolderId), "test");
       // Go to next test.
-      do_timeout(0, run_next_test);
+      run_next_test();
     });
   }
   else {
     // All tests finished.
     remove_all_bookmarks();
     do_test_finished();
   }
 }
 
 /**
  * Convert a folder item id to a JSON representation of it and its contents.
  */
-function folderToJSON(aItemId) {
-  return Task.spawn(function() {
-    let query = PlacesUtils.history.getNewQuery();
-    query.setFolders([aItemId], 1);
-    let options = PlacesUtils.history.getNewQueryOptions();
-    options.queryType = Ci.nsINavHistoryQueryOptions.QUERY_TYPE_BOOKMARKS;
-    let root = PlacesUtils.history.executeQuery(query, options).root;
-    let writer = {
-      value: "",
-      write: function PU_wrapNode__write(aStr, aLen) {
-        this.value += aStr;
-      }
-    };
-    yield BookmarkJSONUtils.serializeNodeAsJSONToOutputStream(root, writer,
-                                                              false, false);
-    do_check_true(writer.value.length > 0);
-    throw new Task.Result(writer.value);
-  });
+function folderIdToHierarchy(aFolderId) {
+  let root = PlacesUtils.getFolderContents(aFolderId).root;
+  let hier = JSON.stringify(hierarchyToObj(root));
+  root.containerOpen = false;
+  return hier;
 }
 
-/**
- * Compare the JSON representation of 2 nodes, skipping everchanging properties
- * like dates.
- */
-function compareJSON(aNodeJSON_1, aNodeJSON_2) {
-  let node1 = JSON.parse(aNodeJSON_1);
-  let node2 = JSON.parse(aNodeJSON_2);
-
-  // List of properties we should not compare (expected to be different).
-  const SKIP_PROPS = ["dateAdded", "lastModified", "id"];
-
-  function compareObjects(obj1, obj2) {
-    function count(o) { var n = 0; for (let p in o) n++; return n; }
-    do_check_eq(count(obj1), count(obj2));
-    for (let prop in obj1) {
-      // Skip everchanging values.
-      if (SKIP_PROPS.indexOf(prop) != -1)
-        continue;
-      // Skip undefined objects, otherwise we hang on them.
-      if (!obj1[prop])
-        continue;
-      if (typeof(obj1[prop]) == "object")
-        return compareObjects(obj1[prop], obj2[prop]);
-      if (obj1[prop] !== obj2[prop]) {
-        print(prop + ": " + obj1[prop] + "!=" + obj2[prop]);
-        return false;
-      }
+function hierarchyToObj(aNode) {
+  let o = {}
+  o.title = aNode.title;
+  o.annos = PlacesUtils.getAnnotationsForItem(aNode.itemId)
+  if (PlacesUtils.nodeIsURI(aNode)) {
+    o.uri = aNode.uri;
+  }
+  else if (PlacesUtils.nodeIsFolder(aNode)) {
+    o.children = [];
+    PlacesUtils.asContainer(aNode).containerOpen = true;
+    for (let i = 0; i < aNode.childCount; ++i) {
+      o.children.push(hierarchyToObj(aNode.getChild(i)));
     }
-    return true;
+    aNode.containerOpen = false;
   }
-
-  return compareObjects(node1, node2);
+  return o;
 }
--- a/toolkit/components/places/BookmarkJSONUtils.jsm
+++ b/toolkit/components/places/BookmarkJSONUtils.jsm
@@ -168,38 +168,16 @@ this.BookmarkJSONUtils = Object.freeze({
 
       // Do not write to the tmp folder, otherwise if it has a different
       // filesystem writeAtomic will fail.  Eventual dangling .tmp files should
       // be cleaned up by the caller.
       yield OS.File.writeAtomic(aFilePath, jsonString,
                                 { tmpPath: OS.Path.join(aFilePath + ".tmp") });
       return { count: count, hash: hash };
     });
-  },
-
-  /**
-   * Serializes the given node (and all its descendents) as JSON
-   * and writes the serialization to the given output stream.
-   *
-   * @param   aNode
-   *          An nsINavHistoryResultNode
-   * @param   aStream
-   *          An nsIOutputStream. NOTE: it only uses the write(str, len)
-   *          method of nsIOutputStream. The caller is responsible for
-   *          closing the stream.
-   * @return {Promise}
-   * @resolves When node have been serialized and wrote to output stream.
-   * @rejects JavaScript exception.
-   *
-   * @note    This is likely to go away (bug 970291), so it's suggested to not
-   *          add more uses of it.  This is not yet firing a deprecation warning
-   *          cause it still has some internal usage.
-   */
-  serializeNodeAsJSONToOutputStream: function (aNode, aStream) {
-    return BookmarkNode.serializeAsJSONToOutputStream(aNode, aStream);
   }
 });
 
 function BookmarkImporter(aReplace) {
   this._replace = aReplace;
 }
 BookmarkImporter.prototype = {
   /**
@@ -528,217 +506,8 @@ function notifyObservers(topic) {
 function fixupQuery(aQueryURI, aFolderIdMap) {
   let convert = function(str, p1, offset, s) {
     return "folder=" + aFolderIdMap[p1];
   }
   let stringURI = aQueryURI.spec.replace(/folder=([0-9]+)/g, convert);
 
   return NetUtil.newURI(stringURI);
 }
-
-let BookmarkNode = {
-  /**
-   * Serializes the given node (and all its descendents) as JSON
-   * and writes the serialization to the given output stream.
-   *
-   * @param   aNode
-   *          An nsINavHistoryResultNode
-   * @param   aStream
-   *          An nsIOutputStream. NOTE: it only uses the write(str, len)
-   *          method of nsIOutputStream. The caller is responsible for
-   *          closing the stream.
-   * @returns Task promise
-   * @resolves the number of serialized uri nodes.
-   */
-  serializeAsJSONToOutputStream: function (aNode, aStream) {
-
-    return Task.spawn(function* () {
-      // Serialize to stream
-      let array = [];
-      let result = yield this._appendConvertedNode(aNode, null, array);
-      if (result.appendedNode) {
-        let jsonString = JSON.stringify(array[0]);
-        aStream.write(jsonString, jsonString.length);
-      } else {
-        throw Cr.NS_ERROR_UNEXPECTED;
-      }
-      return result.nodeCount;
-    }.bind(this));
-  },
-
-  _appendConvertedNode: function (bNode, aIndex, aArray) {
-    return Task.spawn(function* () {
-      let node = {};
-      let nodeCount = 0;
-
-      // Set index in order received
-      // XXX handy shortcut, but are there cases where we don't want
-      // to export using the sorting provided by the query?
-      if (aIndex)
-        node.index = aIndex;
-
-      this._addGenericProperties(bNode, node);
-
-      let parent = bNode.parent;
-      let grandParent = parent ? parent.parent : null;
-
-      if (PlacesUtils.nodeIsURI(bNode)) {
-        // Tag root accept only folder nodes
-        if (parent && parent.itemId == PlacesUtils.tagsFolderId)
-          return { appendedNode: false, nodeCount: nodeCount };
-
-        // Check for url validity, since we can't halt while writing a backup.
-        // This will throw if we try to serialize an invalid url and it does
-        // not make sense saving a wrong or corrupt uri node.
-        try {
-          NetUtil.newURI(bNode.uri);
-        } catch (ex) {
-          return { appendedNode: false, nodeCount: nodeCount };
-        }
-
-        yield this._addURIProperties(bNode, node);
-        nodeCount++;
-      } else if (PlacesUtils.nodeIsContainer(bNode)) {
-        // Tag containers accept only uri nodes
-        if (grandParent && grandParent.itemId == PlacesUtils.tagsFolderId)
-          return { appendedNode: false, nodeCount: nodeCount };
-
-        this._addContainerProperties(bNode, node);
-      } else if (PlacesUtils.nodeIsSeparator(bNode)) {
-        // Tag root accept only folder nodes
-        // Tag containers accept only uri nodes
-        if ((parent && parent.itemId == PlacesUtils.tagsFolderId) ||
-            (grandParent && grandParent.itemId == PlacesUtils.tagsFolderId))
-          return { appendedNode: false, nodeCount: nodeCount };
-
-        this._addSeparatorProperties(bNode, node);
-      }
-
-      if (!node.feedURI && node.type == PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER) {
-        nodeCount += yield this._appendConvertedComplexNode(node,
-                                                           bNode,
-                                                           aArray)
-        return { appendedNode: true, nodeCount: nodeCount };
-      }
-
-      aArray.push(node);
-      return { appendedNode: true, nodeCount: nodeCount };
-    }.bind(this));
-  },
-
-  _addGenericProperties: function (aPlacesNode, aJSNode) {
-    aJSNode.title = aPlacesNode.title;
-    aJSNode.id = aPlacesNode.itemId;
-    if (aJSNode.id != -1) {
-      let parent = aPlacesNode.parent;
-      if (parent)
-        aJSNode.parent = parent.itemId;
-      let dateAdded = aPlacesNode.dateAdded;
-      if (dateAdded)
-        aJSNode.dateAdded = dateAdded;
-      let lastModified = aPlacesNode.lastModified;
-      if (lastModified)
-        aJSNode.lastModified = lastModified;
-
-      // XXX need a hasAnnos api
-      let annos = [];
-      try {
-        annos =
-          PlacesUtils.getAnnotationsForItem(aJSNode.id);
-      } catch(ex) {}
-      if (annos.length != 0)
-        aJSNode.annos = annos;
-    }
-    // XXXdietrich - store annos for non-bookmark items
-  },
-
-  _addURIProperties: function BN__addURIProperties(
-    aPlacesNode, aJSNode) {
-    return Task.spawn(function() {
-      aJSNode.type = PlacesUtils.TYPE_X_MOZ_PLACE;
-      aJSNode.uri = aPlacesNode.uri;
-      if (aJSNode.id && aJSNode.id != -1) {
-        // Harvest bookmark-specific properties
-        let keyword = PlacesUtils.bookmarks.getKeywordForBookmark(aJSNode.id);
-        if (keyword)
-          aJSNode.keyword = keyword;
-      }
-
-      if (aPlacesNode.tags)
-        aJSNode.tags = aPlacesNode.tags;
-
-      // Last character-set
-      let uri = NetUtil.newURI(aPlacesNode.uri);
-      let lastCharset = yield PlacesUtils.getCharsetForURI(uri)
-      if (lastCharset)
-        aJSNode.charset = lastCharset;
-    });
-  },
-
-  _addSeparatorProperties: function BN__addSeparatorProperties(
-    aPlacesNode, aJSNode) {
-    aJSNode.type = PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR;
-  },
-
-  _addContainerProperties: function BN__addContainerProperties(
-    aPlacesNode, aJSNode) {
-    let concreteId = PlacesUtils.getConcreteItemId(aPlacesNode);
-    if (concreteId != -1) {
-      // This is a bookmark or a tag container.
-      if (PlacesUtils.nodeIsQuery(aPlacesNode) ||
-          concreteId != aPlacesNode.itemId) {
-        aJSNode.type = PlacesUtils.TYPE_X_MOZ_PLACE;
-        aJSNode.uri = aPlacesNode.uri;
-        aJSNode.concreteId = concreteId;
-      } else {
-        // Bookmark folder or a shortcut we should convert to folder.
-        aJSNode.type = PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER;
-
-        // Mark root folders.
-        if (aJSNode.id == PlacesUtils.placesRootId)
-          aJSNode.root = "placesRoot";
-        else if (aJSNode.id == PlacesUtils.bookmarksMenuFolderId)
-          aJSNode.root = "bookmarksMenuFolder";
-        else if (aJSNode.id == PlacesUtils.tagsFolderId)
-          aJSNode.root = "tagsFolder";
-        else if (aJSNode.id == PlacesUtils.unfiledBookmarksFolderId)
-          aJSNode.root = "unfiledBookmarksFolder";
-        else if (aJSNode.id == PlacesUtils.toolbarFolderId)
-          aJSNode.root = "toolbarFolder";
-      }
-    } else {
-      // This is a grouped container query, generated on the fly.
-      aJSNode.type = PlacesUtils.TYPE_X_MOZ_PLACE;
-      aJSNode.uri = aPlacesNode.uri;
-    }
-  },
-
-  _appendConvertedComplexNode: function (aNode, aSourceNode, aArray) {
-    return Task.spawn(function* () {
-      let repr = {};
-      let nodeCount = 0;
-
-      for (let [name, value] in Iterator(aNode))
-        repr[name] = value;
-
-      // Write child nodes
-      let children = repr.children = [];
-      if (!aNode.annos ||
-          !aNode.annos.some(anno => anno.name == PlacesUtils.LMANNO_FEEDURI)) {
-        PlacesUtils.asContainer(aSourceNode);
-        let wasOpen = aSourceNode.containerOpen;
-        if (!wasOpen)
-          aSourceNode.containerOpen = true;
-        let cc = aSourceNode.childCount;
-        for (let i = 0; i < cc; ++i) {
-          let childNode = aSourceNode.getChild(i);
-          let result = yield this._appendConvertedNode(aSourceNode.getChild(i), i, children);
-          nodeCount += result.nodeCount;
-        }
-        if (!wasOpen)
-          aSourceNode.containerOpen = false;
-      }
-
-      aArray.push(repr);
-      return nodeCount;
-    }.bind(this));
-  }
-}