Remove all useless import: Promise = require("promise")

RESOLVED FIXED in Firefox 57

Status

()

Firefox
Developer Tools
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: ochameau, Assigned: ochameau)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

4 months ago
All DevTools sandboxes already have DOM Promises in their global scope exposed as "Promise", so we should be able to remove all the import of the form:

  Promise = require("promise")

http://searchfox.org/mozilla-central/search?q=Promise+%3D+require(%22promise%22)&case=true&regexp=false&path=devtools%2F
(Assignee)

Comment 1

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=220f7e1b89303ccc9cfd97f56c63e37f1d080a04
(Assignee)

Comment 2

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=904e4082ec093335c1139c416d0e03d6d36d6e9f
Comment hidden (mozreview-request)

Comment 4

4 months ago
mozreview-review
Comment on attachment 8893493 [details]
Bug 1387122 - Remove all useless Promise = require("promise").

https://reviewboard.mozilla.org/r/164570/#review170008

Thanks for doing this.

I found one thing that I think needs to be addressed.  The patch is ok with that.

::: devtools/server/main.js:968
(Diff revision 1)
>     */
>    setupInChild({ module, setupChild, args, waitForEval }) {
>      if (this._childMessageManagers.size == 0) {
>        return Promise.resolve();
>      }
> -    let deferred = Promise.defer();
> +    let deferred = defer();

It seems to me that if you know you want to switch this to `new Promise`, you might as well just do it immediately.

Either way is ok, though, I suppose.  So I'm not turning this into an issue.

::: devtools/shared/defer.js
(Diff revision 1)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
>  // See bug 1273941 to understand this choice of promise.
> -const Promise = require("promise");

For this change the comment seems relevant; I don't remember what's going on here; but either the comment stays and this hunk can't be landed, or the comment should be removed as well.
Attachment #8893493 - Flags: review+
(Assignee)

Comment 5

4 months ago
(In reply to Tom Tromey :tromey from comment #4)
> Comment on attachment 8893493 [details]
> Bug 1387122 - Remove all useless Promise = require("promise").
> 
> https://reviewboard.mozilla.org/r/164570/#review170008
> 
> Thanks for doing this.
> 
> I found one thing that I think needs to be addressed.  The patch is ok with
> that.
> 
> ::: devtools/server/main.js:968
> (Diff revision 1)
> >     */
> >    setupInChild({ module, setupChild, args, waitForEval }) {
> >      if (this._childMessageManagers.size == 0) {
> >        return Promise.resolve();
> >      }
> > -    let deferred = Promise.defer();
> > +    let deferred = defer();
> 
> It seems to me that if you know you want to switch this to `new Promise`,
> you might as well just do it immediately.
> 
> Either way is ok, though, I suppose.  So I'm not turning this into an issue.

Ok, I'll try to make it use new Promise. I thought this patch would be straightforward and could be done automatically, but no.
So I'm going to review many each change.

Note that I won't do that in bug 1387123 as there is way to many occurences of promise.defer and I want to first be able to remove Promise.jsm.

> 
> ::: devtools/shared/defer.js
> (Diff revision 1)
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> >  "use strict";
> >  
> >  // See bug 1273941 to understand this choice of promise.
> > -const Promise = require("promise");
> 
> For this change the comment seems relevant; I don't remember what's going on
> here; but either the comment stays and this hunk can't be landed, or the
> comment should be removed as well.

I imagine that's because of bug 1273941 comment 1, at the time you made this comment native DOM Promise were lacking some debugging support. I looks like it is no longer the case, so I'll remove this comment as well.
(Assignee)

Comment 6

4 months ago
Note that I didn't r?. I was waiting for try results, and there are not good.
Even for this simple which is the simplest in the "removal of promise.jsm" serie...

I have to look into these two xpcshell failures:
TEST-UNEXPECTED-FAIL | devtools/server/tests/unit/test_protocol_stack.js | run_test/onConnect/< - [run_test/onConnect/< : 86] Incomplete stack - false == true
TEST-UNEXPECTED-FAIL | devtools/shared/tests/unit/test_executeSoon.js |  - Incomplete stack - false == true

And a couple of mochitest failures like this one:
TEST-UNEXPECTED-FAIL | devtools/client/framework/test/browser_target_events.js | A promise chain failed to handle a rejection: unsafe CPOW usage forbidden - stack: handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:140:9
INFO - emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:194:13
INFO - onLocationChange@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/target.js:789:9

I'm wondering if the mochitest only starts failing because of unhandled promise rejection being better reported?!
Comment hidden (mozreview-request)

Comment 8

4 months ago
hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try
searching for changes
remote: abort: abandoned transaction found!
remote: (run 'hg recover' to clean up transaction)
abort: stream ended unexpectedly (got 0 bytes, expected 4)
(Assignee)

Comment 9

4 months ago
Comment on attachment 8893493 [details]
Bug 1387122 - Remove all useless Promise = require("promise").

It looks like we should be able to proceed with this first piece.
I stripped out the devtools/shared/defer modification and modified server/main to use 'new Promise' instead of 'defer'.
Attachment #8893493 - Flags: review?(ttromey)

Comment 10

4 months ago
mozreview-review
Comment on attachment 8893493 [details]
Bug 1387122 - Remove all useless Promise = require("promise").

https://reviewboard.mozilla.org/r/164570/#review171156

Thanks for the patch.  This looks good.
Attachment #8893493 - Flags: review?(ttromey) → review+

Comment 11

4 months ago
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ae94dd731cfd
Remove all useless Promise = require("promise"). r=tromey

Comment 12

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ae94dd731cfd
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.