1226285 - Extend the debugger protocol to obtain a ServiceWorkerActor from a given registration.

Status: RESOLVED DUPLICATE of bug 1563607

(DevTools :: Debugger, enhancement, P5)

Description

[Eddy Bruel [:ejpbruel]]

Once bug 1220741 lands, we will be able to get the current state of a service worker registration. This includes the installing/waiting/activeWorker for that registration, which are represented by a ServiceWorkerActor.

In this bug, we will rename WorkerActor to WebWorkerActor, and inherit both this and ServiceWorkerActor from a common base class WorkerActor. This will allow clients to attach to both types of worker actor in the same way.

The main difference between ServiceWorkerActor and WebWorkerActor is how the WorkerDebugger for the worker is obtained: for WebWorkerActor, we can use the WorkerDebugger directly. For ServiceWorkerActor, we have to use the API defined in bug 1220740, which will force a WorkerPrivate to be created it it doesn't exist, and keeps it alive while it is being debugged.

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: Refactor the worker actors to use the Class constructor.

This patch refactors the worker actors to use the SDK's class constructor. This will make it easier to inherit classes from each other.

Comment 2

[Eddy Bruel [:ejpbruel]]

Attachment: Split WorkerActor into WebWorkerActor and ServiceWorkerActor.

This patch splits WorkerActor into WebWorkerActor and ServiceWorkerActor, as previously discussed.

Comment 3

[Jan Keromnes [:janx]]

Comment on attachment 8689642 [details] [diff] [review]
Refactor the worker actors to use the Class constructor.

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

Looks good to me, but I'd love to have Alex's opinion on this refactor.

::: devtools/server/actors/worker.js
@@ +49,1 @@
>    actorPrefix: "worker",

Nit: I think we won't need this anymore.

@@ -62,4 @@
>    form: function () {
>      return {
>        actor: this.actorID,
> -      consoleActor: this._consoleActor,

Did you remove this on purpose? Can you please double-check with Brian that the Console doesn't need this anymore?

@@ +252,1 @@
>    actorPrefix: "serviceWorker",

Nit: I think we won't need this anymore.

@@ +273,1 @@
>    actorPrefix: "serviceWorkerRegistration",

Nit: I think we won't need this anymore.

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 8689642 [details] [diff] [review]
Refactor the worker actors to use the Class constructor.

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

Can't we used protocol.js instead?
It really feels like we are doing protocol.js actor the old way,
which is counter productive and misleading.

Comment 5

[Eddy Bruel [:ejpbruel]]

(In reply to Jan Keromnes [:janx] from comment #3)
> Comment on attachment 8689642 [details] [diff] [review]
> @@ -62,4 @@
> >    form: function () {
> >      return {
> >        actor: this.actorID,
> > -      consoleActor: this._consoleActor,
> 
> Did you remove this on purpose? Can you please double-check with Brian that
> the Console doesn't need this anymore?

Yes, this was on purpose. this._consoleActor is not set until after the onConnect request, so it will actually be undefined by the time the actor is formed.

Comment 6

[Eddy Bruel [:ejpbruel]]

(In reply to Alexandre Poirot [:ochameau] from comment #4)
> Comment on attachment 8689642 [details] [diff] [review]
> Refactor the worker actors to use the Class constructor.
> 
> Review of attachment 8689642 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can't we used protocol.js instead?
> It really feels like we are doing protocol.js actor the old way,
> which is counter productive and misleading.

None of the other worker actors use protocol.js at the moment, so let's file a followup bug to convert them all to protocol.js.

Comment 7

[Alexandre Poirot [:ochameau]]

(In reply to Eddy Bruel [:ejpbruel] from comment #6)
> (In reply to Alexandre Poirot [:ochameau] from comment #4)
> > Comment on attachment 8689642 [details] [diff] [review]
> > Refactor the worker actors to use the Class constructor.
> > 
> > Review of attachment 8689642 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Can't we used protocol.js instead?
> > It really feels like we are doing protocol.js actor the old way,
> > which is counter productive and misleading.
> 
> None of the other worker actors use protocol.js at the moment, so let's file
> a followup bug to convert them all to protocol.js.

I'm not sure you need to convert any other actor to convert these one to protocol.js.
Isn't it just about replacing `Class` by `Actor` and converting requestTypes into `method` on each function? It would be handy to not have to refactor the exact same things twice.

Comment 8

[Eddy Bruel [:ejpbruel]]

(In reply to Alexandre Poirot [:ochameau] from comment #7)
> (In reply to Eddy Bruel [:ejpbruel] from comment #6)
> > (In reply to Alexandre Poirot [:ochameau] from comment #4)
> > > Comment on attachment 8689642 [details] [diff] [review]
> > > Refactor the worker actors to use the Class constructor.
> > > 
> > > Review of attachment 8689642 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Can't we used protocol.js instead?
> > > It really feels like we are doing protocol.js actor the old way,
> > > which is counter productive and misleading.
> > 
> > None of the other worker actors use protocol.js at the moment, so let's file
> > a followup bug to convert them all to protocol.js.
> 
> I'm not sure you need to convert any other actor to convert these one to
> protocol.js.
> Isn't it just about replacing `Class` by `Actor` and converting requestTypes
> into `method` on each function? It would be handy to not have to refactor
> the exact same things twice.

Fair enough. If it's really that simple, I'll make sure to change the implementation to protocol.js before landing.

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: convert WorkerActor to protocol.js

I didn't wanted to send you in a minefield, so I verified it was possible.
I didn't knew how much familiar you were with protocol.js,
but here you go, you get the correct manage(this) and parent constructor calls.
We need to pass the connection to the WorkerActorList
as protocol.js needs the connection object when calling the Actor constructor.
I also verified that request/response field for protocol.js can be tweaked
to match existing RDP message layouts.

Comment 10

[Jan Keromnes [:janx]]

Comment on attachment 8689643 [details] [diff] [review]
Split WorkerActor into WebWorkerActor and ServiceWorkerActor.

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

Please switch to using protocol.js as suggested by Alex, and then rebase this patch on top.

Please also direct subsequent reviews of this to Alex.

::: devtools/server/actors/worker.js
@@ +160,5 @@
> +    ).then(({
> +      threadActor,
> +      transport,
> +      consoleActor
> +    }) => {

Nit: I think the arguments would be more readable in a single line.

Comment 11

[Jan Keromnes [:janx]]

Not part of Push DevTools for DevEdition 47 anymore.