Bug 137381 - Use is<>() / downcast<>() for CSSValue subclasses
Summary: Use is<>() / downcast<>() for CSSValue subclasses
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on: 137056
Blocks:
  Show dependency treegraph
 
Reported: 2014-10-02 22:37 PDT by Chris Dumez
Modified: 2014-10-03 16:17 PDT (History)
5 users (show)

See Also:


Attachments
Patch (271.48 KB, patch)
2014-10-02 23:16 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (270.92 KB, patch)
2014-10-03 00:13 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (270.88 KB, patch)
2014-10-03 10:05 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (270.89 KB, patch)
2014-10-03 10:36 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (270.61 KB, patch)
2014-10-03 15:24 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (270.61 KB, patch)
2014-10-03 15:34 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2014-10-02 22:37:27 PDT
Use is<>() / downcast<>() for CSSValue subclasses
Comment 1 Chris Dumez 2014-10-02 23:16:35 PDT
Created attachment 239180 [details]
Patch
Comment 2 WebKit Commit Bot 2014-10-02 23:17:59 PDT
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.
Comment 3 Chris Dumez 2014-10-03 00:13:46 PDT
Created attachment 239182 [details]
Patch
Comment 4 WebKit Commit Bot 2014-10-03 00:16:14 PDT
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.
Comment 5 Chris Dumez 2014-10-03 10:05:44 PDT
Created attachment 239208 [details]
Patch
Comment 6 WebKit Commit Bot 2014-10-03 10:06:43 PDT
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.
Comment 7 Chris Dumez 2014-10-03 10:36:26 PDT
Created attachment 239209 [details]
Patch
Comment 8 WebKit Commit Bot 2014-10-03 10:38:08 PDT
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.
Comment 9 Benjamin Poulain 2014-10-03 14:37:11 PDT
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
Comment 10 Chris Dumez 2014-10-03 15:24:12 PDT
Created attachment 239243 [details]
Patch
Comment 11 Chris Dumez 2014-10-03 15:34:06 PDT
Created attachment 239245 [details]
Patch
Comment 12 WebKit Commit Bot 2014-10-03 15:52:38 PDT
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.
Comment 13 WebKit Commit Bot 2014-10-03 16:17:04 PDT
Comment on attachment 239245 [details]
Patch

Clearing flags on attachment: 239245

Committed r174300: <http://trac.webkit.org/changeset/174300>
Comment 14 WebKit Commit Bot 2014-10-03 16:17:09 PDT
All reviewed patches have been landed.  Closing bug.