Bug 137381 – Use is<>() / downcast<>() for CSSValue subclasses

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

[Chris Dumez]

Use is<>() / downcast<>() for CSSValue subclasses

[Chris Dumez]

Created attachment 239180 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/editing/EditingStyle.cpp:1494:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/css/CSSValueList.h:99:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 63 files


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

[Chris Dumez]

Created attachment 239182 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/editing/EditingStyle.cpp:1494:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/css/CSSValueList.h:99:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 63 files


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

[Chris Dumez]

Created attachment 239208 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/editing/EditingStyle.cpp:1494:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/css/CSSValueList.h:99:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 63 files


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

[Chris Dumez]

Created attachment 239209 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/editing/EditingStyle.cpp:1494:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/css/CSSValueList.h:99:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 63 files


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

[Benjamin Poulain]

Comment on attachment 239209 [details]
Patch

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

> Source/WebCore/css/CSSGradientValue.h:237
> +SPECIALIZE_TYPE_TRAITS_CSS_VALUE(CSSGradientValue, isGradientValue())
> +SPECIALIZE_TYPE_TRAITS_CSS_VALUE(CSSLinearGradientValue, isLinearGradientValue())
> +SPECIALIZE_TYPE_TRAITS_CSS_VALUE(CSSRadialGradientValue, isRadialGradientValue())
> +

IMHO it is better to keep the traits close to the declaration of the class.

> Source/WebCore/css/CSSParser.cpp:5968
>          RefPtr<CSSValue> centerX;
>          RefPtr<CSSValue> centerY;
>          parseFillPosition(args, centerX, centerY);

It's weird that this is using CSSValue instead of CSSPrimitiveValue directly.

> Source/WebCore/css/CSSToStyleMap.cpp:548
> +    for (unsigned i = 0 ; i < borderImage.length() ; ++i) {

Style: semi-color space.

> Source/WebCore/css/CSSValueList.h:100
> +    { }

Shouldn't this be on two lines.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:983
> +                ASSERT(is<CSSPrimitiveValue>(item));

Gosh, I hate this. This should be an ASSERT_WITH_MESSAGE.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:1181
> -        int length = list ? list->length() : 0;
> +        CSSValueList& list = downcast<CSSValueList>(*value);
> +        int length = list.length();

Useless code, good catch.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:1224
> +            int len = list.length();

While you are fixing this cap, let's rename len to length.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:1282
> +            ASSERT_WITH_SECURITY_IMPLICATION(is<CSSPrimitiveValue>(item));

IIRC, downcast<> already does that (if not it should).
You should be able to drop the assertion.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:1447
> +            ASSERT_WITH_SECURITY_IMPLICATION(is<CSSPrimitiveValue>(item));

ditto

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:1474
> +        else if (primitiveValue.isLength()) {
> +            lineHeight = primitiveValue.computeLength<Length>(csstoLengthConversionDataWithTextZoomFactor(*styleResolver));
> +        } else if (primitiveValue.isPercentage()) {

Coding style!!!

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:1557
> +        else if (primitiveValue.isLength()) {
> +            wordSpacing = primitiveValue.computeLength<Length>(csstoLengthConversionDataWithTextZoomFactor(*styleResolver));
> +        } else if (primitiveValue.isPercentage())

coding style

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:1943
>          EResize r = RESIZE_NONE;

arrrrrrr...

Probably coded on talk-like-a-pirate day.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2254
> +        for (size_t i = 0; i < valueList.length(); i++) {

++i

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2256
> +            CSSValue* item = valueList.itemWithoutBoundsCheck(i);
> +            if (!is<CSSPrimitiveValue>(*item))

Why not use a reference from the beginning?
    CSSValue& item = *valueList.itemWithoutBoundsCheck(i);

> Source/WebCore/css/MediaQueryEvaluator.cpp:200
> +    if (is<CSSPrimitiveValue>(*value)
> +        && downcast<CSSPrimitiveValue>(*value).isNumber()) {

Could be on one line.

> Source/WebCore/css/StyleResolver.cpp:2274
> +                ASSERT_WITH_SECURITY_IMPLICATION(is<CSSPrimitiveValue>(*first));
> +                ASSERT_WITH_SECURITY_IMPLICATION(is<CSSPrimitiveValue>(*second));

Necessary?

> Source/WebCore/css/StyleResolver.cpp:3541
> +            CSSValue* argument = filterValue.itemWithoutBoundsCheck(0);

Why not CSSValue& argument?

> Source/WebCore/css/StyleResolver.cpp:3625
> +            CSSValue* cssValue = filterValue.itemWithoutBoundsCheck(0);

CSSValue& ?

> Source/WebCore/css/TransformFunctions.cpp:93
>          CSSValue* currValue = i.value();

ditto?

> Source/WebCore/css/WebKitCSSMatrix.cpp:66
> +        if (!value || (is<CSSPrimitiveValue>(*value) && downcast<CSSPrimitiveValue>(*value).getValueID() == CSSValueNone))

Couldn't you remove the !value and use is<CSSPrimitiveValue>(value)?

> Source/WebCore/editing/ApplyStyleCommand.cpp:1513
> +    ASSERT(is<CSSPrimitiveValue>(value.get()));

This shouldn't be needed.

> Source/WebCore/editing/EditingStyle.cpp:1082
> +            value = nullptr; // text-decoration: none is equivalent to not having the property

Missing period.

> Source/WebCore/editing/EditingStyle.cpp:1511
> +    for (size_t i = 0; i < valuesInRefTextDecoration.length(); i++)

++i

> Source/WebCore/rendering/RenderTextControl.cpp:87
> +            if (is<CSSPrimitiveValue>(value.get()))

I would be nice to teach the is<> template about Ref and RefPtr.

> Source/WebCore/svg/SVGFontFaceElement.cpp:259
>          for (unsigned i = 0; i < srcLength; i++) {

++i

[Chris Dumez]

Created attachment 239243 [details]
Patch

[Chris Dumez]

Created attachment 239245 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/editing/EditingStyle.cpp:1494:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/css/CSSValueList.h:99:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 63 files


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

[WebKit Commit Bot]

Comment on attachment 239245 [details]
Patch

Clearing flags on attachment: 239245

Committed r174300: <http://trac.webkit.org/changeset/174300>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.