984813 - Remove deprecated promise.js usage in Firefox for Metro

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: General, defect)

Description

[:Paolo Amadini]

Attachment: The patch

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

Comment 1

[Matt Brubeck (:mbrubeck)]

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.

Comment 2

[:Paolo Amadini]

(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!

Comment 3

[Matt Brubeck (:mbrubeck)]

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.

Comment 4

[:Paolo Amadini]

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

[Matt Brubeck (:mbrubeck)]

(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

[Matt Brubeck (:mbrubeck)]

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.

Comment 7

[:Paolo Amadini]

Comment on attachment 8392798 [details] [diff] [review]
The patch

You're saying that I should consider this as an r+ now?

Comment 8

[Matt Brubeck (:mbrubeck)]

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).

Comment 9

[:Paolo Amadini]

Fixed Promise.all and pushed to fx-team:

https://hg.mozilla.org/integration/fx-team/rev/552787e99ae6

Comment 10

[:Paolo Amadini]

I filed bug 993002 about fixing the remaining test failures on the Metro project branch.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/552787e99ae6