Closed Bug 1263557 Opened 4 years ago Closed 4 years ago

Switch to use plain promise in CustomizeMode.jsm

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: gasolin, Assigned: gasolin)

References

Details

Attachments

(1 file)

follow up of bug 1260718

We should update the main PanelUI and CUI/CustomizeMode code to no longer use Promise.defer . It's non-standard and often harder to read than code using new Promise().
Depends on: 1260718
Assignee: nobody → gasolin
Comment on attachment 8739898 [details]
MozReview Request: Bug 1263557 - use plain promise in CustomizeMode.jsm; r?Gijs

https://reviewboard.mozilla.org/r/45425/#review41981

r=me with the ordering in the transition method thing fixed.

::: browser/components/customizableui/CustomizeMode.jsm:582
(Diff revision 1)
> +      this.document.documentElement.removeAttribute("customize-entered");
> +    }
> +
> +    let catchAll = () => customizeTransitionEnd("timedout");
> +    let catchAllTimeout = this.window.setTimeout(catchAll, kMaxTransitionDurationMs);
> +    return new Promise(resolve => {

If you store this in a temporary, say, `customizeTransitionEndPromise` or whatever, and return that temporary variable at the end of the method, you can keep the order of the function the same and reduce the size of the diff / blame contamination for this refactoring. That also avoids subtle issues about when you start the `setTimeout` call and when you add the `transitionend` event listener and when you set the attributes that trigger the transition (which I *think* shouldn't matter here because of JS run-to-completion semantics, but I'd rather not find out that I'm wrong through intermittent bugs / oranges... :-) ).
Attachment #8739898 - Flags: review?(gijskruitbosch+bugs) → review+
Summary: Switch to using use plain promise in CustomizeMode.jsm → Switch to use plain promise in CustomizeMode.jsm
Comment on attachment 8739898 [details]
MozReview Request: Bug 1263557 - use plain promise in CustomizeMode.jsm; r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45425/diff/1-2/
Comment on attachment 8739898 [details]
MozReview Request: Bug 1263557 - use plain promise in CustomizeMode.jsm; r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45425/diff/2-3/
I've update the patch and moved catchAll and catchAllTimeout into `customizeTransitionEndPromise` function. Triggered a new try to see how it progress.
https://hg.mozilla.org/mozilla-central/rev/04f9767b4abe
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.