Bug 113292

Summary: Move box-shadow/webkit-box-shadow/text-shadow code to StyleBuilder
Product: WebKit Reporter: Emil A Eklund <eae>
Component: CSSAssignee: Emil A Eklund <eae>
Status: RESOLVED CONFIGURATION CHANGED    
Severity: Normal CC: allan.jensen, bfulgham, dominicc, esprehn+autocc, koivisto, macpherson, menard, ojan.autocc, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 89879    
Attachments:
Description Flags
Patch koivisto: review-, koivisto: commit-queue-

Emil A Eklund
Reported 2013-03-26 03:46:29 PDT
Move the CSSPropertyBoxShadow, CSSPropertyTextShadow and CSSPropertyWebkitBoxShadow applying logic out of StyleResolver::applyProperty.
Attachments
Patch (11.51 KB, patch)
2013-03-26 03:51 PDT, Emil A Eklund 		koivisto: review-
koivisto: commit-queue-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Emil A Eklund
Comment 1 2013-03-26 03:51:54 PDT
Created attachment 195053 [details] Patch
Antti Koivisto
Comment 2 2013-03-27 05:46:08 PDT
The refactored code is less understandable than the current one. It would be nice to refactor StyleBuilder to be more sane before moving anything more there.
Emil A Eklund
Comment 3 2013-03-27 06:23:58 PDT
(In reply to comment #2) > The refactored code is less understandable than the current one. It would be nice to refactor StyleBuilder to be more sane before moving anything more there. Is there anything in particular you find harder to understand or just the general concept of using templates this way? I'll gladly change it to make it easier to understand but knowing what you find hard to follow would makes that a lot easier. Thanks.
Antti Koivisto
Comment 4 2013-03-29 07:16:42 PDT
This factors a relatively straightforward switch branch into something that involves templates and function pointers without any obvious gain. How is this a progression?
Antti Koivisto
Comment 5 2013-03-29 07:21:24 PDT
An obvious first step for cleaning this up would be to move StyleResolver::applyProperty and pals to StyleBuilder. Then we could start figuring out how to have a one sane path of doing this.
Allan Sandfeld Jensen
Comment 6 2013-04-04 05:50:13 PDT
Comment on attachment 195053 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195053&action=review > Source/WebCore/css/StyleBuilder.cpp:1274 > + static void applyInheritValue(CSSPropertyID, StyleResolver* styleResolver) > + { > + const ShadowData* shadowData = (styleResolver->parentStyle()->*getterFunction)(); > + (styleResolver->style()->*setterFunction)(shadowData ? adoptPtr(new ShadowData(*shadowData)) : nullptr, 0 /* add */); > + } I agree with antti. High order functions are cool and all, but it doesn't really make it any cleaner here. These methods all already have the CSSPropertyID, so you could use that to select the methods like the original code.
Brent Fulgham
Comment 7 2022-07-13 14:29:34 PDT
This code has been significantly refactored since this patch was proposed. There doesn't seem to be any action we can take here.
Note You need to log in before you can comment on or make changes to this bug.