This bug would add more Arc types across the FFI boundary, so I'd like to make it depend on bug 1319614.

Taking this as I have been working on this.

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=914caccaf8d17d8d39cc44d71ad4dd66ded66881 I'm so sad that the numbers in stylo-failures.md increase a lot... Probably because those tests no longer terminates after accessing a null media rule... We'll probably need to triage the new failures... I guess most of them are media rule parsing issues in servo.

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #20) > try push: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=914caccaf8d17d8d39cc44d71ad4dd66ded66881 > > I'm so sad that the numbers in stylo-failures.md increase a lot... Probably > because those tests no longer terminates after accessing a null media rule... > > We'll probably need to triage the new failures... I guess most of them are > media rule parsing issues in servo. That's ok - the big upside is that we've reduced uncertainty and uncovered a chunk of work we didn't know about before, which is great. Parsing issues are also nice because we have so many volunteers interested in working on them. :-)

Comment on attachment 8845346 [details] Bug 1315601 part 4 - Add raw Servo types for MediaList and MediaRule. https://reviewboard.mozilla.org/r/118536/#review120744

Comment on attachment 8845343 [details] Bug 1315601 part 1 - Move type_traits include down in nsCSSValue.h. https://reviewboard.mozilla.org/r/118530/#review120808

Comment on attachment 8845344 [details] Bug 1315601 part 2 - Replace macros with template function with lambda in nsMediaList. https://reviewboard.mozilla.org/r/118532/#review120836 ::: layout/style/nsMediaList.h:309 (Diff revision 1) > + template<typename Func> > + nsresult nsMediaList::WrapMediaChange(Func aCallback); Hmm, why does "nsMediaList::" have to be mentioned here? Anyway, looks fine, though I would prefer a different name. "Wrap" sounds like something that is being done to some objects or something. How about just "DoMediaChange"? And a quick comment here above the declaration would be good.

Comment on attachment 8845345 [details] Bug 1315601 part 3 - Add base class MediaList and move part of nsMediaList to it. https://reviewboard.mozilla.org/r/118534/#review120838 ::: layout/style/MediaList.h:23 (Diff revision 1) > +// XXX This class doesn't use the branch dispatch approach that we use > +// elsewhere for stylo, but instead just relies on virtual call. > +// That's because this class should not be critical to performance, > +// and using branch dispatch would make it much more complicated. > +// Performance critical path should hold a subclass of this class > +// directly. We may want to determine in the future whether the > +// above is correct. This sounds right to me. ::: layout/style/MediaList.h:48 (Diff revision 1) > + NS_IMETHOD GetMediaText(nsAString& aMediaText) final; > + NS_IMETHOD SetMediaText(const nsAString& aMediaText) final; > + NS_IMETHOD GetLength(uint32_t* aLength) final; > + NS_IMETHOD Item(uint32_t aIndex, nsAString& aReturn) final; > + NS_IMETHOD DeleteMedium(const nsAString& aOldMedium) final; > + NS_IMETHOD AppendMedium(const nsAString& aNewMedium) final; Why expand out NS_DECL_NSIDOMMEDIALIST like this?

Comment on attachment 8845345 [details] Bug 1315601 part 3 - Add base class MediaList and move part of nsMediaList to it. https://reviewboard.mozilla.org/r/118534/#review120838 > Why expand out NS_DECL_NSIDOMMEDIALIST like this? Mainly for adding the `final`s I think... Not sure whether it's worth.

Comment on attachment 8845347 [details] Bug 1315601 part 5 - Implement MediaList for Stylo. https://reviewboard.mozilla.org/r/118538/#review120844 ::: layout/style/ServoMediaList.h:33 (Diff revision 1) > + nsresult Delete(const nsAString & aOldMedium) final; > + nsresult Append(const nsAString & aNewMedium) final; Nit: no space before "&"s. ::: servo/ports/geckolib/glue.rs:913 (Diff revision 1) > property: nsCSSPropertyID) { > remove_property(declarations, get_property_id_from_nscsspropertyid!(property, ())) > } > > +#[no_mangle] > +pub extern "C" fn Servo_MediaList_GetText(list: RawServoMediaListBorrowed, result: *mut nsAString) -> () { No need for the "-> ()" here and elsewhere. (I know some other functions do this in this file.)

Comment on attachment 8845348 [details] Bug 1315601 part 6 - Move GroupRule-related code into a separate source file. https://reviewboard.mozilla.org/r/118540/#review120856

Comment on attachment 8845349 [details] Bug 1315601 part 7 - Simplify ConditionRule. https://reviewboard.mozilla.org/r/118542/#review120858 ::: layout/style/GroupRule.h:104 (Diff revision 1) > > // Implementation of WebIDL CSSConditionRule. > class ConditionRule : public GroupRule > { > protected: > - ConditionRule(uint32_t aLineNumber, uint32_t aColumnNumber); > + using GroupRule::GroupRule; Thank you for teaching me this. :-)

Comment on attachment 8845350 [details] Bug 1315601 part 8 - Constify Rule::GetCssText. https://reviewboard.mozilla.org/r/118544/#review120860 (Not really sure why this is needed but I guess I might found out in later patches.)

Comment on attachment 8845351 [details] Bug 1315601 part 9 - Split Gecko-specific GroupRule logic into a separate struct. https://reviewboard.mozilla.org/r/118546/#review120862 ::: layout/style/GroupRule.h:115 (Diff revision 1) > > public: > void AppendStyleRule(Rule* aRule); > > - int32_t StyleRuleCount() const { return mRules.Count(); } > - Rule* GetStyleRuleAt(int32_t aIndex) const; > + DEFINE_REDIRECT_TO_INNER0(int32_t, StyleRuleCount, const) > + DEFINE_REDIRECT_TO_INNER(Rule*, GetStyleRuleAt, const, (int32_t, aIndex)) I find these DEFINE_* macros a bit hard to read. Are you sure it wouldn't be clearer to (a) include the forwarding code inline here, so we don't need to use a macro, and (b) make the inner objects use inheritance and virtual functions, to skip the Variant-based dispatch? Like the media list stuff, this is probably not performance critical to require non-virtual-function-based dispatch. WDYT?

Comment on attachment 8845352 [details] Bug 1315601 part 10 - Make ServoCSSRuleList support being nested. https://reviewboard.mozilla.org/r/118548/#review120866

Comment on attachment 8845354 [details] Bug 1315601 part 12 - Remove useless retval out param from InsertRuleIntoGroup. https://reviewboard.mozilla.org/r/118552/#review120884

Comment on attachment 8845351 [details] Bug 1315601 part 9 - Split Gecko-specific GroupRule logic into a separate struct. https://reviewboard.mozilla.org/r/118546/#review120862 > I find these DEFINE_* macros a bit hard to read. Are you sure it wouldn't be clearer to (a) include the forwarding code inline here, so we don't need to use a macro, and (b) make the inner objects use inheritance and virtual functions, to skip the Variant-based dispatch? Like the media list stuff, this is probably not performance critical to require non-virtual-function-based dispatch. WDYT? IRC discussion convinced me that we can keep the Variant-based approach. But I think it would be nice to write out the declarations a bit more, like: Rule* GetStyleRuleAt(int32_t aIndex) const { return CALL_INNER(GetStyleRuleAt, aIndex); } It's a bit longer, but I think makes it clearer (and more readable) when reading the code for the declarations.

Comment on attachment 8845351 [details] Bug 1315601 part 9 - Split Gecko-specific GroupRule logic into a separate struct. https://reviewboard.mozilla.org/r/118546/#review120888 r=me with the change to the forwarding functions.

Comment on attachment 8845353 [details] Bug 1315601 part 11 - Add Servo support to GroupRule. https://reviewboard.mozilla.org/r/118550/#review120890 ::: layout/style/GroupRule.h:84 (Diff revision 1) > + // Do we ever clone Servo rules? > + MOZ_ASSERT_UNREACHABLE("stylo: Cloning GroupRule not implemented"); We don't at the moment, but I think we'll have to to support the copy-on-write style sheet stuff.

Comment on attachment 8845355 [details] Bug 1315601 part 13 - Move common code of DeleteRuleFromGroup/InsertRuleIntoGroup from CSSStyleSheet to StyleSheet. https://reviewboard.mozilla.org/r/118554/#review120892

Comment on attachment 8845356 [details] Bug 1315601 part 14 - Add InsertRuleIntoGroup support to ServoStyleSheet. https://reviewboard.mozilla.org/r/118556/#review120894

Comment on attachment 8845357 [details] Bug 1315601 part 15 - Move some common methods to a new CSSMediaRule binding class. https://reviewboard.mozilla.org/r/118558/#review120906

Comment on attachment 8845358 [details] Bug 1315601 part 16 - Implement ServoMediaRule. https://reviewboard.mozilla.org/r/118560/#review120908 ::: commit-message-d01e4:1 (Diff revision 1) > +Bug 1315601 part 16 - Implemnet ServoMediaRule. r?heycam *Implement

Comment on attachment 8845359 [details] Bug 1315601 part 17 - Update test expectation. https://reviewboard.mozilla.org/r/118562/#review120926

Comment on attachment 8845351 [details] Bug 1315601 part 9 - Split Gecko-specific GroupRule logic into a separate struct. It should be more readable?

Comment on attachment 8845351 [details] Bug 1315601 part 9 - Split Gecko-specific GroupRule logic into a separate struct. Yes, I think that looks better, thanks!

Comment on attachment 8845347 [details] Bug 1315601 part 5 - Implement MediaList for Stylo. https://reviewboard.mozilla.org/r/118538/#review121378

Pushed by manishearth@gmail.com: https://hg.mozilla.org/integration/autoland/rev/edd8f422534c part 1 - Move type_traits include down in nsCSSValue.h. r=heycam https://hg.mozilla.org/integration/autoland/rev/bbf15d220ee4 part 2 - Replace macros with template function with lambda in nsMediaList. r=heycam https://hg.mozilla.org/integration/autoland/rev/ddb17c50f4f0 part 3 - Add base class MediaList and move part of nsMediaList to it. r=heycam https://hg.mozilla.org/integration/autoland/rev/067410e1cc20 part 4 - Add raw Servo types for MediaList and MediaRule. r=manishearth https://hg.mozilla.org/integration/autoland/rev/f89b8ee3008c part 5 - Implement MediaList for Stylo. r=heycam,manishearth https://hg.mozilla.org/integration/autoland/rev/1b0514e7d9ab part 6 - Move GroupRule-related code into a separate source file. r=heycam https://hg.mozilla.org/integration/autoland/rev/915ca91769c9 part 7 - Simplify ConditionRule. r=heycam https://hg.mozilla.org/integration/autoland/rev/dd3258136cd2 part 8 - Constify Rule::GetCssText. r=heycam https://hg.mozilla.org/integration/autoland/rev/962cf3fafa83 part 9 - Split Gecko-specific GroupRule logic into a separate struct. r=heycam https://hg.mozilla.org/integration/autoland/rev/3514cd5db5ba part 10 - Make ServoCSSRuleList support being nested. r=heycam https://hg.mozilla.org/integration/autoland/rev/d0d3a86720b2 part 11 - Add Servo support to GroupRule. r=heycam https://hg.mozilla.org/integration/autoland/rev/ca7be213dd48 part 12 - Remove useless retval out param from InsertRuleIntoGroup. r=heycam https://hg.mozilla.org/integration/autoland/rev/6de9f799a27c part 13 - Move common code of DeleteRuleFromGroup/InsertRuleIntoGroup from CSSStyleSheet to StyleSheet. r=heycam https://hg.mozilla.org/integration/autoland/rev/ff1725928cbe part 14 - Add InsertRuleIntoGroup support to ServoStyleSheet. r=heycam https://hg.mozilla.org/integration/autoland/rev/d45f19e7fa28 part 15 - Move some common methods to a new CSSMediaRule binding class. r=heycam https://hg.mozilla.org/integration/autoland/rev/cdee044cb563 part 16 - Implement ServoMediaRule. r=heycam https://hg.mozilla.org/integration/autoland/rev/cc79715e83a5 part 17 - Update test expectation. r=heycam

https://hg.mozilla.org/mozilla-central/rev/edd8f422534c https://hg.mozilla.org/mozilla-central/rev/bbf15d220ee4 https://hg.mozilla.org/mozilla-central/rev/ddb17c50f4f0 https://hg.mozilla.org/mozilla-central/rev/067410e1cc20 https://hg.mozilla.org/mozilla-central/rev/f89b8ee3008c https://hg.mozilla.org/mozilla-central/rev/1b0514e7d9ab https://hg.mozilla.org/mozilla-central/rev/915ca91769c9 https://hg.mozilla.org/mozilla-central/rev/dd3258136cd2 https://hg.mozilla.org/mozilla-central/rev/962cf3fafa83 https://hg.mozilla.org/mozilla-central/rev/3514cd5db5ba https://hg.mozilla.org/mozilla-central/rev/d0d3a86720b2 https://hg.mozilla.org/mozilla-central/rev/ca7be213dd48 https://hg.mozilla.org/mozilla-central/rev/6de9f799a27c https://hg.mozilla.org/mozilla-central/rev/ff1725928cbe https://hg.mozilla.org/mozilla-central/rev/d45f19e7fa28 https://hg.mozilla.org/mozilla-central/rev/cdee044cb563 https://hg.mozilla.org/mozilla-central/rev/cc79715e83a5