Bug 1383138 - 'Bookmark all tabs' dialog is broken with async Places transactions. r=adw
authorMarco Bonardo <mbonardo@mozilla.com>
Tue, 25 Jul 2017 22:24:05 +0200
changeset 371409 79c3b71475c7563d725da61dbd24d18ae73caf65
parent 371408 452f3e6d539b68941487365917c437e66352401e
child 371410 7597e4363f50c118b5da0efc8d602814545649fe
push id32246
push userkwierso@gmail.com
push dateThu, 27 Jul 2017 23:10:07 +0000
treeherdermozilla-central@a4afa89bfdd1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersadw
bugs1383138
milestone56.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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);
+    }
+  );
+});