Bug 468305 - nsINavBookmarksObserver has no "onBeforeItemRemoved" callback

Not having this callback makes life difficult for anyone trying to synchronize
bookmarks.  You cannot get any additional information about a bookmark when
onItemRemoved is called, since it's been removed.  Things like a guid no longer
exist, which results in a lot of pain for add-ons that want to synchronize.
r=dietrich

browser/base/content/browser-places.js

@@ -1022,6 +1022,9 @@ var PlacesStarButton = {
       this.updateState();
   },
 
+  onBeforeItemRemoved: function PSB_onBeforeItemRemoved(aItemId) {
+  },
+
   onItemRemoved: function PSB_onItemRemoved(aItemId, aFolder, aIndex) {
     if (!this._batching)
       this.updateState();

browser/components/places/content/editBookmarkOverlay.js

@@ -1157,6 +1157,7 @@ var gEditItemOverlay = {
 
   onBeginUpdateBatch: function() { },
   onEndUpdateBatch: function() { },
+  onBeforeItemRemoved: function() { },
   onItemRemoved: function() { },
   onItemVisited: function() { },
 };

browser/components/places/tests/unit/test_placesTxn.js

@@ -95,6 +95,8 @@ var observer = {
     this._itemAddedParent = folder;
     this._itemAddedIndex = index;
   },
+  onBeforeItemRemoved: function(id) {
+  },
   onItemRemoved: function(id, folder, index) {
     this._itemRemovedId = id;
     this._itemRemovedFolder = folder;

browser/fuel/src/fuelApplication.js

@@ -408,6 +408,9 @@ Bookmark.prototype = {
     // bookmark object doesn't exist at this point
   },
 
+  onBeforeItemRemoved : function bm_obir(aId) {
+  },
+
   onItemRemoved : function bm_oir(aId, aFolder, aIndex) {
     if (this._id == aId)
       this._events.dispatch("remove", aId);
@@ -564,6 +567,9 @@ BookmarkFolder.prototype = {
       this._events.dispatch("addchild", aId);
   },
 
+  onBeforeItemRemoved : function bmf_oir(aId) {
+  },
+
   onItemRemoved : function bmf_oir(aId, aFolder, aIndex) {
     // handle root folder events
     if (!this._parent || this._id == aId)

toolkit/components/places/public/nsINavBookmarksService.idl

@@ -57,7 +57,7 @@ interface nsINavHistoryBatchCallback;
  * Observer for bookmark changes.
  */
 
-[scriptable, uuid(f9828ba8-9c70-4d95-b926-60d9e4378d7d)]
+[scriptable, uuid(a3544e1e-36a8-404a-9b30-918abf8e005e)]
 interface nsINavBookmarkObserver : nsISupports
 {
   /**
@@ -88,6 +88,15 @@ interface nsINavBookmarkObserver : nsISupports
   void onItemAdded(in long long aItemId, in long long aFolder,
                    in long aIndex);
 
+  /**
+   * Notify this observer that an item is about to be removed.  Called before
+   * the actual removal will take place.
+   *
+   * @param aItemId
+   *        The id of the bookmark to be removed.
+   */
+  void onBeforeItemRemoved(in long long aItemId);
+
   /**
    * Notify this observer that an item was removed.  Called after the actual
    * remove took place. The items following the index will be shifted up, but

toolkit/components/places/src/nsLivemarkService.js

@@ -489,6 +489,7 @@ LivemarkService.prototype = {
   onItemChanged: function() { },
   onItemVisited: function() { },
   onItemMoved: function() { },
+  onBeforeItemRemoved: function() { },
 
   onItemRemoved: function(aItemId, aParentId, aIndex) {
     // we don't need to remove annotations since itemAnnotations

toolkit/components/places/src/nsNavBookmarks.cpp

@@ -1185,6 +1185,9 @@ nsNavBookmarks::RemoveItem(PRInt64 aItemId)
     return NS_OK;
   }
 
+  ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
+                      OnBeforeItemRemoved(aItemId))
+
   mozStorageTransaction transaction(mDBConn, PR_FALSE);
 
   // First, remove item annotations
@@ -1593,6 +1596,9 @@ nsNavBookmarks::RemoveFolder(PRInt64 aFolderId)
 {
   NS_ENSURE_TRUE(aFolderId != mRoot, NS_ERROR_INVALID_ARG);
 
+  ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
+                      OnBeforeItemRemoved(aFolderId))
+
   mozStorageTransaction transaction(mDBConn, PR_FALSE);
 
   nsresult rv;
@@ -1780,12 +1786,18 @@ nsNavBookmarks::RemoveFolderChildren(PRInt64 aFolderId)
   nsCString foldersToRemove;
   for (PRUint32 i = 0; i < folderChildrenArray.Length(); i++) {
     folderChildrenInfo child = folderChildrenArray[i];
+
+    // Notify observers that we are about to remove this child.
+    ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
+                        OnBeforeItemRemoved(child.itemId))
+
     if (child.itemType == TYPE_FOLDER) {
       foldersToRemove.AppendLiteral(",");
       foldersToRemove.AppendInt(child.itemId);
 
       // If this is a dynamic container, try to notify its service that we
       // are going to remove it.
+      // XXX (bug 484094) this should use a bookmark observer!
       if (child.folderType.Length() > 0) {
         nsCOMPtr<nsIDynamicContainer> bmcServ =
           do_GetService(child.folderType.get());
@@ -2755,6 +2767,10 @@ nsNavBookmarks::SetItemIndex(PRInt64 aItemId, PRInt32 aNewIndex)
   rv = mDBSetItemIndex->Execute();
   NS_ENSURE_SUCCESS(rv, rv);
 
+  // XXX (bug 484096) this is really inefficient and we should look into using
+  //     onItemChanged here!
+  ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
+                      OnBeforeItemRemoved(aItemId))
   ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
                       OnItemRemoved(aItemId, parent, oldIndex))
   ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,

toolkit/components/places/src/nsNavHistoryResult.cpp

@@ -2884,6 +2884,11 @@ nsNavHistoryQueryResultNode::OnItemAdded(PRInt64 aItemId, PRInt64 aFolder,
   return NS_OK;
 }
 NS_IMETHODIMP
+nsNavHistoryQueryResultNode::OnBeforeItemRemoved(PRInt64 aItemId)
+{
+  return NS_OK;
+}
+NS_IMETHODIMP
 nsNavHistoryQueryResultNode::OnItemRemoved(PRInt64 aItemId, PRInt64 aFolder,
                                             PRInt32 aIndex)
 {
@@ -3466,6 +3471,15 @@ nsNavHistoryFolderResultNode::OnItemAdded(PRInt64 aItemId,
 }
 
 
+// nsNavHistoryFolderResultNode::OnBeforeItemRemoved (nsINavBookmarkObserver)
+
+NS_IMETHODIMP
+nsNavHistoryFolderResultNode::OnBeforeItemRemoved(PRInt64 aItemId)
+{
+  return NS_OK;
+}
+
+
 // nsNavHistoryFolderResultNode::OnItemRemoved (nsINavBookmarkObserver)
 
 NS_IMETHODIMP
@@ -4151,6 +4165,16 @@ nsNavHistoryResult::OnItemAdded(PRInt64 aItemId,
 }
 
 
+// nsNavHistoryResult::OnBeforeItemRemoved (nsINavBookmarkObserver)
+
+NS_IMETHODIMP
+nsNavHistoryResult::OnBeforeItemRemoved(PRInt64 aItemId)
+{
+  // Nobody actually does anything with this method, so we do not need to notify
+  return NS_OK;
+}
+
+
 // nsNavHistoryResult::OnItemRemoved (nsINavBookmarkObserver)
 
 NS_IMETHODIMP

toolkit/components/places/src/nsPlacesDBFlush.js

@@ -240,6 +240,7 @@ nsPlacesDBFlush.prototype = {
       this._flushWithQueries([kQuerySyncPlacesId]);
   },
 
+  onBeforeItemRemoved: function() { },
   onItemRemoved: function() { },
   onItemVisited: function() { },
   onItemMoved: function() { },

toolkit/components/places/src/nsTaggingService.js

@@ -359,6 +359,8 @@ TaggingService.prototype = {
         this._bms.getItemType(aItemId) == this._bms.TYPE_FOLDER)
       this._tagFolders[aItemId] = this._bms.getItemTitle(aItemId);
   },
+  onBeforeItemRemoved: function(aItemId) {
+  },
   onItemRemoved: function(aItemId, aFolderId, aIndex){
     if (aFolderId == this._bms.tagsFolder && this._tagFolders[aItemId])
       delete this._tagFolders[aItemId];

toolkit/components/places/tests/bookmarks/test_393498.js

@@ -52,6 +52,7 @@ var observer = {
     this._itemAddedParent = folder;
     this._itemAddedIndex = index;
   },
+  onBeforeItemRemoved: function(id) {},
   onItemRemoved: function(id, folder, index) {},
   _itemChangedProperty: null,
   onItemChanged: function(id, property, isAnnotationProperty, value) {

toolkit/components/places/tests/bookmarks/test_bookmarks.js

@@ -71,6 +71,8 @@ var observer = {
     this._itemAddedParent = folder;
     this._itemAddedIndex = index;
   },
+  onBeforeItemRemoved: function(id) {
+  },
   onItemRemoved: function(id, folder, index) {
     this._itemRemovedId = id;
     this._itemRemovedFolder = folder;

toolkit/components/places/tests/bookmarks/test_onBeforeItemRemoved_observer.js

@@ -0,0 +1,170 @@
+/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* 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 mozilla.org code.
+ *
+ * The Initial Developer of the Original Code is
+ * Mozilla Corporation.
+ * Portions created by the Initial Developer are Copyright (C) 2009
+ * the Initial Developer. All Rights Reserved.
+ *
+ * Contributor(s):
+ *   Shawn Wilsher <me@shawnwilsher.com> (Original Author)
+ *
+ * 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 ***** */
+
+/**
+ * Added with bug 468305 to test that onBeforeItemRemoved is dispatched before
+ * removed always with the right id.
+ */
+
+////////////////////////////////////////////////////////////////////////////////
+//// Globals and Constants
+
+let bs = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
+         getService(Ci.nsINavBookmarksService);
+
+////////////////////////////////////////////////////////////////////////////////
+//// Observer
+
+function Observer(aExpectedId)
+{
+}
+Observer.prototype =
+{
+  checked: false,
+  onBeginUpdateBatch: function() {
+  },
+  onEndUpdateBatch: function() {
+  },
+  onItemAdded: function(id, folder, index) {
+  },
+  onBeforeItemRemoved: function(id) {
+    this.removedId = id;
+  },
+  onItemRemoved: function(id, folder, index) {
+    do_check_false(this.checked);
+    do_check_eq(this.removedId, id);
+    this.checked = true;
+  },
+  onItemChanged: function(id, property, isAnnotationProperty, value) {
+  },
+  onItemVisited: function(id, visitID, time) {
+  },
+  onItemMoved: function(id, oldParent, oldIndex, newParent, newIndex) {
+  },
+  QueryInterface: function(iid) {
+    if (iid.equals(Ci.nsINavBookmarkObserver) ||
+        iid.equals(Ci.nsISupports)) {
+      return this;
+    }
+    throw Cr.NS_ERROR_NO_INTERFACE;
+  }
+};
+
+////////////////////////////////////////////////////////////////////////////////
+//// Test Functions
+
+function test_removeItem()
+{
+  // First we add the item we are going to remove.
+  let id = bs.insertBookmark(bs.unfiledBookmarksFolder,
+                             uri("http://mozilla.org"), bs.DEFAULT_INDEX, "t");
+
+  // Add our observer, and remove it.
+  let observer = new Observer(id);
+  bs.addObserver(observer, false);
+  bs.removeItem(id);
+
+  // Make sure we were notified!
+  do_check_true(observer.checked);
+  bs.removeObserver(observer);
+}
+
+function test_removeFolder()
+{
+  // First we add the item we are going to remove.
+  let id = bs.createFolder(bs.unfiledBookmarsFolder, "t", bs.DEFAULT_INDEX);
+
+  // Add our observer, and remove it.
+  let observer = new Observer(id);
+  bs.addObserver(observer, false);
+  bs.removeItem(id);
+
+  // Make sure we were notified!
+  do_check_true(observer.checked);
+  bs.removeObserver(observer);
+}
+
+function test_removeFolderChildren()
+{
+  // First we add the item we are going to remove.
+  let fid = bs.createFolder(bs.unfiledBookmarsFolder, "tf", bs.DEFAULT_INDEX);
+  let id = bs.insertBookmark(fid, uri("http://mozilla.org"), bs.DEFAULT_INDEX,
+                             "t");
+
+  // Add our observer, and remove it.
+  let observer = new Observer(id);
+  bs.addObserver(observer, false);
+  bs.removeFolderChildren(fid);
+
+  // Make sure we were notified!
+  do_check_true(observer.checked);
+  bs.removeObserver(observer);
+}
+
+function test_setItemIndex()
+{
+  // First we add the item we are going to move, and one additional one.
+  let id = bs.insertBookmark(bs.unfiledBookmarksFolder,
+                             uri("http://mozilla.org/1"), 0, "t1");
+  bs.insertBookmark(bs.unfiledBookmarksFolder, uri("http://mozilla.org/2"), 1,
+                    "t1");
+
+  // Add our observer, and move it.
+  let observer = new Observer(id);
+  bs.addObserver(observer, false);
+  bs.setItemIndex(id, 2);
+
+  // Make sure we were notified!
+  do_check_true(observer.checked);
+  bs.removeObserver(observer);
+}
+
+////////////////////////////////////////////////////////////////////////////////
+//// Test Runner
+
+let tests = [
+  test_removeItem,
+  test_removeFolder,
+  test_removeFolderChildren,
+  test_setItemIndex,
+];
+function run_test()
+{
+  tests.forEach(function(test) test());
+}

toolkit/components/places/tests/chrome/test_329534.xul

@@ -82,6 +82,7 @@ var observer =
       }
     }
   },
+  onBeforeItemRemoved: function(bookmarkId){},
   onItemRemoved: function(bookmarkId, bookmark, folder, index){},
   onItemChanged: function(bookmarkId, property, isAnnotationProperty, value){},
   onItemVisited: function(bookmarkId, bookmark, aVisitID, time){},

toolkit/components/places/tests/chrome/test_342484.xul

@@ -54,6 +54,7 @@ var observer =
     bmsvc.removeObserver(this);
   },
   onItemAdded: function(itemId, folder, index) {},
+  onBeforeItemRemoved: function(itemId){},
   onItemRemoved: function(itemId, folder, index){},
   onItemChanged: function(itemId, property, isAnnotationProperty, value){},
   onItemVisited: function(itemId, aVisitID, time){},

toolkit/components/places/tests/chrome/test_371798.xul

@@ -60,6 +60,7 @@ var observer =
   onBeginUpdateBatch: function(){},
   onEndUpdateBatch: function(){},
   onItemAdded: function(bookmarkId, bookmark, folder, index) {},
+  onBeforeItemRemoved: function(bookmarkId){},
   onItemRemoved: function(bookmarkId, bookmark, folder, index){},
   onItemChanged: function(bookmarkId, property, isAnnotationProperty, value){
     runTest();

toolkit/components/places/tests/chrome/test_381357.xul

@@ -59,6 +59,7 @@ var observer =
       SimpleTest.finish();
     }
   },
+  onBeforeItemRemoved: function(itemId){},
   onItemRemoved: function(itemId, folder, index){},
   onItemChanged: function(itemId, property, isAnnotationProperty, value){},
   onItemVisited: function(itemId, aVisitID, time){},

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

@@ -62,6 +62,7 @@ var deletedBookmarkIds = [];
 var observer = {
   // cached ordered array of notified items
   _onItemRemovedItemIds: [],
+  onBeforeItemRemoved: function(aItemId) { },
   onItemRemoved: function(aItemId, aParentId, aIndex) {
     // We should first get notifications for children, then for their parent
     do_check_eq(this._onItemRemovedItemIds.indexOf(aParentId), -1);

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

@@ -54,6 +54,7 @@ var observer = {
   onItemAdded: function(id, folder, index) {
     do_check_true(id > 0);
   },
+  onBeforeItemRemoved: function() {},
   onItemRemoved: function() {},
   onItemChanged: function() {},
   onItemVisited: function() {},