From 25650cf0ce0592d1413f5b6bf52c04851900fcf7 Mon Sep 17 00:00:00 2001 From: "dietrich@mozilla.com" Date: Sun, 13 Apr 2008 11:17:46 -0700 Subject: [PATCH] Bug 428133 - removeItem and removeFolder accept ids of the wrong type (r=mano, a=beltzner) --- .../places/public/nsINavBookmarksService.idl | 7 +- .../components/places/src/nsNavBookmarks.cpp | 77 +++++++++++-------- .../components/places/src/nsTaggingService.js | 5 +- .../places/tests/bookmarks/test_removeItem.js | 70 +++++++++++++++++ 4 files changed, 126 insertions(+), 33 deletions(-) create mode 100644 toolkit/components/places/tests/bookmarks/test_removeItem.js diff --git a/toolkit/components/places/public/nsINavBookmarksService.idl b/toolkit/components/places/public/nsINavBookmarksService.idl index 50d7ee9e0f62..5d41fa80d975 100644 --- a/toolkit/components/places/public/nsINavBookmarksService.idl +++ b/toolkit/components/places/public/nsINavBookmarksService.idl @@ -259,10 +259,15 @@ interface nsINavBookmarksService : nsISupports * @returns the ID of the newly-inserted folder */ long long createDynamicContainer(in long long aParentFolder, in AUTF8String aName, - in AString aContractId, in long aIndex); + in AString aContractId, in long aIndex); /** * Removes a folder from the bookmarks tree. + * + * NOTE: This API is deprecated. The correct method to use is removeItem. + * This will be removed in the next release after Firefox 3.0. The + * removal is in bug 428558. + * * @param aFolder * The id of the folder to remove. */ diff --git a/toolkit/components/places/src/nsNavBookmarks.cpp b/toolkit/components/places/src/nsNavBookmarks.cpp index 4fc2a8318922..2c3e5973747b 100644 --- a/toolkit/components/places/src/nsNavBookmarks.cpp +++ b/toolkit/components/places/src/nsNavBookmarks.cpp @@ -657,16 +657,16 @@ nsNavBookmarks::FillBookmarksHash() // @see RecursiveAddBookmarkHash nsresult -nsNavBookmarks::AddBookmarkToHash(PRInt64 aBookmarkId, PRTime aMinTime) +nsNavBookmarks::AddBookmarkToHash(PRInt64 aPlaceId, PRTime aMinTime) { // this function might be called before our hashtable is initialized (for // example, on history import), just ignore these, we'll pick up the add when // the hashtable is initialized later if (! mBookmarksHash.IsInitialized()) return NS_OK; - if (! mBookmarksHash.Put(aBookmarkId, aBookmarkId)) + if (! mBookmarksHash.Put(aPlaceId, aPlaceId)) return NS_ERROR_OUT_OF_MEMORY; - return RecursiveAddBookmarkHash(aBookmarkId, aBookmarkId, aMinTime); + return RecursiveAddBookmarkHash(aPlaceId, aPlaceId, aMinTime); } @@ -683,7 +683,7 @@ nsNavBookmarks::AddBookmarkToHash(PRInt64 aBookmarkId, PRTime aMinTime) // to search all history for redirects. nsresult -nsNavBookmarks::RecursiveAddBookmarkHash(PRInt64 aBookmarkID, +nsNavBookmarks::RecursiveAddBookmarkHash(PRInt64 aPlaceID, PRInt64 aCurrentSource, PRTime aMinTime) { @@ -717,7 +717,7 @@ nsNavBookmarks::RecursiveAddBookmarkHash(PRInt64 aBookmarkID, if (mBookmarksHash.Get(curID, &alreadyExistingOne)) continue; - if (! mBookmarksHash.Put(curID, aBookmarkID)) + if (! mBookmarksHash.Put(curID, aPlaceID)) return NS_ERROR_OUT_OF_MEMORY; // save for recursion later @@ -727,7 +727,7 @@ nsNavBookmarks::RecursiveAddBookmarkHash(PRInt64 aBookmarkID, // recurse on each found item now that we're done with the statement for (PRUint32 i = 0; i < found.Length(); i ++) { - rv = RecursiveAddBookmarkHash(aBookmarkID, found[i], aMinTime); + rv = RecursiveAddBookmarkHash(aPlaceID, found[i], aMinTime); NS_ENSURE_SUCCESS(rv, rv); } @@ -747,10 +747,10 @@ nsNavBookmarks::RecursiveAddBookmarkHash(PRInt64 aBookmarkID, PR_STATIC_CALLBACK(PLDHashOperator) RemoveBookmarkHashCallback(nsTrimInt64HashKey::KeyType aKey, - PRInt64& aBookmark, void* aUserArg) + PRInt64& aPlaceId, void* aUserArg) { const PRInt64* removeThisOne = reinterpret_cast(aUserArg); - if (aBookmark == *removeThisOne) + if (aPlaceId == *removeThisOne) return PL_DHASH_REMOVE; return PL_DHASH_NEXT; } @@ -986,21 +986,13 @@ nsNavBookmarks::InsertBookmark(PRInt64 aFolder, nsIURI *aItem, PRInt32 aIndex, NS_IMETHODIMP nsNavBookmarks::RemoveItem(PRInt64 aItemId) { - mozIStorageConnection *dbConn = DBConn(); - mozStorageTransaction transaction(dbConn, PR_FALSE); - + nsresult rv; PRInt32 childIndex; PRInt64 placeId, folderId; PRInt32 itemType; nsCAutoString buffer; nsCAutoString spec; - // First, remove item annotations - nsAnnotationService* annosvc = nsAnnotationService::GetAnnotationService(); - NS_ENSURE_TRUE(annosvc, NS_ERROR_OUT_OF_MEMORY); - nsresult rv = annosvc->RemoveItemAnnotations(aItemId); - NS_ENSURE_SUCCESS(rv, rv); - { // scoping to ensure the statement gets reset mozStorageStatementScoper scope(mDBGetItemProperties); mDBGetItemProperties->BindInt64Parameter(0, aItemId); @@ -1022,6 +1014,21 @@ nsNavBookmarks::RemoveItem(PRInt64 aItemId) } } + if (itemType == TYPE_FOLDER) { + rv = RemoveFolder(aItemId); + NS_ENSURE_SUCCESS(rv, rv); + return NS_OK; + } + + mozIStorageConnection *dbConn = DBConn(); + mozStorageTransaction transaction(dbConn, PR_FALSE); + + // First, remove item annotations + nsAnnotationService* annosvc = nsAnnotationService::GetAnnotationService(); + NS_ENSURE_TRUE(annosvc, NS_ERROR_OUT_OF_MEMORY); + rv = annosvc->RemoveItemAnnotations(aItemId); + NS_ENSURE_SUCCESS(rv, rv); + buffer.AssignLiteral("DELETE FROM moz_bookmarks WHERE id = "); buffer.AppendInt(aItemId); @@ -1402,23 +1409,19 @@ nsNavBookmarks::GetParentAndIndexOfFolder(PRInt64 aFolder, PRInt64* aParent, } NS_IMETHODIMP -nsNavBookmarks::RemoveFolder(PRInt64 aFolder) +nsNavBookmarks::RemoveFolder(PRInt64 aFolderId) { mozIStorageConnection *dbConn = DBConn(); mozStorageTransaction transaction(dbConn, PR_FALSE); - // First, remove item annotations - nsAnnotationService* annosvc = nsAnnotationService::GetAnnotationService(); - NS_ENSURE_TRUE(annosvc, NS_ERROR_OUT_OF_MEMORY); - nsresult rv = annosvc->RemoveItemAnnotations(aFolder); - NS_ENSURE_SUCCESS(rv, rv); + nsresult rv; PRInt64 parent; - PRInt32 index; + PRInt32 index, type; nsCAutoString folderType; { mozStorageStatementScoper scope(mDBGetItemProperties); - rv = mDBGetItemProperties->BindInt64Parameter(0, aFolder); + rv = mDBGetItemProperties->BindInt64Parameter(0, aFolderId); NS_ENSURE_SUCCESS(rv, rv); PRBool results; @@ -1428,30 +1431,42 @@ nsNavBookmarks::RemoveFolder(PRInt64 aFolder) return NS_ERROR_INVALID_ARG; // folder is not in the hierarchy } + type = mDBGetItemProperties->AsInt32(kGetItemPropertiesIndex_Type); parent = mDBGetItemProperties->AsInt64(kGetItemPropertiesIndex_Parent); index = mDBGetItemProperties->AsInt32(kGetItemPropertiesIndex_Position); rv = mDBGetItemProperties->GetUTF8String(kGetItemPropertiesIndex_ServiceContractId, folderType); NS_ENSURE_SUCCESS(rv, rv); } + if (type != TYPE_FOLDER) { + NS_WARNING("RemoveFolder(): aFolderId is not a folder!"); + return NS_ERROR_INVALID_ARG; // aFolderId is not a folder! + } + + // First, remove item annotations + nsAnnotationService* annosvc = nsAnnotationService::GetAnnotationService(); + NS_ENSURE_TRUE(annosvc, NS_ERROR_OUT_OF_MEMORY); + rv = annosvc->RemoveItemAnnotations(aFolderId); + NS_ENSURE_SUCCESS(rv, rv); + // If this is a container bookmark, try to notify its service. if (folderType.Length() > 0) { // There is a type associated with this folder; it's a livemark. nsCOMPtr bmcServ = do_GetService(folderType.get()); if (bmcServ) { - rv = bmcServ->OnContainerRemoving(aFolder); + rv = bmcServ->OnContainerRemoving(aFolderId); if (NS_FAILED(rv)) NS_WARNING("Remove folder container notification failed."); } } // Remove all of the folder's children - RemoveFolderChildren(aFolder); + RemoveFolderChildren(aFolderId); // Remove the folder from its parent nsCAutoString buffer; buffer.AssignLiteral("DELETE FROM moz_bookmarks WHERE id = "); - buffer.AppendInt(aFolder); + buffer.AppendInt(aFolderId); rv = dbConn->ExecuteSimpleSQL(buffer); NS_ENSURE_SUCCESS(rv, rv); @@ -1464,12 +1479,12 @@ nsNavBookmarks::RemoveFolder(PRInt64 aFolder) rv = transaction.Commit(); NS_ENSURE_SUCCESS(rv, rv); - if (aFolder == mToolbarFolder) { + if (aFolderId == mToolbarFolder) { mToolbarFolder = 0; } ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver, - OnItemRemoved(aFolder, parent, index)) + OnItemRemoved(aFolderId, parent, index)) return NS_OK; } @@ -2135,6 +2150,8 @@ nsNavBookmarks::FolderCount(PRInt64 aFolder) NS_IMETHODIMP nsNavBookmarks::IsBookmarked(nsIURI *aURI, PRBool *aBookmarked) { + NS_ENSURE_ARG(aURI); + nsNavHistory* history = History(); NS_ENSURE_TRUE(history, NS_ERROR_UNEXPECTED); diff --git a/toolkit/components/places/src/nsTaggingService.js b/toolkit/components/places/src/nsTaggingService.js index d1deaa4016d7..9193c33be7c9 100644 --- a/toolkit/components/places/src/nsTaggingService.js +++ b/toolkit/components/places/src/nsTaggingService.js @@ -201,8 +201,9 @@ TaggingService.prototype = { var cc = node.childCount; if (wasOpen) node.containerOpen = false; - if (cc == 0) - this._bms.removeFolder(aTagId); + if (cc == 0) { + this._bms.removeFolder(node.itemId); + } }, // nsITaggingService diff --git a/toolkit/components/places/tests/bookmarks/test_removeItem.js b/toolkit/components/places/tests/bookmarks/test_removeItem.js new file mode 100644 index 000000000000..f26bff87f916 --- /dev/null +++ b/toolkit/components/places/tests/bookmarks/test_removeItem.js @@ -0,0 +1,70 @@ +/* -*- 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 Bug 384370 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 + * + * 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 ***** */ + +Components.utils.import("resource://gre/modules/utils.js"); +var tests = []; + + +const DEFAULT_INDEX = PlacesUtils.bookmarks.DEFAULT_INDEX; + +function run_test() { + // folder to hold this test + var folderId = + PlacesUtils.bookmarks.createFolder(PlacesUtils.toolbarFolderId, + "", DEFAULT_INDEX); + + // add a bookmark to the new folder + var bookmarkURI = uri("http://iasdjkf"); + do_check_false(PlacesUtils.bookmarks.isBookmarked(bookmarkURI)); + var bookmarkId = PlacesUtils.bookmarks.insertBookmark(folderId, bookmarkURI, + DEFAULT_INDEX, ""); + do_check_eq(PlacesUtils.bookmarks.getItemTitle(bookmarkId), ""); + + // try to remove the bookmark using removeFolder + try { + PlacesUtils.bookmarks.removeFolder(bookmarkId); + do_throw("no exception when removing a bookmark via removeFolder()!"); + } catch(ex) {} + do_check_true(PlacesUtils.bookmarks.isBookmarked(bookmarkURI)); + + // remove the folder using removeItem + PlacesUtils.bookmarks.removeItem(folderId); + do_check_eq(PlacesUtils.bookmarks.getBookmarkIdsForURI(bookmarkURI, {}).length, 0); + do_check_false(PlacesUtils.bookmarks.isBookmarked(bookmarkURI)); + do_check_eq(PlacesUtils.bookmarks.getItemIndex(bookmarkId), -1); +}