Eddy Bruel [:ejpbruel] Assignee
	Description • 8 years ago

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.

	Eddy Bruel [:ejpbruel] Assignee
	Comment 1 • 8 years ago

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

	Eddy Bruel [:ejpbruel] Assignee
	Comment 2 • 8 years ago

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.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 3 • 8 years ago

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.

	Jim Blandy :jimb
	Comment 4 • 8 years ago

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.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 5 • 8 years ago

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.

	Eddy Bruel [:ejpbruel] Assignee
	Comment 6 • 8 years ago

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

	Eddy Bruel [:ejpbruel] Assignee
	Comment 7 • 8 years ago

Now with 100% more the correct patch!

	Jim Blandy :jimb
	Comment 8 • 8 years ago

(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?

	Jim Blandy :jimb
	Comment 9 • 8 years ago

> Now with 100% more the correct patch!

LOL

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 10 • 8 years ago

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?

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 11 • 8 years ago

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

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 12 • 8 years ago

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.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 13 • 8 years ago

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.

	Eddy Bruel [:ejpbruel] Assignee
	Comment 14 • 8 years ago

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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 15 • 8 years ago

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

	Eddy Bruel [:ejpbruel] Assignee
	Comment 16 • 8 years ago

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

	Pulsebot
	Comment 17 • 8 years ago

https://hg.mozilla.org/integration/fx-team/rev/315ad011b1fd

	Carsten Book [:Tomcat]
	Comment 18 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/315ad011b1fd