[webkit-reviews] review denied: [Bug 109271] Keyframe rule should be ignored if keyframe selector has invalid value : [Attachment 198324] Patch 5

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has denied Alexander Shalamov
<alexander.shalamov at gmail.com>'s request for review:
Bug 109271: Keyframe rule should be ignored if keyframe selector has invalid
value
https://bugs.webkit.org/show_bug.cgi?id=109271

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

------- Additional Comments from Dean Jackson <dino at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=198324&action=review


Nearly there!

You might want to test -0% as well.

> Source/WebCore/ChangeLog:7
> +	   Keyframe rule should be ignored if keyframe selector has invalid
value
> +	   https://bugs.webkit.org/show_bug.cgi?id=109271
> +
> +	   Reviewed by NOBODY (OOPS!).
> +

Please outline what you changed. You updated the CSS grammar - it's important
to describe why you did that.

> Source/WebCore/ChangeLog:26
> +	   * css/CSSGrammar.y.in:
> +	   * css/CSSParser.cpp:
> +	   (WebCore::CSSParser::createKeyframesRule):
> +	   (WebCore::CSSParser::updateSpecifiers):
> +	   * css/StyleResolver.cpp:
> +	   (WebCore::StyleResolver::keyframeStylesForAnimation):
> +	   * css/WebKitCSSKeyframeRule.cpp:
> +	   (WebCore::StyleKeyframe::keyText):
> +	   (WebCore):
> +	   (WebCore::StyleKeyframe::setKeyText):
> +	   (WebCore::StyleKeyframe::setKeys):
> +	   (WebCore::StyleKeyframe::parseKeyString):
> +	   (WebCore::StyleKeyframe::reportMemoryUsage):
> +	   * css/WebKitCSSKeyframeRule.h:
> +	   (StyleKeyframe):
> +	   (WebCore::StyleKeyframe::isValid):
> +	   (WebCore::StyleKeyframe::getKeys):

And provide some details here too.

> Source/WebCore/css/CSSParser.cpp:11351
> +	   if (keyframe->isValid()) {
> +	       rule->parserAppendKeyframe(keyframe);
> +	       continue;
> +	   }
> +	   return 0;

You should do this the other way around.

if (!keyframe->isValid())
  return 0;

rule->parserAppendKeyframe(keyframe);

> Source/WebCore/css/CSSParser.cpp:11630
> +	   if (value && value->unit == CSSPrimitiveValue::CSS_NUMBER) {
> +	       keyframeKeys.append(static_cast<float>(value->fValue / 100));
> +	       continue;
> +	   }
> +	   keyframeKeys.clear();
> +	   break;

Same here. Early break/return in the invalid case.