Bug 457473 - Bookmark copy creates bookmark with duplicate guid - regression in FF 3.0.3 (r=mano, sr=gavin, a=dveditz)

git-svn-id: svn://10.0.0.236/trunk@255086 18797224-902f-48f8-a5cc-f745e15eee43

mozilla/browser/components/places/content/utils.js

@@ -56,6 +56,7 @@ __defineGetter__("PlacesUtils", function() {
 
 const LOAD_IN_SIDEBAR_ANNO = "bookmarkProperties/loadInSidebar";
 const DESCRIPTION_ANNO = "bookmarkProperties/description";
+const GUID_ANNO = "placesInternal/GUID";
 const LMANNO_FEEDURI = "livemark/feedURI";
 const LMANNO_SITEURI = "livemark/siteURI";
 const ORGANIZER_FOLDER_ANNO = "PlacesOrganizer/OrganizerFolder";
@@ -206,11 +207,13 @@ var PlacesUIUtils = {
     var itemTitle = aData.title;
     var keyword = aData.keyword || null;
     var annos = aData.annos || [];
-    if (aExcludeAnnotations) {
-      annos = annos.filter(function(aValue, aIndex, aArray) {
-        return aExcludeAnnotations.indexOf(aValue.name) == -1;
-      });
-    }
+    // always exclude GUID when copying any item
+    var excludeAnnos = [GUID_ANNO];
+    if (aExcludeAnnotations)
+      excludeAnnos = excludeAnnos.concat(aExcludeAnnotations);
+    annos = annos.filter(function(aValue, aIndex, aArray) {
+      return excludeAnnos.indexOf(aValue.name) == -1;
+    });
     var childTxns = [];
     if (aData.dateAdded)
       childTxns.push(this.ptm.editItemDateAdded(null, aData.dateAdded));
@@ -301,6 +304,10 @@ var PlacesUIUtils = {
         childItems.push(this.ptm.editItemLastModified(null, aData.lastModified));
 
       var annos = aData.annos || [];
+      annos = annos.filter(function(aAnno) {
+        // always exclude GUID when copying any item
+        return aAnno.name != GUID_ANNO;
+      });
       return this.ptm.createFolder(aData.title, aContainer, aIndex, annos, childItems);
     }
   },
@@ -320,8 +327,9 @@ var PlacesUIUtils = {
         siteURI = PlacesUtils._uri(aAnno.value);
         return false;
       }
-      return true;
-    }, this);
+      // always exclude GUID when copying any item
+      return aAnno.name != GUID_ANNO;
+    });
     return this.ptm.createLivemark(feedURI, siteURI, aData.title, aContainer,
                                    aIndex, aData.annos);
   },
@@ -360,10 +368,8 @@ var PlacesUIUtils = {
         if (copy) {
           // Copying a child of a live-bookmark by itself should result
           // as a new normal bookmark item (bug 376731)
-          var copyBookmarkAnno =
-            this._getBookmarkItemCopyTransaction(data, container, index,
-                                                 ["livemark/bookmarkFeedURI"]);
-          return copyBookmarkAnno;
+          return this._getBookmarkItemCopyTransaction(data, container, index,
+                                                      ["livemark/bookmarkFeedURI"]);
         }
         else
           return this.ptm.moveItem(data.id, container, index);

mozilla/browser/components/places/tests/browser/Makefile.in

@@ -46,6 +46,7 @@ include $(topsrcdir)/config/rules.mk
 _BROWSER_TEST_FILES = \
 	browser_425884.js \
 	browser_423515.js \
+	browser_457473_no_copy_guid.js \
 	$(NULL)
 
 libs:: $(_BROWSER_TEST_FILES)

mozilla/browser/components/places/tests/browser/browser_457473_no_copy_guid.js

@@ -0,0 +1,153 @@
+/* vim:set ts=2 sw=2 sts=2 et: */
+/* ***** BEGIN LICENSE BLOCK *****
+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
+ *
+ * The contents of this file are subject to the Mozilla Public License Version
+ * 1.1 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ * http://www.mozilla.org/MPL/
+ *
+ * Software distributed under the License is distributed on an "AS IS" basis,
+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
+ * for the specific language governing rights and limitations under the
+ * License.
+ *
+ * The Original Code is Places test code.
+ *
+ * The Initial Developer of the Original Code is Mozilla Corp.
+ * Portions created by the Initial Developer are Copyright (C) 2008
+ * the Initial Developer. All Rights Reserved.
+ *
+ * Contributor(s):
+ *  Dietrich Ayala <dietrich@mozilla.com>
+ *
+ * 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
+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
+ * in which case the provisions of the GPL or the LGPL are applicable instead
+ * of those above. If you wish to allow use of your version of this file only
+ * under the terms of either the GPL or the LGPL, and not to allow others to
+ * use your version of this file under the terms of the MPL, indicate your
+ * decision by deleting the provisions above and replace them with the notice
+ * and other provisions required by the GPL or the LGPL. If you do not delete
+ * the provisions above, a recipient may use your version of this file under
+ * the terms of any one of the MPL, the GPL or the LGPL.
+ *
+ * ***** END LICENSE BLOCK ***** */
+
+function test() {
+  // sanity check
+  ok(PlacesUtils, "checking PlacesUtils, running in chrome context?");
+  ok(PlacesUIUtils, "checking PlacesUIUtils, running in chrome context?");
+
+  /*
+  - create, a test folder, add bookmark, separator, livemark to it
+  - fetch guids for all
+  - copy the folder
+  - test that guids are all different
+  - undo copy
+  - redo copy
+  - test that guids for the copy stay the same
+  */
+
+  var toolbarId = PlacesUtils.toolbarFolderId;
+  var toolbarNode = PlacesUtils.getFolderContents(toolbarId).root;
+
+  var oldCount = toolbarNode.childCount;
+  var testRootId = PlacesUtils.bookmarks.createFolder(toolbarId, "test root", -1);
+  is(toolbarNode.childCount, oldCount+1, "confirm test root node is a container, and is empty");
+  var testRootNode = toolbarNode.getChild(toolbarNode.childCount-1);
+  asContainer(testRootNode);
+  testRootNode.containerOpen = true;
+  is(testRootNode.childCount, 0, "confirm test root node is a container, and is empty");
+
+  // create folder A, fill it w/ each item type
+  var folderAId = PlacesUtils.bookmarks.createFolder(testRootId, "A", -1);
+  PlacesUtils.bookmarks.insertBookmark(folderAId, PlacesUtils._uri("http://foo"),
+                                       -1, "test bookmark");
+  PlacesUtils.bookmarks.insertSeparator(folderAId, -1);
+  PlacesUtils.livemarks.createLivemarkFolderOnly(folderAId, "test livemark",
+                                                 PlacesUtils._uri("http://test"),
+                                                 PlacesUtils._uri("http://test"), -1);
+
+  var folderANode = testRootNode.getChild(0);
+  var folderAGUIDs = getGUIDs(folderANode);
+
+  // test the test function
+  ok(checkGUIDs(folderANode, folderAGUIDs, true), "confirm guid test works");;
+
+  // serialize the folder
+  var serializedNode = PlacesUtils.wrapNode(folderANode, PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER);
+  var rawNode = PlacesUtils.unwrapNodes(serializedNode, PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER).shift();
+  ok(rawNode.type, "confirm json node was made");
+
+  // create a transaction from the serialization
+  // this exercises the guid-filtering
+  var transaction = PlacesUIUtils.makeTransaction(rawNode,
+                                                  PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER,
+                                                  testRootId, -1, true);
+  ok(transaction, "create transaction");
+
+  // execute it, copying to the test root folder
+  PlacesUIUtils.ptm.doTransaction(transaction);
+  is(testRootNode.childCount, 2, "create test folder via copy");
+
+  // check GUIDs are different
+  var folderBNode = testRootNode.getChild(1);
+  ok(checkGUIDs(folderBNode, folderAGUIDs, false), "confirm folder A GUIDs don't match folder B GUIDs");
+  var folderBGUIDs = getGUIDs(folderBNode);
+  ok(checkGUIDs(folderBNode, folderBGUIDs, true), "confirm test of new GUIDs");
+  
+  // undo the transaction, confirm the removal
+  PlacesUIUtils.ptm.undoTransaction();
+  is(testRootNode.childCount, 1, "confirm undo removed the copied folder");
+
+  // redo the transaction
+  // confirming GUIDs persist through undo/redo
+  PlacesUIUtils.ptm.redoTransaction();
+  is(testRootNode.childCount, 2, "confirm redo re-copied the folder");
+  folderBNode = testRootNode.getChild(1);
+  ok(checkGUIDs(folderBNode, folderAGUIDs, false), "folder B GUIDs after undo/redo don't match folder A GUIDs"); // sanity check
+
+  // XXXdietrich fails since GUIDs are created lazily. the anno
+  // isn't present at the time the transaction is first executed,
+  // and undo just undoes the original transaction - doesn't pull
+  // in any new changes.
+  //ok(checkGUIDs(folderBNode, folderBGUIDs, true, true), "folder B GUIDs after under/redo should match pre-undo/redo folder B GUIDs");
+
+  // clean up
+  PlacesUIUtils.ptm.undoTransaction();
+  PlacesUtils.bookmarks.removeItem(testRootId);
+}
+
+function getGUIDs(aNode) {
+  asContainer(aNode);
+  aNode.containerOpen = true;
+  var GUIDs = {
+    folder: PlacesUtils.bookmarks.getItemGUID(aNode.itemId),
+    bookmark: PlacesUtils.bookmarks.getItemGUID(aNode.getChild(0).itemId),
+    separator: PlacesUtils.bookmarks.getItemGUID(aNode.getChild(1).itemId),
+    livemark: PlacesUtils.bookmarks.getItemGUID(aNode.getChild(2).itemId)
+  };
+  aNode.containerOpen = false;
+  return GUIDs;
+}
+
+function checkGUIDs(aFolderNode, aGUIDs, aShouldMatch) {
+
+  function check(aNode, aGUID, aEquals) {
+    var nodeGUID = PlacesUtils.bookmarks.getItemGUID(aNode.itemId);
+    return aEquals ? (nodeGUID == aGUID) : (nodeGUID != aGUID);
+  }
+
+  asContainer(aFolderNode);
+  aFolderNode.containerOpen = true;
+
+  var allMatch = check(aFolderNode, aGUIDs.folder, aShouldMatch) &&
+                 check(aFolderNode.getChild(0), aGUIDs.bookmark, aShouldMatch) &&
+                 check(aFolderNode.getChild(1), aGUIDs.separator, aShouldMatch) &&
+                 check(aFolderNode.getChild(2), aGUIDs.livemark, aShouldMatch);
+
+  aFolderNode.containerOpen = false;
+  return allMatch;
+}

mozilla/toolkit/components/places/src/utils.js

@@ -1479,7 +1479,9 @@ var PlacesUtils = {
     serializeNodeToJSONStream(aNode, null);
   },
 
-  // XXX testing serializers
+  /**
+   * Serialize a JS object to JSON
+   */
   toJSONString: function PU_toJSONString(aObj) {
     var JSON = Cc["@mozilla.org/dom/json;1"].createInstance(Ci.nsIJSON);
     return JSON.encode(aObj);