Closed Bug 1387122 Opened 7 years ago Closed 7 years ago

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

Categories

(DevTools :: General, enhancement)

Product:
DevTools
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(1 file)

Bug 1387122 - Remove all useless Promise = require("promise").
59 bytes, text/x-review-board-request
tromey
: review+
Details 
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
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+
(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.
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?!
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)
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 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+
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ae94dd731cfd
Remove all useless Promise = require("promise"). r=tromey
https://hg.mozilla.org/mozilla-central/rev/ae94dd731cfd
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: