[webkit-reviews] review granted: [Bug 70408] Move rule matching and applying to separate functions from CSSStyleSelector::styleForElement : [Attachment 111587] patch

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> has granted Antti Koivisto <koivisto at iki.fi>'s
request for review:
Bug 70408: Move rule matching and applying to separate functions from
CSSStyleSelector::styleForElement
https://bugs.webkit.org/show_bug.cgi?id=70408

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

------- Additional Comments from Andreas Kling <kling at webkit.org>
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.