Closed Bug 983331 Opened 10 years ago Closed 10 years ago

The debugger server protocol should describe itself.

Categories

(DevTools :: General, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 31

People

(Reporter: dcamp, Assigned: dcamp)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch protocol-describe.diff (obsolete) — 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).
Attachment #8390761 - Flags: review?(rFobic)
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)`
Attached patch v2Splinter Review
Attachment #8390761 - Attachment is obsolete: true
Attachment #8390761 - Flags: review?(rFobic)
Attachment #8407761 - Flags: review?(rFobic)
Attachment #8407761 - Flags: review?(rFobic) → review+
https://hg.mozilla.org/integration/fx-team/rev/98d6d4c38fcc
Assignee: nobody → dcamp
Whiteboard: [fixed-in-fx-team]
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
QA Whiteboard: [qa-]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.