Commit a911f374 authored by Jonathan Sudiaman's avatar Jonathan Sudiaman
Browse files

Bug 1820979 - Remove instant apply bookmarks panels code r=mak 

Remove the `browser.bookmarks.editDialog.delayedApply.enabled` pref, and for all usages in conditional branches, assume that it is `true`. (Since we want to be using delayed apply logic going forward.)

`delayed_apply` spinoffs of tests are removed and merged with their instant apply counterparts. The only test removed without a corresponding spinoff is `browser_editBookmark_tags_liveUpdate.js`. That test is checking whether tags are updated in the edit dialog, if they are updated from a different place. The problem is that we don't really want this behavior anymore. It was previously set to only run with `browser.bookmarks.editDialog.delayedApply.enabled` set to `false`, but now that it's no longer configurable, it made most sense to simply get rid of this test.

https://treeherder.mozilla.org/jobs?revision=c0741eca62212a75a9dd52fc2c5c2b6c34f9f2d7&repo=try

Differential Revision: https://phabricator.services.mozilla.com/D176370
parent fd9e2d7b

browser/app/profile/firefox.js

+0 −3
Original line number Diff line number Diff line
@@ -1272,9 +1272,6 @@ pref("browser.bookmarks.editDialog.firstEditField", "namePicker"); 
// The number of recently selected folders in the edit bookmarks dialog.

pref("browser.bookmarks.editDialog.maxRecentFolders", 7);



// Whether the Edit Bookmark dialog is delayed-apply.

pref("browser.bookmarks.editDialog.delayedApply.enabled", true);



#if defined(XP_WIN) && defined(MOZ_SANDBOX)

  // This controls the strength of the Windows content process sandbox for

  // testing purposes. This will require a restart.

browser/base/content/browser-places.js

+3 −91
Original line number Diff line number Diff line
@@ -32,7 +32,6 @@ ChromeUtils.defineModuleGetter( 


var StarUI = {

  _itemGuids: null,

  _batching: false,

  _isNewBookmark: false,

  _isComposing: false,

  _autoCloseTimer: 0,
@@ -80,11 +79,7 @@ var StarUI = { 
      case "popuphidden": {

        clearTimeout(this._autoCloseTimer);

        if (aEvent.originalTarget == this.panel) {

          if (gEditItemOverlay.delayedApplyEnabled) {

          this._handlePopupHiddenEvent().catch(console.error);

          } else {

            this._handlePopupHiddenEventInstantApply().catch(console.error);

          }

        }

        break;

      }
@@ -176,7 +171,7 @@ var StarUI = { 
  },



  /**

   * Handle popup hidden event in delayed apply mode.

   * Handle popup hidden event.

   */

  async _handlePopupHiddenEvent() {

    const {
@@ -211,41 +206,6 @@ var StarUI = { 
    }

  },



  /**

   * Handle popup hidden event in instant apply mode.

   */

  async _handlePopupHiddenEventInstantApply() {

    const { selectedFolderGuid, didChangeFolder } = gEditItemOverlay;

    gEditItemOverlay.uninitPanel(true);



    // Capture _removeBookmarksOnPopupHidden and _itemGuids values. Reset them

    // before we handle the next popup.

    const removeBookmarksOnPopupHidden = this._removeBookmarksOnPopupHidden;

    this._removeBookmarksOnPopupHidden = false;

    const guidsForRemoval = this._itemGuids;

    this._itemGuids = null;



    if (this._batching) {

      this.endBatch();

    }



    if (removeBookmarksOnPopupHidden && guidsForRemoval) {

      if (this._isNewBookmark) {

        await PlacesTransactions.undo();

        return;

      }

      // Remove all bookmarks for the bookmark's url, this also removes

      // the tags for the url.

      await PlacesTransactions.Remove(guidsForRemoval).transact();

    } else if (this._isNewBookmark) {

      this.showConfirmation();

    }



    if (!removeBookmarksOnPopupHidden) {

      await this._storeRecentlyUsedFolder(selectedFolderGuid, didChangeFolder);

    }

  },



  async showEditBookmarkPopup(aNode, aIsNewBookmark, aUrl) {

    // Slow double-clicks (not true double-clicks) shouldn't

    // cause the panel to flicker.
@@ -286,10 +246,6 @@ var StarUI = { 


    this._setIconAndPreviewImage();



    if (!gEditItemOverlay.delayedApplyEnabled) {

      this.beginBatch();

    }



    let onPanelReady = fn => {

      let target = this.panel;

      if (target.parentNode) {
@@ -353,40 +309,6 @@ var StarUI = { 
    this.panel.hidePopup();

  },



  // Matching the way it is used in the Library, editBookmarkOverlay implements

  // an instant-apply UI, having no batched-Undo/Redo support.

  // However, in this context (the Star UI) we have a Cancel button whose

  // expected behavior is to undo all the operations done in the panel.

  // Sometime in the future this needs to be reimplemented using a

  // non-instant apply code path, but for the time being, we patch-around

  // editBookmarkOverlay so that all of the actions done in the panel

  // are treated by PlacesTransactions as a single batch.  To do so,

  // we start a PlacesTransactions batch when the star UI panel is shown, and

  // we keep the batch ongoing until the panel is hidden.

  _batchBlockingDeferred: null,

  beginBatch() {

    if (this._batching) {

      return;

    }

    this._batchBlockingDeferred = PromiseUtils.defer();

    PlacesTransactions.batch(async () => {

      // First await for the batch to be concluded.

      await this._batchBlockingDeferred.promise;

      // And then for any pending promises added in the meanwhile.

      await Promise.all(gEditItemOverlay.transactionPromises);

    });

    this._batching = true;

  },



  endBatch() {

    if (!this._batching) {

      return;

    }



    this._batchBlockingDeferred.resolve();

    this._batching = false;

  },



  async _storeRecentlyUsedFolder(selectedFolderGuid, didChangeFolder) {

    if (!selectedFolderGuid) {

      return;
@@ -514,17 +436,7 @@ var PlacesCommandHook = { 
        console.error(e);

      }



      if (showEditUI && !gEditItemOverlay.delayedApplyEnabled) {

        // If we bookmark the page here but open right into a cancelable

        // state (i.e. new bookmark in Library), start batching here so

        // all of the actions can be undone in a single undo step.

        StarUI.beginBatch();

      }



      if (

        !gEditItemOverlay.delayedApplyEnabled ||

        !StarUI.showForNewBookmarks

      ) {

      if (!StarUI.showForNewBookmarks) {

        info.guid = await PlacesTransactions.NewBookmark(info).transact();

      } else {

        info.guid = PlacesUtils.bookmarks.unsavedGuid;

browser/base/content/browser.js

+1 −6
Original line number Diff line number Diff line
@@ -240,12 +240,7 @@ XPCOMUtils.defineLazyScriptGetter( 
XPCOMUtils.defineLazyScriptGetter(

  this,

  "gEditItemOverlay",

  Services.prefs.getBoolPref(

    "browser.bookmarks.editDialog.delayedApply.enabled",

    false

  )

    ? "chrome://browser/content/places/editBookmark.js"

    : "chrome://browser/content/places/instantEditBookmark.js"

  "chrome://browser/content/places/editBookmark.js"

);

XPCOMUtils.defineLazyScriptGetter(

  this,

browser/base/content/test/pageActions/browser_PageActions_bookmark.js

+4 −124
Original line number Diff line number Diff line
@@ -27,126 +27,9 @@ add_task(async function starButtonCtrlClick() { 
});



add_task(async function bookmark() {

  // Open a unique page.

  // eslint-disable-next-line @microsoft/sdl/no-insecure-url

  let url = "http://example.com/browser_page_action_menu";



  // Open a new window with delayed apply mode disabled. In the new window,

  // opening the star panel should instantly create a new bookmark.

  await SpecialPowers.pushPrefEnv({

    set: [["browser.bookmarks.editDialog.delayedApply.enabled", false]],

  });

  const win = await BrowserTestUtils.openNewBrowserWindow();

  registerCleanupFunction(async () => {

    await BrowserTestUtils.closeWindow(win);

  });



  await BrowserTestUtils.withNewTab(

    { gBrowser: win.gBrowser, url },

    async () => {

      // The bookmark button should read "Bookmark this page ([shortcut])" and not

      // be starred.

      let bookmarkButton = win.BrowserPageActions.urlbarButtonNodeForActionID(

        "bookmark"

      );

      await TestUtils.waitForCondition(

        () =>

          document.l10n.getAttributes(bookmarkButton).id ===

          "urlbar-star-add-bookmark",

        "Expecting the tooltip text to be updated. Tooltip text: " +

          bookmarkButton.getAttribute("tooltiptext")

      );

      Assert.ok(!bookmarkButton.hasAttribute("starred"));



      info("Click the button.");

      // The button ignores activation while the bookmarked status is being

      // updated. So, wait for it to finish updating.

      await TestUtils.waitForCondition(

        () => win.BookmarkingUI.status != win.BookmarkingUI.STATUS_UPDATING

      );

      let onItemAddedPromise = PlacesTestUtils.waitForNotification(

        "bookmark-added",

        events => events.some(event => event.url == url)

      );

      let promise = BrowserTestUtils.waitForPopupEvent(

        win.StarUI.panel,

        "shown"

      );

      EventUtils.synthesizeMouseAtCenter(bookmarkButton, {}, win);

      await promise;

      await onItemAddedPromise;



      Assert.equal(

        win.BookmarkingUI.starBox.getAttribute("open"),

        "true",

        "Star has open attribute"

      );

      // The bookmark button should now read "Edit this bookmark ([shortcut])" and

      // be starred.

      await TestUtils.waitForCondition(

        () =>

          document.l10n.getAttributes(bookmarkButton).id ===

          "urlbar-star-edit-bookmark",

        "Expecting the tooltip text to be updated. Tooltip text: " +

          bookmarkButton.getAttribute("tooltiptext")

      );

      Assert.equal(bookmarkButton.firstChild.getAttribute("starred"), "true");



      win.StarUI.panel.hidePopup();

      Assert.ok(

        !win.BookmarkingUI.starBox.hasAttribute("open"),

        "Star no longer has open attribute"

      );



      info("Click it again.");

      // The button ignores activation while the bookmarked status is being

      // updated. So, wait for it to finish updating.

      await TestUtils.waitForCondition(

        () => win.BookmarkingUI.status != win.BookmarkingUI.STATUS_UPDATING

      );

      promise = BrowserTestUtils.waitForPopupEvent(win.StarUI.panel, "shown");

      EventUtils.synthesizeMouseAtCenter(bookmarkButton, {}, win);

      await promise;



      let onItemRemovedPromise = PlacesTestUtils.waitForNotification(

        "bookmark-removed",

        events => events.some(event => event.url == url)

      );

      // Click the remove-bookmark button in the panel.

      win.StarUI._element("editBookmarkPanelRemoveButton").click();

      // Wait for the bookmark to be removed before continuing.

      await onItemRemovedPromise;



      // Check there's no contextual menu on the button.

      let contextMenuPromise = promisePopupNotShown("pageActionContextMenu");

      // The button ignores activation while the bookmarked status is being

      // updated. So, wait for it to finish updating.

      await TestUtils.waitForCondition(

        () => win.BookmarkingUI.status != win.BookmarkingUI.STATUS_UPDATING

      );

      EventUtils.synthesizeMouseAtCenter(

        bookmarkButton,

        {

          type: "contextmenu",

          button: 2,

        },

        win

      );

      await contextMenuPromise;

    }

  );

});



add_task(async function bookmarkDelayedApply() {

  // eslint-disable-next-line @microsoft/sdl/no-insecure-url

  const url = "http://example.com/browser_page_action_menu_delayed_apply";

  const url = "http://example.com/browser_page_action_menu";



  // Open a new window with delayed apply mode enabled. In the new window,

  // opening the star panel should delay creating a new bookmark until the

  // panel is dismissed.

  await SpecialPowers.pushPrefEnv({

    set: [["browser.bookmarks.editDialog.delayedApply.enabled", true]],

  });

  const win = await BrowserTestUtils.openNewBrowserWindow();

  registerCleanupFunction(async () => {

    await BrowserTestUtils.closeWindow(win);
@@ -209,16 +92,13 @@ add_task(async function bookmarkDelayedApply() { 
  );

});



add_task(async function bookmarkDelayedApplyNoEditDialog() {

add_task(async function bookmarkNoEditDialog() {

  const url =

    // eslint-disable-next-line @microsoft/sdl/no-insecure-url

    "http://example.com/browser_page_action_menu_delayed_apply_no_edit_dialog";

    "http://example.com/browser_page_action_menu_no_edit_dialog";



  await SpecialPowers.pushPrefEnv({

    set: [

      ["browser.bookmarks.editDialog.delayedApply.enabled", true],

      ["browser.bookmarks.editDialog.showForNewBookmarks", false],

    ],

    set: [["browser.bookmarks.editDialog.showForNewBookmarks", false]],

  });

  const win = await BrowserTestUtils.openNewBrowserWindow();

  registerCleanupFunction(async () => {

browser/components/places/PlacesUIUtils.sys.mjs

+0 −43
Original line number Diff line number Diff line
@@ -567,49 +567,6 @@ export var PlacesUIUtils = { 
    let features = "centerscreen,chrome,modal,resizable=no";

    let bookmarkGuid;



    if (

      !Services.prefs.getBoolPref(

        "browser.bookmarks.editDialog.delayedApply.enabled",

        false

      )

    ) {

      // Set the transaction manager into batching mode.

      let topUndoEntry = lazy.PlacesTransactions.topUndoEntry;

      let batchBlockingDeferred = lazy.PromiseUtils.defer();

      let batchCompletePromise = lazy.PlacesTransactions.batch(async () => {

        await batchBlockingDeferred.promise;

      });



      if (!aParentWindow) {

        aParentWindow = Services.wm.getMostRecentWindow(null);

      }



      if (aParentWindow.gDialogBox) {

        await aParentWindow.gDialogBox.open(dialogURL, aInfo);

      } else {

        aParentWindow.openDialog(dialogURL, "", features, aInfo);

      }



      bookmarkGuid =

        ("bookmarkGuid" in aInfo && aInfo.bookmarkGuid) || undefined;



      batchBlockingDeferred.resolve();



      // Ensure the batch has completed before we start the undo/resolve

      // the deferred promise.

      await batchCompletePromise.catch(console.error);



      if (

        !bookmarkGuid &&

        topUndoEntry != lazy.PlacesTransactions.topUndoEntry

      ) {

        await lazy.PlacesTransactions.undo().catch(console.error);

      }



      this.lastBookmarkDialogDeferred.resolve(bookmarkGuid);

      return bookmarkGuid;

    }



    if (!aParentWindow) {

      aParentWindow = Services.wm.getMostRecentWindow(null);

    }