Closed Bug 995170 Opened 6 years ago Closed 6 years ago

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

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(firefox31 fixed)

VERIFIED FIXED
Firefox 31
Tracking Status
firefox31 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

(Whiteboard: p=1 s=it-31c-30a-29b.3 [qa-])

Attachments

(1 file)

Attached patch The patchSplinter Review
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.
Attachment #8405331 - Flags: review?(dteller)
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.
Blocks: 995184
(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.
Flags: firefox-backlog?
Bug 984365 landed, so BuiltinCommands.jsm no longer exists, and its replacement doesn't use promise.js.
(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 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.
Attachment #8405331 - Flags: review?(dteller) → review+
(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.
Tryserver build of the updated patch:

https://tbpl.mozilla.org/?tree=Try&rev=081600d80e54
(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.
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.
Flags: needinfo?(dteller)
(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.
Flags: firefox-backlog? → firefox-backlog+
Comment on attachment 8405331 [details] [diff] [review]
The patch

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

rubberstamp=me
Attachment #8405331 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/008bbf75bccb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Whiteboard: p=0 s=it-31c-30a-29b.3 [qa?]
Flags: needinfo?(dteller)
Marking [qa-] as per in-testsuite coverage. Please needinfo me if you want more testing.
Whiteboard: p=0 s=it-31c-30a-29b.3 [qa?] → p=0 s=it-31c-30a-29b.3 [qa-]
Status: RESOLVED → VERIFIED
Whiteboard: p=0 s=it-31c-30a-29b.3 [qa-] → p=1 s=it-31c-30a-29b.3 [qa-]
Flags: needinfo?(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.
Flags: needinfo?(jwalker)
Blocks: 996596
(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.
Blocks: 996671
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.