Bug 59414 – Implement CSS border width and related properties in CSSStyleApplyProperty.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Luke Macpherson]

Implement CSS border width and related properties in CSSStyleApplyProperty.

[Luke Macpherson]

Created attachment 91049 [details]
Patch

[Luke Macpherson]

May require a manual merge after https://bugs.webkit.org/show_bug.cgi?id=59314 goes in.

[Eric Seidel (no email)]

Luke, you might be interesting to know that Nico (thakis) brought to my attention today, that CSSStyleSelector.cpp is one of the largest files in WebCore (in terms of source fed to the compiler) due to all the macros it uses.  This re-working your doing may help that. :)

[Nico Weber]

CSSStyleSelector was actually big in terms of generated binary code size (i.e. .o file size) too.

[Luke Macpherson]

(In reply to comment #3)
> Luke, you might be interesting to know that Nico (thakis) brought to my attention today, that CSSStyleSelector.cpp is one of the largest files in WebCore (in terms of source fed to the compiler) due to all the macros it uses.  This re-working your doing may help that. :)

Yeah, apparently CSSStyleSelector::applyProperty is the largest WebKit symbol in the Chromium build by a fair margin - almost twice the size of the next largest which is HTMLTokenizer::nextToken.

[Adam Barth]

HTMLTokenizer::nextToken is the entire tokenizer flattened to a single symbol.  (We use goto instead of normal functions calls for performance.)

[Darin Adler]

Comment on attachment 91049 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=91049&action=review

Do we have some performance testing results for this change?

> Source/WebCore/css/CSSStyleApplyProperty.cpp:87
> +    ApplyPropertyDefaultBase(T (RenderStyle::*getter)() const, void (RenderStyle::*setter)(T), T (*initial)())

These pointer-to-member-function and function types are a little hard to read. Maybe we could use typedefs instead of putting the type here directly to make the code easier to read.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:104
> +    virtual void applyValue(CSSStyleSelector*, CSSValue*) const {}

Can this be a pure virtual function instead? Is the empty version of the function ever helpful?

> Source/WebCore/css/CSSStyleApplyProperty.cpp:121
> +    virtual void applyValue(CSSStyleSelector* selector, CSSValue* value) const

I normally prefer to make a function like this private since it's only intended to be called through the base class. Making it private potentially catches virtual function calls that could instead be more-direct calls.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:124
> +            (selector->style()->*ApplyPropertyDefaultBase<T>::m_setter)(*(static_cast<CSSPrimitiveValue*>(value)));

The extra set of parentheses here around the static_cast makes this slightly harder to read.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:328
> +        if (width >= 0)

I think it would be worthwhile to have a “why” comment about this, although it looks like you just moved the code.

Also, this if could go inside the CSSValueInvalid case of the switch rather than out here.

[Nikolas Zimmermann]

(In reply to comment #7)
> (From update of attachment 91049 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91049&action=review
> 
> Do we have some performance testing results for this change?
> 
> > Source/WebCore/css/CSSStyleApplyProperty.cpp:87
> > +    ApplyPropertyDefaultBase(T (RenderStyle::*getter)() const, void (RenderStyle::*setter)(T), T (*initial)())
> 
> These pointer-to-member-function and function types are a little hard to read. Maybe we could use typedefs instead of putting the type here directly to make the code easier to read.
> 
> > Source/WebCore/css/CSSStyleApplyProperty.cpp:104
> > +    virtual void applyValue(CSSStyleSelector*, CSSValue*) const {}
> 
> Can this be a pure virtual function instead? Is the empty version of the function ever helpful?

Just for the record, I asked for these things (typedef) after CSSStyleApplyProperty has landed nearly 2 months ago :(
Luke, could you make those changes? I'm happy to review the patch...

[Luke Macpherson]

Comment on attachment 91049 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=91049&action=review

>>> Source/WebCore/css/CSSStyleApplyProperty.cpp:87
>>> +    ApplyPropertyDefaultBase(T (RenderStyle::*getter)() const, void (RenderStyle::*setter)(T), T (*initial)())
>> 
>> These pointer-to-member-function and function types are a little hard to read. Maybe we could use typedefs instead of putting the type here directly to make the code easier to read.
> 
> Just for the record, I asked for these things (typedef) after CSSStyleApplyProperty has landed nearly 2 months ago :(
> Luke, could you make those changes? I'm happy to review the patch...

Done. Previously these types were not used repeatedly and so it didn't make sense to typedef them. Now that these types are used in many places it does.

>> Source/WebCore/css/CSSStyleApplyProperty.cpp:104
>> +    virtual void applyValue(CSSStyleSelector*, CSSValue*) const {}
> 
> Can this be a pure virtual function instead? Is the empty version of the function ever helpful?

Removed.

>> Source/WebCore/css/CSSStyleApplyProperty.cpp:121
>> +    virtual void applyValue(CSSStyleSelector* selector, CSSValue* value) const
> 
> I normally prefer to make a function like this private since it's only intended to be called through the base class. Making it private potentially catches virtual function calls that could instead be more-direct calls.

Done everywhere.

>> Source/WebCore/css/CSSStyleApplyProperty.cpp:124
>> +            (selector->style()->*ApplyPropertyDefaultBase<T>::m_setter)(*(static_cast<CSSPrimitiveValue*>(value)));
> 
> The extra set of parentheses here around the static_cast makes this slightly harder to read.

Done.

>> Source/WebCore/css/CSSStyleApplyProperty.cpp:328
>> +        if (width >= 0)
> 
> I think it would be worthwhile to have a “why” comment about this, although it looks like you just moved the code.
> 
> Also, this if could go inside the CSSValueInvalid case of the switch rather than out here.

Yes, just moved code. Added comment to reflect that negative border widths are invalid. I've moved the < 0 check into the switch statement as suggested.

[Luke Macpherson]

Created attachment 91221 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 91221 [details]
Patch

It's confusing to have ApplyPropertyDefault and DefaultBase, why the distinction?
The same question applies to all of the existing classes in CSSStyleApplyProperty..
Why have ApplyPropertColor -> ApplyPropertyColorBase -> ApplyPropertyBase - I don't see the need for the extra level FooBarBase.
Can we please focus first on cleaning up CSSStyleApplyProperty, before adding new classes...

[Maciej Stachowiak]

Comment on attachment 91221 [details]
Patch

I'm not sure if the direction of this refactoring is making the code easier to understand and modify. The roles of the different classes and their inheritence relationships are pretty unclear. I think that when you have a whole family of Foo and FooBase classes, that in itself is an indication of an anti-pattern - "Base" doesn't really explain how the role of the other class is different. Looking through both the patch and the existing code, for example, it seems that nothing inherits from ApplyPropertyColorBase other than ApplyPropertyColor. Only one place actually instantiates an ApplyPropertyColor instead of ApplyPropertyColorBase, and it is mysterious in the code why.

Also, it's dynamically allocated, but it doesn't use the create() pattern or smart pointers to get the memory management right; and (b) it looks like a simple value object so it may be surprising that we do dynamic allocations for this.

Finally, when it is dynamically allocated, it is passed to a function setPropertyValue which does not in fact set a property value; rather it records a setter object in a hashtable for later use in actually setting values.

Maybe it is time to step back and see if WebCore CSS hackers actually find all of these changes to be an improvement in clarity.

[Maciej Stachowiak]

(In reply to comment #7)
> (From update of attachment 91049 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91049&action=review
> 
> Do we have some performance testing results for this change?

BTW it looks like this question from an earlier rev of the patch was never answered.

[Dimitri Glazkov (Google)]

(In reply to comment #13)
> (In reply to comment #7)
> > (From update of attachment 91049 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=91049&action=review
> > 
> > Do we have some performance testing results for this change?
> 
> BTW it looks like this question from an earlier rev of the patch was never answered.

Maciej, there has been extensive talking with CSS hacker and performance testing done earlier. See bug 54707 for details. In summary, anttik, smfr, and dhyatt endorsed the approach. Anttik made Luke test the crap of the new approach to ensure no performance degradation.

[Luke Macpherson]

(In reply to comment #11)
> (From update of attachment 91221 [details])
> It's confusing to have ApplyPropertyDefault and DefaultBase, why the distinction?
> The same question applies to all of the existing classes in CSSStyleApplyProperty..
> Why have ApplyPropertColor -> ApplyPropertyColorBase -> ApplyPropertyBase - I don't see the need for the extra level FooBarBase.
> Can we please focus first on cleaning up CSSStyleApplyProperty, before adding new classes...

The reason is that ApplyPropertyBase casts a CSSPrimitiveValue to T in applyValue(). That is not always a valid thing to do, so the compiler complains. That is the reason for the separation in this case. I would have liked to avoid it, but it wasn't possible in this case. This was briefly explained in the changelog entry.

[Luke Macpherson]

(In reply to comment #15)
> (In reply to comment #11)
> > (From update of attachment 91221 [details] [details])
> > It's confusing to have ApplyPropertyDefault and DefaultBase, why the distinction?
> > The same question applies to all of the existing classes in CSSStyleApplyProperty..
> > Why have ApplyPropertColor -> ApplyPropertyColorBase -> ApplyPropertyBase - I don't see the need for the extra level FooBarBase.
> > Can we please focus first on cleaning up CSSStyleApplyProperty, before adding new classes...
> 
> The reason is that ApplyPropertyBase casts a CSSPrimitiveValue to T in applyValue(). That is not always a valid thing to do, so the compiler complains. That is the reason for the separation in this case. I would have liked to avoid it, but it wasn't possible in this case. This was briefly explained in the changelog entry.

s/ApplyPropertyBase/ApplyPropertyDefaultValue

[Luke Macpherson]

Created attachment 91391 [details]
Patch

[Luke Macpherson]

Last patch is just a merge required after https://bugs.webkit.org/show_bug.cgi?id=59623 went in.

[Luke Macpherson]

Created attachment 91402 [details]
Patch

[Luke Macpherson]

(In reply to comment #12)
> (From update of attachment 91221 [details])
> I'm not sure if the direction of this refactoring is making the code easier to understand and modify. The roles of the different classes and their inheritence relationships are pretty unclear. I think that when you have a whole family of Foo and FooBase classes, that in itself is an indication of an anti-pattern - "Base" doesn't really explain how the role of the other class is different. Looking through both the patch and the existing code, for example, it seems that nothing inherits from ApplyPropertyColorBase other than ApplyPropertyColor. Only one place actually instantiates an ApplyPropertyColor instead of ApplyPropertyColorBase, and it is mysterious in the code why.

The ApplyPropertyColor/ApplyPropertyColorBase issue has been fixed already. The ApplyPropertyDefault/ApplyPropertyDefaultBas issues is a bit more annoying - because the behavior of applyProperty is to do a static cast from primitiveValue to the template type T. This can't work for some types. You could probably use a dynamic cast, but I expect that has higher runtime cost than adding to the class hierarchy. Still, you shouldn't read this as a whole family of x/xBase classes, that's just my lack of naming imagination at work.

> Also, it's dynamically allocated, but it doesn't use the create() pattern or smart pointers to get the memory management right; and (b) it looks like a simple value object so it may be surprising that we do dynamic allocations for this.

CSSStyleApplyProperty itself is a statically allocated singleton.

> Finally, when it is dynamically allocated, it is passed to a function setPropertyValue which does not in fact set a property value; rather it records a setter object in a hashtable for later use in actually setting values.

Yes, it is poorly named. That isn't terribly hard to fix fortunately.

> Maybe it is time to step back and see if WebCore CSS hackers actually find all of these changes to be an improvement in clarity.

[Luke Macpherson]

Well, I spent the day trying to find a way to use template specialization to remove the ApplyPropertyDefault/ApplyPropertyDefaultBase distinction, but couldn't find anything more succinct than was already there. Open to suggestions if someone with better template foo can see an elegant solution.

[Luke Macpherson]

Who, if anyone, is actually going to r+/cq+ this patch?

[Eric Seidel (no email)]

Comment on attachment 91402 [details]
Patch

I don't understand the default/base stuff.  But I think this is still moving us in the right direction.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 91402 [details]:

http/tests/inspector/resource-tree/resource-tree-frame-add.html bug 59771 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 91402 [details]
Patch

Clearing flags on attachment: 91402

Committed r85289: <http://trac.webkit.org/changeset/85289>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.