Closed Bug 1265730 Opened 4 years ago Closed 3 years ago

Decouple fronts from actors in style editor.

Categories

(DevTools :: Style Editor, enhancement, P1)

enhancement

Tracking

(firefox49 fixed, firefox50 fixed)

RESOLVED FIXED
Firefox 49
Iteration:
50.2 - Jul 4
Tracking Status
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: ejpbruel, Assigned: ejpbruel)

References

Details

Attachments

(4 files, 5 obsolete files)

No description provided.
Depends on: 1265429
Blocks: 1263289
Severity: normal → enhancement
Whiteboard: [devtools-html]
Flags: qe-verify-
Priority: -- → P2
The style actors are used by the inspector actor, so the former will have to be decoupled before the latter.
Assignee: nobody → ejpbruel
Depends on: 1265722
Depends on: 1268461
Blocks: 1265722
No longer depends on: 1265722
Priority: P2 → P1
I ran into some problems because the stylesheet actors are defined in the reverse order as their dependencies. This first patch changes the order in which the stylesheet actors are defined, so I can decouple the leaves of the dependency tree first.
Attachment #8746500 - Flags: review?(jryans)
All of this is pretty much the same was what we did for the inspector and highlighter actors.
Attachment #8746501 - Flags: review?(jryans)
Attachment #8746502 - Flags: review?(jryans)
Attachment #8746503 - Flags: review?(jryans)
Attachment #8746504 - Flags: review?(jryans)
Status: NEW → ASSIGNED
Iteration: --- → 49.1 - May 9
Comment on attachment 8746500 [details] [diff] [review]
Change the order in which the stylesheet actors are defined.

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

Why is this order change needed?  Is it because of `addActorType` rejecting redeclaration?  If so, please see my previous comment in bug 1265722 comment 13.  I think we could change `addActorType` to allow this case, making p.js less confusing to work with.
Attachment #8746500 - Flags: review?(jryans)
Attachment #8746501 - Flags: review?(jryans) → review+
Attachment #8746502 - Flags: review?(jryans) → review+
No longer depends on: 1268461
I didn't realise that the style actors and the style editor actors are separate concepts, so I this bug is not actually a dependency for the inspector actors.
Assignee: ejpbruel → nobody
No longer blocks: 1265722
Status: ASSIGNED → NEW
Iteration: 49.1 - May 9 → ---
Priority: P1 → P2
Comment on attachment 8746503 [details] [diff] [review]
Decouple StyleSheetFront from StyleSheetActor.

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

::: devtools/server/actors/stylesheets.js
@@ -868,5 @@
> -    }.bind(this));
> -  }
> -});
> -
> -exports.StyleSheetFront = StyleSheetFront;

Is it safe to remove the export already?  I would think you need to keep it until users are updated?
Attachment #8746503 - Flags: review?(jryans) → review-
Comment on attachment 8746503 [details] [diff] [review]
Decouple StyleSheetFront from StyleSheetActor.

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

::: devtools/server/actors/stylesheets.js
@@ -868,5 @@
> -    }.bind(this));
> -  }
> -});
> -
> -exports.StyleSheetFront = StyleSheetFront;

Ah, I see, it seems it wasn't used externally.
Attachment #8746503 - Flags: review- → review+
Attachment #8746504 - Flags: review?(jryans) → review+
See question in comment 7.
Flags: needinfo?(ejpbruel)
Comment on attachment 8746500 [details] [diff] [review]
Change the order in which the stylesheet actors are defined.

This bug should be for the style editor actors only. I've opened a separate bug (bug 1268461) for the stylesheet actors, and will move the patches I already attached to this bug there.
Attachment #8746500 - Attachment is obsolete: true
Attachment #8746501 - Attachment is obsolete: true
Attachment #8746502 - Attachment is patch: false
Attachment #8746502 - Attachment is obsolete: true
Attachment #8746502 - Attachment is patch: true
Attachment #8746503 - Attachment is obsolete: true
Attachment #8746504 - Attachment is obsolete: true
I agree that we could probably refactor protocol.js to make addActorType not throw if the actor type is already defined. I am reluctant to do so, however, because sometimes redefining an actor really should be an error (for instance, when someone tries to create two different instances of ActorClass with the same typename). The problem, as I see it, is that protocol.js doesn't distinguish between 'declaring' an actor type, and 'defining' it, so addActorType is used for both.

I'd really like us to think about this a bit better. At the same time, I want us to focus on the minimal effort required to finish the front/actor separation, so if the problem can be fixed by reordering a few classes, I prefer that over making architectural changes to protocol.js at the moment. I'd be happy to open a followup bug for this if you'd like.
Flags: needinfo?(ejpbruel)
(In reply to Eddy Bruel [:ejpbruel] from comment #13)
> I agree that we could probably refactor protocol.js to make addActorType not
> throw if the actor type is already defined. I am reluctant to do so,
> however, because sometimes redefining an actor really should be an error
> (for instance, when someone tries to create two different instances of
> ActorClass with the same typename). The problem, as I see it, is that
> protocol.js doesn't distinguish between 'declaring' an actor type, and
> 'defining' it, so addActorType is used for both.
> 
> I'd really like us to think about this a bit better. At the same time, I
> want us to focus on the minimal effort required to finish the front/actor
> separation, so if the problem can be fixed by reordering a few classes, I
> prefer that over making architectural changes to protocol.js at the moment.
> I'd be happy to open a followup bug for this if you'd like.

Yes, please at least file a follow up bug for it.  We should have changed this in protocol.js long ago.
Flags: needinfo?(ejpbruel)
I opened bug 1270751 for this.
Flags: needinfo?(ejpbruel)
Assignee: nobody → ejpbruel
Status: NEW → ASSIGNED
Iteration: --- → 49.2 - May 23
Priority: P2 → P1
Attachment #8751763 - Flags: review?(jryans) → review+
Attachment #8751764 - Flags: review?(jryans) → review+
Comment on attachment 8751765 [details] [diff] [review]
Decouple StyleEditorFront from StyleEditorActor.

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

Since you have removed the front exports here, should you also update the other usages of the front code?  Maybe that's in a separate patch.
Attachment #8751765 - Flags: review?(jryans) → review+
Try push for the patch to change the order in which the style editor actors are defined:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fec58e650e87
Try push for the patch to decouple OldStyleSheetsFront from OldStyleSheetsActor:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e9e601b8055
Iteration: 49.2 - May 23 → 49.3 - Jun 6
https://hg.mozilla.org/mozilla-central/rev/d721681cae58
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Try push for the patch to decouple StyleEditorFront from StyleEditorActor:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=071396f9c94a
Status: REOPENED → ASSIGNED
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/b6ba2b9fbe8a
Decouple StyleEditorFront from StyleEditorActor;r=jryans
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=9709475&repo=fx-team
Flags: needinfo?(ejpbruel)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/486cf151cbef
Backed out changeset b6ba2b9fbe8a for bc7 failures in browser_parsable_script.js
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/0979bb255f40
Decouple StyleEditorFront from StyleEditorActor;r=jryans
Keywords: leave-open
Blocks: 1277706
No longer blocks: 1263289
Blocks: 1263289
https://hg.mozilla.org/mozilla-central/rev/0979bb255f40
Status: ASSIGNED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Flags: needinfo?(ejpbruel)
As it turns out, we *did* land the StyleEditorActor/Front decoupling, but we never removed the old StyleEditorActor/Front definitions. This didn't lead to problems because the new definitions came before the old ones, so they were always overridden.

This patch removes the old StyleEditorActor/Front definitions.
Attachment #8761972 - Flags: review?(nfitzgerald)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [devtools-html] → [devtools-html] [triage]
Status: REOPENED → ASSIGNED
Iteration: 49.3 - Jun 6 → 50.1
Whiteboard: [devtools-html] [triage] → [devtools-html]
Attachment #8761972 - Flags: review?(nfitzgerald) → review+
Iteration: 50.1 → 50.2
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/7687a980146e
Remove old StyleEditorActor/Front definitions;r=fitzgen
https://hg.mozilla.org/mozilla-central/rev/7687a980146e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Whiteboard: [devtools-html]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.