Bug 412132 - after changing a bookmark's location, need to update the frecency of the "old" uri, r=dietrich
authorDrew Willcoxon <adw@mozilla.com>
Tue, 13 Jan 2009 11:48:27 +0100
changeset 23593 6ddb1b2244aa314c5cbdd8a26190e93c64030594
parent 23592 23273773e4523b8d3e93f1af68a3b5d92b2cddec
child 23594 ccd12ae3f34d6c36f595f68f5ee68ce262125dc4
push id4625
push usermak77@bonardo.net
push dateTue, 13 Jan 2009 11:44:36 +0000
treeherdermozilla-central@5b18412c4c4d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdietrich
bugs412132
milestone1.9.2a1pre
Bug 412132 - after changing a bookmark's location, need to update the frecency of the "old" uri, r=dietrich
toolkit/components/places/src/nsNavBookmarks.cpp
toolkit/components/places/tests/unit/test_412132.js
toolkit/components/places/tests/unit/test_expiration.js
--- a/toolkit/components/places/src/nsNavBookmarks.cpp
+++ b/toolkit/components/places/src/nsNavBookmarks.cpp
@@ -16,16 +16,17 @@
  *
  * The Initial Developer of the Original Code is
  * Google Inc.
  * Portions created by the Initial Developer are Copyright (C) 2005
  * the Initial Developer. All Rights Reserved.
  *
  * Contributor(s):
  *   Brian Ryner <bryner@brianryner.com> (original author)
+ *   Drew Willcoxon <adw@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
@@ -2526,44 +2527,90 @@ nsNavBookmarks::ChangeBookmarkURI(PRInt6
   mozStorageTransaction transaction(mDBConn, PR_FALSE);
 
   PRInt64 placeId;
   nsresult rv = History()->GetUrlIdFor(aNewURI, &placeId, PR_TRUE);
   NS_ENSURE_SUCCESS(rv, rv);
   if (!placeId)
     return NS_ERROR_INVALID_ARG;
 
+  // We need the bookmark's current corresponding places ID below, so get it now
+  // before we change it.  GetBookmarkURI will fail if aBookmarkId is bad.
+  nsCOMPtr<nsIURI> oldURI;
+  PRInt64 oldPlaceId;
+  rv = GetBookmarkURI(aBookmarkId, getter_AddRefs(oldURI));
+  NS_ENSURE_SUCCESS(rv, rv);
+  rv = History()->GetUrlIdFor(oldURI, &oldPlaceId, PR_FALSE);
+  NS_ENSURE_SUCCESS(rv, rv);
+
   nsCOMPtr<mozIStorageStatement> statement;
   rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
       "UPDATE moz_bookmarks SET fk = ?1 WHERE id = ?2"),
     getter_AddRefs(statement));
   statement->BindInt64Parameter(0, placeId);
   statement->BindInt64Parameter(1, aBookmarkId);
 
   rv = statement->Execute();
   NS_ENSURE_SUCCESS(rv, rv);
 
   rv = SetItemDateInternal(mDBSetItemLastModified, aBookmarkId, PR_Now());
   NS_ENSURE_SUCCESS(rv, rv);
 
   rv = transaction.Commit();
   NS_ENSURE_SUCCESS(rv, rv);
 
-  // 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 */);
+  // Upon changing the URI for a bookmark, update the frecency for the new place.
+  // UpdateFrecency needs to know whether placeId is bookmarked (as opposed
+  // to a livemark item).  Bookmarking it is exactly what we did above.
+  rv = History()->UpdateFrecency(placeId, PR_TRUE /* isBookmarked */);
   NS_ENSURE_SUCCESS(rv, rv);
 
-#if 0
-  // upon changing the uri for a bookmark, update the frecency for the old place
-  // XXX todo, we need to get the oldPlaceId (fk) before changing it above
-  // and then here, we need to determine if that oldPlaceId is still a bookmark (and not a livemark)
-  rv = History()->UpdateFrecency(oldPlaceId,  PR_FALSE /* isBookmark */);
+  // Upon changing the URI for a bookmark, update the frecency for the old place.
+  // UpdateFrecency again needs to know whether oldPlaceId is bookmarked.  It may
+  // no longer be, so we need to figure out whether it still is.  Our strategy
+  // is: find all bookmarks corresponding to oldPlaceId that are not livemark
+  // items, i.e., whose parents are not livemarks.  If any such bookmarks exist,
+  // oldPlaceId is still bookmarked.
+  PRBool isBookmarked = PR_FALSE;
+
+  nsCOMPtr<mozIStorageStatement> isBookmarkedStmt;
+  rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
+      "SELECT id "
+      "FROM moz_bookmarks "
+      "WHERE fk = ?1 AND "
+            "type = ?2 AND "
+            "parent NOT IN ("
+              "SELECT a.item_id "
+              "FROM moz_items_annos a "
+              "JOIN moz_anno_attributes n ON a.anno_attribute_id = n.id "
+              "WHERE n.name = ?3"
+            ")"),
+    getter_AddRefs(isBookmarkedStmt));
   NS_ENSURE_SUCCESS(rv, rv);
-#endif
+
+  {
+    mozStorageStatementScoper scope(isBookmarkedStmt);
+
+    rv = isBookmarkedStmt->BindInt64Parameter(0, oldPlaceId);
+    NS_ENSURE_SUCCESS(rv, rv);
+    rv = isBookmarkedStmt->BindInt32Parameter(1, TYPE_BOOKMARK);
+    NS_ENSURE_SUCCESS(rv, rv);
+    rv = isBookmarkedStmt->BindUTF8StringParameter(
+           2, NS_LITERAL_CSTRING(LMANNO_FEEDURI));
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    // If executing isBookmarkedStmt returns any rows, then there exists at least
+    // one bookmark corresponding to oldPlaceId that is not a livemark item.
+    // isBookmarked will be set to true in that case and false otherwise.
+    rv = isBookmarkedStmt->ExecuteStep(&isBookmarked);
+    NS_ENSURE_SUCCESS(rv, rv);
+  }
+
+  rv = History()->UpdateFrecency(oldPlaceId, isBookmarked);
+  NS_ENSURE_SUCCESS(rv, rv);
 
   nsCAutoString spec;
   rv = aNewURI->GetSpec(spec);
   NS_ENSURE_SUCCESS(rv, rv);
 
   // Pass the new URI to OnItemChanged.
   ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
     OnItemChanged(aBookmarkId, NS_LITERAL_CSTRING("uri"), PR_FALSE, spec))
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/unit/test_412132.js
@@ -0,0 +1,347 @@
+/* -*- 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) 2009
+ * the Initial Developer. All Rights Reserved.
+ *
+ * Contributor(s):
+ *   Drew Willcoxon <adw@mozilla.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 ***** */
+
+/*
+ * TEST DESCRIPTION:
+ *
+ * Tests patch to Bug 412132:
+ * https://bugzilla.mozilla.org/show_bug.cgi?id=412132
+ */
+
+const bmServ =
+  Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
+  getService(Ci.nsINavBookmarksService);
+const histServ =
+  Cc["@mozilla.org/browser/nav-history-service;1"].
+  getService(Ci.nsINavHistoryService);
+const lmServ =
+  Cc["@mozilla.org/browser/livemark-service;2"].
+  getService(Ci.nsILivemarkService);
+
+var defaultBookmarksMaxId;
+var dbConn;
+var tests = [];
+
+tests.push({
+  desc: ["Frecency of unvisited, separately bookmarked livemark item's URI ",
+         "should be zero after bookmark's URI changed."].join(""),
+  run: function ()
+  {
+    var lmItemURL;
+    var lmItemURI;
+    var bmId;
+    var frecencyBefore;
+    var frecencyAfter;
+
+    // Add livemark and bookmark.  Bookmark's URI is the URI of the livemark's
+    // only item.
+    lmItemURL = "http://example.com/livemark-item";
+    lmItemURI = uri(lmItemURL);
+    createLivemark(lmItemURI);
+    bmId = bmServ.insertBookmark(bmServ.unfiledBookmarksFolder,
+                                 lmItemURI,
+                                 bmServ.DEFAULT_INDEX,
+                                 "bookmark title");
+
+    // Bookmarked => frecency of URI should be != 0.
+    frecencyBefore = getFrecency(lmItemURL);
+    do_check_neq(frecencyBefore, 0);
+
+    bmServ.changeBookmarkURI(bmId, uri("http://example.com/new-uri"));
+
+    // URI's only "bookmark" is now unvisited livemark item => frecency = 0.
+    frecencyAfter = getFrecency(lmItemURL);
+    do_check_eq(frecencyAfter, 0);
+  }
+});
+
+tests.push({
+  desc: ["Frecency of visited, separately bookmarked livemark item's URI ",
+         "should not be zero after bookmark's URI changed."].join(""),
+  run: function ()
+  {
+    var lmItemURL;
+    var lmItemURI;
+    var bmId;
+    var frecencyBefore;
+    var frecencyAfter;
+
+    // Add livemark and bookmark.  Bookmark's URI is the URI of the livemark's
+    // only item.
+    lmItemURL = "http://example.com/livemark-item";
+    lmItemURI = uri(lmItemURL);
+    createLivemark(lmItemURI);
+    bmId = bmServ.insertBookmark(bmServ.unfiledBookmarksFolder,
+                                 lmItemURI,
+                                 bmServ.DEFAULT_INDEX,
+                                 "bookmark title");
+
+    // Bookmarked => frecency of URI should be != 0.
+    frecencyBefore = getFrecency(lmItemURL);
+    do_check_neq(frecencyBefore, 0);
+
+    visit(lmItemURI);
+
+    bmServ.changeBookmarkURI(bmId, uri("http://example.com/new-uri"));
+
+    // URI's only "bookmark" is now *visited* livemark item => frecency != 0.
+    frecencyAfter = getFrecency(lmItemURL);
+    do_check_neq(frecencyAfter, 0);
+  }
+});
+
+tests.push({
+  desc: ["After changing URI of bookmark, frecency of bookmark's original URI ",
+         "should be zero if original URI is unvisited and no longer ",
+         "bookmarked."].join(""),
+  run: function ()
+  {
+    var url;
+    var bmId;
+    var frecencyBefore;
+    var frecencyAfter;
+
+    url = "http://example.com/1";
+    bmId = bmServ.insertBookmark(bmServ.unfiledBookmarksFolder,
+                                 uri(url),
+                                 bmServ.DEFAULT_INDEX,
+                                 "bookmark title");
+
+    // Bookmarked => frecency of URI should be != 0.
+    frecencyBefore = getFrecency(url);
+    do_check_neq(frecencyBefore, 0);
+
+    bmServ.changeBookmarkURI(bmId, uri("http://example.com/2"));
+
+    // Unvisited URI no longer bookmarked => frecency should = 0.
+    frecencyAfter = getFrecency(url);
+    do_check_eq(frecencyAfter, 0);
+  }
+});
+
+tests.push({
+  desc: ["After changing URI of bookmark, frecency of bookmark's original URI ",
+         "should not be zero if original URI is visited."].join(""),
+  run: function ()
+  {
+    var bmURL;
+    var bmURI;
+    var bmId;
+    var frecencyBefore;
+    var frecencyAfter;
+
+    bmURL = "http://example.com/1";
+    bmURI = uri(bmURL);
+
+    bmId = bmServ.insertBookmark(bmServ.unfiledBookmarksFolder,
+                                 bmURI,
+                                 bmServ.DEFAULT_INDEX,
+                                 "bookmark title");
+
+    // Bookmarked => frecency of URI should be != 0.
+    frecencyBefore = getFrecency(bmURL);
+    do_check_neq(frecencyBefore, 0);
+
+    visit(bmURI);
+    bmServ.changeBookmarkURI(bmId, uri("http://example.com/2"));
+
+    // *Visited* URI no longer bookmarked => frecency should != 0.
+    frecencyAfter = getFrecency(bmURL);
+    do_check_neq(frecencyAfter, 0);
+  }
+});
+
+tests.push({
+  desc: ["After changing URI of bookmark, frecency of bookmark's original URI ",
+         "should not be zero if original URI is still bookmarked."].join(""),
+  run: function ()
+  {
+    var bmURL;
+    var bmURI;
+    var bm1Id;
+    var bm2Id;
+    var frecencyBefore;
+    var frecencyAfter;
+
+    bmURL = "http://example.com/1";
+    bmURI = uri(bmURL);
+
+    bm1Id = bmServ.insertBookmark(bmServ.unfiledBookmarksFolder,
+                                 bmURI,
+                                 bmServ.DEFAULT_INDEX,
+                                 "bookmark 1 title");
+
+    bm2Id = bmServ.insertBookmark(bmServ.unfiledBookmarksFolder,
+                                  bmURI,
+                                  bmServ.DEFAULT_INDEX,
+                                  "bookmark 2 title");
+
+
+    // Bookmarked => frecency of URI should be != 0.
+    frecencyBefore = getFrecency(bmURL);
+    do_check_neq(frecencyBefore, 0);
+
+    bmServ.changeBookmarkURI(bm1Id, uri("http://example.com/2"));
+
+    // URI still bookmarked => frecency should != 0.
+    frecencyAfter = getFrecency(bmURL);
+    do_check_neq(frecencyAfter, 0);
+  }
+});
+
+tests.push({
+  desc: "Changing the URI of a nonexistent bookmark should fail.",
+  run: function ()
+  {
+    var stmt;
+    var maxId;
+    var bmId;
+
+    function tryChange(itemId)
+    {
+      var failed;
+
+      failed= false;
+      try
+      {
+        bmServ.changeBookmarkURI(itemId + 1, uri("http://example.com/2"));
+      }
+      catch (exc)
+      {
+        failed= true;
+      }
+      do_check_true(failed);
+    }
+
+    // First try a straight-up bogus item ID, one greater than the current max
+    // ID.
+    stmt = dbConn.createStatement("SELECT MAX(id) FROM moz_bookmarks");
+    stmt.executeStep();
+    maxId = stmt.getInt32(0);
+    stmt.finalize();
+    tryChange(maxId + 1);
+
+    // Now add a bookmark, delete it, and check.
+    bmId = bmServ.insertBookmark(bmServ.unfiledBookmarksFolder,
+                                 uri("http://example.com/"),
+                                 bmServ.DEFAULT_INDEX,
+                                 "bookmark title");
+    bmServ.removeItem(bmId);
+    tryChange(bmId);
+  }
+});
+
+/******************************************************************************/
+
+function createLivemark(lmItemURI)
+{
+  var lmId;
+  var lmItemId;
+
+  lmId = lmServ.createLivemarkFolderOnly(bmServ.unfiledBookmarksFolder,
+                                         "livemark title",
+                                         uri("http://www.mozilla.org/"),
+                                         uri("http://www.mozilla.org/news.rdf"),
+                                         -1);
+  lmItemId = bmServ.insertBookmark(lmId,
+                                   lmItemURI,
+                                   bmServ.DEFAULT_INDEX,
+                                   "livemark item title");
+  return lmItemId;
+}
+
+function getFrecency(url)
+{
+  var sql;
+  var stmt;
+  var frecency;
+
+  sql = "SELECT frecency FROM moz_places_view WHERE url = ?1";
+  stmt = dbConn.createStatement(sql);
+  stmt.bindUTF8StringParameter(0, url);
+  do_check_true(stmt.executeStep());
+  frecency = stmt.getInt32(0);
+  print("frecency=" + frecency);
+  stmt.finalize();
+
+  return frecency;
+}
+
+function prepTest(testIndex, testName)
+{
+  print("Test " + testIndex + ": " + testName);
+  histServ.QueryInterface(Ci.nsIBrowserHistory).removeAllPages();
+  dbConn.executeSimpleSQL("DELETE FROM moz_places_view");
+  dbConn.executeSimpleSQL("DELETE FROM moz_bookmarks WHERE id > " +
+                          defaultBookmarksMaxId);
+}
+
+function visit(uri)
+{
+  histServ.addVisit(uri,
+                    Date.now() * 1000,
+                    null,
+                    histServ.TRANSITION_BOOKMARK,
+                    false,
+                    0);
+}
+
+/******************************************************************************/
+
+function run_test()
+{
+  var stmt;
+
+  dbConn =
+    Cc["@mozilla.org/browser/nav-history-service;1"].
+    getService(Ci.nsPIPlacesDatabase).
+    DBConnection;
+
+  stmt = dbConn.createStatement("SELECT MAX(id) FROM moz_bookmarks");
+  stmt.executeStep();
+  defaultBookmarksMaxId = stmt.getInt32(0);
+  stmt.finalize();
+  do_check_true(defaultBookmarksMaxId > 0);
+
+  for (let i= 0; i < tests.length; i++)
+  {
+    prepTest(i, tests[i].desc);
+    tests[i].run();
+  }
+}
--- a/toolkit/components/places/tests/unit/test_expiration.js
+++ b/toolkit/components/places/tests/unit/test_expiration.js
@@ -624,17 +624,22 @@ confirmation:
 */
 function startExpireBoth() {
   // setup
   histsvc.removeAllPages();
   observer.expiredURI = null;
   dump("starting expiration test 3: both criteria met\n");
   // force a sync, this will ensure that later we will have the same place in
   // both temp and disk table, and that the expire site cap count is correct.
-  bmsvc.changeBookmarkURI(bookmark, testURI);
+  // adw: Commented out per bug 412132.  The patch to that bug made it illegal
+  // to pass bad item IDs into changeBookmarkURI; |bookmark| was removed up in
+  // run_test, and it's never added or used again.  The only thing this call
+  // ended up doing was updating testURI's frecency and notifying bookmark
+  // observers that testURI had changed.  Neither appears relevant to this test.
+  //bmsvc.changeBookmarkURI(bookmark, testURI);
   // add visits
   // 2 days old, in microseconds
   var age = (Date.now() - (86400 * 2 * 1000)) * 1000;
   dump("AGE: " + age + "\n");
   histsvc.addVisit(testURI, age, null, histsvc.TRANSITION_TYPED, false, 0);
   annosvc.setPageAnnotation(testURI, testAnnoName, testAnnoVal, 0, annosvc.EXPIRE_NEVER);
 
   // set sites cap to 1