Bug 75223 – StyleResolver need not allocate each MediaQueryResult on the heap

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

[Darin Adler]

Update CSSStyleSelector so it does not keep a vector of MediaQueryResult pointers that need to be deleted

[Darin Adler]

Created attachment 120528 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 120528 [details]
Patch

Attachment 120528 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11017953

[WebKit Review Bot]

Comment on attachment 120528 [details]
Patch

Attachment 120528 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11017000

[Early Warning System Bot]

Comment on attachment 120528 [details]
Patch

Attachment 120528 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11019988

[Darin Adler]

Created attachment 120531 [details]
Patch

[WebKit Review Bot]

Comment on attachment 120531 [details]
Patch

Attachment 120531 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11018986

New failing tests:
fast/media/media-query-invalid-value.html

[Darin Adler]

Created attachment 120556 [details]
Patch

[Daniel Bates]

Comment on attachment 120556 [details]
Patch

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".

[Luke Macpherson]

Comment on attachment 120556 [details]
Patch

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

> Source/WebCore/css/MediaQueryEvaluator.cpp:64
> +// FIXME: color_index, min-color-index, and max_color_index are not implemented.

dashes or underscores? I'm guessing these should be consistent.

[Darin Adler]

Created attachment 144274 [details]
Patch

[Darin Adler]

Committed r118650: <http://trac.webkit.org/changeset/118650>

[Chris Dumez]

This patch caused more than 300 failures on EFL and GTK bots:
http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release/builds/24372

[WebKit Review Bot]

Re-opened since this is blocked by 87639

[Darin Adler]

Thanks for rolling this out for me. I’ll retest and find out what I screwed up.

[Darin Adler]

At some point, we fixed the style resolver to not allocate these on the heap, so closing.