Bug 70408 – Move rule matching and applying to separate functions from CSSStyleSelector::styleForElement

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Antti Koivisto]

A cleanup.

[Antti Koivisto]

Created attachment 111587 [details]
patch

[Andreas Kling]

Comment on attachment 111587 [details]
patch

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

r=me.

I suspect that this code is vulnerable to some integer overflows with all the assignments of unsigned/size_t values to int variables (the MatchResult members.)
Still, r+ since you're just moving the code around.

> Source/WebCore/css/CSSStyleSelector.cpp:752
> +    if (!resolveForRootDefault) {

We should use early return here.

> Source/WebCore/css/CSSStyleSelector.cpp:760
> +            // Ask if the HTML element has mapped attributes.

This comment implies that m_styledElement is an HTML element, but given the isHTMLElement() check further down, is that always the case?

> Source/WebCore/css/CSSStyleSelector.cpp:764
> +                for (unsigned i = 0; i < map->length(); i++) {

Style, ++i.
NamedNodeMap::length() returns size_t, not unsigned.

> Source/WebCore/css/CSSStyleSelector.cpp:769
> +                            result.firstAuthorRule =result.lastAuthorRule;

Also style, should have space after =.

> Source/WebCore/css/CSSStyleSelector.cpp:786
> +                    for (unsigned i = 0; i < additionalDeclsSize; i++)

Style, ++i.

> Source/WebCore/css/CSSStyleSelector.cpp:1653
> +    // If we have first-letter pseudo style, do not share this style

Might as well tack on a period at the end of this comment.

> Source/WebCore/css/CSSStyleSelector.cpp:2189
> +    // Line-height is set when we are sure we decided on the font-size

Ditto.

[Andreas Kling]

Comment on attachment 111587 [details]
patch

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

> Source/WebCore/css/CSSStyleSelector.cpp:782
> +                    unsigned additionalDeclsSize = m_additionalAttributeStyleDecls.size();

Vector::size() returns size_t.

[Antti Koivisto]

Created attachment 111632 [details]
new patch

Expanded the refactoring a bit so requesting re-review. This patch uses the MatchResult struct in more places and gets rid of the resolveForRootDefault parameter for the new functions.

[Andreas Kling]

Comment on attachment 111632 [details]
new patch

r=me, though let's fight about size_t vs unsigned indexing at some point.

[Antti Koivisto]

http://trac.webkit.org/changeset/97962