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

RESOLVED FIXED in Firefox 60

Status

enhancement
P2
normal
RESOLVED FIXED
a year ago
11 months ago

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)

(Assignee)

Description

a year ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

a year ago
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.
(Assignee)

Updated

a year ago

Comment 7

a year ago
mozreview-review
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 8

a year ago
mozreview-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 9

a year ago
mozreview-review-reply
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

a year ago
mozreview-review
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 11

a year ago
mozreview-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 12

a year ago
mozreview-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 13

a year ago
mozreview-review-reply
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.
(Assignee)

Comment 14

a year ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Blocks: 1437828
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8949680 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8949681 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8949682 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8949683 - Attachment is obsolete: true

Comment 22

a year ago
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

Comment 23

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/46a5b62580f6
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.