Closed
Bug 984806
Opened 11 years ago
Closed 11 years ago
Convert to Promise.jsm in the Webapp Runtime
Categories
(Firefox Graveyard :: Webapp Runtime, defect, P2)
Firefox Graveyard
Webapp Runtime
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 31
People
(Reporter: Paolo, Assigned: Paolo)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
marco
:
review+
|
Details | Diff | Splinter Review |
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
Attachment #8392786 -
Flags: review?(mar.castelluccio)
Updated•11 years ago
|
Priority: -- → P2
Comment 1•11 years ago
|
||
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 } = ...
Attachment #8392786 -
Flags: review?(mar.castelluccio) → feedback+
Assignee | ||
Comment 2•11 years ago
|
||
(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?
Attachment #8392786 -
Attachment is obsolete: true
Attachment #8393366 -
Flags: review?(mar.castelluccio)
Comment 3•11 years ago
|
||
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?
Attachment #8393366 -
Flags: review?(mar.castelluccio) → review+
Assignee | ||
Comment 4•11 years ago
|
||
(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.
Flags: needinfo?(mar.castelluccio)
Comment 5•11 years ago
|
||
(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).
Flags: needinfo?(mar.castelluccio)
Assignee | ||
Comment 6•11 years ago
|
||
(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•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Updated•9 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•