1272724 - Split the `domnode` actor spec out to its own module

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

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

This is causing me issues while decoupling the performance tool's actors.

Comment 1

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

Attachment: Split the `domnode` actor spec out to its own module

We are doing this forward declaration thing right now, but it isn't necessary
because there is no circular dependency here.

Comment 2

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8752277 [details] [diff] [review]
Split the `domnode` actor spec out to its own module

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

I agree with the intent of this patch. I just have a few style comments/nits, which you are free to ignore.

Make sure to run this by jryans before you land it. I had a conversation about this change with him earlier, where I interpreted what he said as preferring a solution where we change protocol.js to distinguish between predeclaring/defining a type.

::: devtools/shared/specs/domnode.js
@@ +16,5 @@
> +  // The original image dimensions
> +  size: "json"
> +});
> +
> +exports.module = generateActorSpec({

I understand why you're doing it like this, but I would just do exports.nodeSpec, since that's what we do everywhere else. I personally don't like default exports because they force me to think about how each module should be used.

::: devtools/shared/specs/inspector.js
@@ -64,5 @@
> -    }
> -  }
> -});
> -
> -exports.nodeSpec = nodeSpec;

Everything that requires the inspector specs is also going to have to pull in the node spec. I would re-export nodeSpec here to make that relationship explicit, as opposed to having to require node spec separately everywhere we require the inspector specs.

Comment 3

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

Comment on attachment 8752277 [details] [diff] [review]
Split the `domnode` actor spec out to its own module

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

This seems fine to me.  If we don't need to predeclare, then it makes sense to remove it like this.

Comment 4

[Eddy Bruel [:ejpbruel]]

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/fe57228e70aa
Move nodeSpec into its own file;r=ejpbruel

Comment 6

[Carsten Book [:Tomcat]]

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

Comment 7

[Carsten Book [:Tomcat]]

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