Bug 398914 Creating an additional bookmark with "Add a keyword for this search" overwrites the POST_DATA of existing bookmark(s) with the same location (r=sspitzer)

browser/app/profile/firefox.js

@@ -587,6 +587,11 @@ pref("browser.places.importBookmarksHTML", true);
 // if false, will add the "Smart Bookmarks" folder to the personal toolbar
 pref("browser.places.createdSmartBookmarks", false);
 
+// If true, will migrate uri post-data annotations to
+// bookmark post-data annotations (bug 398914)
+// XXX to be removed after beta 2 (bug 391419)
+pref("browser.places.migratePostDataAnnotations", true);
+
 // Controls behavior of the "Add Exception" dialog launched from SSL error pages
 // 0 - don't pre-populate anything
 // 1 - pre-populate site URL, but don't fetch certificate

browser/base/content/browser-places.js

@@ -822,3 +822,37 @@ var PlacesStarButton = {
   onItemVisited: function() { },
   onItemMoved: function() { }
 };
+
+/**
+ * Various migration tasks.
+ */
+function placesMigrationTasks() {
+  // bug 398914 - move all post-data annotations from URIs to bookmarks
+  // XXX - REMOVE ME FOR BETA 3 (bug 391419)
+  if (gPrefService.getBoolPref("browser.places.migratePostDataAnnotations")) {
+    const annosvc = PlacesUtils.annotations;
+    const bmsvc = PlacesUtils.bookmarks;
+    const oldPostDataAnno = "URIProperties/POSTData";
+    var pages = annosvc.getPagesWithAnnotation(oldPostDataAnno, {});
+    for (let i = 0; i < pages.length; i++) {
+      try {
+        let uri = pages[i];
+        var postData = annosvc.getPageAnnotation(uri, oldPostDataAnno);
+        // We can't know which URI+keyword combo this postdata was for, but
+        // it's very likely that if this URI is bookmarked and has a keyword
+        // *and* the URI has postdata, then this bookmark was using the
+        // postdata. Propagate the annotation to all bookmarks for this URI
+        // just to be safe.
+        let bookmarks = bmsvc.getBookmarkIdsForURI(uri, {});
+        for (let i = 0; i < bookmarks.length; i++) {
+          var keyword = bmsvc.getKeywordForBookmark(bookmarks[i]);
+          if (keyword)
+            annosvc.setItemAnnotation(bookmarks[i], POST_DATA_ANNO, postData, 0, annosvc.EXPIRE_NEVER); 
+        }
+        // Remove the old annotation.
+        annosvc.removePageAnnotation(uri, oldPostDataAnno);
+      } catch(ex) {}
+    }
+    gPrefService.setBoolPref("browser.places.migratePostDataAnnotations", false);
+  }
+}

browser/base/content/browser.js

@@ -913,6 +913,9 @@ function delayedStartup()
 
   UpdateUrlbarSearchSplitterState();
   
+  try {
+    placesMigrationTasks();
+  } catch(ex) {}
   initBookmarksToolbar();
   PlacesStarButton.init();
 
@@ -1661,11 +1664,8 @@ function getShortcutOrURI(aURL, aPostDataRef) {
   if (engine)
     return engine.getSubmission(param, null).uri.spec;
 
-  try {
-    var shortcutURI = PlacesUtils.bookmarks.getURIForKeyword(keyword);
-    shortcutURL = shortcutURI.spec;
-    aPostDataRef.value = PlacesUtils.getPostDataForURI(shortcutURI);
-  } catch(ex) {}
+  [shortcutURL, aPostDataRef.value] =
+    PlacesUtils.getURLAndPostDataForKeyword(keyword);
 
   if (!shortcutURL)
     return aURL;

browser/components/places/content/bookmarkProperties.js

@@ -929,16 +929,16 @@ var BookmarkPropertiesPanel = {
         PlacesUtils.ptm.editBookmarkMicrosummary(-1, microsummary));
     }
 
+    if (this._postData) {
+      childTransactions.push(
+        PlacesUtils.ptm.editBookmarkPostData(-1, this._postData));
+    }
+
     var transactions = [PlacesUtils.ptm.createItem(uri, aContainer, aIndex,
                                                    title, keyword,
                                                    annotations,
                                                    childTransactions)];
 
-    if (this._postData) {
-      transactions.push(
-        PlacesUtils.ptm.editURIPostData(uri, this._postData));
-    }
-
     return PlacesUtils.ptm.aggregateTransactions(this._getDialogTitle(), transactions);
   },
 

browser/components/places/content/utils.js

@@ -50,7 +50,7 @@ Components.utils.import("resource://gre/modules/JSON.jsm");
 
 const LOAD_IN_SIDEBAR_ANNO = "bookmarkProperties/loadInSidebar";
 const DESCRIPTION_ANNO = "bookmarkProperties/description";
-const POST_DATA_ANNO = "URIProperties/POSTData";
+const POST_DATA_ANNO = "bookmarkProperties/POSTData";
 const LMANNO_FEEDURI = "livemark/feedURI";
 const LMANNO_SITEURI = "livemark/siteURI";
 const ORGANIZER_QUERY_ANNO = "PlacesOrganizer/OrganizerQuery";
@@ -1446,33 +1446,58 @@ var PlacesUtils = {
   },
 
   /**
-   * Set the POST data associated with a URI, if any.
+   * Set the POST data associated with a bookmark, if any.
    * Used by POST keywords.
-   *   @param aURI
+   *   @param aBookmarkId
    *   @returns string of POST data
    */
-  setPostDataForURI: function PU_setPostDataForURI(aURI, aPostData) {
+  setPostDataForBookmark: function PU_setPostDataForBookmark(aBookmarkId, aPostData) {
     const annos = this.annotations;
     if (aPostData)
-      annos.setPageAnnotation(aURI, POST_DATA_ANNO, aPostData, 
+      annos.setItemAnnotation(aBookmarkId, POST_DATA_ANNO, aPostData, 
                               0, Ci.nsIAnnotationService.EXPIRE_NEVER);
-    else if (annos.pageHasAnnotation(aURI, POST_DATA_ANNO))
-      annos.removePageAnnotation(aURI, POST_DATA_ANNO);
+    else if (annos.itemHasAnnotation(aBookmarkId, POST_DATA_ANNO))
+      annos.removeItemAnnotation(aBookmarkId, POST_DATA_ANNO);
   },
 
   /**
    * Get the POST data associated with a bookmark, if any.
-   * @param aURI
-   * @returns string of POST data if set for aURI. null otherwise.
+   * @param aBookmarkId
+   * @returns string of POST data if set for aBookmarkId. null otherwise.
    */
-  getPostDataForURI: function PU_getPostDataForURI(aURI) {
+  getPostDataForBookmark: function PU_getPostDataForBookmark(aBookmarkId) {
     const annos = this.annotations;
-    if (annos.pageHasAnnotation(aURI, POST_DATA_ANNO))
-      return annos.getPageAnnotation(aURI, POST_DATA_ANNO);
+    if (annos.itemHasAnnotation(aBookmarkId, POST_DATA_ANNO))
+      return annos.getItemAnnotation(aBookmarkId, POST_DATA_ANNO);
 
     return null;
   },
 
+  /**
+   * Get the URI (and any associated POST data) for a given keyword.
+   * @param aKeyword string keyword
+   * @returns an array containing a string URL and a string of POST data
+   */
+  getURLAndPostDataForKeyword: function PU_getURLAndPostDataForKeyword(aKeyword) {
+    var url = null, postdata = null;
+    try {
+      var uri = this.bookmarks.getURIForKeyword(aKeyword);
+      if (uri) {
+        url = uri.spec;
+        var bookmarks = this.bookmarks.getBookmarkIdsForURI(uri, {});
+        for (let i = 0; i < bookmarks.length; i++) {
+          var bookmark = bookmarks[i];
+          var kw = this.bookmarks.getKeywordForBookmark(bookmark);
+          if (kw == aKeyword) {
+            postdata = this.getPostDataForBookmark(bookmark);
+            break;
+          }
+        }
+      }
+    } catch(ex) {}
+    return [url, postdata];
+  },
+
   /**
    * Retrieve the description of an item
    * @param aItemId

browser/components/places/public/nsIPlacesTransactionsService.idl

@@ -52,7 +52,7 @@ interface nsITransaction;
  * the global scope of a js window.
  */
 
-[scriptable, uuid(939bccbd-ecb1-4742-9c38-a33af91ec872)]
+[scriptable, uuid(89f61a91-c8f7-4abb-b880-895cb9852c35)]
 interface nsIPlacesTransactionsService : nsITransactionManager
 {
   /**
@@ -237,19 +237,19 @@ interface nsIPlacesTransactionsService : nsITransactionManager
    *        new keyword for the bookmark
    * @returns nsITransaction object
    */
-  nsITransaction editBookmarkKeyword(in long long id,
-                                           in AString newKeyword);
+  nsITransaction editBookmarkKeyword(in long long aItemId,
+                                     in AString aNewKeyword);
 
   /**
-   * Transaction for editing the post data associated with a URI
+   * Transaction for editing the post data associated with a bookmark.
    *
-   * @param aURI
-   *        uri to edit
+   * @param aItemId
+   *        id of the bookmark to edit
    * @param aPostData
    *        post data
    * @returns nsITransaction object
    */
-  nsITransaction editURIPostData(in nsIURI aURI,
+  nsITransaction editBookmarkPostData(in long long aItemId,
                                       in AString aPostData);
 
   /**

browser/components/places/src/nsPlacesImportExportService.cpp

@@ -127,7 +127,7 @@ static NS_DEFINE_CID(kParserCID, NS_PARSER_CID);
 
 #define LOAD_IN_SIDEBAR_ANNO NS_LITERAL_CSTRING("bookmarkProperties/loadInSidebar")
 #define DESCRIPTION_ANNO NS_LITERAL_CSTRING("bookmarkProperties/description")
-#define POST_DATA_ANNO NS_LITERAL_CSTRING("URIProperties/POSTData")
+#define POST_DATA_ANNO NS_LITERAL_CSTRING("bookmarkProperties/POSTData")
 #define LAST_CHARSET_ANNO NS_LITERAL_CSTRING("URIProperties/characterSet")
 #define STATIC_TITLE_ANNO NS_LITERAL_CSTRING("bookmarks/staticTitle")
 
@@ -950,7 +950,7 @@ BookmarkContentSink::HandleLinkBegin(const nsIParserNode& node)
 
     // post data
     if (!postData.IsEmpty()) {
-      mAnnotationService->SetPageAnnotationString(frame.mPreviousLink, POST_DATA_ANNO,
+      mAnnotationService->SetItemAnnotationString(frame.mPreviousId, POST_DATA_ANNO,
                                                   postData, 0,
                                                   nsIAnnotationService::EXPIRE_NEVER);
     }
@@ -1855,12 +1855,12 @@ nsPlacesImportExportService::WriteItem(nsINavHistoryResultNode* aItem,
   NS_ENSURE_SUCCESS(rv, rv);
   
   PRBool hasPostData;
-  rv = mAnnotationService->PageHasAnnotation(pageURI, POST_DATA_ANNO,
+  rv = mAnnotationService->ItemHasAnnotation(itemId, POST_DATA_ANNO,
                                              &hasPostData);
   NS_ENSURE_SUCCESS(rv, rv);
   if (hasPostData) {
     nsAutoString postData;
-    rv = mAnnotationService->GetPageAnnotationString(pageURI, POST_DATA_ANNO,
+    rv = mAnnotationService->GetItemAnnotationString(itemId, POST_DATA_ANNO,
                                                      postData);
     NS_ENSURE_SUCCESS(rv, rv);
     rv = aOutput->Write(kPostDataAttribute, sizeof(kPostDataAttribute)-1, &dummy);

browser/components/places/src/nsPlacesTransactionsService.js

@@ -116,12 +116,12 @@ placesTransactionsService.prototype = {
     return new placesEditItemDescriptionTransactions(aItemId, aDescription);
   },
 
-  editBookmarkKeyword: function placesEditBkmkKwd(id, newKeyword) {
-    return new placesEditBookmarkKeywordTransactions(id, newKeyword);
+  editBookmarkKeyword: function placesEditBkmkKwd(aItemId, newKeyword) {
+    return new placesEditBookmarkKeywordTransactions(aItemId, newKeyword);
   },
 
-  editURIPostData: function placesEditURIPdata(aURI, aPostData) {
-    return new placesEditURIPostDataTransactions(aURI, aPostData);
+  editBookmarkPostData: function placesEditBookmarkPostdata(aItemId, aPostData) {
+    return new placesEditBookmarkPostDataTransactions(aItemId, aPostData);
   },
 
   editLivemarkSiteURI: function placesEditLvmkSiteURI(folderId, uri) {
@@ -206,7 +206,7 @@ placesBaseTransaction.prototype = {
   },
 
   // nsITransaction
-  redoTransaction: function PIT_redoTransaction() {
+  redoTransaction: function PBT_redoTransaction() {
     throw Cr.NS_ERROR_NOT_IMPLEMENTED;
   },
 
@@ -640,23 +640,23 @@ placesEditBookmarkKeywordTransactions.prototype = {
   }
 };
 
-function placesEditURIPostDataTransactions(aURI, aPostData) {
-  this._uri = aURI;
+function placesEditBookmarkPostDataTransactions(aItemId, aPostData) {
+  this.id = aItemId;
   this._newPostData = aPostData;
   this._oldPostData = null;
   this.redoTransaction = this.doTransaction;
 }
 
-placesEditURIPostDataTransactions.prototype = {
+placesEditBookmarkPostDataTransactions.prototype = {
   __proto__: placesBaseTransaction.prototype,
 
   doTransaction: function PEUPDT_doTransaction() {
-    this._oldPostData = PlacesUtils.getPostDataForURI(this._uri);
-    PlacesUtils.setPostDataForURI(this._uri, this._newPostData);
+    this._oldPostData = PlacesUtils.getPostDataForBookmark(this._id);
+    PlacesUtils.setPostDataForBookmark(this.id, this._newPostData);
   },
 
   undoTransaction: function PEUPDT_undoTransaction() {
-    PlacesUtils.setPostDataForURI(this._uri, this._oldPostData);
+    PlacesUtils.setPostDataForBookmark(this.id, this._oldPostData);
   }
 };
 

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

@@ -38,9 +38,9 @@
  * ***** END LICENSE BLOCK ***** */
 
 const NS_APP_USER_PROFILE_50_DIR = "ProfD";
-const Ci = Components.interfaces;
-const Cc = Components.classes;
-const Cr = Components.results;
+var Ci = Components.interfaces;
+var Cc = Components.classes;
+var Cr = Components.results;
 
 function LOG(aMsg) {
   aMsg = ("*** PLACES TESTS: " + aMsg);

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

@@ -0,0 +1,122 @@
+/* -*- 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.com code.
+ *
+ * The Initial Developer of the Original Code is Mozilla Corp.
+ * Portions created by the Initial Developer are Copyright (C) 2007
+ * 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 ***** */
+
+version(170);
+
+var loader = Cc["@mozilla.org/moz/jssubscript-loader;1"].
+             getService(Ci.mozIJSSubScriptLoader);
+loader.loadSubScript("chrome://global/content/debug.js");
+loader.loadSubScript("chrome://browser/content/places/utils.js");
+
+const bmsvc = PlacesUtils.bookmarks;
+const testFolderId = PlacesUtils.bookmarksMenuFolderId;
+
+// main
+function run_test() {
+
+  var testURI = uri("http://foo.com");
+
+  /*
+  1. Create a bookmark for a URI, with a keyword and post data.
+  2. Create a bookmark for the same URI, with a different keyword and different post data.
+  3. Confirm that our method for getting a URI+postdata retains bookmark affinity.
+  */
+  var bm1 = bmsvc.insertBookmark(testFolderId, testURI, -1, "blah");
+  bmsvc.setKeywordForBookmark(bm1, "foo");
+  PlacesUtils.setPostDataForBookmark(bm1, "pdata1");
+  var bm2 = bmsvc.insertBookmark(testFolderId, testURI, -1, "blah");
+  bmsvc.setKeywordForBookmark(bm2, "bar");
+  PlacesUtils.setPostDataForBookmark(bm2, "pdata2");
+
+  // check kw, pd for bookmark 1
+  var url, postdata;
+  [url, postdata] = PlacesUtils.getURLAndPostDataForKeyword("foo");
+  do_check_eq(testURI.spec, url);
+  do_check_eq(postdata, "pdata1");
+
+  // check kw, pd for bookmark 2
+  [url, postdata] = PlacesUtils.getURLAndPostDataForKeyword("bar");
+  do_check_eq(testURI.spec, url);
+  do_check_eq(postdata, "pdata2");
+
+  // cleanup
+  bmsvc.removeItem(bm1);
+  bmsvc.removeItem(bm2);
+
+  /*
+  1. Create two bookmarks with the same URI and keyword.
+  2. Confirm that the most recently created one is returned for that keyword.
+  */
+  var bm1 = bmsvc.insertBookmark(testFolderId, testURI, -1, "blah");
+  bmsvc.setKeywordForBookmark(bm1, "foo");
+  PlacesUtils.setPostDataForBookmark(bm1, "pdata1");
+  var bm2 = bmsvc.insertBookmark(testFolderId, testURI, -1, "blah");
+  bmsvc.setKeywordForBookmark(bm2, "foo");
+  PlacesUtils.setPostDataForBookmark(bm2, "pdata2");
+
+  [url, postdata] = PlacesUtils.getURLAndPostDataForKeyword("foo");
+  do_check_eq(testURI.spec, url);
+  do_check_eq(postdata, "pdata2");
+
+  // cleanup
+  bmsvc.removeItem(bm1);
+  bmsvc.removeItem(bm2);
+
+  /*
+  1. Create two bookmarks with the same URI and keyword.
+  2. Modify the first-created bookmark.
+  3. Confirm that the most recently modified one is returned for that keyword.
+  */
+  var bm1 = bmsvc.insertBookmark(testFolderId, testURI, -1, "blah");
+  bmsvc.setKeywordForBookmark(bm1, "foo");
+  PlacesUtils.setPostDataForBookmark(bm1, "pdata1");
+  var bm2 = bmsvc.insertBookmark(testFolderId, testURI, -1, "blah");
+  bmsvc.setKeywordForBookmark(bm2, "foo");
+  PlacesUtils.setPostDataForBookmark(bm2, "pdata2");
+
+  // modify the older bookmark
+  bmsvc.setItemTitle(bm1, "change");
+
+  [url, postdata] = PlacesUtils.getURLAndPostDataForKeyword("foo");
+  do_check_eq(testURI.spec, url);
+  do_check_eq(postdata, "pdata1");
+
+  // cleanup
+  bmsvc.removeItem(bm1);
+  bmsvc.removeItem(bm2);
+}

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

@@ -81,7 +81,7 @@ try {
 
 const DESCRIPTION_ANNO = "bookmarkProperties/description";
 const LOAD_IN_SIDEBAR_ANNO = "bookmarkProperties/loadInSidebar";
-const POST_DATA_ANNO = "URIProperties/POSTData";
+const POST_DATA_ANNO = "bookmarkProperties/POSTData";
 const LAST_CHARSET_ANNO = "URIProperties/characterSet";
 
 // main
@@ -241,11 +241,12 @@ function testCanonicalBookmarks(aFolder) {
   do_check_eq(testBookmark1.lastModified/1000000, 1177375423);
 
   // post data
-  var pageURI = iosvc.newURI(testBookmark1.uri, "", null);
-  do_check_true(annosvc.pageHasAnnotation(pageURI, POST_DATA_ANNO));
+  do_check_true(annosvc.itemHasAnnotation(testBookmark1.itemId, POST_DATA_ANNO));
   do_check_eq("hidden1%3Dbar&text1%3D%25s",
-              annosvc.getPageAnnotation(pageURI, POST_DATA_ANNO));
+              annosvc.getItemAnnotation(testBookmark1.itemId, POST_DATA_ANNO));
+
   // last charset 
+  var pageURI = iosvc.newURI(testBookmark1.uri, "", null);
   do_check_true(annosvc.pageHasAnnotation(pageURI, LAST_CHARSET_ANNO));
   do_check_eq("ISO-8859-1", annosvc.getPageAnnotation(pageURI,
                                                       LAST_CHARSET_ANNO));

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

@@ -381,6 +381,17 @@ function run_test() {
   do_check_eq(observer._itemChangedId, bId);
   do_check_true(!mss.hasMicrosummary(bId));
 
-  // Testing edit Post Data...
-  // mmm.. cant figure out a good way to test this.
+  // Testing edit Post Data
+  const POST_DATA_ANNO = "bookmarkProperties/POSTData";
+  var postData = "foo";
+  var postDataURI = uri("http://foo.com");
+  ptSvc.doTransaction(
+    ptSvc.createItem(postDataURI, root, -1, "postdata test", null, null, null));
+  var postDataId = (bmsvc.getBookmarkIdsForURI(postDataURI,{}))[0];
+  var postDataTxn = ptSvc.editBookmarkPostData(postDataId, postData);
+  postDataTxn.doTransaction();
+  do_check_true(annotationService.itemHasAnnotation(postDataId, POST_DATA_ANNO))
+  do_check_eq(annotationService.getItemAnnotation(postDataId, POST_DATA_ANNO), postData);
+  postDataTxn.undoTransaction();
+  do_check_false(annotationService.itemHasAnnotation(postDataId, POST_DATA_ANNO))
 }

toolkit/components/places/src/nsNavBookmarks.cpp

@@ -139,6 +139,8 @@ nsNavBookmarks::Init()
   NS_ENSURE_SUCCESS(rv, rv);
 
   // mDBFindURIBookmarks
+  // NOTE: Do not modify the ORDER BY segment of the query, as certain
+  // features depend on it. See bug 398914 for an example.
   rv = dbConn->CreateStatement(NS_LITERAL_CSTRING(
       "SELECT a.id "
       "FROM moz_bookmarks a, moz_places h "