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

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P2)

Description

[Alexandre Poirot [:ochameau]]

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

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1436978 - Stop using Promise.jsm in devtools/server in favor of DOM Promises.

$ sed -i 's/new promise(/new Promise(/' $(egrep -rl 'new promise' devtools/server)

Review commit: https://reviewboard.mozilla.org/r/219018/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219018/

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1436978 - Replace all other lower case usages.

$ sed -i -E "s/promise\.(resolve|reject|all|race)\(/Promise.\1(/" $(egrep -rlE "promise.(resolve|reject|all|race)\(" devtools/server/)

Review commit: https://reviewboard.mozilla.org/r/219020/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219020/

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1436978 - Drop all the require("promise").

$ sed -i '/promise = require("promise")/d' $(egrep -rl 'require\("promise"\)' devtools/server)

Review commit: https://reviewboard.mozilla.org/r/219022/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219022/

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1436978 - Also remove the lazy require to promise.

$ sed -i '/lazyRequireGetter(this, "promise")/d' $(egrep -rl 'lazyRequireGetter\(this, "promise"\)' devtools/server)

Review commit: https://reviewboard.mozilla.org/r/219024/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219024/

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1436978 - Manual fixes

Review commit: https://reviewboard.mozilla.org/r/219026/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219026/

Comment 6

[Alexandre Poirot [:ochameau]]

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 7

[Julian Descottes [:jdescottes]]

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

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8949682 [details]
Bug 1436978 - Also remove the lazy require to promise.

https://reviewboard.mozilla.org/r/219024/#review224762

Comment 9

[Julian Descottes [:jdescottes]]

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 10

[Julian Descottes [:jdescottes]]

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.

Comment 11

[Julian Descottes [:jdescottes]]

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?

Comment 12

[Julian Descottes [:jdescottes]]

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.

Comment 13

[Julian Descottes [:jdescottes]]

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.

Comment 14

[Alexandre Poirot [:ochameau]]

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

Comment 15

[Alexandre Poirot [:ochameau]]

Comment on attachment 8949679 [details]
Bug 1436978 - Stop using Promise.jsm in devtools/server in favor of DOM Promises.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219018/diff/1-2/

Comment 16

[Alexandre Poirot [:ochameau]]

Comment on attachment 8949680 [details]
Bug 1436978 - Replace all other lower case usages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219020/diff/1-2/

Comment 17

[Alexandre Poirot [:ochameau]]

Comment on attachment 8949681 [details]
Bug 1436978 - Drop all the require("promise").

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219022/diff/1-2/

Comment 18

[Alexandre Poirot [:ochameau]]

Comment on attachment 8949682 [details]
Bug 1436978 - Also remove the lazy require to promise.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219024/diff/1-2/

Comment 19

[Alexandre Poirot [:ochameau]]

Comment on attachment 8949683 [details]
Bug 1436978 - Manual fixes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219026/diff/1-2/

Comment 20

[Alexandre Poirot [:ochameau]]

Green try:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=7932fbf00e64da73af87fbafa50dc404d8dda2ba

Comment 21

[Alexandre Poirot [:ochameau]]

Comment on attachment 8949679 [details]
Bug 1436978 - Stop using Promise.jsm in devtools/server in favor of DOM Promises.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219018/diff/2-3/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46a5b62580f6
Stop using Promise.jsm in devtools/server in favor of DOM Promises. r=jdescottes

Comment 23

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/46a5b62580f6