Bug 163605 – [CSS Parser] Get all the properties turned on

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

[Dave Hyatt]

Get all the properties turned on.

[Dave Hyatt]

Created attachment 291961 [details]
Patch

[WebKit Commit Bot]

Attachment 291961 [details] did not pass style-queue:


ERROR: Source/WebCore/css/parser/CSSPropertyParser.cpp:58:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/css/parser/CSSPropertyParser.cpp:804:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/css/parser/CSSPropertyParser.cpp:959:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/css/parser/CSSPropertyParser.cpp:1047:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/css/parser/CSSPropertyParser.cpp:1241:  'spreadDistance' is incorrectly named. It should be named 'protector' or 'protectedNullptr'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/css/parser/CSSPropertyParser.cpp:1312:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/css/parser/CSSPropertyParser.cpp:1788:  'separator' is incorrectly named. It should be named 'protector' or 'protectedNullptr'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/css/parser/CSSPropertyParser.cpp:2209:  'value' is incorrectly named. It should be named 'protector' or 'protectedNullptr'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/css/parser/CSSPropertyParser.cpp:2236:  'value' is incorrectly named. It should be named 'protector' or 'protectedNullptr'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/css/parser/CSSPropertyParser.cpp:2464:  'firstValue' is incorrectly named. It should be named 'protector' or 'protectedList'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/css/parser/CSSPropertyParser.cpp:2770:  'result' is incorrectly named. It should be named 'protector' or 'protectedLineNames'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/css/parser/CSSPropertyParser.cpp:3541:  'verticalSpacing' is incorrectly named. It should be named 'protector' or 'protectedHorizontalSpacing'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/css/parser/CSSPropertyParser.cpp:3594:  'maxWidth' is incorrectly named. It should be named 'protector' or 'protectedMinWidth'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/css/parser/CSSPropertyParser.cpp:3607:  'maxHeight' is incorrectly named. It should be named 'protector' or 'protectedMinHeight'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/css/parser/CSSPropertyParser.cpp:3757:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
ERROR: Source/WebCore/css/parser/CSSPropertyParser.cpp:4086:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/css/parser/CSSPropertyParser.cpp:4092:  'endValue' is incorrectly named. It should be named 'protector' or 'protectedNullptr'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/css/parser/CSSPropertyParser.cpp:4393:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/css/parser/CSSPropertyParser.cpp:4395:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/css/parser/CSSPropertyParser.cpp:4397:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/css/parser/CSSPropertyParser.cpp:4399:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 21 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dave Hyatt]

Created attachment 291963 [details]
Patch for style

[WebKit Commit Bot]

Attachment 291963 [details] did not pass style-queue:


ERROR: Source/WebCore/css/parser/CSSPropertyParser.cpp:2462:  'firstValue' is incorrectly named. It should be named 'protector' or 'protectedList'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/css/parser/CSSPropertyParser.cpp:2768:  'result' is incorrectly named. It should be named 'protector' or 'protectedLineNames'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/css/parser/CSSPropertyParser.cpp:3539:  'verticalSpacing' is incorrectly named. It should be named 'protector' or 'protectedHorizontalSpacing'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/css/parser/CSSPropertyParser.cpp:3592:  'maxWidth' is incorrectly named. It should be named 'protector' or 'protectedMinWidth'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/css/parser/CSSPropertyParser.cpp:3605:  'maxHeight' is incorrectly named. It should be named 'protector' or 'protectedMinHeight'.  [readability/naming/protected] [4]
Total errors found: 5 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Comment on attachment 291963 [details]
Patch for style

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

> Source/WebCore/css/parser/CSSPropertyParser.cpp:400
>      static const unsigned tagNameLength = 4;

Do we still need this variable? We use FontTag::size() below. Can we replace the other uses of tagNameLength?

> Source/WebCore/css/parser/CSSPropertyParser.cpp:415
> +        tag[i] = toASCIILower(character);

Blink didn't do this step. Why do we?

> Source/WebCore/css/parser/CSSPropertyParser.cpp:708
> -        return CSSFontFamilyValue::create(range.consumeIncludingWhitespace().value().toString());
> +        return CSSPrimitiveValue::create(range.consumeIncludingWhitespace().value().toString(), CSSPrimitiveValue::UnitTypes::CSS_STRING);

Should we have a CSSFontFamilyValue?

> Source/WebCore/css/parser/CSSPropertyParser.cpp:851
> -    bool hasEachLine = false;
> +//    bool hasEachLine = false;

Remove this. Or put FIXME-NEWPARSER.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:891
> +// FIXME-NEWPARSER: Drop the prefix on min-content, max-content and fit-content.
> +static bool validWidthOrHeightKeyword(CSSValueID id, const CSSParserContext& /*context*/)
>  {
>      if (id == CSSValueWebkitMinContent || id == CSSValueWebkitMaxContent || id == CSSValueWebkitFillAvailable || id == CSSValueWebkitFitContent
> -        || id == CSSValueMinContent || id == CSSValueMaxContent || id == CSSValueFitContent) {
> -        if (context.useCounter()) {
> -            switch (id) {
> -            case CSSValueWebkitMinContent:
> -                context.useCounter()->count(UseCounter::CSSValuePrefixedMinContent);
> -                break;
> -            case CSSValueWebkitMaxContent:
> -                context.useCounter()->count(UseCounter::CSSValuePrefixedMaxContent);
> -                break;
> -            case CSSValueWebkitFillAvailable:
> -                context.useCounter()->count(UseCounter::CSSValuePrefixedFillAvailable);
> -                break;
> -            case CSSValueWebkitFitContent:
> -                context.useCounter()->count(UseCounter::CSSValuePrefixedFitContent);
> -                break;
> -            default:
> -                break;
> -            }
> -        }
> +        || id == CSSValueWebkitMinContent || id == CSSValueWebkitMaxContent || id == CSSValueWebkitFitContent) {

I think you're testing for CSSValueWebkitMinContent, etc twice here

> Source/WebCore/css/parser/CSSPropertyParser.cpp:950
> +    auto rect = Rect::create();
> +    rect->setLeft(left.releaseNonNull());

Weird that there isn't a Rect constructor that takes all of these. Although I guess it means you can't screw up the order :)

> Source/WebCore/css/parser/CSSPropertyParser.cpp:-1220
> -static CSSValue* consumeSteps(CSSParserTokenRange& range)
> -{

We want this function. We support the steps() timing function.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:-1284
> -        || id == CSSValueEaseOut || id == CSSValueEaseInOut || id == CSSValueStepStart
> -        || id == CSSValueStepEnd || id == CSSValueStepMiddle)

We need these.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:-1289
> -    if (function == CSSValueSteps)
> -        return consumeSteps(range);

Ditto.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:-1582
> -static CSSValue* consumePath(CSSParserTokenRange& range)
> -{
> -    // FIXME: Add support for <url>, <basic-shape>, <geometry-box>.
> -    if (range.peek().functionId() != CSSValuePath)
> -        return nullptr;

Hmmm.. I think we want this one too.

Oh, unless it isn't for clip paths, but is just for motion paths. In which case, we don't support it yet.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:1480
> -    case CSSValueRotateX:
> -    case CSSValueRotateY:
> -    case CSSValueRotateZ:
> -    case CSSValueSkewX:
> -    case CSSValueSkewY:
> +    case CSSValueRotatex:
> +    case CSSValueRotatey:
> +    case CSSValueRotatez:
> +    case CSSValueSkewx:
> +    case CSSValueSkewy:

I wonder how they got theirs to uppercase the XYZ parts. They must have manually set it. Not that it matters.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:1587
> -        return CSSPrimitiveValue::create(percent, CSSPrimitiveValue::UnitType::Percentage);
> +        return CSSPrimitiveValue::create(percent, CSSPrimitiveValue::UnitTypes::CSS_PERCENTAGE);

It would be nice to make our UnitTypes a class enum with friendly names.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:1844
>          return consumeIdent(range);
> -    CSSPrimitiveValue* parsedValue = consumeLength(range, cssParserMode, ValueRangeAll);
> -    if (!parsedValue && (unresolvedProperty == CSSPropertyAliasWebkitPerspective)) {
> -        double perspective;
> -        if (!consumeNumberRaw(range, perspective))
> -            return nullptr;
> -        parsedValue = CSSPrimitiveValue::create(perspective, CSSPrimitiveValue::UnitType::Pixels);
> -    }
> +    RefPtr<CSSPrimitiveValue> parsedValue = consumeLength(range, cssParserMode, ValueRangeAll);

Won't this throw out our support for unitless perspective values? I think we need to keep that.

e.g. LayoutTests/compositing/layer-creation/overlap-transformed-preserved-3d.html
21:      -webkit-perspective: 200;

[Dave Hyatt]

Landed in r207479.

[Ryan Haddad]

This change broke the Windows build:

https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/81251