Closed Bug 1451289 Opened 2 years ago Closed Last year

Rename / merge all CSS rule classes into their corresponding dom::CSS*Rule

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: xidorn, Assigned: canaltinova)

References

Details

Attachments

(12 files)

59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
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.
Priority: -- → P3
I think there is no reason that prevents doing this now. I will work on it.
Assignee: nobody → canaltinova
Status: NEW → ASSIGNED
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
Attachment #8983860 - Flags: review?(emilio) → review+
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 :)
Attachment #8983859 - Flags: review?(emilio) → review+
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.
Attachment #8983860 - Flags: review+ → review-
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.
Attachment #8983861 - Flags: review?(emilio) → review-
Comment on attachment 8983862 [details]
Bug 1451289 - Part 4: Rename ServoStyleRule to CSSStyleRule

https://reviewboard.mozilla.org/r/249704/#review256116
Attachment #8983862 - Flags: review?(emilio) → review+
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.
Attachment #8983863 - Flags: review?(emilio) → review+
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.
Attachment #8983864 - Flags: review?(emilio) → review+
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?
Attachment #8983865 - Flags: review?(emilio) → review-
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.
Attachment #8983866 - Flags: review?(emilio) → review+
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::`.
Attachment #8983867 - Flags: review?(emilio) → review+
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.
Attachment #8983868 - Flags: review?(emilio) → review-
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.
Attachment #8983869 - Flags: review?(emilio) → review+
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.
Attachment #8983870 - Flags: review?(emilio) → review+
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 :)
Attachment #8983860 - Flags: review?(emilio) → review+
Comment on attachment 8983861 [details]
Bug 1451289 - Part 3: Merge ServoMediaRule and CSSMediaRule

https://reviewboard.mozilla.org/r/249702/#review256264
Attachment #8983861 - Flags: review?(emilio) → review+
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.
Attachment #8983868 - Flags: review?(emilio)
Comment on attachment 8983865 [details]
Bug 1451289 - Part 7: Merge ServoPageRule and CSSPageRule

https://reviewboard.mozilla.org/r/249710/#review256272
Attachment #8983865 - Flags: review?(emilio) → review+
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).
Attachment #8983868 - Flags: review?(emilio) → review+
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
You need to log in before you can comment on or make changes to this bug.