Bug 103003

Summary: Re-virtualize CSSRule.
Product: WebKit Reporter: Andreas Kling <kling>
Component: CSSAssignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, cmarcelo, eric.carlson, feature-media-reviews, koivisto, macpherson, menard, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch andersca: review+

Description Andreas Kling 2012-11-21 18:24:41 PST
CSSRule was split into CSSRule (CSSOM wrapper) and StyleRule (internal data), and keeping it lean is far less important as it's only instantiated on CSSOM API access.
We should make it virtual again to make it more hackable and less error prone.
Comment 1 Andreas Kling 2012-11-21 18:34:08 PST
Created attachment 175560 [details]
Patch
Comment 2 Andreas Kling 2012-11-21 18:39:47 PST
Created attachment 175561 [details]
Patch
Comment 3 Anders Carlsson 2012-11-21 19:05:23 PST
Comment on attachment 175561 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175561&action=review

> Source/WebCore/css/StyleResolver.cpp:2573
> -        if (cssRule->isImportRule())
> +        if (cssRule->type() == CSSRule::IMPORT_RULE)
>              collectCSSOMWrappers(wrapperMap, static_cast<CSSImportRule*>(cssRule)->styleSheet());
> -        else if (cssRule->isMediaRule())
> +        else if (cssRule->type() == CSSRule::MEDIA_RULE)
>              collectCSSOMWrappers(wrapperMap, static_cast<CSSMediaRule*>(cssRule));
>  #if ENABLE(CSS_REGIONS)
> -        else if (cssRule->isRegionRule())
> +        else if (cssRule->type() == CSSRule::WEBKIT_REGION_RULE)
>              collectCSSOMWrappers(wrapperMap, static_cast<WebKitCSSRegionRule*>(cssRule));
>  #endif
> -        else if (cssRule->isStyleRule()) {
> +        else if (cssRule->type() == CSSRule::STYLE_RULE) {
>              CSSStyleRule* cssStyleRule = static_cast<CSSStyleRule*>(cssRule);
>              wrapperMap.add(cssStyleRule->styleRule(), cssStyleRule);
>          }

This looks like it should be a switch statement instead.
Comment 4 Andreas Kling 2012-11-21 19:47:31 PST
Committed r135465: <http://trac.webkit.org/changeset/135465>
Comment 5 Antti Koivisto 2012-11-22 01:42:24 PST
Comment on attachment 175560 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175560&action=review

> Source/WebCore/css/CSSStyleRule.cpp:129
> +    ASSERT(rule->isStyleRule());
> +    m_styleRule = static_cast<StyleRule*>(rule);

Would be good to add asStyleFooRule() casting functions at some point.