After bug 1449068 and bug 1449087, all rule objects are really only for CSSOM (and not used by the style system otherwise). It makes sense to merge them into their corresponding CSS*Rule super class (or rename to, for @font-face and @counter-style, which get ported after the old style system removal). Merging them may remove some unnecessary virtual calls, and that can also simplify code as well as Bindings.conf.

I think there is no reason that prevents doing this now. I will work on it.

Comment on attachment 8983859 [details] Bug 1451289 - Part 1: Merge ServoNamespaceRule and css::CSSNamespaceRule https://reviewboard.mozilla.org/r/249698/#review255904 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: layout/style/CSSNamespaceRule.h:52 (Diff revision 1) > + > void GetPrefix(DOMString& aPrefix) { > aPrefix.SetKnownLiveAtom(GetPrefix(), DOMString::eTreatNullAsEmpty); > } > > - size_t SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const override = 0; > + size_t SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const override final; Warning: Virtual function declarations should specify only one of `virtual`, `final`, or `override` [cpp-virtual-final]

Comment on attachment 8983860 [details] Bug 1451289 - Part 2: Merge ServoKeyframesRule and CSSKeyframesRule https://reviewboard.mozilla.org/r/249700/#review256108

Comment on attachment 8983859 [details] Bug 1451289 - Part 1: Merge ServoNamespaceRule and css::CSSNamespaceRule https://reviewboard.mozilla.org/r/249698/#review256110 ::: layout/style/CSSNamespaceRule.h:25 (Diff revision 1) > - > public: > + CSSNamespaceRule(already_AddRefed<RawServoNamespaceRule> aRule, > + uint32_t aLine, uint32_t aColumn) > + : css::Rule(aLine, aColumn) > + , mRawRule(std::move(aRule)) {} nit: I think we usually put the block on the same line, but no big deal. ::: layout/style/CSSNamespaceRule.h:52 (Diff revision 1) > + > void GetPrefix(DOMString& aPrefix) { > aPrefix.SetKnownLiveAtom(GetPrefix(), DOMString::eTreatNullAsEmpty); > } > > - size_t SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const override = 0; > + size_t SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const override final; just final or override are fine :)

Comment on attachment 8983860 [details] Bug 1451289 - Part 2: Merge ServoKeyframesRule and CSSKeyframesRule https://reviewboard.mozilla.org/r/249700/#review256112 You can't just move the members and remove the cycle collection bits. Please fix that up so that cycle collection traverses and unlinks the same things. ::: layout/style/CSSKeyframeRule.cpp:109 (Diff revision 1) > + , mRaw(aRaw) {} > + > +CSSKeyframeRule::~CSSKeyframeRule() > +{ > + if (mDeclaration) { > + mDeclaration->DropReference(); Actually... This used (and needs to) be in the cycle collection macros. Otherwise this can leak. Please put back in the UNLINK implementation.

Comment on attachment 8983861 [details] Bug 1451289 - Part 3: Merge ServoMediaRule and CSSMediaRule https://reviewboard.mozilla.org/r/249702/#review256114 Similarly, this needs to unlink and traverse the medialist and such. Please preserve those bits.

Comment on attachment 8983862 [details] Bug 1451289 - Part 4: Rename ServoStyleRule to CSSStyleRule https://reviewboard.mozilla.org/r/249704/#review256116

Comment on attachment 8983863 [details] Bug 1451289 - Part 5: Rename ServoFontFaceRule to CSSFontFaceRule https://reviewboard.mozilla.org/r/249706/#review256118 ::: dom/bindings/Bindings.conf:181 (Diff revision 1) > 'CSSCounterStyleRule': { > 'nativeType': 'mozilla::ServoCounterStyleRule', > 'headerFile': 'mozilla/ServoCounterStyleRule.h', > }, > > 'CSSFontFaceRule': { You may not need this block anymore.

Comment on attachment 8983864 [details] Bug 1451289 - Part 6: Rename ServoCounterStyleRule to CSSCounterStyleRule https://reviewboard.mozilla.org/r/249708/#review256124 ::: dom/bindings/Bindings.conf:178 (Diff revision 1) > 'headerFile': 'mozilla/css/GroupRule.h', > }, > > 'CSSCounterStyleRule': { > - 'nativeType': 'mozilla::ServoCounterStyleRule', > - 'headerFile': 'mozilla/ServoCounterStyleRule.h', > + 'nativeType': 'mozilla::dom::CSSCounterStyleRule', > + 'headerFile': 'mozilla/dom/CSSCounterStyleRule.h', Same here.

Comment on attachment 8983865 [details] Bug 1451289 - Part 7: Merge ServoPageRule and CSSPageRule https://reviewboard.mozilla.org/r/249710/#review256126 Need to restore the cycle collection bits that traversed / unlinked mDecls. ::: layout/style/CSSPageRule.cpp:13 (Diff revision 1) > #include "mozilla/dom/CSSPageRuleBinding.h" > > +#include "mozilla/DeclarationBlock.h" > +#include "mozilla/ServoBindings.h" > + > +using namespace mozilla::dom; This is not needed, is it?

Comment on attachment 8983866 [details] Bug 1451289 - Part 8: Merge ServoSupportsRule and CSSSupportsRule https://reviewboard.mozilla.org/r/249712/#review256128 I think this is fine because the cycle collector bits were empty and only called into the base class, but please double-check with xidorn / cam if you're not sure :). ::: layout/style/CSSSupportsRule.cpp:63 (Diff revision 1) > +{ > + Servo_SupportsRule_GetCssText(mRawRule, &aCssText); > +} > + > +/* virtual */ size_t > +CSSSupportsRule::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) nit: You can remove `mozilla::` here and put the const in the previous line.

Comment on attachment 8983867 [details] Bug 1451289 - Part 9: Merge ServoDocumentRule and CSSMozDocumentRule https://reviewboard.mozilla.org/r/249714/#review256130 Same question / gotcha as the previous patch. ::: layout/style/CSSMozDocumentRule.cpp:115 (Diff revision 1) > +{ > + Servo_MozDocumentRule_GetCssText(mRawRule, &aCssText); > +} > + > +/* virtual */ size_t > +CSSMozDocumentRule::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) ditto regarding removing `mozilla::`.

Comment on attachment 8983868 [details] Bug 1451289 - Part 10: Merge ServoImportRule and CSSImportRule https://reviewboard.mozilla.org/r/249716/#review256132 This does need the cycle collection bits. ::: dom/animation/AnimationEffect.cpp:39 (Diff revision 1) > NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY > NS_INTERFACE_MAP_ENTRY(nsISupports) > NS_INTERFACE_MAP_END > > + > +AnimationEffect::AnimationEffect(nsIDocument* aDocument, const TimingParams& aTiming) Why did this need to move? Seems like an unrelated change. ::: layout/style/CSSImportRule.cpp:57 (Diff revision 1) > + // When Bug 1326509 is fixed, we can assert mChildSheet instead. > + return mChildSheet ? mChildSheet->Media() : nullptr; > +} > + > +StyleSheet* > +CSSImportRule::GetStyleSheet() const This can be inline.

Comment on attachment 8983869 [details] Bug 1451289 - Part 11: Merge ServoFontFeatureValuesRule and CSSFontFeatureValuesRule https://reviewboard.mozilla.org/r/249718/#review256134 ::: layout/style/CSSFontFeatureValuesRule.cpp:21 (Diff revision 1) > + : Rule(aLine, aColumn) > + , mRawRule(std::move(aRawRule)) > +{ > +} > + > +CSSFontFeatureValuesRule::~CSSFontFeatureValuesRule() nit: (here and in the other commits), you may want to use `= default;` here.

Comment on attachment 8983870 [details] Bug 1451289 - Part 12: Remove the intermediate macro and make CASE_RULE macro work with CSS*Rule instead https://reviewboard.mozilla.org/r/249720/#review256136 ::: layout/style/CSSFontFeatureValuesRule.cpp:92 (Diff revision 1) > return Rule::IsCCLeaf(); > } > > /* virtual */ JSObject* > CSSFontFeatureValuesRule::WrapObject(JSContext* aCx, > - JS::Handle<JSObject*> aGivenProto) > + JS::Handle<JSObject*> aGivenProto) This change should be in another commit.

Oh sorry about the cycle collection bits. Completely ignored them somehow. Fixed them and other review comments.

Comment on attachment 8983868 [details] Bug 1451289 - Part 10: Merge ServoImportRule and CSSImportRule https://reviewboard.mozilla.org/r/249716/#review256132 > Why did this need to move? Seems like an unrelated change. After removing some files, unified source file started to give error probably about circular dependencies. Had to remove it here to fix it.

Comment on attachment 8983860 [details] Bug 1451289 - Part 2: Merge ServoKeyframesRule and CSSKeyframesRule https://reviewboard.mozilla.org/r/249700/#review256246 Thanks! Much better :)

Comment on attachment 8983861 [details] Bug 1451289 - Part 3: Merge ServoMediaRule and CSSMediaRule https://reviewboard.mozilla.org/r/249702/#review256264

Comment on attachment 8983868 [details] Bug 1451289 - Part 10: Merge ServoImportRule and CSSImportRule https://reviewboard.mozilla.org/r/249716/#review256266 ::: dom/animation/AnimationEffect.cpp:42 (Diff revision 2) > > + > +AnimationEffect::AnimationEffect(nsIDocument* aDocument, const TimingParams& aTiming) > + : mDocument(aDocument) > + , mTiming(aTiming) > +{ This looks not addressed, why was this moved? ::: layout/style/nsDOMCSSRect.h:15 (Diff revision 2) > #define nsDOMCSSRect_h_ > > #include "mozilla/Attributes.h" > #include "mozilla/RefPtr.h" > > +using namespace mozilla; please don't add this to a header, just add the necessary `mozilla::` qualification in the places they were missing.

Comment on attachment 8983865 [details] Bug 1451289 - Part 7: Merge ServoPageRule and CSSPageRule https://reviewboard.mozilla.org/r/249710/#review256272

Comment on attachment 8983868 [details] Bug 1451289 - Part 10: Merge ServoImportRule and CSSImportRule https://reviewboard.mozilla.org/r/249716/#review257112 ::: dom/animation/AnimationEffect.cpp:38 (Diff revision 3) > NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(AnimationEffect) > NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY > NS_INTERFACE_MAP_ENTRY(nsISupports) > NS_INTERFACE_MAP_END > > +AnimationEffect::AnimationEffect(nsIDocument* aDocument, const TimingParams& aTiming) Please file a bug for the header mess and such, so at least it's tracked :) ::: layout/style/CSSImportRule.h:14 (Diff revision 3) > > #include "mozilla/css/Rule.h" > > namespace mozilla { > + > +class ServoMediaList; nit: ServoMediaList doesn't exist anymore, so this can be removed. ::: layout/style/CSSImportRule.h:42 (Diff revision 3) > // WebIDL interface > uint16_t Type() const final { return CSSRuleBinding::IMPORT_RULE; } > - virtual void GetHref(nsAString& aHref) const = 0; > - virtual dom::MediaList* GetMedia() const = 0; > - virtual StyleSheet* GetStyleSheet() const = 0; > + void GetCssText(nsAString& aCssText) const override; > + void GetHref(nsAString& aHref) const; > + dom::MediaList* GetMedia() const; > + inline StyleSheet* GetStyleSheet() const { return mChildSheet; } nit: no need for `inline` (it's implicit in C++ if you define the method in there).

Comment on attachment 8983868 [details] Bug 1451289 - Part 10: Merge ServoImportRule and CSSImportRule https://reviewboard.mozilla.org/r/249716/#review257112 > Please file a bug for the header mess and such, so at least it's tracked :) There is no need to file a bug now. Fixed it.

Pushed by canaltinova@gmail.com: https://hg.mozilla.org/integration/autoland/rev/6299a6e593f8 Part 1: Merge ServoNamespaceRule and css::CSSNamespaceRule r=emilio https://hg.mozilla.org/integration/autoland/rev/369dae118647 Part 2: Merge ServoKeyframesRule and CSSKeyframesRule r=emilio https://hg.mozilla.org/integration/autoland/rev/676772080481 Part 3: Merge ServoMediaRule and CSSMediaRule r=emilio https://hg.mozilla.org/integration/autoland/rev/b9cf191f3443 Part 4: Rename ServoStyleRule to CSSStyleRule r=emilio https://hg.mozilla.org/integration/autoland/rev/77f31a0e6340 Part 5: Rename ServoFontFaceRule to CSSFontFaceRule r=emilio https://hg.mozilla.org/integration/autoland/rev/828e691c05dd Part 6: Rename ServoCounterStyleRule to CSSCounterStyleRule r=emilio https://hg.mozilla.org/integration/autoland/rev/711e54740d20 Part 7: Merge ServoPageRule and CSSPageRule r=emilio https://hg.mozilla.org/integration/autoland/rev/6c6acab22f7f Part 8: Merge ServoSupportsRule and CSSSupportsRule r=emilio https://hg.mozilla.org/integration/autoland/rev/d570b529fedd Part 9: Merge ServoDocumentRule and CSSMozDocumentRule r=emilio https://hg.mozilla.org/integration/autoland/rev/abddd5433879 Part 10: Merge ServoImportRule and CSSImportRule r=emilio https://hg.mozilla.org/integration/autoland/rev/bd47a1b3d97b Part 11: Merge ServoFontFeatureValuesRule and CSSFontFeatureValuesRule r=emilio https://hg.mozilla.org/integration/autoland/rev/a17ceb1adbd6 Part 12: Remove the intermediate macro and make CASE_RULE macro work with CSS*Rule instead r=emilio

https://hg.mozilla.org/mozilla-central/rev/6299a6e593f8 https://hg.mozilla.org/mozilla-central/rev/369dae118647 https://hg.mozilla.org/mozilla-central/rev/676772080481 https://hg.mozilla.org/mozilla-central/rev/b9cf191f3443 https://hg.mozilla.org/mozilla-central/rev/77f31a0e6340 https://hg.mozilla.org/mozilla-central/rev/828e691c05dd https://hg.mozilla.org/mozilla-central/rev/711e54740d20 https://hg.mozilla.org/mozilla-central/rev/6c6acab22f7f https://hg.mozilla.org/mozilla-central/rev/d570b529fedd https://hg.mozilla.org/mozilla-central/rev/abddd5433879 https://hg.mozilla.org/mozilla-central/rev/bd47a1b3d97b https://hg.mozilla.org/mozilla-central/rev/a17ceb1adbd6