Closed Bug 1226285 Opened 6 years ago Closed 2 years ago

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

Categories

(DevTools :: Debugger, enhancement, P5)

enhancement

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1563607

People

(Reporter: ejpbruel, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

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.
This patch refactors the worker actors to use the SDK's class constructor. This will make it easier to inherit classes from each other.
Attachment #8689642 - Flags: review?(janx)
This patch splits WorkerActor into WebWorkerActor and ServiceWorkerActor, as previously discussed.
Attachment #8689643 - Flags: review?(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.
Attachment #8689642 - Flags: review?(poirot.alex)
Attachment #8689642 - Flags: review?(janx)
Attachment #8689642 - Flags: feedback+
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.
Attachment #8689642 - Flags: review?(poirot.alex)
(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.
(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.
(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.
(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.
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 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.
Attachment #8689643 - Flags: review?(janx) → feedback+
No longer blocks: 1212797
Not part of Push DevTools for DevEdition 47 anymore.
No longer blocks: 1214248
Priority: -- → P3
Priority: P3 → --
Assignee: ejpbruel → nobody
Product: Firefox → DevTools
Blocks: dbg-worker
Type: defect → enhancement
Priority: -- → P5
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1563607
You need to log in before you can comment on or make changes to this bug.