Bug 164673 – Handle filter() image type in new CSS Parser

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dean Jackson]

Handle filter() image type in new CSS Parser

[Dean Jackson]

<rdar://problem/29231376>

[Dean Jackson]

Created attachment 294582 [details]
Patch

[Darin Adler]

Comment on attachment 294582 [details]
Patch

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

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1093
> +    RefPtr<CSSValue> imageValue = consumeImageOrNone(args, context);

I prefer auto in cases like this.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1097
> +    RefPtr<CSSValue> filterValue = consumeFilter(args, context);

Ditto.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1102
> +    return CSSFilterImageValue::create(imageValue.releaseNonNull(), filterValue.releaseNonNull());

What guarantees filterValue is non-null?

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1189
> +    RefPtr<CSSFunctionValue> filterValue = CSSFunctionValue::create(filterType);

I prefer auto in a case like this.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1198
> +            // FIXME (crbug.com/397061): Support calc expressions like calc(10% + 0.5)

Do we really need this crbug URL?

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1207
> +            // FIXME (crbug.com/397061): Support calc expressions like calc(10% + 0.5)

Ditto.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1230
> +    RefPtr<CSSValueList> list = CSSValueList::createSpaceSeparated();

I prefer auto in a case like this.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1232
> +        RefPtr<CSSValue> filterValue = consumeUrl(range);

Ditto.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1258
> +    RefPtr<CSSPrimitiveValue> horizontalOffset = consumeLength(range, cssParserMode, ValueRangeAll);

I prefer auto.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1262
> +    RefPtr<CSSPrimitiveValue> verticalOffset = consumeLength(range, cssParserMode, ValueRangeAll);

Ditto.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1266
> +    RefPtr<CSSPrimitiveValue> blurRadius = consumeLength(range, cssParserMode, ValueRangeAll);

Ditto.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1286
> +    // We pass RefPtrs, since they can actually be null.

I don’t think we need this comment.

[Dean Jackson]

Comment on attachment 294582 [details]
Patch

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

>> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1232
>> +        RefPtr<CSSValue> filterValue = consumeUrl(range);
> 
> Ditto.

I couldn't do this one, because auto thought it was a CSSPrimitiveValue, and then got angry when it tried to assign a CSSFunctionValue below.

[Dean Jackson]

Committed r208700: <http://trac.webkit.org/changeset/208700>