895359 - [New Tab Page] Switch from promise.js to Promise.jsm

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: switch to Promise.jsm and remove remaining callbacks

Jared: sorry for all the recent review requests, I think you're a good fit to review all those changes. If I overloaded you, feel free to tell me to redirect those requests!

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/105e1f064e67

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Backed out for now:

https://hg.mozilla.org/integration/fx-team/rev/3943077329f8

Bug 894876 caused a couple of failures.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-895359-Pass-draggedSite-to-updateGrid-to-avoid-i.patch

I never got around to address the intermittent failures on Windows and thought I'd do it this weekend :)

Turns out that the grid updater shouldn't rely on the global gDrag.draggedSite because depending on the order of dragend/drop events being fired it might have been nulled already and cause tests to time out.

I fixed this by passing the draggedSite to gUpdater.updateGrid() so that it can appropriately exclude this site from being slided around.

https://tbpl.mozilla.org/?tree=Try&rev=ffff73ea8373

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Pushed the slightly modified patch that is up for review to try again:

https://tbpl.mozilla.org/?tree=Try&rev=e4d79dac7c76

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8376797 [details] [diff] [review]
0002-Bug-895359-Pass-draggedSite-to-updateGrid-to-avoid-i.patch

Review of attachment 8376797 [details] [diff] [review]:
-----------------------------------------------------------------

r=me but I'm curious what you think about the following.

::: browser/base/content/newtab/transformations.js
@@ +136,5 @@
>    /**
>     * Rearranges a given array of sites and moves them to their new positions or
>     * fades in/out new/removed sites.
>     * @param aSites An array of sites to rearrange.
> +   * @param aDraggedSite The currently dragged site, may be null.

Seems like it would be a more minimal change if you had `let draggedSite = gDrag.draggedSite` declared within the rearrangeSites function but outside of the `function* promises`.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Jared Wein [:jaws] (please needinfo? me) [Away 2/20-2/23] from comment #6)
> Seems like it would be a more minimal change if you had `let draggedSite =
> gDrag.draggedSite` declared within the rearrangeSites function but outside
> of the `function* promises`.

That unfortunately wouldn't work as the promises generator is executed/finished synchronously. I could have made the change you suggested in gUpdater.updateGrid() and keep the draggedSite around until rearrangeSites() is called but I figured that it would probably be clearer to pass it updateGrid() then.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/84da123a0af0
https://hg.mozilla.org/integration/fx-team/rev/5d896e33c6a6

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/84da123a0af0
https://hg.mozilla.org/mozilla-central/rev/5d896e33c6a6

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Backed out for causing bug 983478 and possibly a rise of intermittent failures in bug 898317.

https://hg.mozilla.org/integration/fx-team/rev/ea26e5a6c565
https://hg.mozilla.org/integration/fx-team/rev/d0a1e314aed9

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

I went for a straight backout here because I remember the drag/drop behavior on different platforms to be really tricky and other than cleanup this bug doesn't actually buy us much. We might be able to re-land this with some more time to investigate but we can currently use our time for more important stuff.

Comment 12

[Marco Bonardo [:mak]]

was this change intended? I think Paolo is removing usage of the old promises and this is going the other direction:

1.12 -Cu.import("resource://gre/modules/Promise.jsm");
1.13 +Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js");

(note that just this by itself may be cause of intermittent failures since new promises resolve on the next tick)

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Yeah this was one of the reasons I wanted to land this patch but the extra tick seems to introduce problems here somehow.

Comment 14

[:Paolo Amadini]

(In reply to Tim Taubert [:ttaubert] from comment #11)
> I went for a straight backout here because I remember the drag/drop behavior
> on different platforms to be really tricky and other than cleanup this bug
> doesn't actually buy us much.

Thanks for helping with the conversion, I'll give this some thought.

In general, however, we're out of luck using Promises as part of a response to any DOM event that expects a synchronous response, including drag and drop. Even "sync" promises are unsafe to use there.

What I can do here is to fix any test issues and convert any unsafe use of Promises to be callbacks or to work differently.

Comment 15

[obrufau]

After backing it out, I have bug 989500.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Backed out from Aurora:

https://hg.mozilla.org/releases/mozilla-aurora/rev/3aaca223b673
https://hg.mozilla.org/releases/mozilla-aurora/rev/e1b7b7176710

Comment 17

[:Paolo Amadini]

Tim, do you have any recommendation on how I could proceed here, and which things I should be aware of possibly regressing?

My immediate goal is just the Promise.jsm conversion, since this is now the only remaining problematic instance of legacy promises outside of devtools, and if we fix this one we can then evaluate copying the legacy "promise.js" code to devtools instead of keeping it in the Add-on SDK or fixing all of the devtools uses.

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-895359-New-Tab-Page-Switch-from-promise.js-to-Pr.patch

Ok, ten steps back and one forward. Let's just switch to Promise.jsm and defer all the other refactoring.

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=6e186b9e1fae

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

This looks pretty good on try, yay.

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8403883 [details] [diff] [review]
0001-Bug-895359-New-Tab-Page-Switch-from-promise.js-to-Pr.patch

Review of attachment 8403883 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but I don't know why the automated tests that we currently have didn't detect bug 983478.

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #21)
> r=me, but I don't know why the automated tests that we currently have didn't
> detect bug 983478.

Me neither :/ I will test this patch on Windows before landing to make sure this doesn't have the same problem, although it really shouldn't.

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/4487e7ade34c

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4487e7ade34c