Bug 1120110 - Consistently save pages bookmarked using 'Bookmark This Page' anywhere into the 'Other Bookmarks' folder. r=mak

Since we're not passing an optional parent folder around anymore, this patch also
removes PlacesCommandHook.bookmarkCurrentPage() in favor of a simplified
PlacesCommandHook.bookmarkPage() signature.

MozReview-Commit-ID: HmzwmATgQyw

--HG--
extra : rebase_source : 384e7dba2ab58b212273d39bf6344424a9841e4f

browser/base/content/browser-places.js

@@ -404,9 +404,6 @@ var PlacesCommandHook = {
    *
    * @param aBrowser
    *        a <browser> element.
-   * @param [optional] aParent
-   *        The folder in which to create a new bookmark if the page loaded in
-   *        aBrowser isn't bookmarked yet, defaults to the unfiled root.
    * @param [optional] aShowEditUI
    *        whether or not to show the edit-bookmark UI for the bookmark item
    * @param [optional] aUrl
@@ -415,9 +412,9 @@ var PlacesCommandHook = {
    *        Option to provide a title for a bookmark to use rather than the
    *        getting the current page's title
    */
-  async bookmarkPage(aBrowser, aParent, aShowEditUI, aUrl = null, aTitle = null) {
+  async bookmarkPage(aBrowser, aShowEditUI, aUrl = null, aTitle = null) {
     if (PlacesUIUtils.useAsyncTransactions) {
-      await this._bookmarkPagePT(aBrowser, aParent, aShowEditUI, aUrl, aTitle);
+      await this._bookmarkPagePT(aBrowser, aShowEditUI, aUrl, aTitle);
       return;
     }
 
@@ -450,10 +447,9 @@ var PlacesCommandHook = {
         StarUI.beginBatch();
       }
 
-      var parent = aParent !== undefined ?
-                   aParent : PlacesUtils.unfiledBookmarksFolderId;
       var descAnno = { name: PlacesUIUtils.DESCRIPTION_ANNO, value: description };
-      var txn = new PlacesCreateBookmarkTransaction(uri, parent,
+      var txn = new PlacesCreateBookmarkTransaction(uri,
+                                                    PlacesUtils.unfiledBookmarksFolderId,
                                                     PlacesUtils.bookmarks.DEFAULT_INDEX,
                                                     title, null, [descAnno]);
       PlacesUtils.transactionManager.doTransaction(txn);
@@ -485,16 +481,14 @@ var PlacesCommandHook = {
 
   // TODO: Replace bookmarkPage code with this function once legacy
   // transactions are removed.
-  async _bookmarkPagePT(aBrowser, aParentId, aShowEditUI, aUrl, aTitle) {
+  async _bookmarkPagePT(aBrowser, aShowEditUI, aUrl, aTitle) {
     // If aUrl is provided, we want to bookmark that url rather than the
     // the current page
     let url = aUrl ? new URL(aUrl) : new URL(aBrowser.currentURI.spec);
     let info = await PlacesUtils.bookmarks.fetch({ url });
     let isNewBookmark = !info;
     if (!info) {
-      let parentGuid = aParentId !== undefined ?
-                         await PlacesUtils.promiseItemGuid(aParentId) :
-                         PlacesUtils.bookmarks.unfiledGuid;
+      let parentGuid = PlacesUtils.bookmarks.unfiledGuid;
       info = { url, parentGuid };
       // Bug 1148838 - Make this code work for full page plugins.
       let description = null;
@@ -570,14 +564,6 @@ var PlacesCommandHook = {
     });
   },
 
-  /**
-   * Adds a bookmark to the page loaded in the current tab.
-   */
-  bookmarkCurrentPage: function PCH_bookmarkCurrentPage(aShowEditUI, aParent) {
-    this.bookmarkPage(gBrowser.selectedBrowser, aParent, aShowEditUI)
-        .catch(Components.utils.reportError);
-  },
-
   /**
    * Adds a bookmark to the page targeted by a link.
    * @param parentId
@@ -1821,7 +1807,7 @@ var BookmarkingUI = {
         BrowserUtils.setToolbarButtonHeightProperty(this.star);
         this.star.setAttribute("animate", "true");
       }
-      PlacesCommandHook.bookmarkCurrentPage(true);
+      PlacesCommandHook.bookmarkPage(gBrowser.selectedBrowser, true);
     }
   },
 

browser/base/content/browser-sets.inc

@@ -58,7 +58,7 @@
 #endif
     <!-- work-around bug 392512 -->
     <command id="Browser:AddBookmarkAs"
-             oncommand="PlacesCommandHook.bookmarkCurrentPage(true, PlacesUtils.bookmarksMenuFolderId);"/>
+             oncommand="PlacesCommandHook.bookmarkPage(gBrowser.selectedBrowser, true);"/>
     <!-- The command disabled state must be manually updated through
          PlacesCommandHook.updateBookmarkAllTabsCommand() -->
     <command id="Browser:BookmarkAllTabs"

browser/base/content/nsContextMenu.js

@@ -1412,7 +1412,7 @@ nsContextMenu.prototype = {
 
   bookmarkThisPage: function CM_bookmarkThisPage() {
     window.top.PlacesCommandHook
-              .bookmarkPage(this.browser, PlacesUtils.bookmarksMenuFolderId, true)
+              .bookmarkPage(this.browser, true)
               .catch(Components.utils.reportError);
   },
 

browser/base/content/test/general/browser_bookmark_titles.js

@@ -96,7 +96,7 @@ async function checkBookmarkedPageTitle(url, default_title, overridden_title) {
 
   // Here we test that if we provide a url and a title to bookmark, it will use the
   // title provided rather than the one provided by the current page
-  PlacesCommandHook.bookmarkPage(gBrowser.selectedBrowser, undefined, false, url, overridden_title);
+  PlacesCommandHook.bookmarkPage(gBrowser.selectedBrowser, false, url, overridden_title);
   await promiseBookmark;
 
   let bookmark = await PlacesUtils.bookmarks.fetch({url});
@@ -119,7 +119,7 @@ async function checkBookmark(url, expected_title) {
 
   let promiseBookmark = PlacesTestUtils.waitForNotification("onItemAdded",
     (id, parentId, index, type, itemUrl) => itemUrl.equals(gBrowser.selectedBrowser.currentURI));
-  PlacesCommandHook.bookmarkCurrentPage(false);
+  PlacesCommandHook.bookmarkPage(gBrowser.selectedBrowser);
   await promiseBookmark;
 
   let bookmark = await PlacesUtils.bookmarks.fetch({url});

toolkit/modules/NewTabUtils.jsm

@@ -1257,7 +1257,6 @@ var ActivityStreamLinks = {
       const {url, title} = aData;
       return aBrowser.ownerGlobal.PlacesCommandHook.bookmarkPage(
               aBrowser,
-              undefined,
               true,
               url,
               title);