For devtools.html, we have to be able to create protocol.js fronts in environments where the server is not available. This is currently not possible, because the factory function used to create a front constructor takes the constructor for the corresponding actor as argument. This factory function uses the prototype of the actor's constructor to automatically generate the protocol specification for the actor. Instead of generating the protocol specification for an actor from its constructor, we should define it as a separate entity. This will allow us to create a front constructor by passing the protocol specification to the factory function, instead of the actor constructor. Similarly, instead of generating the protocol specification from the prototype of the actor's constructor, we will pass it as an argument to the factory function for the actor's constructor.

Marking this as P1 since it's part of the devtools.html push for Q2.

Here's a proof of concept. I've defined two additional factory functions, ActorClassWithSpec and FrontClassWithSpec, respectively. These each take an actor specification as argument (as opposed to obtaining it from the actor prototype). I've kept ActorClass and FrontClass for backwards compatibility. Note that we need to call actorClass on the actor specification, because in addition to generating the actor specification from the prototype, actorProto used to do some preprocessing on it, and we want to keep doing this preprocessing manually. I've tested this approach on NodeActor in inspector.js. I've moved the actor specification into inspector-specs.js, and the front definition to inspector-fronts.js. The idea is that in a final version, the client would require inspector-fronts.js, without ever touching inspector.js. Both these files would in turn require inspector-specs.js, which contains the protocol specification for each front/actor.

Comment on attachment 8742391 [details] [diff] [review] Proof of concept. Review of attachment 8742391 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me! Like how this allows us to move actors over incrementally and doesn't require we do them all at once. This is pretty much exactly as I imagined when I proposed the idea on the mailing list.

Comment on attachment 8742391 [details] [diff] [review] Proof of concept. Review of attachment 8742391 [details] [diff] [review]: ----------------------------------------------------------------- I don't really have any comments on this; if Nick says this looks like what he expected, then that's good enough for me.

Comment on attachment 8742391 [details] [diff] [review] Proof of concept. I thought I'd take a look since I've worked on protocol.js in the past. Looks good to me overall! As far as file organization, do we want to keep the fronts in /devtools/server/actors? Would it make more sense for them to be in /devtools/client/fronts or something? Ideally it would eventually become possible to use the client without any code from the /devtools/server directory. Moving the files could of course be a separate task since you're already making new files here anyway. I don't mean to add any additional requirements, just curious.

Hi Ryan. Given your recent experience with protocol.js, you seemed like the best person to flag for review for this patch.

Now with 100% more the correct patch!

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5) > As far as file organization, do we want to keep the fronts in > /devtools/server/actors? Would it make more sense for them to be in > /devtools/client/fronts or something? Ideally it would eventually become > possible to use the client without any code from the /devtools/server > directory. Why not devtools/shared/protocol?

> Now with 100% more the correct patch! LOL

Comment on attachment 8743377 [details] [diff] [review] Refactor protocol.js to allow fronts to be decoupled from actors. Review of attachment 8743377 [details] [diff] [review]: ----------------------------------------------------------------- Overall, I think it looks quite nice! We're able to support both the old and new paths without much change in this approach. ::: devtools/server/protocol.js @@ +942,5 @@ > return fn; > } > > /** > + * Generate an actor specification from a prototype. Maybe "Register an actor type from a specification."? Some better terminology would help here I think... This function takes in a list of methods / events, process them to attach things like `Request`, registers this processed data as the `actorSpec`, and return that too. If we want to keep calling output a "spec" for consistency with how p.js used to work, then what is a good name for the input, since it's not a "proto" anymore? @@ +947,2 @@ > */ > +var actorSpec = function(actorProto) { Since p.js is stateful (by knowing what types are registered) and this method registers the actor type, I think it should have a name with a verb, like `addActorFromSpec` / `addActorSpec` or something. (I considered suggesting that you just make this a part of the existing `addActorType` if it is passed a spec instead of a type name, but there's already enough going in that method... Still worth thinking about perhaps.) With your new approach, `actorProto` is really just a plain object that describes the actor. (Sometimes it is prototype for unconverted actors, but that sort of becomes a detail.) See previous comments that a new name for either the input or output seems needed. @@ +952,3 @@ > }; > > // Find method and form specifications attached to prototype properties. Similarly, we should probably stop talking about "prototype" here and elsewhere. @@ +1022,5 @@ > +/** > + * Process an actor definition from its specification and prototype, and > + * generate request handlers. > + */ > +var actorProto = function(actorSpec, actorProto) { It's twisting my mind a little that this function and one of its arguments has the same name. @@ +1107,5 @@ > +/** > + * Create an actor class for the given actor specification and prototype. > + * > + * @param object actorSpec > + * The actor specification. Must have a 'typename property. Nit: 'typeName' @@ +1112,5 @@ > + * @param object proto > + * The object prototype. Should have method definitions, can have event > + * definitions. > + */ > +var ActorClassWithSpec = function(actorSpec, proto) { Maybe `ActorClassFromSpec` since it's created "from" the data in the spec? @@ +1319,3 @@ > * request methods. > */ > +var frontProto = function(actorSpec, frontProto) { Same as with `actorProto`, can we rename one of them? @@ +1473,5 @@ > + * @param object proto > + * The object prototype. Must have a 'typeName' property, > + * should have method definitions, can have event definitions. > + */ > +var FrontClassWithSpec = function(actorSpec, proto) { Maybe `FrontClassFromSpec` since it's created "from" the data in the spec?

(In reply to Jim Blandy :jimb from comment #8) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5) > > As far as file organization, do we want to keep the fronts in > > /devtools/server/actors? Would it make more sense for them to be in > > /devtools/client/fronts or something? Ideally it would eventually become > > possible to use the client without any code from the /devtools/server > > directory. > > Why not devtools/shared/protocol? More discussion on directory plans in bug 1265722, the first bug to actually split up an actor with this approach.

Is there a reason this was feedback and not review? What is still required here? It blocks all the others decoupling, which is most of devtools-html track 4. I don't think we should this bug (and therefore all the other decoupling) while working on the inspector decoupling in particular. Taking as per the devtools-html track 4 standup today.

Sounds like :ejpbruel is still working on this. We discussed the review comments on Vidyo and we've agreed on how to best solve them. I'll jump on the review for the next version when it appears.

New patch with comments by jryans addressed (as discussed over Vidyo).

Comment on attachment 8746114 [details] [diff] [review] Refactor protocol.js to allow fronts to be decoupled from actors. Review of attachment 8746114 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks for making those tweaks! ::: devtools/server/protocol.js @@ +1107,5 @@ > +/** > + * Create an actor class for the given actor specification and prototype. > + * > + * @param object actorSpec > + * The actor specification. Must have a 'typename' property. Nit: typeName

Try push for this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b93137cdbd5