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.

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

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.

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