[webkit-reviews] review denied: [Bug 97006] Invalid values for media query features are not handled : [Attachment 167979] Patch 1

Wed Oct 10 05:19:05 PDT 2012

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Alexander Shalamov
<alexander.shalamov at gmail.com>'s request for review:
Bug 97006: Invalid values for media query features are not handled
https://bugs.webkit.org/show_bug.cgi?id=97006

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

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=167979&action=review

> LayoutTests/fast/media/media-query-invalid-value.html:6
>  <head>
>      <style type="text/css">
> -	   @media (min-width) {
> +	   @media (min-width: 100px) {
>	       div { background-color: green; }
>	   }
>	   @media (min-width: blah) {

I don't get this. The test it supposed to fail right, as it is testing invalid
values! @media (min-width: 100px) is valid! not invalid which @media
(min-width) is!

> LayoutTests/fast/media/media-query-serialization.html:5
> -<style type="text/css" media="NOT braille, tv and (orientation: 'landscape')
AND (min-WIDTH:100px) and (max-width: 200px ), all and (color) and (color)">
> +<style type="text/css" media="NOT braille, tv and (orientation: landscape)
AND (min-WIDTH:100px) and (max-width: 200px ), all and (color) and (color)">

Are you sure 'landscape' is invalid? shouldn't it just not be false? spec link
please

> Source/WebCore/ChangeLog:8
> +
> +	   Additional information of the change such as approach, rationale.
Please add per-function descriptions below (OOPS!).

OOPS!

> Source/WebCore/css/CSSGrammar.y:538
> +	   // if restrictor is specified, ignore expression

Comments should be proper sentences. ie. // If ... expression.

> Source/WebCore/css/MediaQueryEvaluator.cpp:252
> +	   if (width > height) // Square viewport is portrait

Dot at end

> Source/WebCore/css/MediaQueryEvaluator.cpp:257
> +    // orientation could be calculated

// Orientation could be calculated.

Is that a FIXME? I dont understand the comment

> Source/WebCore/css/MediaQueryExp.cpp:70
> +static inline bool featureWithPositiveInteger(const AtomicString&
mediaFeature)
> +{
> +    return mediaFeature == MediaFeatureNames::colorMediaFeature
> +	   || mediaFeature == MediaFeatureNames::max_colorMediaFeature
> +	   || mediaFeature == MediaFeatureNames::min_colorMediaFeature
> +	   || mediaFeature == MediaFeatureNames::min_monochromeMediaFeature
> +	   || mediaFeature == MediaFeatureNames::max_monochromeMediaFeature;
> +}

Why not

static inline bool featureWithValidPositiveInteger(const AtomicString&
mediaFeature)
{
    if (!value->isInt || value->fValue < 0)
	return false;

    return mediaFeature == MediaFeatureNames::colorMediaFeature
	|| mediaFeature == MediaFeatureNames::max_colorMediaFeature
	|| mediaFeature == MediaFeatureNames::min_colorMediaFeature
	|| mediaFeature == MediaFeatureNames::min_monochromeMediaFeature
	|| mediaFeature == MediaFeatureNames::max_monochromeMediaFeature;
}

Also I guess it is cheaper to check the value first

> Source/WebCore/css/MediaQueryExp.cpp:131
> +	       // Media features that use CSSValueIDs

Punctuation mark at end

> Source/WebCore/css/MediaQueryExp.cpp:136
> +	       else if (featureWithPositiveLenghtOrNumber(mediaFeature) &&
(((value->unit >= CSSPrimitiveValue::CSS_EMS && value->unit <=
CSSPrimitiveValue::CSS_PC) || value->unit == CSSPrimitiveValue::CSS_REMS) ||
value->unit == CSSPrimitiveValue::CSS_NUMBER) && value->fValue >= 0)

Why not move that checking to the other method, and rename it like
featureWithValidPositiveLengthOrNumber ?