Eddy Bruel [:ejpbruel] Assignee
	Description • 8 years ago

The stylesheet actors are used by the style actors, so the former have to be decoupled before the latter.

	Eddy Bruel [:ejpbruel] Assignee
	Comment 1 • 8 years ago

I didn't realise that the style actors and the style editor actors are separate concepts, so I made this bug a dependency of the wrong bug.

	Eddy Bruel [:ejpbruel] Assignee
	Comment 2 • 8 years ago

I ran into some issues because the order in which the stylesheet actors are defined does not reflect their dependency tree. This patch changes the order in which the stylesheet actors are defined so the leaves of the dependency tree are defined first, their parents next, and so on.

	Eddy Bruel [:ejpbruel] Assignee
	Comment 3 • 8 years ago

Try push for the patch to change the order in which the stylesheet actors are defined:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d26b5eab581d

	Eddy Bruel [:ejpbruel] Assignee
	Comment 5 • 8 years ago

Try push for the patch to decouple OriginalSourceFront from OriginalSourceActor:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=db023887cfbb

	Eddy Bruel [:ejpbruel] Assignee
	Comment 7 • 8 years ago

Try push for the patch to decouple MediaRuleFront from MediaRuleActor:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=06752aa49934

	Eddy Bruel [:ejpbruel] Assignee
	Comment 9 • 8 years ago

Try push for the patch to decouple StyleSheetFront from StyleSheetActor:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b10d5d831740

	Eddy Bruel [:ejpbruel] Assignee
	Comment 11 • 8 years ago

Ryan, you already reviewed most of these patches in bug 1265370. I obsoleted them there and reattached them here because we wanted a separate bug for the stylesheet actors.

I am re-requesting review mainly because I had to manually rebase all my patches due to a style change in the code (function() -> function ()), which caused my patches to no longer apply. I tried addressing the merge conflicts automatically using eslint, but unfortunately, that didn't work: even after I update my commit, it is still based on an old commit (which still has function() everywhere), so the resulting diff still doesn't apply. I tried using eslint on the patch directly, but that didn't work either. Even manually search/replacing all instances of function() in my patch didn't make it apply.

You can probably just rubberstamp these patches, but since I had to manually rebase, I wanted to give you an opportunity to re-review.

Needinfo to make sure you're reading this before you start reviewing.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 12 • 8 years ago

Comment on attachment 8747611 [details] [diff] [review]
Change the order in which the stylesheet actors are defined.

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

::: devtools/server/actors/stylesheets.js
@@ +267,5 @@
>      return this.conn.getActor(this._form.parentStyleSheet);
>    }
>  });
>  
> +types.addActorType("stylesheet");

Is declaring this type here needed?  Is seems like it should not be, since the next block fully defines the actor for that type.

@@ +978,2 @@
>   */
> +var StyleSheetsActor = exports.StyleSheetsActor = protocol.ActorClass({

This file sets `exports.StyleSheetsActor` twice, so let's remove one of them.

@@ +1186,2 @@
>  
> +XPCOMUtils.defineLazyGetter(this, "DOMUtils", function () {

Move this up to the top of the file where imports are usually found.

	Eddy Bruel [:ejpbruel] Assignee
	Comment 13 • 8 years ago

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #12)
> Comment on attachment 8747611 [details] [diff] [review]
> Change the order in which the stylesheet actors are defined.
> 
> Review of attachment 8747611 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/server/actors/stylesheets.js
> @@ +267,5 @@
> >      return this.conn.getActor(this._form.parentStyleSheet);
> >    }
> >  });
> >  
> > +types.addActorType("stylesheet");
> 
> Is declaring this type here needed?  Is seems like it should not be, since
> the next block fully defines the actor for that type.
> 

This is needed because the stylesheet actor refers to itself, and this is otherwise not possible.

> @@ +978,2 @@
> >   */
> > +var StyleSheetsActor = exports.StyleSheetsActor = protocol.ActorClass({
> 
> This file sets `exports.StyleSheetsActor` twice, so let's remove one of them.
> 
> @@ +1186,2 @@
> >  
> > +XPCOMUtils.defineLazyGetter(this, "DOMUtils", function () {
> 
> Move this up to the top of the file where imports are usually found.

Ok!

	Pulsebot
	Comment 14 • 8 years ago

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

	Pulsebot
	Comment 15 • 8 years ago

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

	Pulsebot
	Comment 16 • 8 years ago

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

	Pulsebot
	Comment 17 • 8 years ago

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

	Eddy Bruel [:ejpbruel] Assignee
	Comment 18 • 8 years ago

Try push for patch to decouple MediaRuleFront from MediaRuleActor had test failures. New push with issues addressed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa453ae70b92

	Carsten Book [:Tomcat]
	Comment 19 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/36bdf393d086
https://hg.mozilla.org/mozilla-central/rev/9528f18eb334

	Eddy Bruel [:ejpbruel] Assignee
	Comment 20 • 8 years ago

I introduced an error to the patch to decouple MediaRuleFront from MediaRuleActor, causing the try run to fail. Here is yet another push, with that issue addressed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=919edd7b2d3b

	Eddy Bruel [:ejpbruel] Assignee
	Comment 21 • 8 years ago

Running ahead of things a bit, here is another try push for the patch to decouple StyleSheetFront from StyleSheetActor:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f890054616af

	Pulsebot
	Comment 22 • 8 years ago

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

	Eddy Bruel [:ejpbruel] Assignee
	Comment 23 • 8 years ago

Try push for the patch to decouple StyleSheetFront from StyleSheetActor had test failures due to a missing require. Here is a new try push with that issue addressed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e453ac65ebf

	Eddy Bruel [:ejpbruel] Assignee
	Comment 24 • 8 years ago

Try push for the patch to decouple StyleSheetsFront from StyleSheets actor:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=69cfe3423d0a

	Ryan VanderMeulen [:RyanVM]
	Comment 25 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/2e135e4c3c91

	Pulsebot
	Comment 26 • 8 years ago

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

	Pulsebot
	Comment 27 • 8 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 28 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/9f9f74fa03fe
https://hg.mozilla.org/mozilla-central/rev/9e9cb8b89a10