Task.jsm is used in many places in devtools but must be replaced for the de-chrome-ification project. One idea might be to turn it into a .js and copy it somewhere in our tree.

There is https://github.com/mozilla/task.js that Task.jsm is based on. Task.jsm has grown organically inside m-c to learn more promise and stack reporting tricks which we may want to preserve, but at the same time those tricks might not be available to content JS.

Probably depends on loader plan.

In bug 1264625, I'm about to copy XPCOMUtils.jsm into /devtools/. I imagine we could do something similar here. Task.jsm doesn't use much "chrome" things. It seems to be only about: Cu.permitCPOWsInScope(this); Cu.import("resource://gre/modules/Promise.jsm"); What do you think about copying it, ignore the first permitCPOWsInScope (which looks useless for us) and replace Cu.import by require("promise") ? Then we would replace all Cu.import(Task.jsm) by require("devtools/shared/task") ? (or something similar)

(In reply to Alexandre Poirot [:ochameau] from comment #3) > What do you think about copying it, ignore the first permitCPOWsInScope > (which looks useless for us) and replace Cu.import by require("promise") ? I dislike code duplication, but since that seems inevitable, I think this approach would be fine by me.

One arlternative could be to tweak Task.jsm itself, so that it can be loaded as a SDK modules without using Cu. We already have some modules that can be loaded as JSM or module, but we would need some magic to not use Cu in module loaded case. Or split Task.jsm into two files, like Promise.jsm and Promise-backend.js... But it is any better than duplication?

I'd be concerned that duplicating xpcom code into a content-compatible version becomes a pattern, I've seen a few such bugs fly by. I see it may be inevitable, but in that case maybe it makes sense to duplicate this to somewhere in toolkit/ so that other parts of the Mozilla codebase could make use of it too. If you agree, then consequently it might actually make sense to split into two files.

We discussed this at the meeting and agreed that we'd go forward with the "copy" plan. One open question is how to handle code in devtools/shared. At the meeting we talked about putting task.js into devtools/client/shared, but I totally forgot about devtools/shared there :( I think there was general agreement that it's probably premature for us to try to fix this in toolkit.

Seems like somewhere in devtools/shared is more appropriate since it's used heavily by both client and server.

I was thinking the server code would continue to use Task.jsm. But I suppose there's really no requirement that devtools/shared continue to do so.

(In reply to Tom Tromey :tromey from comment #9) > I was thinking the server code would continue to use Task.jsm. > But I suppose there's really no requirement that devtools/shared continue to > do so. Ah, I see... I do think a better ideal state is only one version of general libraries like this for all of DevTools, as opposed to a content version for client and chrome version for server (if it's possible for everyone to just use the content focused version), at least if it can be done without inflating scope. We may end up with different choices for each general library like this as well, I think it depends a lot on the details. For this case, I would hope there won't be much change to consumers of Task as far as the API goes, at most we'd just change the require ID. So, hopefully we could continue with just one copy for DevTools.

This copies Task.jsm and turns it into a regular .js. Review commit: https://reviewboard.mozilla.org/r/52261/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/52261/

This changes everything in devtools to use the new task.js. This patch is pretty boring. It just rewrites imports; it's split out to make the review more obvious. I tried to preserve coding style everywhere, though I may have missed a spot or two. In some cases I changed lazy imports to non-lazy ones; but if this is a concern I can go back and fix these. Review commit: https://reviewboard.mozilla.org/r/52263/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/52263/

Testing revealed some spots that were relying on Cu.import to define Task. Review commit: https://reviewboard.mozilla.org/r/52265/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/52265/

The big "mechanical" patch caused some eslint failures, fixed here. Review commit: https://reviewboard.mozilla.org/r/52267/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/52267/

Comment on attachment 8751875 [details] MozReview Request: Bug 1265869 - use task.js in devtools; r=jryans https://reviewboard.mozilla.org/r/52263/#review49219 ::: devtools/client/debugger/debugger-controller.js:151 (Diff revision 1) > var DebuggerEditor = require("devtools/client/sourceeditor/debugger"); > var {Tooltip} = require("devtools/client/shared/widgets/Tooltip"); > var FastListWidget = require("devtools/client/shared/widgets/FastListWidget"); > var {LocalizationHelper} = require("devtools/client/shared/l10n"); > var {PrefsHelper} = require("devtools/client/shared/prefs"); > +const {Task} = require("devtools/shared/task"); Make this `var` to match local style ::: devtools/client/inspector/fonts/fonts.js:16 (Diff revision 1) > const Services = require("Services"); > > const DEFAULT_PREVIEW_TEXT = "Abc"; > const PREVIEW_UPDATE_DELAY = 150; > > Cu.import("resource://gre/modules/XPCOMUtils.jsm"); Can you remove XPCOMUtils now? What about Cu? ::: devtools/client/webconsole/console-output.js:15 (Diff revision 1) > > const Services = require("Services"); > > loader.lazyImporter(this, "VariablesView", "resource://devtools/client/shared/widgets/VariablesView.jsm"); > loader.lazyImporter(this, "escapeHTML", "resource://devtools/client/shared/widgets/VariablesView.jsm"); > -loader.lazyImporter(this, "Task", "resource://gre/modules/Task.jsm"); > +const {Task} = require("devtools/shared/task"); Maybe move down to requires below? ::: devtools/server/tests/mochitest/director-helpers.js:21 (Diff revision 1) > const { DirectorRegistry, > DirectorRegistryFront } = require("devtools/server/actors/director-registry"); > > const { DirectorManagerFront } = require("devtools/server/actors/director-manager"); > > -const {Task} = require("resource://gre/modules/Task.jsm"); > +const {Task} = require("devtools/shared/task"); { Task } to match local style

Comment on attachment 8751876 [details] MozReview Request: Bug 1265869 - add missing task imports; r=jryans https://reviewboard.mozilla.org/r/52265/#review49225 ::: devtools/client/promisedebugger/promise-panel.js:7 (Diff revision 1) > /* vim: set ft=javascript ts=2 et sw=2 tw=80: */ > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > -/* global PromisesController, promise */ > +/* global PromisesController, promise, Task */ import-globals-from promise-controller is probably better

Comment on attachment 8751877 [details] MozReview Request: Bug 1265869 - eslint fixes from previous changes; r=jryans https://reviewboard.mozilla.org/r/52267/#review49227

Comment on attachment 8751874 [details] MozReview Request: Bug 1265869 - add task.js; r?jryans https://reviewboard.mozilla.org/r/52261/#review49233 ::: devtools/shared/task.js:90 (Diff revision 1) > + */ > + > +//////////////////////////////////////////////////////////////////////////////// > +//// Globals > + > +const { Promise } = require("resource://gre/modules/Promise.jsm"); Since you know you are inside the DevTools loader, `require("promise")` seems more natural here. ::: devtools/shared/task.js:446 (Diff revision 1) > + this.deferred.reject(exception); > + }, > + > + get callerStack() { > + // Cut `this._stack` at the last line of the first block that > + // contains Task.jsm, keep the tail. Update this comment ::: devtools/shared/task.js:448 (Diff revision 1) > + > + get callerStack() { > + // Cut `this._stack` at the last line of the first block that > + // contains Task.jsm, keep the tail. > + for (let [line, index] of linesOf(this._stack || "")) { > + if (line.indexOf("/Task.jsm:") == -1) { Seems like this needs a change of some kind ::: devtools/shared/task.js:494 (Diff revision 1) > + generateReadableStack: function (topStack, prefix = "") { > + if (!gCurrentTask) { > + return topStack; > + } > + > + // Cut `topStack` at the first line that contains Task.jsm, keep the head. Update this comment ::: devtools/shared/task.js:497 (Diff revision 1) > + } > + > + // Cut `topStack` at the first line that contains Task.jsm, keep the head. > + let lines = []; > + for (let [line] of linesOf(topStack)) { > + if (line.indexOf("/Task.jsm:") != -1) { Seems like this needs a change of some kind

Comment on attachment 8751874 [details] MozReview Request: Bug 1265869 - add task.js; r?jryans Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52261/diff/1-2/

Comment on attachment 8751875 [details] MozReview Request: Bug 1265869 - use task.js in devtools; r=jryans Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52263/diff/1-2/

Comment on attachment 8751876 [details] MozReview Request: Bug 1265869 - add missing task imports; r=jryans Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52265/diff/1-2/

Comment on attachment 8751877 [details] MozReview Request: Bug 1265869 - eslint fixes from previous changes; r=jryans Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52267/diff/1-2/

Comment on attachment 8751874 [details] MozReview Request: Bug 1265869 - add task.js; r?jryans https://reviewboard.mozilla.org/r/52261/#review49507

https://reviewboard.mozilla.org/r/52261/#review49700 ::: devtools/shared/task.js:12 (Diff revision 2) > +/** > + * This module implements a subset of "Task.js" <http://taskjs.org/>. > + * It is a copy of toolkit/modules/Task.jsm. Please try not to > + * diverge the API here. For what it's worth, I'd support having a Task-backend.js module using the same approach as Promise-backend.js. If you're still going for the copy approach, please use "hg copy" instead of adding a new file.

(In reply to :Paolo Amadini from comment #25) > For what it's worth, I'd support having a Task-backend.js module using the > same approach as Promise-backend.js. I think that won't work for us, because the idea is to have something available to devtools when they are running in content. > If you're still going for the copy approach, please use "hg copy" instead of > adding a new file. Ok.

Review commit: https://reviewboard.mozilla.org/r/53312/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53312/

This is the same patch, but rebased, squashed, and with "hg cp". I will fix the try failures in a subsequent push, which should let interdiffing work.

Comment on attachment 8753479 [details] MozReview Request: Bug 1265869 - add task.js and use in devtools; r=jryans https://reviewboard.mozilla.org/r/53312/#review50092

Comment on attachment 8753479 [details] MozReview Request: Bug 1265869 - add task.js and use in devtools; r=jryans Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53312/diff/1-2/

Comment on attachment 8753479 [details] MozReview Request: Bug 1265869 - add task.js and use in devtools; r=jryans Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53312/diff/2-3/

Now with an eslint fix.

Needs rebasing against fx-team tip.

Comment on attachment 8753479 [details] MozReview Request: Bug 1265869 - add task.js and use in devtools; r=jryans Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53312/diff/3-4/

Rebased.

I notice that during rebasing a few more Task.jsm uses crept in.

Comment on attachment 8753479 [details] MozReview Request: Bug 1265869 - add task.js and use in devtools; r=jryans Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53312/diff/4-5/