Bug 140058

Summary: Get rid of some unnecessary custom StyleBuilder code
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: CSSAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, kling, koivisto, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Chris Dumez 2015-01-04 14:35:48 PST
Get rid of some unnecessary custom StyleBuilder code and leverage the StyleBuilder generator more.
Comment 1 Chris Dumez 2015-01-04 15:12:33 PST
Created attachment 243941 [details]
Patch
Comment 2 Chris Dumez 2015-01-04 16:11:43 PST
Created attachment 243947 [details]
Patch
Comment 3 Darin Adler 2015-01-04 21:06:48 PST
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?
Comment 4 Chris Dumez 2015-01-04 22:02:58 PST
Created attachment 243963 [details]
Patch
Comment 5 Chris Dumez 2015-01-04 22:51:23 PST
(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 6 WebKit Commit Bot 2015-01-05 10:29:47 PST
Comment on attachment 243963 [details]
Patch

Clearing flags on attachment: 243963

Committed r177915: <http://trac.webkit.org/changeset/177915>
Comment 7 WebKit Commit Bot 2015-01-05 10:29:52 PST
All reviewed patches have been landed.  Closing bug.