Closed
Bug 983331
Opened 10 years ago
Closed 10 years ago
The debugger server protocol should describe itself.
Categories
(DevTools :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 31
People
(Reporter: dcamp, Assigned: dcamp)
References
Details
Attachments
(1 file, 1 obsolete file)
10.56 KB,
patch
|
irakli
:
review+
|
Details | Diff | Splinter Review |
Protocol.js has quite a bit of metadata about actors and their requests, it should be able to serialize that and send it over the protocol. Prototype patch adds a 'describeActors' request to the root (although it should probably be describeTypes).
Assignee | ||
Updated•10 years ago
|
Attachment #8390761 -
Flags: review?(rFobic)
Comment 1•10 years ago
|
||
Comment on attachment 8390761 [details] [diff] [review] protocol-describe.diff Review of attachment 8390761 [details] [diff] [review]: ----------------------------------------------------------------- Since I'm still fairly new to this codebase I'd prefer to get responses before finalizing this review. ::: toolkit/devtools/server/protocol.js @@ +858,5 @@ > if (spec.response) Object.freeze(spec.response); > return fn; > } > > +var allActorClasses = [] Nit: missing ; @@ +951,5 @@ > actorProto.requestTypes[spec.request.type] = handler; > }); > > actorProto._actorSpec = protoSpec; > + allActorClasses.push(actorProto) Nit: Missing ; Only problem I see here is, that actors are registered right away but never get unregistered. I don't think it will be a big problem in practice (even though add-on actors will have limited lifetime) but it maybe more reasonable if actor would be added / removed on addGlobalActor / removeGlobalActor (assuming that's how actors get registered / deregistered). @@ +1263,5 @@ > + methods: [], > + events: {} > + }; > + > + function serializeTemplate(template) { Nit: I think it would make more sense to move this function definition to an outer scope, since it's pure anyhow and that way you could avoid function instantiation at each dump. @@ +1323,5 @@ > + } > + } > + } > + > + for (let actorType of allActorClasses) { Unless I misunderstand something all the registered actors will be already in the `registeredTypes` and have a `type.category === "actor"`, which suggests `allActorClasses` is probably obsolete & instead you could just add `else if (type.category === "actor") exports.dumpActorSpec(type)`
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8390761 -
Attachment is obsolete: true
Attachment #8390761 -
Flags: review?(rFobic)
Attachment #8407761 -
Flags: review?(rFobic)
Updated•10 years ago
|
Attachment #8407761 -
Flags: review?(rFobic) → review+
Assignee | ||
Comment 3•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/98d6d4c38fcc
Assignee: nobody → dcamp
Whiteboard: [fixed-in-fx-team]
Comment 4•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/98d6d4c38fcc
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
Updated•5 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•