Bug 163605 - [CSS Parser] Get all the properties turned on
Summary: [CSS Parser] Get all the properties turned on
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dave Hyatt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-10-18 11:25 PDT by Dave Hyatt
Modified: 2016-10-18 13:42 PDT (History)
2 users (show)

See Also:


Attachments
Patch (265.06 KB, patch)
2016-10-18 11:28 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch for style (264.73 KB, patch)
2016-10-18 11:44 PDT, Dave Hyatt
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Hyatt 2016-10-18 11:25:30 PDT
Get all the properties turned on.
Comment 1 Dave Hyatt 2016-10-18 11:28:32 PDT
Created attachment 291961 [details]
Patch
Comment 2 WebKit Commit Bot 2016-10-18 11:30:58 PDT
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.
Comment 3 Dave Hyatt 2016-10-18 11:44:14 PDT
Created attachment 291963 [details]
Patch for style
Comment 4 WebKit Commit Bot 2016-10-18 11:46:43 PDT
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.
Comment 5 Dean Jackson 2016-10-18 12:40:58 PDT
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;
Comment 6 Dave Hyatt 2016-10-18 13:06:17 PDT
Landed in r207479.
Comment 7 Ryan Haddad 2016-10-18 13:42:32 PDT
This change broke the Windows build:

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