[webkit-reviews] review granted: [Bug 68944] REGRESSION(80893): HTML5 spec takes 2s longer to load due to time spent in CSSStyleSelector : [Attachment 111716] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Luke Macpherson
<macpherson at chromium.org>'s request for review:
Bug 68944: REGRESSION(80893): HTML5 spec takes 2s longer to load due to time
spent in CSSStyleSelector
https://bugs.webkit.org/show_bug.cgi?id=68944

Attachment 111716: Patch
https://bugs.webkit.org/attachment.cgi?id=111716&action=review

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
View in context: https://bugs.webkit.org/attachment.cgi?id=111716&action=review


Looks like a good improvement. r=me

> Source/WebCore/css/CSSStyleApplyProperty.cpp:275
> +template <Length (RenderStyle::*getterFunction)() const,
> +	     void (RenderStyle::*setterFunction)(Length),
> +	     Length (*initialFunction)(),
> +	     LengthAuto autoEnabled = AutoDisabled,
>	     LengthIntrinsic intrinsicEnabled = IntrinsicDisabled,
>	     LengthMinIntrinsic minIntrinsicEnabled = MinIntrinsicDisabled,
>	     LengthNone noneEnabled = NoneDisabled,
>	     LengthUndefined noneUndefined = UndefinedDisabled,
>	     LengthFlexDirection flexDirection = FlexDirectionDisabled>
> -class ApplyPropertyLength : public ApplyPropertyDefaultBase<Length> {
> +class ApplyPropertyLength {
>  public:
> -    ApplyPropertyLength(GetterFunction getter, SetterFunction setter,
InitialFunction initial)
> -	   : ApplyPropertyDefaultBase<Length>(getter, setter, initial)
> -    {
> -    }
> -
> -private:
> -    virtual void applyValue(CSSStyleSelector* selector, CSSValue* value)
const
> +    static void setValue(RenderStyle* style, Length value) {
(style->*setterFunction)(value); }

Have you verified that the function pointers actually get optimized away and
the RenderStyle setter functions inline using this syntax?

> Source/WebCore/css/CSSStyleApplyProperty.cpp:1019
> +    setPropertyHandler(CSSPropertyPaddingTop,
ApplyPropertyLength<&RenderStyle::paddingTop, &RenderStyle::setPaddingTop,
&RenderStyle::initialPadding>::createHandler());
> +    setPropertyHandler(CSSPropertyPaddingRight,
ApplyPropertyLength<&RenderStyle::paddingRight, &RenderStyle::setPaddingRight,
&RenderStyle::initialPadding>::createHandler());
> +    setPropertyHandler(CSSPropertyPaddingBottom,
ApplyPropertyLength<&RenderStyle::paddingBottom,
&RenderStyle::setPaddingBottom,
&RenderStyle::initialPadding>::createHandler());

It would be nice to refactor this in the future so that it used a static table
as input. This is pretty unreadable.