Bug 140058 – Get rid of some unnecessary custom StyleBuilder code

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

[Chris Dumez]

Get rid of some unnecessary custom StyleBuilder code and leverage the StyleBuilder generator more.

[Chris Dumez]

Created attachment 243941 [details]
Patch

[Chris Dumez]

Created attachment 243947 [details]
Patch

[Darin Adler]

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?

[Chris Dumez]

Created attachment 243963 [details]
Patch

[Chris Dumez]

(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.

[WebKit Commit Bot]

Comment on attachment 243963 [details]
Patch

Clearing flags on attachment: 243963

Committed r177915: <http://trac.webkit.org/changeset/177915>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.