Closed Bug 783420 Opened 9 years ago Closed 9 years ago

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

Categories

(DevTools :: General, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 21

People

(Reporter: fitzgen, Assigned: fitzgen)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
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
Attached patch v1 (obsolete) — Splinter Review
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+
Attached patch v1.1Splinter Review
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)
Attachment #699444 - Flags: checkin?(jwalker) → checkin+
https://hg.mozilla.org/mozilla-central/rev/3fec9243d412
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 21
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.