Bug 1194568 - Renaming live bookmark while adding it renames the wrong one. r=ttaubert a=ritu
authorMarco Bonardo <mbonardo@mozilla.com>
Wed, 02 Sep 2015 16:41:10 +0200
changeset 283760 1620ef4c134182ba58f3c9be80d7e969a26a5e50
parent 283759 72e917cba6a75095e521025a16b6c2d0516ba1b6
child 283761 1712da2be079a391699f5cecf1cb3b6be5a13b76
push id897
push userjlund@mozilla.com
push dateMon, 14 Sep 2015 18:56:12 +0000
treeherdermozilla-release@9411e2d2b214 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersttaubert, ritu
bugs1194568
milestone41.0
Bug 1194568 - Renaming live bookmark while adding it renames the wrong one. r=ttaubert a=ritu
browser/components/places/content/bookmarkProperties.js
browser/components/places/tests/browser/browser.ini
browser/components/places/tests/browser/browser_bookmarkProperties_addLivemark.js
browser/components/places/tests/browser/head.js
toolkit/components/places/PlacesUtils.jsm
--- a/browser/components/places/content/bookmarkProperties.js
+++ b/browser/components/places/content/bookmarkProperties.js
@@ -578,58 +578,54 @@ var BookmarkPropertiesPanel = {
     if (this._description)
       annotations.push(this._getDescriptionAnnotation(this._description));
 
     return new PlacesCreateFolderTransaction(this._title, aContainer,
                                              aIndex, annotations,
                                              childItemsTransactions);
   },
 
-  /**
-   * Returns a transaction for creating a new live-bookmark item representing
-   * the various fields and opening arguments of the dialog.
-   */
-  _getCreateNewLivemarkTransaction:
-  function BPP__getCreateNewLivemarkTransaction(aContainer, aIndex) {
-    return new PlacesCreateLivemarkTransaction(this._feedURI, this._siteURI,
-                                               this._title,
-                                               aContainer, aIndex);
-  },
-
-  _createNewItem: function BPP__getCreateItemTransaction() {
-    var [container, index] = this._getInsertionPointDetails();
-    var txn;
-
+  _createNewItem: Task.async(function* () {
+    let [container, index] = this._getInsertionPointDetails();
+    let txn;
     switch (this._itemType) {
       case BOOKMARK_FOLDER:
         txn = this._getCreateNewFolderTransaction(container, index);
         break;
       case LIVEMARK_CONTAINER:
-        txn = this._getCreateNewLivemarkTransaction(container, index);
+        txn = new PlacesCreateLivemarkTransaction(this._feedURI, this._siteURI,
+                                                  this._title, container, index);
         break;
       default: // BOOKMARK_ITEM
         txn = this._getCreateNewBookmarkTransaction(container, index);
     }
 
     PlacesUtils.transactionManager.doTransaction(txn);
-    this._itemId = PlacesUtils.bookmarks.getIdForItemAt(container, index);
+    // This is a temporary hack until we use PlacesTransactions.jsm
+    if (txn._promise) {
+      yield txn._promise;
+    }
+
+    let folderGuid = yield PlacesUtils.promiseItemGuid(container);
+    let bm = yield PlacesUtils.bookmarks.fetch({
+      parentGuid: folderGuid,
+      index: PlacesUtils.bookmarks.DEFAULT_INDEX
+    });
+    this._itemId = yield PlacesUtils.promiseItemId(bm.guid);
 
     return Object.freeze({
       itemId: this._itemId,
-      get bookmarkGuid() {
-        throw new Error("Node-like bookmarkGuid getter called even though " +
-                        "async transactions are disabled");
-      },
+      bookmarkGuid: bm.guid,
       title: this._title,
       uri: this._uri ? this._uri.spec : "",
       type: this._itemType == BOOKMARK_ITEM ?
               Ci.nsINavHistoryResultNode.RESULT_TYPE_URI :
               Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER
     });
-  },
+  }),
 
   _promiseNewItem: Task.async(function* () {
     if (!PlacesUIUtils.useAsyncTransactions)
       return this._createNewItem();
 
     let txnFunc =
       { [BOOKMARK_FOLDER]: PlacesTransactions.NewFolder,
         [LIVEMARK_CONTAINER]: PlacesTransactions.NewLivemark,
--- a/browser/components/places/tests/browser/browser.ini
+++ b/browser/components/places/tests/browser/browser.ini
@@ -20,16 +20,17 @@ support-files =
 [browser_435851_copy_query.js]
 [browser_475045.js]
 [browser_555547.js]
 [browser_bookmarklet_windowOpen.js]
 support-files =
   pageopeningwindow.html
 [browser_bookmarkProperties_addFolderDefaultButton.js]
 [browser_bookmarkProperties_addKeywordForThisSearch.js]
+[browser_bookmarkProperties_addLivemark.js]
 [browser_bookmarkProperties_editTagContainer.js]
 [browser_bookmarkProperties_readOnlyRoot.js]
 [browser_bookmarksProperties.js]
 [browser_drag_bookmarks_on_toolbar.js]
 skip-if = e10s # Bug ?????? - test fails - "Number of dragged items should be the same. - Got 0, expected 1"
 [browser_forgetthissite_single.js]
 [browser_history_sidebar_search.js]
 skip-if = e10s && (os == 'linux' || os == 'mac') # Bug 1116457
new file mode 100644
--- /dev/null
+++ b/browser/components/places/tests/browser/browser_bookmarkProperties_addLivemark.js
@@ -0,0 +1,39 @@
+"use strict"
+
+add_task(function* () {
+  info("Add a live bookmark editing its data");
+
+  yield withSidebarTree("bookmarks", function* (tree) {
+    let itemId = PlacesUIUtils.leftPaneQueries["UnfiledBookmarks"];
+    tree.selectItems([itemId]);
+
+    yield withBookmarksDialog(
+      true,
+      function openDialog() {
+        PlacesCommandHook.addLiveBookmark("http://livemark.com/",
+                                          "livemark", "description");
+      },
+      function* test(dialogWin) {
+        let promiseTitleChangeNotification = promiseBookmarksNotification(
+          "onItemChanged", (itemId, prop, isAnno, val) => prop == "title" && val == "modified");
+
+        fillBookmarkTextField("editBMPanel_namePicker", "modified", dialogWin);
+
+        yield promiseTitleChangeNotification;
+
+        let bookmark = yield PlacesUtils.bookmarks.fetch({
+          parentGuid: PlacesUtils.bookmarks.toolbarGuid,
+          index: PlacesUtils.bookmarks.DEFAULT_INDEX
+        });
+
+        is(bookmark.title, "modified", "folder name has been edited");
+
+        let livemark = yield PlacesUtils.livemarks.getLivemark({
+          guid: bookmark.guid
+        });
+        is(livemark.feedURI.spec, "http://livemark.com/", "livemark has the correct url");
+        is(livemark.title, "modified", "livemark has the correct title");
+      }
+    );
+  });
+});
--- a/browser/components/places/tests/browser/head.js
+++ b/browser/components/places/tests/browser/head.js
@@ -292,17 +292,17 @@ let withBookmarksDialog = Task.async(fun
   let closed = false;
   let dialogPromise = new Promise(resolve => {
     Services.ww.registerNotification(function winObserver(subject, topic, data) {
       if (topic == "domwindowopened") {
         let win = subject.QueryInterface(Ci.nsIDOMWindow);
         win.addEventListener("load", function load() {
           win.removeEventListener("load", load);
           ok(win.location.href.startsWith("chrome://browser/content/places/bookmarkProperties"),
-             "The bookmark properties dialog is ready");
+             "The bookmark properties dialog is open");
           // This is needed for the overlay.
           waitForFocus(() => {
             resolve(win);
           }, win);
         });
       } else if (topic == "domwindowclosed") {
         Services.ww.unregisterNotification(winObserver);
         closed = true;
@@ -313,17 +313,19 @@ let withBookmarksDialog = Task.async(fun
   info("withBookmarksDialog: opening the dialog");
   // The dialog might be modal and could block our events loop, so executeSoon.
   executeSoon(openFn);
 
   info("withBookmarksDialog: waiting for the dialog");
   let dialogWin = yield dialogPromise;
 
   // Ensure overlay is loaded
-  ok(dialogWin.gEditItemOverlay.initialized, "EditItemOverlay is initialized");
+  info("waiting for the overlay to be loaded");
+  yield waitForCondition(() => dialogWin.gEditItemOverlay.initialized,
+                         "EditItemOverlay should be initialized");
 
   info("withBookmarksDialog: executing the task");
   try {
     yield taskFn(dialogWin);
   } finally {
     if (!closed) {
       if (!autoCancel) {
         ok(false, "The test should have closed the dialog!");
--- a/toolkit/components/places/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -2896,17 +2896,17 @@ this.PlacesCreateLivemarkTransaction =
   this.item.annotations = aAnnotations;
 }
 
 PlacesCreateLivemarkTransaction.prototype = {
   __proto__: BaseTransaction.prototype,
 
   doTransaction: function CLTXN_doTransaction()
   {
-    PlacesUtils.livemarks.addLivemark(
+    this._promise = PlacesUtils.livemarks.addLivemark(
       { title: this.item.title
       , feedURI: this.item.feedURI
       , parentId: this.item.parentId
       , index: this.item.index
       , siteURI: this.item.siteURI
       }).then(aLivemark => {
         this.item.id = aLivemark.id;
         if (this.item.annotations && this.item.annotations.length > 0) {
@@ -2915,17 +2915,17 @@ PlacesCreateLivemarkTransaction.prototyp
         }
       }, Cu.reportError);
   },
 
   undoTransaction: function CLTXN_undoTransaction()
   {
     // The getLivemark callback may fail, but it is used just to serialize,
     // so it doesn't matter.
-    PlacesUtils.livemarks.getLivemark({ id: this.item.id })
+    this._promise = PlacesUtils.livemarks.getLivemark({ id: this.item.id })
       .then(null, null).then( () => {
         PlacesUtils.bookmarks.removeItem(this.item.id);
       });
   }
 };
 
 
 /**