Bug 1383138 - 'Bookmark all tabs' dialog is broken with async Places transactions. r=adw draft
authorMarco Bonardo <mbonardo@mozilla.com>
Tue, 25 Jul 2017 22:24:05 +0200
changeset 615841 75b2ad7b1bdaa57ee038b2d98ec08709fdb51596
parent 615105 07484bfdb96bc7297c404e377eea93f1d8ca4442
child 615846 bcad7bf7011f510f574c49b168bb4b4ccf5cc8f2
push id70491
push usermak77@bonardo.net
push dateWed, 26 Jul 2017 11:31:38 +0000
reviewersadw
bugs1383138
milestone56.0a1
Bug 1383138 - 'Bookmark all tabs' dialog is broken with async Places transactions. r=adw MozReview-Commit-ID: IERtEaCCbjA
browser/components/places/content/bookmarkProperties.js
browser/components/places/tests/browser/browser.ini
browser/components/places/tests/browser/browser_bookmarkProperties_bookmarkAllTabs.js
--- a/browser/components/places/content/bookmarkProperties.js
+++ b/browser/components/places/content/bookmarkProperties.js
@@ -545,18 +545,19 @@ var BookmarkPropertiesPanel = {
 
   /**
    * Returns a childItems-transactions array representing the URIList with
    * which the dialog has been opened.
    */
   _getTransactionsForURIList: function BPP__getTransactionsForURIList() {
     var transactions = [];
     for (let uri of this._URIs) {
-      // uri should be an object in the form { url, title }. Though add-ons
+      // uri should be an object in the form { uri, title }. Though add-ons
       // could still use the legacy form, where it's an nsIURI.
+      // TODO: Remove This from v57 on.
       let [_uri, _title] = uri instanceof Ci.nsIURI ?
         [uri, this._getURITitleFromHistory(uri)] : [uri.uri, uri.title];
 
       let createTxn =
         new PlacesCreateBookmarkTransaction(_uri, -1,
                                             PlacesUtils.bookmarks.DEFAULT_INDEX,
                                             _title);
       transactions.push(createTxn);
@@ -659,20 +660,22 @@ var BookmarkPropertiesPanel = {
     } else if (this._itemType == LIVEMARK_CONTAINER) {
       info.feedUrl = this._feedURI;
       if (this._siteURI)
         info.siteUrl = this._siteURI;
 
       itemGuid = await PlacesTransactions.NewLivemark(info).transact();
     } else if (this._itemType == BOOKMARK_FOLDER) {
       itemGuid = await PlacesTransactions.NewFolder(info).transact();
-      for (let uri of this._URIs) {
-        let placeInfo = await PlacesUtils.history.fetch(uri);
-        let title = placeInfo ? placeInfo.title : "";
-        await PlacesTransactions.transact({ parentGuid: itemGuid, uri, title });
+      // URIs is an array of objects in the form { uri, title }.  It is still
+      // named URIs because for backwards compatibility it could also be an
+      // array of nsIURIs. TODO: Fix the property names from v57.
+      for (let { uri: url, title } of this._URIs) {
+        await PlacesTransactions.NewBookmark({ parentGuid: itemGuid, url, title })
+                                .transact();
       }
     } else {
       throw new Error(`unexpected value for _itemType:  ${this._itemType}`);
     }
 
     this._itemGuid = itemGuid;
     this._itemId = await PlacesUtils.promiseItemId(itemGuid);
     return Object.freeze({
--- a/browser/components/places/tests/browser/browser.ini
+++ b/browser/components/places/tests/browser/browser.ini
@@ -24,16 +24,17 @@ subsuite = clipboard
 [browser_555547.js]
 [browser_addBookmarkForFrame.js]
 [browser_bookmarklet_windowOpen.js]
 support-files =
   pageopeningwindow.html
 [browser_bookmarkProperties_addFolderDefaultButton.js]
 [browser_bookmarkProperties_addKeywordForThisSearch.js]
 [browser_bookmarkProperties_addLivemark.js]
+[browser_bookmarkProperties_bookmarkAllTabs.js]
 [browser_bookmarkProperties_editTagContainer.js]
 [browser_bookmarkProperties_readOnlyRoot.js]
 [browser_bookmarksProperties.js]
 [browser_drag_bookmarks_on_toolbar.js]
 [browser_forgetthissite_single.js]
 [browser_history_sidebar_search.js]
 [browser_library_batch_delete.js]
 [browser_library_commands.js]
new file mode 100644
--- /dev/null
+++ b/browser/components/places/tests/browser/browser_bookmarkProperties_bookmarkAllTabs.js
@@ -0,0 +1,44 @@
+"use strict"
+
+const TEST_URLS = [
+  "about:robots",
+  "about:mozilla",
+];
+
+add_task(async function() {
+  let tabs = [];
+  for (let url of TEST_URLS) {
+    tabs.push(await BrowserTestUtils.openNewForegroundTab(gBrowser, url));
+  }
+  registerCleanupFunction(async function() {
+    for (let tab of tabs) {
+      await BrowserTestUtils.removeTab(tab)
+    }
+  });
+
+  await withBookmarksDialog(true,
+    function open() {
+      document.getElementById("Browser:BookmarkAllTabs").doCommand();
+    },
+    async dialog => {
+      let acceptBtn = dialog.document.documentElement.getButton("accept");
+      ok(!acceptBtn.disabled, "Accept button is enabled");
+
+      let namepicker = dialog.document.getElementById("editBMPanel_namePicker");
+      Assert.ok(!namepicker.readOnly, "Name field is writable");
+      let folderName = dialog.document.getElementById("stringBundle").getString("bookmarkAllTabsDefault");
+      Assert.equal(namepicker.value, folderName, "Name field is correct.");
+
+      let promiseTitleChange = promiseBookmarksNotification(
+        "onItemChanged", (id, prop, isAnno, val) => prop == "title" && val == "folder");
+      fillBookmarkTextField("editBMPanel_namePicker", "folder", dialog);
+      await promiseTitleChange;
+    },
+    dialog => {
+      let savedItemId = dialog.gEditItemOverlay.itemId;
+      ok(savedItemId > 0, "Found the itemId");
+      return PlacesTestUtils.waitForNotification("onItemRemoved",
+                                                 id => id === savedItemId);
+    }
+  );
+});