Closed Bug 463674 Opened 11 years ago Closed 11 years ago

changeBookmarkURI() doesn't update internal bookmark hash

Categories

(Firefox :: Bookmarks & History, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Firefox 3.6a1

People

(Reporter: dietrich, Assigned: adw)

Details

(Keywords: verified1.9.1)

Attachments

(1 file, 2 obsolete files)

Attached patch fix (obsolete) — Splinter Review
per irc from ingrid:

if you add a bookmark and then change it's uri via changeBookmarkURI(), an immediate subsequent call to isBookmarked() fails.

this is because changeBookmarkURI() needs to update the internal bookmark hash that methods like isBookmarked() check against.

patch and test attached.
Attachment #346933 - Flags: review?(mak77)
Comment on attachment 346933 [details] [diff] [review]
fix

diff --git a/toolkit/components/places/src/nsNavBookmarks.cpp b/toolkit/components/places/src/nsNavBookmarks.cpp
--- a/toolkit/components/places/src/nsNavBookmarks.cpp
+++ b/toolkit/components/places/src/nsNavBookmarks.cpp
@@ -2517,6 +2517,10 @@
   rv = transaction.Commit();
   NS_ENSURE_SUCCESS(rv, rv);
 
+  // Add new URI to bookmark hash
+  rv = AddBookmarkToHash(placeId, 0);
+  NS_ENSURE_SUCCESS(rv, rv);
+

you should also get the old place id and call
UpdateBookmarkHashOnRemove(oldPlaceId);
or the results would be wrong for the old bookmark and we would assert in debug
mode since bookmarks hash would be out of sync when the old uri is no more bookmarked.

   // upon changing the uri for a bookmark, update the frecency for the new place
   // no need to check if this is a livemark, because...
   rv = History()->UpdateFrecency(placeId, PR_TRUE /* isBookmark */);
diff --git a/toolkit/components/places/tests/bookmarks/test_changeBookmarkURI.js b/toolkit/components/places/tests/bookmarks/test_changeBookmarkURI.js
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/bookmarks/test_changeBookmarkURI.js
@@ -0,0 +1,71 @@
+/* -*- 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 Places unit test code.
+ *
+ * The Initial Developer of the Original Code is Mozilla Corporation.
+ * 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 ***** */
+
+// Get bookmark service
+try {
+  var bmsvc = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
+              getService(Ci.nsINavBookmarksService);
+} catch(ex) {
+  do_throw("Could not get nav-bookmarks-service\n");
+}
+
+// main
+function run_test() {
+  //Create a folder
+  var collectionId = bmsvc.createFolder(bmsvc.toolbarFolder, "test", -1);

collectionId?

+  
+  //Create 2 uris
+  var oldUriObj = uri("http://www.dogs.com");
+  var newUriObj = uri("http://www.cats.com");

please discard Obj suffix

+  
+  //Bookmark the first one
+  var oldBookmarkId = bmsvc.insertBookmark(collectionId, oldUriObj, -1, "Dogs");
+  
+  //Then change the uri of the bookmarks to the second one
+  bmsvc.changeBookmarkURI(oldBookmarkId, newUriObj);
+  
+  //Then check if the new URI is now considered a bookmark
+  var bookmarkUriObj = bmsvc.getBookmarkedURIFor(newUriObj);

please discard Obj suffix

+  
+  //Check that isBookmarked() works for the new URI
+  do_check_true(bmsvc.isBookmarked(newUriObj));

also add
do_check_false(bmsvc.isBookmarked(oldUriObj));

and clean up trailing spaces from test, please
Attachment #346933 - Flags: review?(mak77) → review-
Priority: -- → P2
Target Milestone: --- → Firefox 3.1
Whiteboard: [good first bug]
Assignee: nobody → adw
Attached patch fix (obsolete) — Splinter Review
Changes re: comment 1
Attachment #346933 - Attachment is obsolete: true
Attachment #356826 - Flags: review?(mak77)
Comment on attachment 356826 [details] [diff] [review]
fix

>diff --git a/toolkit/components/places/tests/bookmarks/test_changeBookmarkURI.js b/toolkit/components/places/tests/bookmarks/test_changeBookmarkURI.js
>new file mode 100644
>+ *
>+ * The Initial Developer of the Original Code is Mozilla Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2008

nit: 2009

>+// Get bookmark service
>+try {
>+  var bmsvc = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
>+              getService(Ci.nsINavBookmarksService);
>+} catch(ex) {
>+  do_throw("Could not get nav-bookmarks-service\n");
>+}

discard the try catch please, we are discarding them in new tests

>+// main
>+function run_test() {
>+  // Create a folder
>+  var folderId = bmsvc.createFolder(bmsvc.toolbarFolder, "test", -1);

use bmsvc.DEFAULT_INDEX please

>+  // Create 2 URIs
>+  var oldUri = uri("http://www.dogs.com");
>+  var newUri = uri("http://www.cats.com");
>+
>+  // Bookmark the first one
>+  var bookmarkId = bmsvc.insertBookmark(folderId, oldUri, -1, "Dogs");

use bmsvc.DEFAULT_INDEX please

>+
>+  // Then change the URI of the bookmarks to the second one

nit: bookmarkS, we only have one

>+  // Then check if the new URI is now considered a bookmark

could you do this before too, for the inverse check (so check that old is considered a bookmark, new is not)

>+  var bookmarkUri = bmsvc.getBookmarkedURIFor(newUri);
>+  do_check_true(!!bookmarkUri);

when a value is set is better checking against the value we expect it to have, rather then only check if it has a value.

>+  // Check that isBookmarked() works for the new URI
>+  do_check_true(bmsvc.isBookmarked(newUri));
>+
>+  // Check that getBookmarkURI() works for the new URI
>+  do_check_true(bmsvc.getBookmarkURI(bookmarkId).equals(newUri));

>+  // Check that isBookmarked() works for the old URI
>+  do_check_false(bmsvc.isBookmarked(oldUri));

the comments should state what we are looking for, so "check newUri is bookmarked", "check bookmarkId uri has been changed", or something like that. That makes the test more readable for a third party.

otherwise, r=mak77
thanks!
Attachment #356826 - Flags: review?(mak77) → review+
Attached patch for checkinSplinter Review
Changes re: comment 3
Attachment #356826 - Attachment is obsolete: true
Attachment #356972 - Flags: review+
Keywords: checkin-needed
Whiteboard: [good first bug]
http://hg.mozilla.org/mozilla-central/rev/28c4ee827acd
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 3.1 → Firefox 3.2a1
Flags: in-testsuite+
Attachment #356972 - Flags: approval1.9.1?
Comment on attachment 356972 [details] [diff] [review]
for checkin

requesting approval for 1.9.1 branch, with tests
low risk, since we use the bookmarks hash to calculate frecency, would be good to have.
Flags: wanted-firefox3.1?
Attachment #356972 - Flags: approval1.9.1? → approval1.9.1+
Flags: wanted-firefox3.5?
verified per checkin and in-testsuite
Status: RESOLVED → VERIFIED
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.