Closed Bug 1272724 Opened 3 years ago Closed 3 years ago

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

Categories

(DevTools :: General, defect, P1)

defect

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Iteration:
49.3 - Jun 6
Tracking Status
firefox49 --- fixed

People

(Reporter: fitzgen, Assigned: ejpbruel)

References

Details

Attachments

(1 file)

This is causing me issues while decoupling the performance tool's actors.
We are doing this forward declaration thing right now, but it isn't necessary
because there is no circular dependency here.
Attachment #8752277 - Flags: review?(ejpbruel)
Blocks: 1265727, 1263289
Whiteboard: [devtools-html]
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Iteration: --- → 49.2 - May 23
Flags: qe-verify-
Priority: -- → P1
Depends on: 1265722
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.
Attachment #8752277 - Flags: review?(ejpbruel) → review+
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.
Attachment #8752277 - Flags: feedback+
Assignee: nfitzgerald → ejpbruel
Iteration: 49.2 - May 23 → 49.3 - Jun 6
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/fe57228e70aa
Move nodeSpec into its own file;r=ejpbruel
Blocks: 1277706
No longer blocks: 1263289
Blocks: 1263289
https://hg.mozilla.org/mozilla-central/rev/fe57228e70aa
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Whiteboard: [devtools-html]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.