1376925 - Fix browser_bookmarksProperties.js for async-transactions

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, enhancement, P1)

Description

[Mark Banner (:standard8)]

In bug 1119282 we attempted to get all tests working with async transactions. Unfortunately browser_bookmarksProperties.js still needs some work, then we should turn on async transactions for all of browser/components/places/test/browser (and maybe more).

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1376925 - Modernise browser_bookmarksProperties.js to use async functions and assert.

Review commit: https://reviewboard.mozilla.org/r/153518/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/153518/

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 8882401 [details]
Bug 1376925 - Modernise browser_bookmarksProperties.js to use async functions and assert.

https://reviewboard.mozilla.org/r/153518/#review158758

::: browser/components/places/content/treeView.js:979
(Diff revision 1)
> +    // finish.
> +    if (this._tree.element.getAttribute("editing")) {
> +      this._editingObserver = new MutationObserver(() => {
> +        Services.tm.dispatchToMainThread(
> +          () => this.invalidateContainer(aContainer));
> +        this._editingObserver.disconnect();

May we may want to also disconnect this when the tree is destroyed? On the other side that is probably causing the editing mode to terminate already, so it may not be strictly necessary, but it would avoid a pointless invalidation.

Comment 3

[Mark Banner (:standard8)]

Attachment: Bug 1376925 - Avoid some warnings about this._paneInfo being undefined in editBookmarkOverlay.js when an event happens during closing the dialog.

Review commit: https://reviewboard.mozilla.org/r/154886/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154886/

Comment 4

[Mark Banner (:standard8)]

Attachment: Bug 1376925 - In the bookmarks folder tree of the edit dialog, don't invalidate elements whilst we are editing. Also improve the stability of browser_bookmarksProperties.js.

Review commit: https://reviewboard.mozilla.org/r/154888/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154888/

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8882401 [details]
Bug 1376925 - Modernise browser_bookmarksProperties.js to use async functions and assert.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153518/diff/1-2/

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 8882401 [details]
Bug 1376925 - Modernise browser_bookmarksProperties.js to use async functions and assert.

https://reviewboard.mozilla.org/r/153518/#review159950

::: browser/components/places/tests/browser/browser_bookmarksProperties.js:119
(Diff revision 2)
> -            // In case this test fails the window will close, the test will fail
> +              // In case this test fails the window will close, the test will fail
> -            // since we didn't set _cleanShutdown.
> +              // since we didn't set _cleanShutdown.
> -            var tree = tagsField.popup.tree;
> +              var tree = tagsField.popup.tree;
> -            // Focus and select first result.
> +              // Focus and select first result.
> -            isnot(tree, null, "Autocomplete results tree exists");
> -            is(tree.view.rowCount, 1, "We have 1 autocomplete result");
> +              Assert.notEqual(tree, null, "Autocomplete results tree exists");
> +              await BrowserTestUtils.waitForCondition(() => tree.view.rowCount == 1,

could you please add a comment about why we need to wait here? what's async populating?

::: browser/components/places/tests/browser/browser_bookmarksProperties.js:158
(Diff revision 2)
>      var tags = PlacesUtils.tagging.getTagsForURI(PlacesUtils._uri(TEST_URL));
> -    is(tags[0], "testTag", "Tag on node has not changed");
> +    Assert.ok(tags[0], "testTag", "Tag on node has not changed");
>  
>      // Cleanup.
>      PlacesUtils.tagging.untagURI(PlacesUtils._uri(TEST_URL), ["testTag"]);
> -    PlacesUtils.bookmarks.removeItem(this._itemId);
> +    await PlacesUtils.bookmarks.remove(this._bookmark.guid);

you can just pass this._bookmark

::: browser/components/places/tests/browser/browser_bookmarksProperties.js:264
(Diff revision 2)
> -    is(tags[0], "testTag", "Tag on node has not changed");
> +    Assert.equal(tags[0], "testTag", "Tag on node has not changed");
>  
>      // Cleanup.
>      PlacesUtils.tagging.untagURI(PlacesUtils._uri(TEST_URL),
>                                   ["testTag"]);
> -    PlacesUtils.bookmarks.removeItem(this._itemId);
> +    await PlacesUtils.bookmarks.remove(this._bookmark.guid);

ditto

::: browser/components/places/tests/browser/browser_bookmarksProperties.js:282
(Diff revision 2)
>  
> -  setup(aCallback) {
> +  async setup() {
>      // Add a visit.
> -    PlacesTestUtils.addVisits(
> -      {uri: PlacesUtils._uri(TEST_URL),
> -        transition: PlacesUtils.history.TRANSITION_TYPED}
> +    await PlacesTestUtils.addVisits({
> +      uri: PlacesUtils._uri(TEST_URL),
> +      transition: PlacesUtils.history.TRANSITION_TYPED

does it really need to be typed? it may be simpler to just await PlacesTestUtils.addVisits(TEST_URL) (the _uri calls are pointless here I think)

::: browser/components/places/tests/browser/browser_bookmarksProperties.js:399
(Diff revision 2)
>      // Get sidebar's Places tree.
> -    var sidebarTreeID = gCurrentTest.sidebar == SIDEBAR_BOOKMARKS_ID ?
> +    var sidebarTreeID = test.sidebar == SIDEBAR_BOOKMARKS_ID ?
> -                                                SIDEBAR_BOOKMARKS_TREE_ID :
> +                                        SIDEBAR_BOOKMARKS_TREE_ID :
> -                                                SIDEBAR_HISTORY_TREE_ID;
> +                                        SIDEBAR_HISTORY_TREE_ID;
>      var tree = sidebar.contentDocument.getElementById(sidebarTreeID);
> -    ok(tree, "Sidebar tree has been loaded");
> +    await BrowserTestUtils.waitForCondition(() => tree, "Sidebar tree has been loaded");

also here, it would be nice to get a comment.

::: browser/components/places/tests/browser/browser_bookmarksProperties.js:420
(Diff revision 2)
>            () => observerWindow.gEditItemOverlay.initialized, "EditItemOverlay is initialized");
> -        gCurrentTest.window = observerWindow;
> +        test.window = observerWindow;
>          try {
> -          gCurrentTest.run();
> +          executeSoon(() => {
> +            resolve();
> +          });

I assume you could executeSoon(resolve); (modulo scope bugs)

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 8883892 [details]
Bug 1376925 - Avoid some warnings about this._paneInfo being undefined in editBookmarkOverlay.js when an event happens during closing the dialog.

https://reviewboard.mozilla.org/r/154886/#review159956

::: browser/components/places/content/editBookmarkOverlay.js:491
(Diff revision 1)
>    },
>  
>    onTagsFieldChange() {
> -    if (this._paneInfo.isURI || this._paneInfo.bulkTagging) {
> +    // Check for _paneInfo existing as the dialog may be closing.
> +    if (this._paneInfo &&
> +        (this._paneInfo.isURI || this._paneInfo.bulkTagging)) {

Ah, basically the focus in on the field, when the dialog is closing it blurs the field, and that causes a change to happen (onTagsFieldChange is registered as the onchange handler).

I wonder if we shouldn't rather explicitly blur() whatever is focused BEFORE closing the dialog and unsetting _paneInfo, since looks like this may cause dataloss. As a user I may edit tags, and press on the closing button without blurring the field first.

Though, pressing the button (either with mouse or keyword) already changes focus, so I wonder if it's just an artifact of the test using accept/cancel methods directly.

It may still be a good idea to blur any selected field in uninitPanel() rather than workarounding like this.

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 8883893 [details]
Bug 1376925 - In the bookmarks folder tree of the edit dialog, don't invalidate elements whilst we are editing. Also improve the stability of browser_bookmarksProperties.js.

https://reviewboard.mozilla.org/r/154888/#review159958

::: browser/components/places/content/treeView.js:982
(Diff revision 1)
>      if (!this._tree)
>        return;
>  
> +    // If we are currently editing, don't invalidate the container until we
> +    // finish.
> +    if (this._tree.element.getAttribute("editing")) {

what happens if we get multiple invalidateContainer calls for the same container? I guess we need a way to merge them into just one per container.
So, _editingObserver may have to become a Map (aContainer => observer)

also if we'd get multiple for different containers, currently we'd overwrite an existing _editingObserver and then on uninit we'd disconnect only one.

::: browser/components/places/tests/browser/browser_bookmarksProperties.js:307
(Diff revision 1)
>      var foldersExpander = this.window.document.getElementById("editBMPanel_foldersExpander");
>      var folderTree = this.window.document.getElementById("editBMPanel_folderTree");
>      var self = this;
>  
>      let unloadPromise = new Promise(resolve => {
> -      this.window.addEventListener("unload", function(event) {
> +      this.window.addEventListener("unload", (event) => {

nit: no need for parenthesis

::: browser/components/places/tests/browser/browser_bookmarksProperties.js:335
(Diff revision 1)
> +            itemType === PlacesUtils.bookmarks.TYPE_FOLDER && isAnnotationProperty);
> +
>          // Create a new folder.
>          var newFolderButton = self.window.document.getElementById("editBMPanel_newFolderButton");
>          newFolderButton.doCommand();
> -        Assert.ok(folderTree.hasAttribute("editing"),
> +        await BrowserTestUtils.waitForCondition(() => folderTree.hasAttribute("editing"),

worth a comment that we are actually waiting for the folder to be created AND then editing starting.

::: browser/components/places/tests/browser/browser_bookmarksProperties.js:342
(Diff revision 1)
>  
>          // Press Escape to discard editing new folder name.
>          EventUtils.synthesizeKey("VK_ESCAPE", {}, self.window);
>          Assert.ok(!folderTree.hasAttribute("editing"),
>             "We have finished editing folder name in folder tree");
> +        await promise;

name the promise to something more meaningful, like promiseItemChanged

::: browser/components/places/tests/browser/browser_bookmarksProperties.js:361
(Diff revision 1)
>    },
>  
>    async cleanup() {
> +    await this._removeObserver;
> +    delete this._removeObserver;
> +    await PlacesTestUtils.promiseAsyncUpdates();

doesn't this test harness already do it for every test?

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8882401 [details]
Bug 1376925 - Modernise browser_bookmarksProperties.js to use async functions and assert.

https://reviewboard.mozilla.org/r/153518/#review159950

> could you please add a comment about why we need to wait here? what's async populating?

I'm not actually sure if this was necessary or not. For now I've taken it out and I'll do some more try pushes later to test it out.

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8883892 [details]
Bug 1376925 - Avoid some warnings about this._paneInfo being undefined in editBookmarkOverlay.js when an event happens during closing the dialog.

https://reviewboard.mozilla.org/r/154886/#review159956

> Ah, basically the focus in on the field, when the dialog is closing it blurs the field, and that causes a change to happen (onTagsFieldChange is registered as the onchange handler).
> 
> I wonder if we shouldn't rather explicitly blur() whatever is focused BEFORE closing the dialog and unsetting _paneInfo, since looks like this may cause dataloss. As a user I may edit tags, and press on the closing button without blurring the field first.
> 
> Though, pressing the button (either with mouse or keyword) already changes focus, so I wonder if it's just an artifact of the test using accept/cancel methods directly.
> 
> It may still be a good idea to blur any selected field in uninitPanel() rather than workarounding like this.

I dug into this a bit more and found that blur() or folderTree.stopEditing() in uninitPanel wasn't fixing the actual issue. Looking again at what is happening, it is the undo of the transactions that is triggering extra updates and causing the paneInfo exceptions.

For now I've expanded the comments a bit more, I don't think there's the possibility of a user-facing issue here.

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 8882401 [details]
Bug 1376925 - Modernise browser_bookmarksProperties.js to use async functions and assert.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153518/diff/2-3/

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 8883892 [details]
Bug 1376925 - Avoid some warnings about this._paneInfo being undefined in editBookmarkOverlay.js when an event happens during closing the dialog.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154886/diff/1-2/

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 8883893 [details]
Bug 1376925 - In the bookmarks folder tree of the edit dialog, don't invalidate elements whilst we are editing. Also improve the stability of browser_bookmarksProperties.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154888/diff/1-2/

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 8883892 [details]
Bug 1376925 - Avoid some warnings about this._paneInfo being undefined in editBookmarkOverlay.js when an event happens during closing the dialog.

https://reviewboard.mozilla.org/r/154886/#review161134

::: browser/components/places/content/editBookmarkOverlay.js:490
(Diff revision 2)
>      this._firstEditedField = "";
>    },
>  
>    onTagsFieldChange() {
> -    if (this._paneInfo.isURI || this._paneInfo.bulkTagging) {
> +    // Check for _paneInfo existing as the dialog may be closing but receiving
> +    // async updates from undoing transactions.

From my understanding, in uninitPanel we removeObserver before setting _paneInfo to null.

But now that we have an async behavior, it's possible we'll handle a promise after the panel has been uninitialized, thus I suspect what happens is that by the time we invoke uninitPanel, a promise resolves and we can't handle it.
If this is the case, please update the comments blaming unresolved promises, otherwise it looks like we are improperly listening to bookmark changes after uninit, that is not the case afaict.

::: browser/components/places/content/editBookmarkOverlay.js:788
(Diff revision 2)
>      return this._appendFolderItemToMenupopup(menupopup, aFolderId);
>    },
>  
>    onFolderMenuListCommand(aEvent) {
> +    // Check for _paneInfo existing as the dialog may be closing but receiving
> +    // async updates from undoing transactions.

ditto

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 8883893 [details]
Bug 1376925 - In the bookmarks folder tree of the edit dialog, don't invalidate elements whilst we are editing. Also improve the stability of browser_bookmarksProperties.js.

https://reviewboard.mozilla.org/r/154888/#review161140

::: browser/components/places/content/treeView.js:986
(Diff revision 2)
>  
> +    // If we are currently editing, don't invalidate the container until we
> +    // finish.
> +    if (this._tree.element.getAttribute("editing")) {
> +      if (!this._editingObserver) {
> +        this._editingObserver = new Map();

either _editingObserverMap or _editingObservers (plural)

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 8882401 [details]
Bug 1376925 - Modernise browser_bookmarksProperties.js to use async functions and assert.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153518/diff/3-4/

Comment 17

[Mark Banner (:standard8)]

Comment on attachment 8883892 [details]
Bug 1376925 - Avoid some warnings about this._paneInfo being undefined in editBookmarkOverlay.js when an event happens during closing the dialog.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154886/diff/2-3/

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 8883893 [details]
Bug 1376925 - In the bookmarks folder tree of the edit dialog, don't invalidate elements whilst we are editing. Also improve the stability of browser_bookmarksProperties.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154888/diff/2-3/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c4a66579add5
Modernise browser_bookmarksProperties.js to use async functions and assert. r=mak
https://hg.mozilla.org/integration/autoland/rev/ceab588f0348
Avoid some warnings about this._paneInfo being undefined in editBookmarkOverlay.js when an event happens during closing the dialog. r=mak
https://hg.mozilla.org/integration/autoland/rev/42a5f0436562
In the bookmarks folder tree of the edit dialog, don't invalidate elements whilst we are editing. Also improve the stability of browser_bookmarksProperties.js. r=mak

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c4a66579add5
https://hg.mozilla.org/mozilla-central/rev/ceab588f0348
https://hg.mozilla.org/mozilla-central/rev/42a5f0436562