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)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 31

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch The patch (obsolete) (deleted) — 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)
Blocks: 856878
Priority: -- → P2
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+
Attached patch Updated patch (deleted) — Splinter Review
(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 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+
(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)
(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)
(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
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Blocks: 996671
No longer blocks: 856878
QA Whiteboard: [qa-]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: