Drop all usages of promise = require(promise) in devtools/server/

RESOLVED FIXED in Firefox 60

Status

enhancement
P2
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: ochameau, Assigned: ochameau)

Tracking

(Depends on 1 bug, Blocks 2 bugs)

unspecified
Firefox 60
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

We can't easily remove Promise.jsm usage from client codebase because of bug 1387128 comment 14.
But now that bug 1392540 is fixed, we can stop using it in the server codebase.
As Site isolation may require some significant work to be done on the server codebase, I think it may help to simplify the promise story.
I also thought it would bring some performance improvement, but DAMP report no significant change.

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=857e22cf6e5b75e4ceae0b10425d0ab2f783b139&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&framework=1&selectedTimeRange=172800
Almost green try:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=857e22cf6e5b75e4ceae0b10425d0ab2f783b139&selectedJob=161120469
I'll repush after review, just before try to land.

All changesets but the last one are automated.
There is a random change in the last changeset about devtools/shared/.
I ended up seeing this error by applying by mistake one of the automated patch to shared module.

I'll merge all the changesets into only one before merging as they can't be applied individually without breaking devtools.
Comment on attachment 8949681 [details]
Bug 1436978 - Drop all the require("promise").

https://reviewboard.mozilla.org/r/219022/#review224760

::: devtools/server/actors/string.js:9
(Diff revision 1)
>  "use strict";
>  
>  var {DebuggerServer} = require("devtools/server/main");
>  
> -var promise = require("promise");
>  

nit: remove extra blank line here
Attachment #8949681 - Flags: review?(jdescottes) → review+
Comment on attachment 8949682 [details]
Bug 1436978 - Also remove the lazy require to promise.

https://reviewboard.mozilla.org/r/219024/#review224762
Attachment #8949682 - Flags: review?(jdescottes) → review+
Comment on attachment 8949681 [details]
Bug 1436978 - Drop all the require("promise").

https://reviewboard.mozilla.org/r/219022/#review224760

> nit: remove extra blank line here

Done in the last commit, missed that!
Comment on attachment 8949683 [details]
Bug 1436978 - Manual fixes

https://reviewboard.mozilla.org/r/219026/#review224766

::: devtools/shared/system.js:314
(Diff revision 1)
>      // TODO bug 1205797, make this work in child processes.
>      try {
>        settingsService = Cc["@mozilla.org/settingsService;1"]
>                            .getService(Ci.nsISettingsService);
>      } catch (e) {
> -      return promise.reject(e);
> +      return deferred.reject(e);

deferred.reject(e) will return undefined, so we can't return it directly. Either do 
  
  return Promise.reject(e);
  
or
  
  deferred.reject(e);
  return deferred.promised.
Attachment #8949683 - Flags: review?(jdescottes) → review+
Comment on attachment 8949680 [details]
Bug 1436978 - Replace all other lower case usages.

https://reviewboard.mozilla.org/r/219020/#review224758

only missing promise.then

::: commit-message-408e2:3
(Diff revision 1)
> +Bug 1436978 - Replace all other lower case usages. r=jdescottes
> +
> +$ sed -i -E "s/promise\.(resolve|reject|all|race)\(/Promise.\1(/" $(egrep -rlE "promise.(resolve|reject|all|race)\(" devtools/server/)

We have three files using promise.then(...) in devtools/server/ , could add it to the rewrite here?
Attachment #8949680 - Flags: review?(jdescottes) → review+
Comment on attachment 8949679 [details]
Bug 1436978 - Stop using Promise.jsm in devtools/server in favor of DOM Promises.

https://reviewboard.mozilla.org/r/219018/#review224768

Thanks for the cleanup! Tiny comments to address, feel free to land afterwards with a green (or green enough) try.
Attachment #8949679 - Flags: review?(jdescottes) → review+
Comment on attachment 8949680 [details]
Bug 1436978 - Replace all other lower case usages.

https://reviewboard.mozilla.org/r/219020/#review224758

> We have three files using promise.then(...) in devtools/server/ , could add it to the rewrite here?

My bad, had not seen this was a promise argument here, and then is not even defined on Promise! Sorry.
(In reply to Julian Descottes [:jdescottes][:julian] from comment #10)
> Comment on attachment 8949683 [details]
> Bug 1436978 - Manual fixes
> 
> https://reviewboard.mozilla.org/r/219026/#review224766
> 
> ::: devtools/shared/system.js:314
> (Diff revision 1)
> >      // TODO bug 1205797, make this work in child processes.
> >      try {
> >        settingsService = Cc["@mozilla.org/settingsService;1"]
> >                            .getService(Ci.nsISettingsService);
> >      } catch (e) {
> > -      return promise.reject(e);
> > +      return deferred.reject(e);
> 
> deferred.reject(e) will return undefined, so we can't return it directly.
> Either do 
>   
>   return Promise.reject(e);
>   
> or
>   
>   deferred.reject(e);
>   return deferred.promised.

I misread this code, I thought it was broken, but it isn't!
I removed this change from my patch queue.
Blocks: 1437828
Attachment #8949680 - Attachment is obsolete: true
Attachment #8949681 - Attachment is obsolete: true
Attachment #8949682 - Attachment is obsolete: true
Attachment #8949683 - Attachment is obsolete: true
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/46a5b62580f6
Stop using Promise.jsm in devtools/server in favor of DOM Promises. r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/46a5b62580f6
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.