Implement CSS border width and related properties in CSSStyleApplyProperty.
Created attachment 91049 [details] Patch
May require a manual merge after https://bugs.webkit.org/show_bug.cgi?id=59314 goes in.
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. :)
CSSStyleSelector was actually big in terms of generated binary code size (i.e. .o file size) too.
(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.
HTMLTokenizer::nextToken is the entire tokenizer flattened to a single symbol. (We use goto instead of normal functions calls for performance.)
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.
(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...
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.
Created attachment 91221 [details] Patch
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...
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.
(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.
(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.
(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.
(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
Created attachment 91391 [details] Patch
Last patch is just a merge required after https://bugs.webkit.org/show_bug.cgi?id=59623 went in.
Created attachment 91402 [details] Patch
(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.
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.
Who, if anyone, is actually going to r+/cq+ this patch?
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.
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: pfeldman@chromium.org) The commit-queue is continuing to process your patch.
Comment on attachment 91402 [details] Patch Clearing flags on attachment: 91402 Committed r85289: <http://trac.webkit.org/changeset/85289>
All reviewed patches have been landed. Closing bug.