1295171 - Reintroduce the old Actor/FrontClass constructors for backwards compatibility.

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[Eddy Bruel [:ejpbruel]]

As pointed out by jryans in bug 1288423, we may have been a bit overzealous in our attempt to remove all references to the old ActorClass/FrontClass constructors. The devtools code may not depend on those anymore, but some add-on code still does!

To avoid breaking add-ons that depend on this, we should rename Actor/FrontClass back to ActorClass/FrontClassWithSpec, and reintroduce the old ActorClass/Frontclass constructors.

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: Reintroduce the old Actor/FrontClass constructors.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8781590 [details] [diff] [review]
Reintroduce the old Actor/FrontClass constructors.

Review of attachment 8781590 [details] [diff] [review]:
-----------------------------------------------------------------

Great, this looks good to me, just a few comments to tweak.

Honza, can you verify that this fixes your add-ons?

::: devtools/server/actors/common.js
@@ +510,5 @@
>  exports.actorBridge = actorBridge;
>  
>  /**
>   * Like `actorBridge`, but without a spec definition, for when the actor is
> + * created with `ActorClassWithSpec` rather than vanilla `ActorClassWithSpec`.

One of these should just say `ActorClass` I guess?

::: devtools/shared/protocol.js
@@ +1089,5 @@
>    return actorProto;
>  };
>  
>  /**
> + * Create an actor class for the given actor prototype.

We should probably add a comment that this a deprecated path preserved only for add-ons.  It's not meant to be used in tree.

@@ +1421,5 @@
>    return frontProto;
>  };
>  
>  /**
> + * Create a front class for the given actor class and front prototype.

Same comment here.

Comment 3

[Eddy Bruel [:ejpbruel]]

Try push for this patch, with comments by jryans addressed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7504513cb3fc

Comment 4

[Jarda Snajdr [:jsnajdr]]

After applying this patch, RDP monitor seems to work fine. But I'm getting errors from the emulation actor that landed recently in bug 1285566. It still uses the FrontClass/ActorClass constructors.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Jarda Snajdr [:jsnajdr] from comment #4)
> After applying this patch, RDP monitor seems to work fine. But I'm getting
> errors from the emulation actor that landed recently in bug 1285566. It
> still uses the FrontClass/ActorClass constructors.

Right, the emulation actor was not in the tree when :ejpbruel created his patch.  We should amend this patch to make the EmulationActor use the *WithSpec names.

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8781590 [details] [diff] [review]
Reintroduce the old Actor/FrontClass constructors.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #2)
> Comment on attachment 8781590 [details] [diff] [review]
> Reintroduce the old Actor/FrontClass constructors.
> 
> Review of attachment 8781590 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great, this looks good to me, just a few comments to tweak.
> 
> Honza, can you verify that this fixes your add-ons?

Nice, the patch fixes the issue with addons.

One question. Addons that dynamically register actors using `ActorRegistryActor` specify path of a module (with custom actor implementation) that should be loaded and source sent to the back-end. The source is consequently evaluated on the backend and new actor registered. 

In order to use new API (i.e. separated `actor-spec`) the actor module needs to require the `spec-module` (shared between an actor object and front object). But that module also needs to be sent to the backend and there is now way how to do it now - except of inserting the actor-spec directly into the same file where the actor is implemented (which sort of breaks the point of the refactoring).

What is the recommendation here?


Honza

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Jan Honza Odvarko [:Honza] from comment #6)
> One question. Addons that dynamically register actors using
> `ActorRegistryActor` specify path of a module (with custom actor
> implementation) that should be loaded and source sent to the back-end. The
> source is consequently evaluated on the backend and new actor registered. 
> 
> In order to use new API (i.e. separated `actor-spec`) the actor module needs
> to require the `spec-module` (shared between an actor object and front
> object). But that module also needs to be sent to the backend and there is
> now way how to do it now - except of inserting the actor-spec directly into
> the same file where the actor is implemented (which sort of breaks the point
> of the refactoring).
> 
> What is the recommendation here?

Yes, this occurred to me as well...  I guess I would say for now it's probably best for add-ons to keep using the older actor style where the spec is inline because of this.  The main reason we broke them up at all in Firefox was because we want a clear client / server separation.  That seems less meaningful for add-ons.

If there's a reason add-ons want to install multi-module actors, let's file a bug for adding such support.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/95e17b89b519
Reintroduce the old Actor/FrontClass constructors. r=jryans

Comment 9

[Eddy Bruel [:ejpbruel]]

FYI: Landed the patch with the amendments requested by jryans.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/81c452fd0a01
Fix eslint errors from previous patch. r=me

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/95e17b89b519
https://hg.mozilla.org/mozilla-central/rev/81c452fd0a01