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

Chris Dumez
Reported 2015-01-04 14:35:48 PST
Get rid of some unnecessary custom StyleBuilder code and leverage the StyleBuilder generator more.
Attachments
Patch (14.61 KB, patch)
2015-01-04 15:12 PST, Chris Dumez
no flags
Patch (13.68 KB, patch)
2015-01-04 16:11 PST, Chris Dumez
no flags
Patch (13.48 KB, patch)
2015-01-04 22:02 PST, Chris Dumez
no flags
Chris Dumez
Comment 1 2015-01-04 15:12:33 PST
Chris Dumez
Comment 2 2015-01-04 16:11:43 PST
Darin Adler
Comment 3 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?
Chris Dumez
Comment 4 2015-01-04 22:02:58 PST
Chris Dumez
Comment 5 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.
WebKit Commit Bot
Comment 6 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>
WebKit Commit Bot
Comment 7 2015-01-05 10:29:52 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.