Bug 1425555 - Avoid a database fetch after showing the new bookmark dialog. r?mak draft
authorMark Banner <standard8@mozilla.com>
Tue, 20 Mar 2018 23:52:58 +0000
changeset 770320 0696dd1a2af895942986806b9ff978956aaeb617
parent 769888 827c686c570935483c3ad8022aa92b9e005574c3
push id103376
push userbmo:standard8@mozilla.com
push dateWed, 21 Mar 2018 00:01:07 +0000
reviewersmak
bugs1425555
milestone61.0a1
Bug 1425555 - Avoid a database fetch after showing the new bookmark dialog. r?mak MozReview-Commit-ID: 1pjynkFFSOd
browser/components/places/PlacesUIUtils.jsm
browser/components/places/content/bookmarkProperties.js
browser/components/places/content/controller.js
browser/components/places/tests/browser/browser.ini
browser/components/places/tests/browser/browser_library_new_bookmark.js
--- 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");
+    });
+});