Closed
Bug 1256397
Opened 8 years ago
Closed 6 years ago
Partially convert ThreadActor to protocol.js
Categories
(DevTools :: Debugger, defect, P3)
DevTools
Debugger
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: ejpbruel, Assigned: ejpbruel)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
2.75 KB,
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
8.00 KB,
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
9.17 KB,
patch
|
jryans
:
feedback+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 2•8 years ago
|
||
The debugger protocol specifies two meaningful fields for error packets: the 'error' field, which specifies that this is an error packet, and indicates the name/type of the error, and the 'message' field, which contains a human readable description of the error. The only way to send an error packet in protocol.js right now is by throwing an error. Protocol.js will stringify whatever value you throw and use that as the 'message' field for the error packet. Unfortunately, for the name field, it always uses 'unknownError'. Many of the script actors have a meaningful 'error' field for their error packets, so we shouldn't just replace these with 'unknownError' (I previously did so for SourceActor and EnvironmentActor: that was a mistake, which I intend to rectify in this bug). To avoid this, I want to propose a minor extension to protocol.js, where instead of stringifying the thrown value, it looks for 'error' and 'message' fields on the thrown object (in practice, we always throw an object as error, so this should be safe), and uses those for the 'error' and 'message' fields of the error packet, respectively.
Attachment #8730331 -
Flags: review?(jryans)
Assignee | ||
Comment 3•8 years ago
|
||
ThreadActor is by far the most complicated script actor we have, so unlike the other script actors, I'm going to refactor it in steps. The first step will be to convert ThreadActor to an instance of ActorClass, without changing any of the request handlers into protocol.js methods yet. A clever use of object.merge allows us to convert these handlers one at a time (basically, object.merge overrides the fields of requestTypes as defined by ActorClass, and points them to the old request handlers. As we start converting the old request handlers to protocol.js methods, we stop overriding their requestTypes field, so that we will use the version defined by ActorClass instead).
Attachment #8730333 -
Flags: review?(jryans)
Assignee | ||
Updated•8 years ago
|
Attachment #8730333 -
Attachment description: patch → Convert ThreadActor into an instance of ActorClass.
Assignee | ||
Comment 4•8 years ago
|
||
This patch converts ThreadActor.prototype.onAttach to a protocol.js method. This is surprisingly tricky, due to our use of nested event loops here. I've added a long comment to the patch to explain the problem, and how I work around it. I hope it makes sense.
Attachment #8730334 -
Flags: review?(jryans)
Assignee | ||
Comment 5•8 years ago
|
||
This is mainly cleanup of the debugger server, so it should be P3.
Priority: -- → P3
Comment on attachment 8730331 [details] [diff] [review] Protocol.js error packets should have meaningful `error` and `message` fields. Review of attachment 8730331 [details] [diff] [review]: ----------------------------------------------------------------- I think this seems fine. It does slightly change the error messages, since error.toString() would print "e.name: e.message", but the error name doesn't really convey much information to begin with.
Attachment #8730331 -
Flags: review?(jryans) → review+
Comment on attachment 8730333 [details] [diff] [review] Convert ThreadActor into an instance of ActorClass. Review of attachment 8730333 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/server/actors/script.js @@ +2015,2 @@ > > +ThreadActor.prototype.requestTypes = object.merge(ThreadActor.prototype.requestTypes, { I think you can use Object.assign and drop the SDK import.
Attachment #8730333 -
Flags: review?(jryans) → review+
I'll have to keep thinking about the last patch.
Comment on attachment 8730334 [details] [diff] [review] Convert ThreadActor.prototype.onAttach to a protocol.js method. Review of attachment 8730334 [details] [diff] [review]: ----------------------------------------------------------------- Okay, I tried to think of other approaches, but I believe you've convinced me we need something like this. :) Please add a section to the protocol.js docs[1] about this new ability. [1]: https://dxr.mozilla.org/mozilla-central/source/devtools/server/docs/protocol.js.md ::: devtools/server/protocol.js @@ +1080,5 @@ > + // request handlers that actually need it (because they enter a nested > + // event loop). For the sake of backwards compatibility, all other > + // request handlers can either return their result directly, or as a > + // promise, which will be used to resolve the promise we queued earlier. > + if (spec.useCallback) { I think just `callback` would better align with naming of other options.
Attachment #8730334 -
Flags: review?(jryans) → feedback+
Assignee | ||
Comment 10•8 years ago
|
||
Try run for "Protocol.js error packets should have meaningful `error` and `message` fields.": https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4300f646bf9
Assignee | ||
Updated•8 years ago
|
Whiteboard: leave-open
Assignee | ||
Comment 12•8 years ago
|
||
(In reply to arni2033 from comment #11) > Your patch accidentally appeared in bug 1253976. Sorry about that! Do you need me to do anything to prevent further confusion?
Flags: needinfo?(ejpbruel)
Assignee | ||
Comment 13•8 years ago
|
||
Try push for "Convert ThreadActor into an instance of ActorClass": https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa287b13d32d
Comment 14•8 years ago
|
||
No confusion, I was only worried about development process. If there's no real breakage - then just hide my comments as spam.
Comment 16•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5fbd6d5079f4
Updated•8 years ago
|
Whiteboard: leave-open → [devtools-html] leave-open
Updated•8 years ago
|
Blocks: devtools-html-phase2
Updated•8 years ago
|
No longer blocks: devtools-html-phase2
Updated•8 years ago
|
Whiteboard: [devtools-html] leave-open → leave-open
Assignee | ||
Updated•7 years ago
|
Assignee: ejpbruel → nobody
No longer blocks: 1289193
This is quite an old bug with half-landed patches. Let's mark this one done and carry over remaining work to somewhere new.
Assignee: nobody → ejpbruel
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Summary: Change ThreadActor to protocol.js → Partially convert ThreadActor to protocol.js
Whiteboard: leave-open
Blocks: 1450284
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•