Closed Bug 1164632 Opened 4 years ago Closed 4 years ago

Use new worker helpers for pretty-printer

Categories

(DevTools :: Debugger, defect)

defect
Not set

Tracking

(firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: jlong, Assigned: jlong)

References

Details

Attachments

(1 file, 5 obsolete files)

We just moved Jordan's worker helpers into toolkit in bug 1164483, so let's convert the debugger's pretty-printer to use it.
Blocks: 1163798
No longer blocks: 670002
No longer depends on: 1163798
Assignee: nobody → jlong
Status: NEW → ASSIGNED
Attached patch 1164632.patch (obsolete) — Splinter Review
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.
Attachment #8605466 - Flags: review?(jsantell)
Attached patch 1164632.patch (obsolete) — Splinter Review
I turned off verbose logging in the worker helper tests because the data that the timestamp plotting returns in huge
Attachment #8605466 - Attachment is obsolete: true
Attachment #8605466 - Flags: review?(jsantell)
Attachment #8605467 - Flags: review?(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
Attachment #8605467 - Flags: review?(jsantell) → review+
(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?
Argh, I meant "turns out we CAN'T use Cu.import to load DevToolsUtils"
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
Attached patch 1164632.patch (obsolete) — Splinter Review
comments addressed
Attachment #8605467 - Attachment is obsolete: true
Attached patch 1164632.patch (obsolete) — Splinter Review
had to fix scratchpad's usage of pretty-print-worker as well
Attachment #8605927 - Attachment is obsolete: true
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
Attached patch 1164632.patch (obsolete) — Splinter Review
rebased, tests pass locally and above try looks green, will push to fx-team
Attachment #8605983 - Attachment is obsolete: true
Attached patch 1164632.patchSplinter Review
I'm not sure why I used `Cu.import`; we can't use `Cu` because of workers. Changed to `require`
Attachment #8607589 - Attachment is obsolete: true
running another try just to be sure, this time with xpcshell: ttps://treeherder.mozilla.org/#/jobs?repo=try&revision=9782380461a3
https://hg.mozilla.org/mozilla-central/rev/7235089b5191
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.