[webkit-reviews] review denied: [Bug 232874] Refactor consumeWillChange() : [Attachment 443672] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has denied Tim Nguyen (:ntim)
<ntim at apple.com>'s request for review:
Bug 232874: Refactor consumeWillChange()
https://bugs.webkit.org/show_bug.cgi?id=232874

Attachment 443672: Patch

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




--- Comment #3 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 443672
  --> https://bugs.webkit.org/attachment.cgi?id=443672
Patch

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

> Source/WebCore/css/parser/CSSPropertyParser.cpp:-502
>      // Every comma-separated list of identifiers is a valid will-change
value,
>      // unless the list includes an explicitly disallowed identifier.
> -    while (true) {
> -	   if (range.peek().type() != IdentToken)
> +    while (!range.atEnd()) {
> +	   switch (range.peek().id()) {
> +	   case CSSValueNone:
> +	   case CSSValueAll:
> +	   case CSSValueAuto:
>	       return nullptr;
> -	   CSSPropertyID propertyID = cssPropertyID(range.peek().value());
> -	   if (propertyID != CSSPropertyInvalid) {
> -	       // Now "all" is used by both CSSValue and CSSPropertyValue.
> -	       // Need to return nullptr when currentValue is CSSPropertyAll.
> -	       if (propertyID == CSSPropertyWillChange || propertyID ==
CSSPropertyAll)
> -		   return nullptr;
> -	      
values->append(CSSValuePool::singleton().createIdentifierValue(propertyID));
> -	       range.consumeIncludingWhitespace();
> -	   } else {

It is not clear to me how this is better. The primary purpose of this loop is
to consume property identifiers which the old code made clear.

After your change the loop looks like it is about CSSValues and property IDs
are a buried special case.