Closed Bug 895359 Opened 6 years ago Closed 5 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 31

People

(Reporter: ttaubert, Assigned: ttaubert)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 2 obsolete files)

No description provided.
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!
Attachment #777731 - Flags: review?(jaws)
Attachment #777731 - Flags: review?(jaws) → review+
Backed out for now:

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

Bug 894876 caused a couple of failures.
Whiteboard: [fixed-in-fx-team]
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
Attachment #8376797 - Flags: review?(jaws)
Pushed the slightly modified patch that is up for review to try again:

https://tbpl.mozilla.org/?tree=Try&rev=e4d79dac7c76
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`.
Attachment #8376797 - Flags: review?(jaws) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/84da123a0af0
https://hg.mozilla.org/mozilla-central/rev/5d896e33c6a6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
Whiteboard: [qa-]
Depends on: 983478
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
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)
Yeah this was one of the reasons I wanted to land this patch but the extra tick seems to introduce problems here somehow.
(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.
Blocks: 856878
After backing it out, I have bug 989500.
Blocks: 989500
Target Milestone: Firefox 30 → ---
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.
Flags: needinfo?(ttaubert)
Flags: needinfo?(ttaubert)
Summary: [New Tab Page] Switch to Promise.jsm and remove remaining callbacks → [New Tab Page] Switch from promise.js to Promise.jsm
Ok, ten steps back and one forward. Let's just switch to Promise.jsm and defer all the other refactoring.
Attachment #777731 - Attachment is obsolete: true
Attachment #8376797 - Attachment is obsolete: true
Attachment #8403883 - Flags: review?(jaws)
This looks pretty good on try, yay.
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.
Attachment #8403883 - Flags: review?(jaws) → review+
(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.
https://hg.mozilla.org/integration/fx-team/rev/4487e7ade34c
Whiteboard: [qa-] → [qa-][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/4487e7ade34c
Status: REOPENED → RESOLVED
Closed: 6 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [qa-][fixed-in-fx-team] → [qa-]
Target Milestone: --- → Firefox 31
Blocks: 996671
No longer blocks: 856878
You need to log in before you can comment on or make changes to this bug.