Get rid of some unnecessary custom StyleBuilder code and leverage the StyleBuilder generator more.
Created attachment 243941 [details] Patch
Created attachment 243947 [details] Patch
Comment on attachment 243947 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=243947&action=review As usually, my comments are about the code you are moving, not the refactor itself. I didn’t set cq+ but I’m OK if you want to. > Source/WebCore/css/StyleBuilderConverter.h:898 > + // For backward compatibility, treat valueless numbers as px. > + Ref<CSSPrimitiveValue> value(CSSPrimitiveValue::create(primitiveValue.getDoubleValue(), CSSPrimitiveValue::CSS_PX)); > + perspective = value.get().computeLength<float>(styleResolver.state().cssToLengthConversionData()); I don’t like this syntax. I prefer the style with the '=' sign or the one with the {} instead of (). The local variable doesn’t add much. If it didn’t make the line crazy long I’d want to do this all in one line. Maybe a helper function could make that readable? Does this idiom come up elsewhere? The “forcing a value to be px” thing?
Created attachment 243963 [details] Patch
(In reply to comment #3) > Comment on attachment 243947 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=243947&action=review > > As usually, my comments are about the code you are moving, not the refactor > itself. I didn’t set cq+ but I’m OK if you want to. > > > Source/WebCore/css/StyleBuilderConverter.h:898 > > + // For backward compatibility, treat valueless numbers as px. > > + Ref<CSSPrimitiveValue> value(CSSPrimitiveValue::create(primitiveValue.getDoubleValue(), CSSPrimitiveValue::CSS_PX)); > > + perspective = value.get().computeLength<float>(styleResolver.state().cssToLengthConversionData()); > > I don’t like this syntax. I prefer the style with the '=' sign or the one > with the {} instead of (). > > The local variable doesn’t add much. If it didn’t make the line crazy long > I’d want to do this all in one line. Maybe a helper function could make that > readable? Does this idiom come up elsewhere? The “forcing a value to be px” > thing? Replaced with: perspective = primitiveValue.getDoubleValue() * styleResolver.state().cssToLengthConversionData().zoom(); Constructing a new CSSPrimitiveValue just for this seems wasteful. computeLength<>() will do just that if the value is in pixels.
Comment on attachment 243963 [details] Patch Clearing flags on attachment: 243963 Committed r177915: <http://trac.webkit.org/changeset/177915>
All reviewed patches have been landed. Closing bug.