1449103 - Merge ServoGroupRuleRules into GroupRule

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1449103 part 1 - Make Rule::mParentRule not necessarily a GroupRule.

For part 2, where CSSKeyframesRule will inherit directly from Rule while
it is parent rule for its CSSKeyframeRules.

It seems there is nothing relying on mParentRule being a GroupRule so we
can just change it.

Review commit: https://reviewboard.mozilla.org/r/231816/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231816/

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1449103 part 2 - Make CSSKeyframesRule inherit from Rule directly.

The spec doesn't say CSSKeyframesRule needs to inherit CSSGroupingRule.
It inherited GroupRule because Gecko's keyframes rule implementation
relies on that to manage its children.

Review commit: https://reviewboard.mozilla.org/r/231818/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231818/

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1449103 part 3 - Merge ServoGroupRuleRules into GroupRule.

Review commit: https://reviewboard.mozilla.org/r/231820/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231820/

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8962976 [details]
Bug 1449103 part 1 - Make Rule::mParentRule not necessarily a GroupRule.

https://reviewboard.mozilla.org/r/231816/#review237358

::: layout/style/Rule.h:97
(Diff revision 1)
>    }
>  
>    virtual void SetStyleSheet(StyleSheet* aSheet);
>  
> -  void SetParentRule(GroupRule* aRule) {
> +  void SetParentRule(Rule* aRule) {
>      // We don't reference count this up reference. The group rule

s/The group rule/the rule/.

Can we assert the kind of rule we have around matches our expectations?

Just to ensure nobody starts incorrectly calling this.

Or maybe add an overload for GroupRule and another for KeyframesRule, while keeping the member type Rule?

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8962977 [details]
Bug 1449103 part 2 - Make CSSKeyframesRule inherit from Rule directly.

https://reviewboard.mozilla.org/r/231818/#review237360

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8962978 [details]
Bug 1449103 part 3 - Merge ServoGroupRuleRules into GroupRule.

https://reviewboard.mozilla.org/r/231820/#review237362

Wow, this is much nicer. Only a single formatting nit because I didn't really have any other complaint, nice cleanup! :)

::: layout/style/GroupRule.h:56
(Diff revision 1)
> -  }
> -  Rule* GetStyleRuleAt(uint32_t aIndex) const {
> -    REDIRECT_TO_INNER(GetStyleRuleAt(aIndex))
> -  }
>  
> +  Rule* GetStyleRuleAt(int32_t aIndex) const {

I'd say "brace to the next line", but we're really inconsistent at this so your call :P

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8962976 [details]
Bug 1449103 part 1 - Make Rule::mParentRule not necessarily a GroupRule.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231816/diff/1-2/

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8962977 [details]
Bug 1449103 part 2 - Make CSSKeyframesRule inherit from Rule directly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231818/diff/1-2/

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8962978 [details]
Bug 1449103 part 3 - Merge ServoGroupRuleRules into GroupRule.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231820/diff/1-2/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/233d968614c0
part 1 - Make Rule::mParentRule not necessarily a GroupRule. r=emilio
https://hg.mozilla.org/integration/autoland/rev/17e8f60b41e8
part 2 - Make CSSKeyframesRule inherit from Rule directly. r=emilio
https://hg.mozilla.org/integration/autoland/rev/83d97a59a659
part 3 - Merge ServoGroupRuleRules into GroupRule. r=emilio

Comment 11

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/233d968614c0
https://hg.mozilla.org/mozilla-central/rev/17e8f60b41e8
https://hg.mozilla.org/mozilla-central/rev/83d97a59a659