Closed
Bug 1164632
Opened 9 years ago
Closed 9 years ago
Use new worker helpers for pretty-printer
Categories
(DevTools :: Debugger, defect)
DevTools
Debugger
Tracking
(firefox41 fixed)
RESOLVED
FIXED
Firefox 41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: jlong, Assigned: jlong)
References
Details
Attachments
(1 file, 5 obsolete files)
21.88 KB,
patch
|
Details | Diff | Splinter Review |
We just moved Jordan's worker helpers into toolkit in bug 1164483, so let's convert the debugger's pretty-printer to use it.
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → jlong
Assignee | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•9 years ago
|
||
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)
Assignee | ||
Comment 2•9 years ago
|
||
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 3•9 years ago
|
||
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+
Assignee | ||
Comment 4•9 years ago
|
||
(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?
Assignee | ||
Comment 5•9 years ago
|
||
Argh, I meant "turns out we CAN'T use Cu.import to load DevToolsUtils"
Comment 6•9 years ago
|
||
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
Assignee | ||
Comment 7•9 years ago
|
||
comments addressed
Attachment #8605467 -
Attachment is obsolete: true
Assignee | ||
Comment 8•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fe5aa1d7e90
Assignee | ||
Comment 9•9 years ago
|
||
had to fix scratchpad's usage of pretty-print-worker as well
Attachment #8605927 -
Attachment is obsolete: true
Assignee | ||
Comment 10•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dea3b5f8a190
Assignee | ||
Comment 11•9 years ago
|
||
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
Assignee | ||
Comment 12•9 years ago
|
||
rebased, tests pass locally and above try looks green, will push to fx-team
Attachment #8605983 -
Attachment is obsolete: true
Comment 14•9 years ago
|
||
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
Assignee | ||
Comment 15•9 years ago
|
||
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
Assignee | ||
Comment 16•9 years ago
|
||
running another try just to be sure, this time with xpcshell: ttps://treeherder.mozilla.org/#/jobs?repo=try&revision=9782380461a3
Assignee | ||
Comment 17•9 years ago
|
||
argh. https://treeherder.mozilla.org/#/jobs?repo=try&revision=9782380461a3
Comment 19•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7235089b5191
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•