[webkit-reviews] review granted: [Bug 75223] Update CSSStyleSelector so it does not keep a vector of MediaQueryResult pointers that need to be deleted : [Attachment 120556] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 75223: Update CSSStyleSelector so it does not keep a vector of
MediaQueryResult pointers that need to be deleted
https://bugs.webkit.org/show_bug.cgi?id=75223

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

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=120556&action=review


This patch looks good. I have some minor nits.

> Source/WebCore/ChangeLog:21
> +	   to be a vector of values rather tha of pointers.

Nit: tha => than

> Source/WebCore/css/CSSStyleSelector.cpp:144
> +    

Nit: There is whitespace at the beginning of this line.

> Source/WebCore/css/MediaQuery.cpp:120
> +} // namespace

Nit: This comment should read "namespace WebCore".

> Source/WebCore/css/MediaQueryEvaluator.cpp:NaN
>  static bool aspect_ratioMediaFeatureEval

I know that this isn't part of your patch. We should consider renaming this
function so that its name conforms to the WebKit style guide. Similarly, the
names of many static functions in this file don't conform to the style guide.
We can do such renames in a separate patch.

> Source/WebCore/css/MediaQueryEvaluator.cpp:512
>  #define ADD_TO_FUNCTIONMAP(name, str)  \

I know that this isn't part of your patch. From my understanding of (15) of
section Names of the WebKit style guide
(http://www.webkit.org/coding/coding-style.html), macro functions should be
named like functions. In particular, they should use CamelCase, where the first
letter is capitalized. That being said, I've seen macro functions, like this
one, that use an all-caps naming notation with underscores for spaces in our
code base.

> Source/WebCore/css/MediaQueryEvaluator.h:78
>  } // namespace

Nit: This comment should read "namespace WebCore".