Bug 232874 – Refactor consumeWillChange()

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

[Tim Nguyen (:ntim)]

See bug 232868 comment 9

[Tim Nguyen (:ntim)]

Created attachment 443671 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 443672 [details]
Patch

[Antti Koivisto]

Comment on attachment 443672 [details]
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.

[Tim Nguyen (:ntim)]

(In reply to Antti Koivisto from comment #3)
> Comment on attachment 443672 [details]
> 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.

This is how the spec defines things, defines values first, and property IDs are just a special cases of <custom-ident>, so to me this is actually easier to read.

[Antti Koivisto]

Comment on attachment 443672 [details]
Patch

Ok then!

[Tim Nguyen (:ntim)]

Committed r285492 (244016@main): <https://commits.webkit.org/244016@main>

[Radar WebKit Bug Importer]

<rdar://problem/85198621>