[webkit-reviews] review denied: [Bug 98244] [Refactoring] Scoped Style related code should have its own class. : [Attachment 167057] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Oct 4 10:35:24 PDT 2012


Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Hajime Morrita
<morrita at google.com>'s request for review:
Bug 98244: [Refactoring] Scoped Style related code should have its own class.
https://bugs.webkit.org/show_bug.cgi?id=98244

Attachment 167057: Patch
https://bugs.webkit.org/attachment.cgi?id=167057&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=167057&action=review


I really like splitting out scope-related things into its own class, and
anything else that makes StyleResolver a smaller seems like a win.


Having said that, the patch is too large and needs to be split up to allow for
meaningful review. Maybe we could start with moving RuleData/RuleSet into their
own files?

Also, is there a performance impact?

> Source/WebCore/css/StyleResolver.cpp:401
> +bool StyleResolver::usesSiblingRules() const
> +{
> +    return !m_features->siblingRules.isEmpty();
> +}
> +
> +bool StyleResolver::usesFirstLineRules() const
> +{
> +    return m_features->usesFirstLineRules;
> +}
> +
> +bool StyleResolver::usesBeforeAfterRules() const
> +{
> +    return m_features->usesBeforeAfterRules;
> +}

Probably should go at the bottom of StyleResolver.h with inline prefix? Or has
this convention changed?

> Source/WebCore/css/StyleResolverRules.h:26
> +#ifndef StyleResolverRules_h

Can we name it to match at least one class name in the header? This way, it
will be easier to find by file name. How about RuleSet, since it's the majority
of the code?


More information about the webkit-reviews mailing list