[webkit-reviews] review granted: [Bug 224097] Media queries with max-width greater than 999999999px evaluate to false : [Attachment 425067] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Tyler Wilcock
<twilco.o at protonmail.com>'s request for review:
Bug 224097: Media queries with max-width greater than 999999999px evaluate to
false
https://bugs.webkit.org/show_bug.cgi?id=224097

Attachment 425067: Patch

https://bugs.webkit.org/attachment.cgi?id=425067&action=review




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 425067
  --> https://bugs.webkit.org/attachment.cgi?id=425067
Patch

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

> Source/WebCore/css/MediaQueryEvaluator.cpp:485
> -static bool computeLength(CSSValue* value, bool strict, const
CSSToLengthConversionData& conversionData, int& result)
> +static bool computeLength(CSSValue* value, bool strict, const
CSSToLengthConversionData& conversionData, double& result)

If we are modifying all the callers of this anyway, we should take the
opportunity to return Optional<double> instead of using an out argument. That’s
the future direction for this kind of code.

> Source/WebCore/css/MediaQueryEvaluator.cpp:493
> +	   result = clampTo<int>(primitiveValue.doubleValue());

Seems strange to compute a double, clamp it to int, then convert back to
double. What’s the rationale here? The proposed code is inefficient (round trip
from floating point to int and back), truncates rather than rounding (is that
what we want), and limits values to the range of a 32-bit integer (not clear
why that is desirable given that this function returns a double).