Status

enhancement
P1
normal
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: tromey, Assigned: tromey)

Tracking

unspecified
Firefox 49
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [devtools-html])

Attachments

(1 attachment, 4 obsolete attachments)

Assignee

Description

3 years ago
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.
Assignee

Comment 2

3 years ago
Probably depends on loader plan.
Depends on: 1248830
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)
Assignee

Comment 4

3 years ago
(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.
Assignee

Comment 7

3 years ago
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.
Assignee

Comment 9

3 years ago
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.
Flags: qe-verify-
Priority: -- → P2
Assignee

Updated

3 years ago
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Iteration: --- → 49.2 - May 23
Priority: P2 → P1
Assignee

Comment 11

3 years ago
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/
Attachment #8751874 - Flags: review?(jryans)
Attachment #8751875 - Flags: review?(jryans)
Attachment #8751876 - Flags: review?(jryans)
Attachment #8751877 - Flags: review?(jryans)
Assignee

Comment 12

3 years ago
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/
Assignee

Comment 13

3 years ago
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/
Assignee

Comment 14

3 years ago
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
Attachment #8751875 - Flags: review?(jryans) → review+
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
Attachment #8751876 - Flags: review?(jryans) → review+
Attachment #8751877 - Flags: review?(jryans) → review+
Comment on attachment 8751877 [details]
MozReview Request: Bug 1265869 - eslint fixes from previous changes; r=jryans

https://reviewboard.mozilla.org/r/52267/#review49227
Attachment #8751874 - Flags: review?(jryans)
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
Assignee

Comment 19

3 years ago
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/
Attachment #8751875 - Attachment description: MozReview Request: Bug 1265869 - use task.js in devtools; r?jryans → MozReview Request: Bug 1265869 - use task.js in devtools; r=jryans
Attachment #8751876 - Attachment description: MozReview Request: Bug 1265869 - add missing task imports; r?jryans → MozReview Request: Bug 1265869 - add missing task imports; r=jryans
Attachment #8751877 - Attachment description: MozReview Request: Bug 1265869 - eslint fixes from previous changes; r?jryans → MozReview Request: Bug 1265869 - eslint fixes from previous changes; r=jryans
Attachment #8751874 - Flags: review?(jryans)
Assignee

Comment 20

3 years ago
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/
Assignee

Comment 21

3 years ago
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/
Assignee

Comment 22

3 years ago
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
Attachment #8751874 - Flags: review?(jryans) → review+

Comment 25

3 years ago
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.
Assignee

Comment 26

3 years ago
(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.
Assignee

Updated

3 years ago
Attachment #8751874 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8751875 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8751876 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8751877 - Attachment is obsolete: true
Assignee

Comment 28

3 years ago
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
Attachment #8753479 - Flags: review?(jryans) → review+
Assignee

Comment 30

3 years ago
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/
Assignee

Comment 32

3 years ago
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/
Assignee

Comment 33

3 years ago
Now with an eslint fix.
Assignee

Updated

3 years ago
Keywords: checkin-needed
Needs rebasing against fx-team tip.
Keywords: checkin-needed
Assignee

Comment 35

3 years ago
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/
Assignee

Comment 36

3 years ago
Rebased.
Keywords: checkin-needed
Assignee

Comment 37

3 years ago
I notice that during rebasing a few more Task.jsm uses crept in.
Keywords: checkin-needed
Assignee

Comment 38

3 years ago
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/
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 41

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c0a6a1b663f7
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.