Bug 1425555 - Avoid a database fetch after showing the new bookmark dialog. r?mak
MozReview-Commit-ID: 1pjynkFFSOd
--- a/browser/components/places/PlacesUIUtils.jsm
+++ b/browser/components/places/PlacesUIUtils.jsm
@@ -257,17 +257,17 @@ var PlacesUIUtils = {
*
* @param aInfo
* Describes the item to be edited/added in the dialog.
* See documentation at the top of bookmarkProperties.js
* @param aWindow
* Owner window for the new dialog.
*
* @see documentation at the top of bookmarkProperties.js
- * @return true if any transaction has been performed, false otherwise.
+ * @return The guid of the item that was created or edited, undefined otherwise.
*/
showBookmarkDialog(aInfo, aParentWindow) {
// Preserve size attributes differently based on the fact the dialog has
// a folder picker or not, since it needs more horizontal space than the
// other controls.
let hasFolderPicker = !("hiddenRows" in aInfo) ||
!aInfo.hiddenRows.includes("folderPicker");
// Use a different chrome url to persist different sizes.
@@ -284,26 +284,26 @@ var PlacesUIUtils = {
topUndoEntry = PlacesTransactions.topUndoEntry;
batchBlockingDeferred = PromiseUtils.defer();
PlacesTransactions.batch(async () => {
await batchBlockingDeferred.promise;
});
aParentWindow.openDialog(dialogURL, "", features, aInfo);
- let performed = ("performed" in aInfo && aInfo.performed);
+ let bookmarkGuid = ("bookmarkGuid" in aInfo && aInfo.bookmarkGuid) || undefined;
batchBlockingDeferred.resolve();
- if (!performed &&
+ if (!bookmarkGuid &&
topUndoEntry != PlacesTransactions.topUndoEntry) {
PlacesTransactions.undo().catch(Cu.reportError);
}
- return performed;
+ return bookmarkGuid;
},
/**
* set and fetch a favicon. Can only be used from the parent process.
* @param browser {Browser} The XUL browser element for which we're fetching a favicon.
* @param principal {Principal} The loading principal to use for the fetch.
* @param uri {URI} The URI to fetch.
*/
--- a/browser/components/places/content/bookmarkProperties.js
+++ b/browser/components/places/content/bookmarkProperties.js
@@ -48,18 +48,18 @@
* - "title"
* - "location"
* - "description"
* - "keyword"
* - "tags"
* - "loadInSidebar"
* - "folderPicker" - hides both the tree and the menu.
*
- * window.arguments[0].performed is set to true if any transaction has
- * been performed by the dialog.
+ * window.arguments[0].bookmarkGuid is set to the guid of the item, if the
+ * dialog is accepted.
*/
/* import-globals-from editBookmark.js */
/* import-globals-from controller.js */
ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
ChromeUtils.defineModuleGetter(this, "PrivateBrowsingUtils",
"resource://gre/modules/PrivateBrowsingUtils.jsm");
@@ -81,17 +81,16 @@ var BookmarkPropertiesPanel = {
if (!this.__strings) {
this.__strings = document.getElementById("stringBundle");
}
return this.__strings;
},
_action: null,
_itemType: null,
- _itemId: -1,
_uri: null,
_loadInSidebar: false,
_title: "",
_description: "",
_URIs: [],
_keyword: "",
_postData: null,
_charSet: "",
@@ -399,24 +398,25 @@ var BookmarkPropertiesPanel = {
},
onDialogAccept() {
// We must blur current focused element to save its changes correctly
document.commandDispatcher.focusedElement.blur();
// We have to uninit the panel first, otherwise late changes could force it
// to commit more transactions.
gEditItemOverlay.uninitPanel(true);
- window.arguments[0].performed = true;
+ if (this._node.bookmarkGuid) {
+ window.arguments[0].bookmarkGuid = this._node.bookmarkGuid;
+ }
},
onDialogCancel() {
// We have to uninit the panel first, otherwise late changes could force it
// to commit more transactions.
gEditItemOverlay.uninitPanel(true);
- window.arguments[0].performed = false;
},
/**
* This method checks to see if the input fields are in a valid state.
*
* @returns true if the input is valid, false otherwise
*/
_inputIsValid: function BPP__inputIsValid() {
@@ -500,21 +500,19 @@ var BookmarkPropertiesPanel = {
info.children = this._URIs.map(item => {
return { url: item.uri, title: item.title };
});
itemGuid = await PlacesTransactions.NewFolder(info).transact();
} else {
throw new Error(`unexpected value for _itemType: ${this._itemType}`);
}
- this._itemGuid = itemGuid;
- this._itemId = await PlacesUtils.promiseItemId(itemGuid);
return Object.freeze({
- itemId: this._itemId,
- bookmarkGuid: this._itemGuid,
+ itemId: await PlacesUtils.promiseItemId(itemGuid),
+ bookmarkGuid: itemGuid,
title: this._title,
uri: this._uri ? this._uri.spec : "",
type: this._itemType == BOOKMARK_ITEM ?
Ci.nsINavHistoryResultNode.RESULT_TYPE_URI :
Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER,
parent: {
itemId: containerId,
bookmarkGuid: parentGuid,
--- a/browser/components/places/content/controller.js
+++ b/browser/components/places/content/controller.js
@@ -702,33 +702,24 @@ PlacesController.prototype = {
* @param aType
* the type of the new item (bookmark/livemark/folder)
*/
async newItem(aType) {
let ip = this._view.insertionPoint;
if (!ip)
throw Cr.NS_ERROR_NOT_AVAILABLE;
- let performed =
+ let bookmarkGuid =
PlacesUIUtils.showBookmarkDialog({ action: "add",
type: aType,
defaultInsertionPoint: ip,
hiddenRows: [ "folderPicker" ]
}, window.top);
- if (performed) {
- // Select the new item.
- // TODO (Bug 1425555): When we remove places transactions, we might be
- // able to improve showBookmarkDialog to return the guid direct, and
- // avoid the fetch.
- let insertedNode = await PlacesUtils.bookmarks.fetch({
- parentGuid: ip.guid,
- index: await ip.getIndex()
- });
-
- this._view.selectItems([insertedNode.guid], false);
+ if (bookmarkGuid) {
+ this._view.selectItems([bookmarkGuid], false);
}
},
/**
* Create a new Bookmark separator somewhere.
*/
async newSeparator() {
var ip = this._view.insertionPoint;
--- a/browser/components/places/tests/browser/browser.ini
+++ b/browser/components/places/tests/browser/browser.ini
@@ -62,16 +62,17 @@ skip-if = (os == 'win' && ccov) # Bug 14
[browser_library_infoBox.js]
skip-if = (os == 'win' && ccov) # Bug 1423667
[browser_library_left_pane_middleclick.js]
skip-if = (os == 'win' && ccov) # Bug 1423667
[browser_library_left_pane_select_hierarchy.js]
skip-if = (os == 'win' && ccov) # Bug 1423667
[browser_library_middleclick.js]
skip-if = (os == 'win' && ccov) # Bug 1423667
+[browser_library_new_bookmark.js]
[browser_library_open_leak.js]
[browser_library_openFlatContainer.js]
skip-if = (os == 'win' && ccov) # Bug 1423667
[browser_library_open_bookmark.js]
skip-if = (os == 'win' && ccov) # Bug 1423667
[browser_library_panel_leak.js]
skip-if = (os == 'win' && ccov) # Bug 1423667
[browser_library_search.js]
new file mode 100644
--- /dev/null
+++ b/browser/components/places/tests/browser/browser_library_new_bookmark.js
@@ -0,0 +1,67 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/
+ */
+
+/**
+ * Test that the a new bookmark is correctly selected after being created via
+ * the bookmark dialog.
+ */
+"use strict";
+
+add_task(async function test_open_bookmark_from_library() {
+ let bm = await PlacesUtils.bookmarks.insertTree({
+ guid: PlacesUtils.bookmarks.unfiledGuid,
+ children: [{
+ url: "https://example1.com",
+ title: "bm1",
+ }, {
+ url: "https://example2.com",
+ title: "bm2",
+ }, {
+ url: "https://example3.com",
+ title: "bm3",
+ }]
+ });
+
+ let library = await promiseLibrary("UnfiledBookmarks");
+
+ registerCleanupFunction(async function() {
+ await promiseLibraryClosed(library);
+ await PlacesUtils.bookmarks.eraseEverything();
+ });
+
+ let bmLibrary = library.ContentTree.view.view.nodeForTreeIndex(1);
+ Assert.equal(bmLibrary.title, bm[1].title, "Found bookmark in the right pane");
+
+ library.ContentTree.view.selectNode(bmLibrary);
+
+ await withBookmarksDialog(
+ false,
+ async () => {
+ // Open the context menu.
+ let placesContext = library.document.getElementById("placesContext");
+ let promisePopup = BrowserTestUtils.waitForEvent(placesContext, "popupshown");
+ synthesizeClickOnSelectedTreeCell(library.ContentTree.view, {
+ button: 2,
+ type: "contextmenu"
+ });
+
+ await promisePopup;
+ let properties = library.document.getElementById("placesContext_new:bookmark");
+ EventUtils.synthesizeMouseAtCenter(properties, {}, library);
+ },
+ async dialogWin => {
+ let promiseLocationChange = PlacesTestUtils.waitForNotification("onItemChanged",
+ (id, parentId, index, itemUrl) => itemUrl === "https://example4.com/");
+
+ fillBookmarkTextField("editBMPanel_locationField", "https://example4.com/", dialogWin, false);
+
+ EventUtils.synthesizeKey("VK_RETURN", {}, dialogWin);
+ await promiseLocationChange;
+
+ let node = library.ContentTree.view.selectedNode;
+ Assert.ok(node, "Should have a selectedNode");
+ Assert.equal(node.uri, "https://example4.com/",
+ "Should have selected the newly created bookmark");
+ });
+});