1265724 - Decouple TimelineFront from TimelineActor

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement, P1)

Comment 1

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

Depends on bug 1265722 for the introduction of the fronts and specs directories and moz.builds, etc.

Comment 2

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

Network monitor only uses the timeline actor.

Comment 3

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

Attachment: Decouple TimelineFront from TimelineActor

This is still a WIP. Seeing "too much recursion" errors in sdk events emitting
things and then (I think) the event listener emitting the same event again on
the same object. I think it is related to this actor bridge crap.

Comment 4

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

Attachment: Decouple TimelineFront from TimelineActor

Alright got the tests working. Again this will need to be rebased on moving fronts to shared.

Comment 5

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8749899 [details] [diff] [review]
Decouple TimelineFront from TimelineActor

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

r+ with comments addressed.

::: devtools/server/actors/common.js
@@ +509,5 @@
>  }
>  exports.actorBridge = actorBridge;
> +
> +/**
> + * Like `actorBridge`, but without a spec definition, for when the actor is

The methods is called actorBridgeWithSpec, but the comment explains that this should be used without a spec definition. I found this somewhat confusing. Perhaps a better name for this would be either 'actorWithSpecBridge' or 'actorWithoutSpec'?

::: devtools/server/actors/timeline.js
@@ -22,2 @@
>  const { Timeline } = require("devtools/server/performance/timeline");
> -const { actorBridge } = require("devtools/server/actors/common");

Is actorBridge still used anywhere? If not, can we remove it? Or even better, simply replace actorBridge with actorBridgeWithSpec?

::: devtools/shared/specs/array-of-numbers-as-strings.js
@@ +12,5 @@
> + * XXX: It would be nice if on local connections (only), we could just *give*
> + * the array directly to the front, instead of going through all this
> + * serialization redundancy.
> + */
> +types.addType("array-of-numbers-as-strings", {

I would just put this in /devtools/shared/specs/timeline.js

Comment 6

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

(In reply to Eddy Bruel [:ejpbruel] from comment #5)
> @@ +509,5 @@
> >  }
> >  exports.actorBridge = actorBridge;
> > +
> > +/**
> > + * Like `actorBridge`, but without a spec definition, for when the actor is
> 
> The methods is called actorBridgeWithSpec, but the comment explains that
> this should be used without a spec definition. I found this somewhat
> confusing. Perhaps a better name for this would be either
> 'actorWithSpecBridge' or 'actorWithoutSpec'?

I like "actorWithSpecBridge".

> ::: devtools/server/actors/timeline.js
> @@ -22,2 @@
> >  const { Timeline } = require("devtools/server/performance/timeline");
> > -const { actorBridge } = require("devtools/server/actors/common");
> 
> Is actorBridge still used anywhere? If not, can we remove it? Or even
> better, simply replace actorBridge with actorBridgeWithSpec?

It is still used in the memory actor.

> ::: devtools/shared/specs/array-of-numbers-as-strings.js
> @@ +12,5 @@
> > + * XXX: It would be nice if on local connections (only), we could just *give*
> > + * the array directly to the front, instead of going through all this
> > + * serialization redundancy.
> > + */
> > +types.addType("array-of-numbers-as-strings", {
> 
> I would just put this in /devtools/shared/specs/timeline.js

I like splitting the types out so that they re reusable and we don't get into situations where we're requiring one actor's spec just to get type definitions.

Comment 7

[Marco Mucci [:MarcoM]]

Removing from release - blocked on dependency.

Comment 8

[Eddy Bruel [:ejpbruel]]

Try push for this patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b051302c4251

Comment 9

[Eddy Bruel [:ejpbruel]]

Try push failed due to TBPL failures. New try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b9d4ef77f5a

Comment 10

[Eddy Bruel [:ejpbruel]]

Second try push also failed due to TBPL failures. Yet another try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=43d3c85d7693

Comment 11

[Eddy Bruel [:ejpbruel]]

Try failures due to path errors. New try push with issues addressed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=21d3b6488e3f

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/6dbe97bc9112
Decouple TimelineFront from TimelineActor; r=ejpbruel

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6dbe97bc9112