1250896 - Change SourceActor to protocol.js

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: Move SourceActor into its own file

Comment 2

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

Comment on attachment 8723010 [details] [diff] [review]
Move SourceActor into its own file

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

::: devtools/server/actors/breakpoint.js
@@ +16,5 @@
> + *        The actor handling the breakpoint hits.
> + * @param Array entryPoints
> + *        An array of objects of the form `{ script, offsets }`.
> + */
> +function setBreakpointAtEntryPoints(actor, entryPoints) {

If you have to pass in a BreakpointActor, couldn't this be (regular, non-RDP) method on BreakpointActor object?  That would seem more natural to me.

Comment 3

[Eddy Bruel [:ejpbruel]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #2)
> Comment on attachment 8723010 [details] [diff] [review]
> Move SourceActor into its own file
> 
> Review of attachment 8723010 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/server/actors/breakpoint.js
> @@ +16,5 @@
> > + *        The actor handling the breakpoint hits.
> > + * @param Array entryPoints
> > + *        An array of objects of the form `{ script, offsets }`.
> > + */
> > +function setBreakpointAtEntryPoints(actor, entryPoints) {
> 
> If you have to pass in a BreakpointActor, couldn't this be (regular,
> non-RDP) method on BreakpointActor object?  That would seem more natural to
> me.

Yeah, that sounds like it makes sense. I'm planning to split off those utility functions from the files defining the actor, so that would be a good moment to decide whether they should go on the actor or not.

Comment 4

[Eddy Bruel [:ejpbruel]]

Try push for moving SourceActor into its own file:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=85ff2272c9dc

Comment 5

[Eddy Bruel [:ejpbruel]]

Attachment: Refactor SourceActor to use protocol.js

Comment 6

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

Comment on attachment 8725226 [details] [diff] [review]
Refactor SourceActor to use protocol.js

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

::: devtools/server/actors/source.js
@@ +428,5 @@
>          };
>        })
>        .then(null, aError => {
>          reportError(aError, "Got an exception during SA_onSource: ");
> +        throw new Error("Could not load the source for " + this.url + ".\n" +

Is it important for the error packet to have the `error` property set to the URL?

Comment 7

[Eddy Bruel [:ejpbruel]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #6)
> Comment on attachment 8725226 [details] [diff] [review]
> Refactor SourceActor to use protocol.js
> 
> Review of attachment 8725226 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/server/actors/source.js
> @@ +428,5 @@
> >          };
> >        })
> >        .then(null, aError => {
> >          reportError(aError, "Got an exception during SA_onSource: ");
> > +        throw new Error("Could not load the source for " + this.url + ".\n" +
> 
> Is it important for the error packet to have the `error` property set to the
> URL?

Protocol.js doesn't allow custom fields on error packets (only a message field). We could change protocol.js to allow for that, but apparently no consumers are relying on that additional field being there, so we might as well leave it out.

Comment 8

[Pulsebot]

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

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c2a4ac657b2c

Comment 10

[Eddy Bruel [:ejpbruel]]

Try push for refactoring SourceActor to use protocol.js:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b3dde1ef176

Comment 11

[Eddy Bruel [:ejpbruel]]

Try push shows a single failing test, but otherwise looks fine. It should be ok to push the patch with that failure addressed.

Comment 12

[Pulsebot]

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

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ee8f304d9d62

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

All the old style actor => protocol.js work doesn't really block devtools-html because the clients are already separate from the actors. It seems to me we shouldn't be tagging these [devtools-html] and all that.

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

See comment 14.

Comment 16

[Eddy Bruel [:ejpbruel]]

I marked them as devtools-html because, as I understand it, converting the last old style actors to protocol.js and decoupling their fronts/actors is a 'should have' goal for devtools-html. That said, I agree that this is not a blocker, since we can always decide to keep the old actors for now, in which case the fronts are already separate (since those are hand written for the old actors).

Comment 17

[Marco Mucci [:MarcoM]]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #14)
> All the old style actor => protocol.js work doesn't really block
> devtools-html because the clients are already separate from the actors. It
> seems to me we shouldn't be tagging these [devtools-html] and all that.

Hi Nick.  I tagged Bug 1250896 as it was blocking Bug 1037992 (meta) which blocks the Track 4 meta (Bug 1263289).  During the breakdown reviews this week I'll be asking the teams if there are tagged bugs which shouldn't be.  If have specific ones you know of already please email me and I'll remove them.  Thanks.

Comment 18

[Yulia Startsev [:yulia]]

This looks like it might have been done, since sourceActor is now in its own file, and has a dedicated actorSpec with methods as per the protocol.js readme https://searchfox.org/mozilla-central/source/devtools/docs/backend/protocol.js.md

https://searchfox.org/mozilla-central/source/devtools/server/actors/source.js#14
https://searchfox.org/mozilla-central/source/devtools/shared/specs/source.js#8

protocol.js has also been updated according to the pr here which updates the error messages: https://bugzilla.mozilla.org/show_bug.cgi?id=1256397

which can be seen here: https://searchfox.org/mozilla-central/source/devtools/shared/protocol.js#963-967,970-971

with the corresponding source changes here: https://searchfox.org/mozilla-central/source/devtools/server/actors/source.js#698-699

Are there any other outstanding issues for this?

Comment 19

[Alexandre Poirot [:ochameau]]

jryans, do you know what Eddy was expecting to do in addition to what already landed?
As Yulia just said, the actor is now protocol.js one and a real one (i.e. not like ThreadActor [1]!!).

Should we specify a few return type more precisely may be?
https://searchfox.org/mozilla-central/source/devtools/shared/specs/source.js#15,19,22,26,40


[1] ThreadActor is using protocol.js class without using protocol.js!!
https://searchfox.org/mozilla-central/source/devtools/server/actors/thread.js#59
  const ThreadActor = ActorClassWithSpec(threadSpec, {
https://searchfox.org/mozilla-central/source/devtools/server/actors/thread.js#1730
  Object.assign(ThreadActor.prototype.requestTypes, {
https://searchfox.org/mozilla-central/source/devtools/shared/specs/script.js#11
  methods: {},

Comment 20

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

Hmm!  Like Yulia points out, it seems to me that everything is addressed here.  The actor itself seems fully converted to protocol.js and there is a spec file defining types.

(In reply to Alexandre Poirot [:ochameau] from comment #19)
> Should we specify a few return type more precisely may be?
> https://searchfox.org/mozilla-central/source/devtools/shared/specs/source.
> js#15,19,22,26,40

We _could_ make dict types here if we wanted, I guess?  Maybe Eddy was worried about compatibility for these older actors and just want to return clear objects.

I think it's probably okay as is for now, until we have clear need for a more complex type, such as lifetime management, sharing types with other actors, etc.  I'll close this bug, but we can file something new if we learn more here.

As for ThreadActor, I agree it seems strangely half-done!  Looks like this happened in bug 1277947, which I don't think I was aware of when it happened (authored by :fitzgen, reviewed by :ejpbruel).

If I remember correctly, ThreadActor was more complex than other conversions because Eddy thought it needed special handling due to the nested event loops used by the debugger.  (I am not sure if the same solution still makes sense or if something else would be better today.)

I filed bug 1450284 to capture what appears to remain for ThreadActor.

As always, please ask if it's unclear what work is needed!