995170 - Convert legacy uses of promise.js in devtools where this doesn't result in test failures

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

[:Paolo Amadini]

Attachment: The patch

The work in bug 990114 identified several uses of promise.js in devtools that can be easily converted to Promise.jsm without causing test failures.

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

Requesting a rubber-stamp from David on this final batch of conversions.

Comment 1

[Joe Walker [:jwalker]]

Comment on attachment 8405331 [details] [diff] [review]
The patch

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

Please could you leave the changes in BuiltinCommands.jsm to bug 984365.

Comment 2

[:Paolo Amadini]

(In reply to Joe Walker [:jwalker] from comment #1)
> Please could you leave the changes in BuiltinCommands.jsm to bug 984365.

Thanks for the heads up, I can definitely sync this landing with bug 984365.

Comment 3

[Joe Walker [:jwalker]]

Bug 984365 landed, so BuiltinCommands.jsm no longer exists, and its replacement doesn't use promise.js.

Comment 4

[:Paolo Amadini]

(In reply to Joe Walker [:jwalker] from comment #3)
> Bug 984365 landed, so BuiltinCommands.jsm no longer exists, and its
> replacement doesn't use promise.js.

Great!

I'll keep the rubberstamp request on this version, will take this change into account when landing.

Comment 5

[Joe Walker [:jwalker]]

Comment on attachment 8405331 [details] [diff] [review]
The patch

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

My opinion:
There are a few places where you've replaced devtools.require(...) or just require(...) with Cu.import(...)
Please could you leave them saying devtools.require/require. That code uses Cu.import under the hood where required, but it makes sense if we're being consistent about the loader that we're using.
"require(...)" is a stepping stone to ES6 modules.

Comment 6

[:Paolo Amadini]

(In reply to Joe Walker [:jwalker] from comment #5)
> There are a few places where you've replaced devtools.require(...) or just
> require(...) with Cu.import(...)
> Please could you leave them saying devtools.require/require. That code uses
> Cu.import under the hood where required, but it makes sense if we're being
> consistent about the loader that we're using.
> "require(...)" is a stepping stone to ES6 modules.

Unfortunately, as far as I can tell, Promise.jsm does not support being imported with "require". We could file a follow-up bug for that, but we should also keep into account that Promise.jsm will be replaced with DOM Promises when the dependencies of bug 939636 are resolved, requiring no import at all, so this might not be worth the effort.

Comment 7

[:Paolo Amadini]

Tryserver build of the updated patch:

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

Comment 8

[Joe Walker [:jwalker]]

(In reply to :Paolo Amadini from comment #6)
> (In reply to Joe Walker [:jwalker] from comment #5)
> > There are a few places where you've replaced devtools.require(...) or just
> > require(...) with Cu.import(...)
> > Please could you leave them saying devtools.require/require. That code uses
> > Cu.import under the hood where required, but it makes sense if we're being
> > consistent about the loader that we're using.
> > "require(...)" is a stepping stone to ES6 modules.
> 
> Unfortunately, as far as I can tell, Promise.jsm does not support being
> imported with "require".

This seems to work:

  var {Promise: promise} = require("resource://gre/modules/Promise.jsm");

https://hg.mozilla.org/integration/fx-team/file/e2aff2cd9a9a/browser/devtools/framework/sidebar.js#l11

> We could file a follow-up bug for that, but we
> should also keep into account that Promise.jsm will be replaced with DOM
> Promises when the dependencies of bug 939636 are resolved, requiring no
> import at all, so this might not be worth the effort.

Right, but we'll need to do further conversion work though because we'll need to change promise.defer to "new Promise".

It's not a huge issue though - It's a small step backwards, so maybe it's not worth fixing in this patch.

Comment 9

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

Also, for modules loaded using the devtools loader you could just remove the require/import call altogether (Promise.jsm is preloaded). That includes at least the server actors. But as Joe says it's not a huge issue.

Comment 10

[:Paolo Amadini]

(In reply to Joe Walker [:jwalker] from comment #8)
> This seems to work:
> 
>   var {Promise: promise} = require("resource://gre/modules/Promise.jsm");
> 
> https://hg.mozilla.org/integration/fx-team/file/e2aff2cd9a9a/browser/
> devtools/framework/sidebar.js#l11

Ah, I wonder if this translates to an actual Cu.import? I mean that this should load the module only once if it has already been imported by Cu.import, as opposed to loading it with loadSubScript.

If so, we can easily convert existing imports to this new syntax (not limited to Promise.jsm). If not, this is probably something to be addressed as well.

> Right, but we'll need to do further conversion work though because we'll
> need to change promise.defer to "new Promise".
> 
> It's not a huge issue though - It's a small step backwards, so maybe it's
> not worth fixing in this patch.

I think I'll avoid wasting the tryserver build, but I'll prepare a follow-up patch to use "require" where possible, based on the response to the above question.

Comment 11

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/008bbf75bccb

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8405331 [details] [diff] [review]
The patch

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

rubberstamp=me

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/008bbf75bccb

Comment 14

[u279076]

Marking [qa-] as per in-testsuite coverage. Please needinfo me if you want more testing.

Comment 15

[Joe Walker [:jwalker]]

(In reply to :Paolo Amadini from comment #10)
> (In reply to Joe Walker [:jwalker] from comment #8)
> > This seems to work:
> > 
> >   var {Promise: promise} = require("resource://gre/modules/Promise.jsm");
> > 
> > https://hg.mozilla.org/integration/fx-team/file/e2aff2cd9a9a/browser/
> > devtools/framework/sidebar.js#l11
> 
> Ah, I wonder if this translates to an actual Cu.import? I mean that this
> should load the module only once if it has already been imported by
> Cu.import, as opposed to loading it with loadSubScript.
> 
> If so, we can easily convert existing imports to this new syntax (not
> limited to Promise.jsm). If not, this is probably something to be addressed
> as well.

It does translate to a Cu.import

https://hg.mozilla.org/integration/fx-team/file/c300fbbd1b1e/addon-sdk/source/lib/toolkit/loader.js#l588

Recently when I've made significant updates to a module, I've requirified the imports.

Comment 16

[:Paolo Amadini]

(In reply to Joe Walker [:jwalker] from comment #15)
> Recently when I've made significant updates to a module, I've requirified
> the imports.

I've filed bug 996596 to take care of the conversions.