That wouldn't hurt to cleanup the various differents ways we are pulling Promise-backend.jsm in our codebase. * Sometimes we are using require("promise") (which I think is what we would like to end up with everywhere). But in some other places we uses require("...Promise.jsm") or Cu.import("...Promise.jsm"). * in many places we import it without actually using it. * We also have at least to sort of symlink to it: browser/devtools/projecteditor/lib/helpers/promise.js toolkit/devtools/gcli/source/lib/gcli/util/promise.js * And finally, we want to use a lowercase version everywhere so that we distinguish clearly with regular DOM promises. I would be surprised if there isn't any bug opened about any of that, but can't find any?

We also use DOM promises in a bunch of places... to be honest I am not sure why we don't just use them everywhere.

Because they are not as debuggable and don't have the auto-report-error-when-noone-catch-rejection? To be clear, this isn't about using different promises here and there (which can change behavior and break stuff). All the refactoring I want to do in this bug is safe refactoring, just to have one single way to import non-DOM promises. So that we end up with only three scenarios instead of many: - Use dom promises, just don't import anything and use `Promise`, - Use Promise.jsm, import `promise = require("promise")` and use `promise`, (This is my main goal. To converge all code to do that) - Use deprecated-sync-promise, import the related js and use `promise`. (I won't do anything regarding this in this bug)

Comment on attachment 8646441 [details] [diff] [review] Remove unused exports - v1 Review of attachment 8646441 [details] [diff] [review]: ----------------------------------------------------------------- I hope you don't mind receiving these reviews of big patches like that. Hopefully you like seeing our codebase be cleaner?! This patch is really simple, I remove all imports of promise that are unused in the scope they are imported. (I took care of not removing them from head.js files or content files that might have shared globals) While I was scanning these files (which had potentiel import with no usage), I also modified some imports to use require("promise"). But I'll make a dedicated patch for that. ::: toolkit/devtools/server/tests/mochitest/inspector-helpers.js @@ +5,5 @@ > const {DebuggerServer} = require("devtools/server/main"); > Cu.import("resource://gre/modules/Task.jsm"); > > const Services = require("Services"); > +const promise = require("promise"); I introduce promise in the helper, as it is used in this file. That allowed me to remove it in all tests that used to import promise and inspector-helpers.js.

Comment on attachment 8646826 [details] [diff] [review] Cleanup gcli promise - v1 Joe, What do you think about gcli and promises? Would you be ok to get rid of the util/promise alias? Is that ok to rename all `Promise` to `promise` in order to match codestyle from everywhere else? Or would you prefer keeping Promise and use DOM Promises?

Comment on attachment 8646826 [details] [diff] [review] Cleanup gcli promise - v1 Review of attachment 8646826 [details] [diff] [review]: ----------------------------------------------------------------- GCLI is used in other projects like Orion, for which we need promises to not work in a mozilla specific way. I'm not strongly tied to that, but if we're going to make that break, I'd like to do so deliberately. The lower case p concerns me. "new promise(...)" just feels like we made things weird. I get that we're dodging DOM Promises, but can't help feeling that DOM Promises are probably an eventual goal, so using the same API makes sense. Since the implementation of the promise that is used here (i.e. in toolkit/devtools/gcli/source/lib/gcli/util/promise.js) is "require('promise')", then perhaps we don't gain much by doing this; ultimately we're not using DOM Promises anyway. So I'm leaning towards thinking we should leave these alone. Thoughts?

(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #7) > Comment on attachment 8646826 [details] [diff] [review] > Cleanup gcli promise - v1 > > Review of attachment 8646826 [details] [diff] [review]: > ----------------------------------------------------------------- > > GCLI is used in other projects like Orion, for which we need promises to not > work in a mozilla specific way. I'm not strongly tied to that, but if we're > going to make that break, I'd like to do so deliberately. This is mostly to have a coherent codebase. It is somewhat ok if gcli is considered as external codebase with its own specifics. > > The lower case p concerns me. "new promise(...)" just feels like we made > things weird. I get that we're dodging DOM Promises, but can't help feeling > that DOM Promises are probably an eventual goal, so using the same API makes > sense. > > Since the implementation of the promise that is used here (i.e. in > toolkit/devtools/gcli/source/lib/gcli/util/promise.js) is > "require('promise')", then perhaps we don't gain much by doing this; > ultimately we're not using DOM Promises anyway. > > So I'm leaning towards thinking we should leave these alone. Thoughts? What about keeping Promise, not importing anything and so using DOM Promises? That would be great if we could end up with only two scenarios: * promise = require("promise") * Promise = DOM Promises

(In reply to Alexandre Poirot [:ochameau] from comment #8) > What about keeping Promise, not importing anything and so using DOM Promises? > That would be great if we could end up with only two scenarios: > > * promise = require("promise") > * Promise = DOM Promises +1

Comment on attachment 8646441 [details] [diff] [review] Remove unused exports - v1 Review of attachment 8646441 [details] [diff] [review]: ----------------------------------------------------------------- I am totally fine reviewing these patches! Seeing things get cleaned up like this is great. Thanks for working on it. :D ::: toolkit/devtools/gcli/source/lib/gcli/util/promise.js @@ +15,5 @@ > */ > > 'use strict'; > > +exports.Promise = require("promise"); Will this change after the discussion with Joe in the comments?

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #10) > Comment on attachment 8646441 [details] [diff] [review] > Remove unused exports - v1 > > Review of attachment 8646441 [details] [diff] [review]: > ----------------------------------------------------------------- > > I am totally fine reviewing these patches! Seeing things get cleaned up > like this is great. Thanks for working on it. :D Great, I hope you would change my mind when I'm going to keep throwing such patches :o > > ::: toolkit/devtools/gcli/source/lib/gcli/util/promise.js > @@ +15,5 @@ > > */ > > > > 'use strict'; > > > > +exports.Promise = require("promise"); > > Will this change after the discussion with Joe in the comments? Most likely, or may be not. It is quite independent, so I don't worry about this change.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=03366f0e1891 Try run with all patches applied.

Comment on attachment 8647516 [details] [diff] [review] Make gcli use DOM Promise - v2 Review of attachment 8647516 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. ::: toolkit/devtools/gcli/source/lib/gcli/cli.js @@ +612,5 @@ > Object.defineProperty(Requisition.prototype, 'conversionContext', { > get: function() { > if (this._conversionContext == null) { > this._conversionContext = { > + defer: defer, I'm fairly sure we could just delete this, but I'll check. No need for this patch.

Comment on attachment 8647514 [details] [diff] [review] Remove projecteditor promise alias in favor of require(promise). r=bgrins Review of attachment 8647514 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me as long as tests pass!

https://hg.mozilla.org/integration/fx-team/rev/dffe88885c1b https://hg.mozilla.org/integration/fx-team/rev/b7b1caeba71d https://hg.mozilla.org/integration/fx-team/rev/9820ec447142

https://hg.mozilla.org/mozilla-central/rev/dffe88885c1b https://hg.mozilla.org/mozilla-central/rev/b7b1caeba71d https://hg.mozilla.org/mozilla-central/rev/9820ec447142