Bug 1120110 - Consistently save pages bookmarked using 'Bookmark This Page' anywhere into the 'Other Bookmarks' folder. r?mak draft
authorMike de Boer <mdeboer@mozilla.com>
Mon, 23 Oct 2017 14:10:04 +0200
changeset 684706 9a767fa1220ee90228c643123ef9fbe573c68cff
parent 684705 fc69caec1db589e48b0975021d83bbc8e5bba3bc
child 736935 ca7eb9bb6a118b6207442931322c5c7e73ed79da
push id85694
push usermdeboer@mozilla.com
push dateMon, 23 Oct 2017 12:16:58 +0000
reviewersmak
bugs1120110
milestone58.0a1
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
browser/base/content/browser-places.js
browser/base/content/browser-sets.inc
browser/base/content/nsContextMenu.js
browser/base/content/test/general/browser_bookmark_titles.js
toolkit/modules/NewTabUtils.jsm
--- a/browser/base/content/browser-places.js
+++ b/browser/base/content/browser-places.js
@@ -399,30 +399,27 @@ function isVisible(element) {
 }
 
 var PlacesCommandHook = {
   /**
    * Adds a bookmark to the page loaded in the given browser.
    *
    * @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
    *        Option to provide a URL to bookmark rather than the current page
    * @param [optional] aTitle
    *        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;
     }
 
     // If aUrl is provided, we want to bookmark that url rather than the
     // the current page
     var uri = aUrl ? Services.io.newURI(aUrl) : aBrowser.currentURI;
     var itemId = PlacesUtils.getMostRecentBookmarkForURI(uri);
     let isNewBookmark = itemId == -1;
@@ -445,20 +442,19 @@ var PlacesCommandHook = {
 
       if (aShowEditUI) {
         // If we bookmark the page here but open right into a cancelable
         // state (i.e. new bookmark in Library), start batching here so
         // all of the actions can be undone in a single undo step.
         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);
       itemId = txn.item.id;
       // Set the character-set.
       if (charset && !PrivateBrowsingUtils.isBrowserPrivate(aBrowser))
         PlacesUtils.setCharsetForURI(uri, charset);
     }
@@ -480,26 +476,24 @@ var PlacesCommandHook = {
 
     // Fall back to showing the panel over the content area.
     await StarUI.showEditBookmarkPopup(itemId, aBrowser, "overlap",
                                        isNewBookmark, uri);
   },
 
   // 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;
       let charset = null;
 
       let docInfo = aUrl ? {} : await this._getPageDetails(aBrowser);
 
       try {
@@ -566,24 +560,16 @@ var PlacesCommandHook = {
         resolve(msg.data);
       });
 
       mm.sendAsyncMessage("Bookmarks:GetPageDetails", { });
     });
   },
 
   /**
-   * 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
    *        The folder in which to create a new bookmark if aURL isn't
    *        bookmarked.
    * @param url (string)
    *        the address of the link target
    * @param title
    *        The link text
@@ -1816,17 +1802,17 @@ var BookmarkingUI = {
   onStarCommand(aEvent) {
     // Ignore non-left clicks on the star, or if we are updating its state.
     if (!this._pendingUpdate && (aEvent.type != "click" || aEvent.button == 0)) {
       let isBookmarked = this._itemGuids.size > 0;
       if (!isBookmarked) {
         BrowserUtils.setToolbarButtonHeightProperty(this.star);
         this.star.setAttribute("animate", "true");
       }
-      PlacesCommandHook.bookmarkCurrentPage(true);
+      PlacesCommandHook.bookmarkPage(gBrowser.selectedBrowser, true);
     }
   },
 
   onCurrentPageContextPopupShowing() {
     this.updateBookmarkPageMenuItem();
   },
 
   handleEvent: function BUI_handleEvent(aEvent) {
--- a/browser/base/content/browser-sets.inc
+++ b/browser/base/content/browser-sets.inc
@@ -53,17 +53,17 @@
     <command id="cmd_findPrevious"
              oncommand="gFindBar.onFindAgainCommand(true);"
              observes="isImage"/>
 #ifdef XP_MACOSX
     <command id="cmd_findSelection" oncommand="gFindBar.onFindSelectionCommand();"/>
 #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"
              oncommand="PlacesCommandHook.bookmarkCurrentPages();"/>
     <command id="Browser:Home"    oncommand="BrowserHome();"/>
     <command id="Browser:Back"    oncommand="BrowserBack();" disabled="true"/>
     <command id="Browser:BackOrBackDuplicate" oncommand="BrowserBack(event);" disabled="true">
       <observes element="Browser:Back" attribute="disabled"/>
--- a/browser/base/content/nsContextMenu.js
+++ b/browser/base/content/nsContextMenu.js
@@ -1407,17 +1407,17 @@ nsContextMenu.prototype = {
     var newWindowPref = gPrefService.getIntPref("browser.link.open_newwindow");
     var where = newWindowPref == 3 ? "tab" : "window";
 
     openUILinkIn(uri, where);
   },
 
   bookmarkThisPage: function CM_bookmarkThisPage() {
     window.top.PlacesCommandHook
-              .bookmarkPage(this.browser, PlacesUtils.bookmarksMenuFolderId, true)
+              .bookmarkPage(this.browser, true)
               .catch(Components.utils.reportError);
   },
 
   bookmarkLink: function CM_bookmarkLink() {
     window.top.PlacesCommandHook.bookmarkLink(PlacesUtils.bookmarksMenuFolderId,
                                               this.linkURL, this.linkTextStr);
   },
 
--- a/browser/base/content/test/general/browser_bookmark_titles.js
+++ b/browser/base/content/test/general/browser_bookmark_titles.js
@@ -91,17 +91,17 @@ add_task(async function check_override_b
 // Bookmark a page and confirm that the new bookmark has the expected title.
 // (Then delete the bookmark.)
 async function checkBookmarkedPageTitle(url, default_title, overridden_title) {
   let promiseBookmark = PlacesTestUtils.waitForNotification("onItemAdded",
     (id, parentId, index, type, itemUrl) => itemUrl.equals(Services.io.newURI(url)));
 
   // 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});
 
   Assert.ok(bookmark, "Found the expected bookmark");
   Assert.equal(bookmark.title, overridden_title, "Bookmark got a good overridden title.");
   Assert.equal(default_title, gBrowser.selectedBrowser.contentTitle,
     "Sanity check that the content is providing us with the correct title");
@@ -114,17 +114,17 @@ async function checkBookmarkedPageTitle(
 // Bookmark the current page and confirm that the new bookmark has the expected
 // title. (Then delete the bookmark.)
 async function checkBookmark(url, expected_title) {
   Assert.equal(gBrowser.selectedBrowser.currentURI.spec, url,
     "Trying to bookmark the expected uri");
 
   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});
 
   Assert.ok(bookmark, "Found the expected bookmark");
   Assert.equal(bookmark.title, expected_title, "Bookmark got a good default title.");
 
   await PlacesUtils.bookmarks.remove(bookmark);
--- a/toolkit/modules/NewTabUtils.jsm
+++ b/toolkit/modules/NewTabUtils.jsm
@@ -1252,17 +1252,16 @@ var ActivityStreamLinks = {
    *          a <browser> element
    *
    * @returns {Promise} Returns a promise set to an object representing the bookmark
    */
   addBookmark(aData, aBrowser) {
       const {url, title} = aData;
       return aBrowser.ownerGlobal.PlacesCommandHook.bookmarkPage(
               aBrowser,
-              undefined,
               true,
               url,
               title);
   },
 
   /**
    * Removes a bookmark
    *