Bug 113292 – Move box-shadow/webkit-box-shadow/text-shadow code to StyleBuilder

• Component: CSS
• Status: RESOLVED CONFIGURATION CHANGED

[Emil A Eklund]

Move the CSSPropertyBoxShadow, CSSPropertyTextShadow and CSSPropertyWebkitBoxShadow applying logic out of StyleResolver::applyProperty.

[Emil A Eklund]

Created attachment 195053 [details]
Patch

[Antti Koivisto]

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]

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

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]

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 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]

This code has been significantly refactored since this patch was proposed. There doesn't seem to be any action we can take here.