Bug 140010 – Move 'font-size' CSS property to the new StyleBuilder

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

[Chris Dumez]

Move 'font-size' CSS property to the new StyleBuilder

[Chris Dumez]

Created attachment 243833 [details]
Patch

[Darin Adler]

Comment on attachment 243833 [details]
Patch

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

My comments are mostly about the code you are moving.

> Source/WebCore/css/StyleBuilderCustom.h:1286
> +    fontDescription.setKeywordSize(CSSValueMedium - CSSValueXxSmall + 1);

It’s irritating that this trick of x - CSSValueXxSmall + 1 is done in at least two places without a comment. I’d rather have this be a helper function that does things like range checking, or even static asserts that all the values are consecutive.

> Source/WebCore/css/StyleBuilderCustom.h:1347
> +    float size = 0;

It would be nicer to define parentSize and parentIsAbsoluteSize right above the paragraph where they are set up, rather than having this and a blank line in between.

So we should declare size lower down in the function, after the if statement. Also, I don’t think we need to initialize it to zero; it’s set in every code path. Or we could initialize it to -1 and then we could get rid of the default/return and the else/return.

> Source/WebCore/css/StyleBuilderCustom.h:1377
> +        case CSSValueWebkitRubyText: {
> +            float rubyTextSizeMultiplier = determineRubyTextSizeMultiplier(styleResolver);
> +            size = rubyTextSizeMultiplier * parentSize;
> +            break;

This would read better without the local variable and the braces:

    size = determineRubyTextSizeMultiplier(styleResolver) * parentSize;

> Source/WebCore/css/StyleBuilderCustom.h:1382
> +        fontDescription.setIsAbsoluteSize(parentIsAbsoluteSize && (ident == CSSValueLarger || ident == CSSValueSmaller || ident == CSSValueWebkitRubyText));

It’s annoying that this is at the end the if clause but ...

> Source/WebCore/css/StyleBuilderCustom.h:1384
> +        fontDescription.setIsAbsoluteSize(parentIsAbsoluteSize || !(primitiveValue.isPercentage() || primitiveValue.isFontRelativeLength()));

... this is at the beginning of the else clause. They should be formatted in a consistent way.

> Source/WebCore/css/StyleBuilderCustom.h:1392
> +            Ref<CalculationValue> calculationValue { primitiveValue.cssCalcValue()->createCalculationValue(styleResolver.state().cssToLengthConversionData().copyWithAdjustedZoom(1.0f)) };
> +            size = calculationValue->evaluate(parentSize);

If you merged these into a single line of code, it would only be 5 characters longer than the first line here. I suggest we do it all in one line. Or if we really want to break it into two lines, I think we could choose a different subexpression, like the argument to createCalculationValue.

> Source/WebCore/css/StyleBuilderCustom.h:1401
> +    // Overly large font sizes will cause crashes on some platforms (such as Windows).
> +    // Cap font size here to make sure that doesn't happen.

I think this comment would be better up where the maximumAllowedFontSize constant is defined, rather than here where it’s used.

[Chris Dumez]

Created attachment 243857 [details]
Patch

[Chris Dumez]

Comment on attachment 243833 [details]
Patch

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

>> Source/WebCore/css/StyleBuilderCustom.h:1286
>> +    fontDescription.setKeywordSize(CSSValueMedium - CSSValueXxSmall + 1);
> 
> It’s irritating that this trick of x - CSSValueXxSmall + 1 is done in at least two places without a comment. I’d rather have this be a helper function that does things like range checking, or even static asserts that all the values are consecutive.

Ok, I added setKeywordSizeFromIdentifier() / keywordSizeAsIdentifier() setter / getter on FontDescription and refactored the code base to use them. Let me know if this looks OK.

>> Source/WebCore/css/StyleBuilderCustom.h:1347
>> +    float size = 0;
> 
> It would be nicer to define parentSize and parentIsAbsoluteSize right above the paragraph where they are set up, rather than having this and a blank line in between.
> 
> So we should declare size lower down in the function, after the if statement. Also, I don’t think we need to initialize it to zero; it’s set in every code path. Or we could initialize it to -1 and then we could get rid of the default/return and the else/return.

Done.

>> Source/WebCore/css/StyleBuilderCustom.h:1377
>> +            break;
> 
> This would read better without the local variable and the braces:
> 
>     size = determineRubyTextSizeMultiplier(styleResolver) * parentSize;

Done.

>> Source/WebCore/css/StyleBuilderCustom.h:1384
>> +        fontDescription.setIsAbsoluteSize(parentIsAbsoluteSize || !(primitiveValue.isPercentage() || primitiveValue.isFontRelativeLength()));
> 
> ... this is at the beginning of the else clause. They should be formatted in a consistent way.

Ok, done.

>> Source/WebCore/css/StyleBuilderCustom.h:1392
>> +            size = calculationValue->evaluate(parentSize);
> 
> If you merged these into a single line of code, it would only be 5 characters longer than the first line here. I suggest we do it all in one line. Or if we really want to break it into two lines, I think we could choose a different subexpression, like the argument to createCalculationValue.

Now on one line.

>> Source/WebCore/css/StyleBuilderCustom.h:1401
>> +    // Cap font size here to make sure that doesn't happen.
> 
> I think this comment would be better up where the maximumAllowedFontSize constant is defined, rather than here where it’s used.

There is already such comment there. I removed this comment here.

[Chris Dumez]

Comment on attachment 243857 [details]
Patch

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

> Source/WebCore/platform/graphics/FontDescription.h:128
> +    CSSValueID keywordSizeAsIdentifier() const

Darin, do the new setter and getter look OK?

[Darin Adler]

Comment on attachment 243857 [details]
Patch

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

>> Source/WebCore/platform/graphics/FontDescription.h:128
>> +    CSSValueID keywordSizeAsIdentifier() const
> 
> Darin, do the new setter and getter look OK?

Sure, that looks OK to me. I would have done the static_assert of each individual value myself, but overall looks good.

> Source/WebCore/platform/graphics/FontDescription.h:165
> +        ASSERT(size >= 0 && size <= 8);

All unsigned values are >= 0, so that half of the assert is pointless.

Separately, I would suggest that any time you write an assert with && you split it into two assertions instead.

[Chris Dumez]

Created attachment 243904 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 243904 [details]
Patch

Clearing flags on attachment: 243904

Committed r177866: <http://trac.webkit.org/changeset/177866>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.