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

Status: RESOLVED FIXED

(DevTools :: General, defect)

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: v1

https://tbpl.mozilla.org/?tree=Try&rev=dfaa2e257dac

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 699351 [details] [diff] [review]
v1

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

LGTM. Welcome back!

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: 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

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

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 6

[Joe Walker [:jwalker]]

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.

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(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.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/3fec9243d412