Bug 1120110 - Consistently save pages bookmarked using 'Bookmark This Page' anywhere into the 'Other Bookmarks' folder. r=mak
authorMike de Boer <mdeboer@mozilla.com>
Mon, 23 Oct 2017 14:10:04 +0200
changeset 387644 7f812d58e5fe2aeb0f6a119f9f3678528a153c1e
parent 387643 e1e574000af4d1d989d2ca3435dc7e7898691893
child 387645 38d2f90e0bc5c0ab9d15457ae0b5c491be2ba075
push id53840
push usermdeboer@mozilla.com
push dateMon, 23 Oct 2017 13:36:09 +0000
treeherderautoland@7f812d58e5fe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1120110
milestone58.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
    *