Move 'font-size' CSS property to the new StyleBuilder
Created attachment 243833 [details] Patch
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.
Created attachment 243857 [details] Patch
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.
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?
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.
Created attachment 243904 [details] Patch
Comment on attachment 243904 [details] Patch Clearing flags on attachment: 243904 Committed r177866: <http://trac.webkit.org/changeset/177866>
All reviewed patches have been landed. Closing bug.