983331 - The debugger server protocol should describe itself.

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Dave Camp (:dcamp)]

Attachment: protocol-describe.diff

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).

Comment 1

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

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)`

Comment 2

[Dave Camp (:dcamp)]

Attachment: v2

Comment 3

[Dave Camp (:dcamp)]

https://hg.mozilla.org/integration/fx-team/rev/98d6d4c38fcc

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/98d6d4c38fcc