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
--- 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
*