1164632 - Use new worker helpers for pretty-printer

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[James Long (:jlongster)]

We just moved Jordan's worker helpers into toolkit in bug 1164483, so let's convert the debugger's pretty-printer to use it.

Comment 1

[James Long (:jlongster)]

Attachment: 1164632.patch

Sorry for so many r?, this is the last round that I should need from you. I copied some functionality that the debugger had for managing workers. It will log messages if `dumpn` is available and you set it to verbose. All workers must also be named for debugging purposes. What do you think?

Sorry the worker-helper.js file changes are hard to read, it's all indented now because I needed to wrap it in a function that is invoked according to the module system. This is because I couldn't use require.js in the pretty-print helper because it conflicts with other libraries it uses.

Comment 2

[James Long (:jlongster)]

Attachment: 1164632.patch

I turned off verbose logging in the worker helper tests because the data that the timestamp plotting returns in huge

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8605467 [details] [diff] [review]
1164632.patch

Review of attachment 8605467 [details] [diff] [review]:
-----------------------------------------------------------------

nits below

::: toolkit/devtools/server/tests/browser/head.js
@@ +6,5 @@
>  const Ci = Components.interfaces;
>  const Cu = Components.utils;
>  
>  Cu.import("resource://gre/modules/Services.jsm");
> +Services.prefs.setBoolPref("devtools.debugger.log", true);

Should reset this to default in

::: toolkit/devtools/shared/worker.js
@@ +33,5 @@
>   *
>   * @param {string} url
>   *        The URL of the worker.
>   */
> +function DevToolsWorker (url, name, verbose) {

name and verbose shouldn't be required IMO -- and second argument should be an options object for these so we can add more options in the future if wanted. Should document these args in the jsdoc ^

@@ +66,5 @@
>    let worker = this._worker;
>    let id = ++MESSAGE_COUNTER;
> +  let payload = { task, id, data };
> +
> +  if(this._verbose && dumpn) {

When would dumpn not exist? Space after `if` (elsewhere too)

@@ +104,5 @@
>    this._destroyed = true;
>  };
>  
> +DevToolsWorker.prototype.onError = function({ message, filename, lineno }) {
> +  Cu.reportError(new Error(message + " @ " + filename + ":" + lineno));

`${message} @ ${filename}:${lineno}` -- maybe also report via dumpn if verbose on

Comment 4

[James Long (:jlongster)]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #3)
> Comment on attachment 8605467 [details] [diff] [review]
> 1164632.patch
> 
> ::: toolkit/devtools/shared/worker.js
> @@ +33,5 @@
> >   *
> >   * @param {string} url
> >   *        The URL of the worker.
> >   */
> > +function DevToolsWorker (url, name, verbose) {
> 
> name and verbose shouldn't be required IMO -- and second argument should be
> an options object for these so we can add more options in the future if
> wanted. Should document these args in the jsdoc ^

verbose isn't required, but name is. I did this when I thought we would always want to turn on logging for stuff like tests, but I noticed that the data returned from GraphsWorker is massive and doesn't really help because you can't read the logs like that anyway. I can make it optional.

> 
> @@ +66,5 @@
> >    let worker = this._worker;
> >    let id = ++MESSAGE_COUNTER;
> > +  let payload = { task, id, data };
> > +
> > +  if(this._verbose && dumpn) {
> 
> When would dumpn not exist? Space after `if` (elsewhere too)

Turns out we can use Cu.import to load DevToolsUtils, so if you look at the top of the file I just don't import it and pass `null` as `dumpn`. Not a big deal, we almost always use require now anyway. That begs the question: why do you need to support non-require environments?

Comment 5

[James Long (:jlongster)]

Argh, I meant "turns out we CAN'T use Cu.import to load DevToolsUtils"

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Reason for supporting JSM was for Graphs.jsm because I couldn't find the loader for some reason, so totally OK with not supporting jsm version

Comment 7

[James Long (:jlongster)]

Attachment: 1164632.patch

comments addressed

Comment 8

[James Long (:jlongster)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fe5aa1d7e90

Comment 9

[James Long (:jlongster)]

Attachment: 1164632.patch

had to fix scratchpad's usage of pretty-print-worker as well

Comment 10

[James Long (:jlongster)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dea3b5f8a190

Comment 11

[James Long (:jlongster)]

Some failures in that push, but some other bugs got backed out which I think were the reasons. Trying again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=40844990e208

Comment 12

[James Long (:jlongster)]

Attachment: 1164632.patch

rebased, tests pass locally and above try looks green, will push to fx-team

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/8908f3baa0f7

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Backed out for various xpcshell failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=3126340&repo=fx-team

https://hg.mozilla.org/integration/fx-team/rev/70b8d85a0fcc

Comment 15

[James Long (:jlongster)]

Attachment: 1164632.patch

I'm not sure why I used `Cu.import`; we can't use `Cu` because of workers. Changed to `require`

Comment 16

[James Long (:jlongster)]

running another try just to be sure, this time with xpcshell: ttps://treeherder.mozilla.org/#/jobs?repo=try&revision=9782380461a3

Comment 17

[James Long (:jlongster)]

argh. https://treeherder.mozilla.org/#/jobs?repo=try&revision=9782380461a3

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/7235089b5191

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7235089b5191