Stop using devtools' Promise.jsm, and start using toolkit/addon-sdk/promise/core.js

RESOLVED FIXED in Firefox 21

Status

()

Firefox
Developer Tools
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

unspecified
Firefox 21
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
Not sure if anything is still using _Promise.jsm other than the debugger for loading sources, but Dave replaced that use in this commit (that has never landed on m-c) during our London meetup.

https://github.com/fitzgen/mozilla-central/commit/33b30ea05c5146701e9ed6dfa22824057db75382
Assignee: nobody → nfitzgerald
Created attachment 699351 [details] [diff] [review]
v1

https://tbpl.mozilla.org/?tree=Try&rev=dfaa2e257dac
Attachment #699351 - Flags: review?(past)
Attachment #699351 - Flags: review?(jwalker)
Comment on attachment 699351 [details] [diff] [review]
v1

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

LGTM. Welcome back!
Attachment #699351 - Flags: review?(past) → review+
Created attachment 699444 [details] [diff] [review]
v1.1

Thanks, Panos! Feels good to be back.

Just fixing a little typo s/Promise.deferred/Promise.defer/ and attacking Promise to imports so that it doesn't leak on to window.

New try push: https://tbpl.mozilla.org/?tree=Try&rev=73340bebc7b7
Attachment #699351 - Attachment is obsolete: true
Attachment #699351 - Flags: review?(jwalker)
Attachment #699444 - Flags: review?(jwalker)
One thing to be aware of is that we generally try to conserve resources in the build infrastructure, so we only run tests that are affected by the patch. In this case you should be fine by just running the mochitest-bc and xpcshell tests.
Comment on attachment 699444 [details] [diff] [review]
v1.1

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

::: toolkit/devtools/debugger/server/dbg-server.js
@@ +24,5 @@
>  Cu.import("resource://gre/modules/jsdebugger.jsm");
>  addDebuggerToGlobal(this);
>  
> +Cu.import("resource://gre/modules/commonjs/promise/core.js");
> +const { defer, resolve, reject } = Promise;

Personal bias - I prefer leaving module names in because it makes subsequent code clearer (to me at least). It's slightly more verbose but you're not left wondering what 'reject' this is at a later date.

I'm guessing that this comment is broader than this file so you're free to ignore it.
Attachment #699444 - Flags: review?(jwalker) → review+
(In reply to Panos Astithas [:past] from comment #5)
> One thing to be aware of is that we generally try to conserve resources in
> the build infrastructure, so we only run tests that are affected by the
> patch. In this case you should be fine by just running the mochitest-bc and
> xpcshell tests.

Consider my chagrined. I have always just used the defaults when pushing to try. I will pay more attention in the future.

(In reply to Joe Walker [:joe_walker] [:jwalker] from comment #6)
> Comment on attachment 699444 [details] [diff] [review]
> v1.1
> 
> Review of attachment 699444 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/debugger/server/dbg-server.js
> @@ +24,5 @@
> >  Cu.import("resource://gre/modules/jsdebugger.jsm");
> >  addDebuggerToGlobal(this);
> >  
> > +Cu.import("resource://gre/modules/commonjs/promise/core.js");
> > +const { defer, resolve, reject } = Promise;
> 
> Personal bias - I prefer leaving module names in because it makes subsequent
> code clearer (to me at least). It's slightly more verbose but you're not
> left wondering what 'reject' this is at a later date.
> 
> I'm guessing that this comment is broader than this file so you're free to
> ignore it.

I don't feel particularly strongly about it either way. If you want me to remove the destructuring, I can.
Attachment #699444 - Flags: checkin?(jwalker)
https://hg.mozilla.org/integration/fx-team/rev/3fec9243d412
Status: NEW → ASSIGNED
Whiteboard: [fixed-in-fx-team]
Attachment #699444 - Flags: checkin?(jwalker) → checkin+
https://hg.mozilla.org/mozilla-central/rev/3fec9243d412
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 21
You need to log in before you can comment on or make changes to this bug.