Closed
Bug 1263557
Opened 8 years ago
Closed 8 years ago
Switch to use plain promise in CustomizeMode.jsm
Categories
(Firefox :: Toolbars and Customization, defect)
Firefox
Toolbars and Customization
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().
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → gasolin
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45425/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45425/
Attachment #8739898 -
Flags: review?(gijskruitbosch+bugs)
Comment 2•8 years ago
|
||
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+
Assignee | ||
Updated•8 years ago
|
Summary: Switch to using use plain promise in CustomizeMode.jsm → Switch to use plain promise in CustomizeMode.jsm
Assignee | ||
Comment 3•8 years ago
|
||
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/
Assignee | ||
Comment 4•8 years ago
|
||
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/
Assignee | ||
Comment 5•8 years ago
|
||
I've update the patch and moved catchAll and catchAllTimeout into `customizeTransitionEndPromise` function. Triggered a new try to see how it progress.
Assignee | ||
Comment 6•8 years ago
|
||
looks good in test result https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2c71c1a1eb4&selectedJob=19388879
Keywords: checkin-needed
Comment 8•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/04f9767b4abe
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in
before you can comment on or make changes to this bug.
Description
•