[webkit-reviews] review granted: [Bug 185809] Modernize RenderStyleConstants.h - Part 1 : [Attachment 340811] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <utatane.tea at gmail.com> has granted Sam Weinig <sam at webkit.org>'s
request for review:
Bug 185809: Modernize RenderStyleConstants.h - Part 1
https://bugs.webkit.org/show_bug.cgi?id=185809

Attachment 340811: Patch

https://bugs.webkit.org/attachment.cgi?id=340811&action=review




--- Comment #6 from Yusuke Suzuki <utatane.tea at gmail.com> ---
Comment on attachment 340811
  --> https://bugs.webkit.org/attachment.cgi?id=340811
Patch

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

r=me, nice! Code becomes significantly readable.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3635
> -	       if (style.aspectRatioType() == AspectRatioAuto)
> +	       if (style.aspectRatioType() == AspectRatioType::Auto)
>		   return cssValuePool.createIdentifierValue(CSSValueAuto);
> -	       if (style.aspectRatioType() == AspectRatioFromDimensions)
> +	       if (style.aspectRatioType() == AspectRatioType::FromDimensions)
>		   return
cssValuePool.createIdentifierValue(CSSValueFromDimensions);
> -	       if (style.aspectRatioType() == AspectRatioFromIntrinsic)
> +	       if (style.aspectRatioType() == AspectRatioType::FromIntrinsic)
>		   return
cssValuePool.createIdentifierValue(CSSValueFromIntrinsic);

How about using `switch`?

> Source/WebCore/css/CSSPrimitiveValueMappings.h:319
> +    return (BorderStyle)(m_value.valueID - CSSValueNone);

Use `static_cast<>`

> Source/WebCore/rendering/HitTestResult.cpp:280
> +		   if (block.style().textOverflow() == TextOverflow::Ellipsis)
{

I like these changes.

> Source/WebCore/rendering/RenderBlockFlow.cpp:2555
>      default:

This `default:` can be `case Clear::None:`.

> Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:521
> +		   default: // BoxAlignment::Start

Does this include `BoxAlignment::Strech`? If so, adding a comment would be
nice.

> Source/WebCore/rendering/RenderElement.cpp:266
> +StyleDifference RenderElement::adjustStyleDifference(StyleDifference diff,
OptionSet<StyleDifferenceContextSensitiveProperty> contextSensitiveProperties)
const

Nice.