Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

StyleRuleGroup should use composition instead of inheritance #11835

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mdubet
Copy link
Contributor

@mdubet mdubet commented Mar 22, 2023

f3dde5a

StyleRuleGroup should use composition instead of inheritance
https://bugs.webkit.org/show_bug.cgi?id=254297
rdar://107103847

Reviewed by NOBODY (OOPS!).

The inheritance comes from the CSSGroupingRule spec,
but we have no reason to follow the same pattern/hierarchy in our
internal representation.

* Source/WebCore/css/CSSConditionRule.cpp:
(WebCore::CSSConditionRule::CSSConditionRule):
* Source/WebCore/css/CSSConditionRule.h:
* Source/WebCore/css/CSSContainerRule.cpp:
(WebCore::CSSContainerRule::styleRuleContainer const):
* Source/WebCore/css/CSSGroupingRule.cpp:
(WebCore::CSSGroupingRule::CSSGroupingRule):
(WebCore::CSSGroupingRule::~CSSGroupingRule):
(WebCore::CSSGroupingRule::insertRule):
(WebCore::CSSGroupingRule::deleteRule):
(WebCore::CSSGroupingRule::cssTextForDeclsAndRules const):
(WebCore::CSSGroupingRule::length const):
(WebCore::CSSGroupingRule::item const):
(WebCore::CSSGroupingRule::reattach):
(WebCore::CSSGroupingRule::groupRule const):
(WebCore::CSSGroupingRule::groupRule):
* Source/WebCore/css/CSSGroupingRule.h:
(WebCore::CSSGroupingRule::rule const):
(WebCore::CSSGroupingRule::groupRule const): Deleted.
(WebCore::CSSGroupingRule::groupRule): Deleted.
* Source/WebCore/css/CSSLayerBlockRule.cpp:
(WebCore::CSSLayerBlockRule::name const):
* Source/WebCore/css/CSSMediaRule.cpp:
(WebCore::CSSMediaRule::mediaQueries const):
(WebCore::CSSMediaRule::setMediaQueries):
* Source/WebCore/css/CSSSupportsRule.cpp:
(WebCore::CSSSupportsRule::conditionText const):
* Source/WebCore/css/StyleRule.cpp:
(WebCore::StyleRuleGroup::StyleRuleGroup):
(WebCore::StyleRuleGroup::childRules):
(WebCore::StyleRuleGroup::fromStyleRuleBase):
(WebCore::StyleRuleMedia::StyleRuleMedia):
(WebCore::StyleRuleSupports::StyleRuleSupports):
(WebCore::StyleRuleLayer::StyleRuleLayer):
(WebCore::StyleRuleContainer::StyleRuleContainer):
* Source/WebCore/css/StyleRule.h:
(WebCore::StyleRuleBase::isStyleRuleWithNesting const):
(WebCore::StyleRuleBase::isGroupRule const):
* Source/WebCore/css/StyleSheetContents.cpp:
(WebCore::traverseRulesInVector):
* Source/WebCore/style/RuleSetBuilder.cpp:
(WebCore::Style::RuleSetBuilder::addChildRule):
* Source/WebCore/style/StyleInvalidator.cpp:
(WebCore::Style::shouldDirtyAllStyle):

f3dde5a

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ wpe-wk2
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ›  gtk
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk1 βœ… πŸ§ͺ gtk-wk2
βœ… πŸ›  tv βœ… πŸ§ͺ mac-wk2 ⏳ πŸ§ͺ api-gtk
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  watch βœ… πŸ§ͺ mac-wk2-stress
βœ… πŸ›  watch-sim

@mdubet mdubet self-assigned this Mar 22, 2023
@mdubet mdubet added the WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit). label Mar 22, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 22, 2023
@mdubet mdubet removed the merging-blocked Applied to prevent a change from being merged label Mar 23, 2023
@mdubet mdubet force-pushed the stylerulegroup-composition branch from 13fd3be to 85bec4b Compare March 23, 2023 00:52
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 23, 2023
Comment on lines -277 to -279
class StyleRuleGroup : public StyleRuleBase {
public:
const Vector<RefPtr<StyleRuleBase>>& childRules() const;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel all this refactoring achieves little. StyleRuleGroup is a trivial helper type that just holds the child rule vector. It has no meaningful functionality.

Also CSSStyleRule in the nesting spec is not a CSSGroupingRule (it just adds insert/deleteRule) so I'm not sure how this is even helps? CSSStyleRule would have all the code dealing with StyleRule/StyleRuleWithNesting and CSSGroupingRule would have the code dealing with StyleRuleGroup.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed this patch doesn't achieve much but it's a small step forward to fix some inconsistencies in the codebase by removing StyleRuleGroup from the Rule hierarchy (StyleRuleGroup is not a Rule and should not inherit from StyleRuleBase).

The CSSGroupingRule discussion is for another PR, we should try to consider them separately.

https://bugs.webkit.org/show_bug.cgi?id=254297
rdar://107103847

Reviewed by NOBODY (OOPS!).

The inheritance comes from the CSSGroupingRule spec,
but we have no reason to follow the same pattern/hierarchy in our
internal representation.

* Source/WebCore/css/CSSConditionRule.cpp:
(WebCore::CSSConditionRule::CSSConditionRule):
* Source/WebCore/css/CSSConditionRule.h:
* Source/WebCore/css/CSSContainerRule.cpp:
(WebCore::CSSContainerRule::styleRuleContainer const):
* Source/WebCore/css/CSSGroupingRule.cpp:
(WebCore::CSSGroupingRule::CSSGroupingRule):
(WebCore::CSSGroupingRule::~CSSGroupingRule):
(WebCore::CSSGroupingRule::insertRule):
(WebCore::CSSGroupingRule::deleteRule):
(WebCore::CSSGroupingRule::cssTextForDeclsAndRules const):
(WebCore::CSSGroupingRule::length const):
(WebCore::CSSGroupingRule::item const):
(WebCore::CSSGroupingRule::reattach):
(WebCore::CSSGroupingRule::groupRule const):
(WebCore::CSSGroupingRule::groupRule):
* Source/WebCore/css/CSSGroupingRule.h:
(WebCore::CSSGroupingRule::rule const):
(WebCore::CSSGroupingRule::groupRule const): Deleted.
(WebCore::CSSGroupingRule::groupRule): Deleted.
* Source/WebCore/css/CSSLayerBlockRule.cpp:
(WebCore::CSSLayerBlockRule::name const):
* Source/WebCore/css/CSSMediaRule.cpp:
(WebCore::CSSMediaRule::mediaQueries const):
(WebCore::CSSMediaRule::setMediaQueries):
* Source/WebCore/css/CSSSupportsRule.cpp:
(WebCore::CSSSupportsRule::conditionText const):
* Source/WebCore/css/StyleRule.cpp:
(WebCore::StyleRuleGroup::StyleRuleGroup):
(WebCore::StyleRuleGroup::childRules):
(WebCore::StyleRuleGroup::fromStyleRuleBase):
(WebCore::StyleRuleMedia::StyleRuleMedia):
(WebCore::StyleRuleSupports::StyleRuleSupports):
(WebCore::StyleRuleLayer::StyleRuleLayer):
(WebCore::StyleRuleContainer::StyleRuleContainer):
* Source/WebCore/css/StyleRule.h:
(WebCore::StyleRuleBase::isStyleRuleWithNesting const):
(WebCore::StyleRuleBase::isGroupRule const):
* Source/WebCore/css/StyleSheetContents.cpp:
(WebCore::traverseRulesInVector):
* Source/WebCore/style/RuleSetBuilder.cpp:
(WebCore::Style::RuleSetBuilder::addChildRule):
* Source/WebCore/style/StyleInvalidator.cpp:
(WebCore::Style::shouldDirtyAllStyle):
@mdubet mdubet removed the merging-blocked Applied to prevent a change from being merged label Mar 23, 2023
@mdubet mdubet force-pushed the stylerulegroup-composition branch from 85bec4b to f3dde5a Compare March 23, 2023 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit).
Projects
None yet
4 participants