Reintroduce the old Actor/FrontClass constructors for backwards compatibility.

RESOLVED FIXED in Firefox 51

Status

()

Firefox
Developer Tools
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: ejpbruel, Assigned: ejpbruel)

Tracking

unspecified
Firefox 51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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.
(Assignee)

Updated

a year ago
Assignee: nobody → ejpbruel
(Assignee)

Comment 1

a year ago
Created attachment 8781590 [details] [diff] [review]
Reintroduce the old Actor/FrontClass constructors.
Attachment #8781590 - Flags: review?(jryans)
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.
Attachment #8781590 - Flags: review?(jryans)
Attachment #8781590 - Flags: review+
Attachment #8781590 - Flags: feedback?(odvarko)
(Assignee)

Comment 3

a year ago
Try push for this patch, with comments by jryans addressed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7504513cb3fc
Priority: -- → P1
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.
(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 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
Attachment #8781590 - Flags: feedback?(odvarko) → feedback+
(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

a year ago
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/95e17b89b519
Reintroduce the old Actor/FrontClass constructors. r=jryans
(Assignee)

Comment 9

a year ago
FYI: Landed the patch with the amendments requested by jryans.

Comment 10

a year ago
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/81c452fd0a01
Fix eslint errors from previous patch. r=me

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/95e17b89b519
https://hg.mozilla.org/mozilla-central/rev/81c452fd0a01
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
You need to log in before you can comment on or make changes to this bug.