984806 - Convert to Promise.jsm in the Webapp Runtime

Status: RESOLVED FIXED

(Firefox Graveyard :: Webapp Runtime, defect, P2)

Description

[:Paolo Amadini]

Attachment: The patch

This one-line patch converts the only legacy import of promise.js to Promise.jsm.

It doesn't seem to cause test failures on try: https://tbpl.mozilla.org/?tree=Try&rev=4ba8512be80d

Comment 1

[Marco Castelluccio [:marco]]

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

Review of attachment 8392786 [details] [diff] [review]:
-----------------------------------------------------------------

We aren't yet running webapprt tests on try :(
You can run them locally using the webapprt-test-chrome mach command.

::: webapprt/content/dbg-webapp-actors.js
@@ +3,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  'use strict';
>  
> +const { Promise: promise } = Cu.import("resource://gre/modules/Promise.jsm", {});

Shouldn't this be the opposite?
const { promise: Promise } = ...

Anyway, there's a single usage of "promise" in the file, you could s/promise/Promise and directly do:
const { Promise } = ...

Comment 2

[:Paolo Amadini]

Attachment: Updated patch

(In reply to Marco Castelluccio [:marco] from comment #1)
> const { promise: Promise } = ...

"Promise: promise" seems to be the correct way (I just copied this from other uses).

> Anyway, there's a single usage of "promise" in the file, you could
> s/promise/Promise and directly do:
> const { Promise } = ...

I did this, here is the new patch.

It seems that webapprt tests are already failing for me locally. Do you think you can verify that this patch does not break anything?

Comment 3

[Marco Castelluccio [:marco]]

Comment on attachment 8393366 [details] [diff] [review]
Updated patch

Review of attachment 8393366 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to :Paolo Amadini from comment #2)
> "Promise: promise" seems to be the correct way (I just copied this from
> other uses).

With "Promise: promise":
TypeError: redeclaration of var promise at chrome://webapprt/content/dbg-webapp-actors.js:7

> It seems that webapprt tests are already failing for me locally. Do you
> think you can verify that this patch does not break anything?

The browser_debugger.js test failure is known (bug 985015). Is "glciActor set" the only failure you see?

Comment 4

[:Paolo Amadini]

(In reply to Marco Castelluccio [:marco] from comment #3)
> With "Promise: promise":
> TypeError: redeclaration of var promise at
> chrome://webapprt/content/dbg-webapp-actors.js:7

Maybe the line should just be removed because "promise" is already imported in the context? In this case, this might inherit the changes we're making in devtools.

> > It seems that webapprt tests are already failing for me locally. Do you
> > think you can verify that this patch does not break anything?
> 
> The browser_debugger.js test failure is known (bug 985015). Is "glciActor
> set" the only failure you see?

I don't even reach the point where the above TypeError is shown, can the failures be specific to Mac?

Probably it's better if you look into this, since you have a working environment. I can go ahead and push the change as is, or remove the import line, if you say it's OK.

Comment 5

[Marco Castelluccio [:marco]]

(In reply to :Paolo Amadini from comment #4)
> I don't even reach the point where the above TypeError is shown, can the
> failures be specific to Mac?
> 
> Probably it's better if you look into this, since you have a working
> environment. I can go ahead and push the change as is, or remove the import
> line, if you say it's OK.

Oh, there's another regression that prevents webapprt tests from running (bug 986163).

Comment 6

[:Paolo Amadini]

(In reply to Marco Castelluccio [:marco] from comment #5)
> Oh, there's another regression that prevents webapprt tests from running
> (bug 986163).

OK, tests pass with the patch on that bug applied, with only the known failure.

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

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d5ed56d8a904