[webkit-reviews] review granted: [Bug 137381] Use is<>() / downcast<>() for CSSValue subclasses : [Attachment 239209] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Chris Dumez
<cdumez at apple.com>'s request for review:
Bug 137381: Use is<>() / downcast<>() for CSSValue subclasses
https://bugs.webkit.org/show_bug.cgi?id=137381

Attachment 239209: Patch
https://bugs.webkit.org/attachment.cgi?id=239209&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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>(csstoLengthConversionDataWithTextZoomFacto
r(*styleResolver));
> +	   } else if (primitiveValue.isPercentage()) {

Coding style!!!

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:1557
> +	   else if (primitiveValue.isLength()) {
> +	       wordSpacing =
primitiveValue.computeLength<Length>(csstoLengthConversionDataWithTextZoomFacto
r(*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