Closed
Bug 984813
Opened 11 years ago
Closed 11 years ago
Remove deprecated promise.js usage in Firefox for Metro
Categories
(Firefox for Metro Graveyard :: General, defect)
Firefox for Metro Graveyard
General
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 31
People
(Reporter: Paolo, Assigned: Paolo)
References
Details
Attachments
(1 file)
(deleted),
patch
|
mbrubeck
:
review+
|
Details | Diff | Splinter Review |
The Add-on SDK Promises module is being deprecated in favor of Promise.jsm, so we're removing all references to it from mozilla-central.
A preliminary limited tryserver run is green:
https://tbpl.mozilla.org/?tree=Try&rev=a31d4205523e
Attachment #8392798 -
Flags: review?(mbrubeck)
Updated•11 years ago
|
Attachment #8392798 -
Flags: review?(mbrubeck) → review+
Comment 1•11 years ago
|
||
Comment on attachment 8392798 [details] [diff] [review]
The patch
Oops, I just checked the Try run and saw there were several failures. Metro tests are now hidden by default, so you need to add "&showall=1" to the TBPL address to see them:
https://tbpl.mozilla.org/php/getParsedLog.php?id=36268758&tree=Try
Notably: Promise.all() expects an iterable. at Promise.all@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:419:5
I can work on fixing the test failures this week. Note that Metro is no longer tier-1, so this bug does not need to block the removal of promise.js on m-c.
Attachment #8392798 -
Flags: review+ → review-
Assignee | ||
Comment 2•11 years ago
|
||
(In reply to Matt Brubeck (:mbrubeck) from comment #1)
> Oops, I just checked the Try run and saw there were several failures. Metro
> tests are now hidden by default, so you need to add "&showall=1" to the TBPL
> address to see them:
>
> https://tbpl.mozilla.org/php/getParsedLog.php?id=36268758&tree=Try
Ah, I see!
> Notably: Promise.all() expects an iterable. at
> Promise.all@resource://gre/modules/Promise.jsm ->
> resource://gre/modules/Promise-backend.js:419:5
This is probably the easiest to fix, Add-on SDK allowed Promise.all(a, b, c) while now it should bew written as Promise.all([a, b, c]).
> I can work on fixing the test failures this week. Note that Metro is no
> longer tier-1, so this bug does not need to block the removal of promise.js
> on m-c.
OK, thanks for your help!
Updated•11 years ago
|
Flags: needinfo?(mbrubeck)
Comment 3•11 years ago
|
||
Updating the Promise.all calls fixes browser_findbar.js, but it does not fix the failures in browser_context_menu_tests.js and browser_context_menu_tests.js. These look like race conditions in either our menu popup code or the test code. Still looking into this.
Assignee | ||
Comment 4•11 years ago
|
||
I think the removal of synchronous Promise will require some more days to be completed, but in case we need to land it before the tests are fixed, can you back it out only from the Metro branch while investigating this bug?
Comment 5•11 years ago
|
||
(In reply to :Paolo Amadini from comment #4)
> I think the removal of synchronous Promise will require some more days to be
> completed, but in case we need to land it before the tests are fixed, can
> you back it out only from the Metro branch while investigating this bug?
Yes -- or we can just disable the failing tests (on the metro branch) until this is fixed. Either way, don't let us block you.
Comment 6•11 years ago
|
||
Sorry, I haven't managed to fix the remaining test failures and I don't think I'll have time to keep pounding on them. Feel free to land the current patch, and we can file follow-up bugs to fix the test failures. Metro tests are not currently run in automation, so there's no impact on TBPL.
Flags: needinfo?(mbrubeck)
Assignee | ||
Comment 7•11 years ago
|
||
Comment on attachment 8392798 [details] [diff] [review]
The patch
You're saying that I should consider this as an r+ now?
Attachment #8392798 -
Flags: review- → review?(mbrubeck)
Comment 8•11 years ago
|
||
Comment on attachment 8392798 [details] [diff] [review]
The patch
Ah, yes. If you could also fix the Promise.all calls before you land this, that would be great (but not required -- again, we can fix this once we get a project branch up and running).
Attachment #8392798 -
Flags: review?(mbrubeck) → review+
Assignee | ||
Comment 9•11 years ago
|
||
Fixed Promise.all and pushed to fx-team:
https://hg.mozilla.org/integration/fx-team/rev/552787e99ae6
Assignee | ||
Comment 10•11 years ago
|
||
I filed bug 993002 about fixing the remaining test failures on the Metro project branch.
Comment 11•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
You need to log in
before you can comment on or make changes to this bug.
Description
•