RESOLVED FIXED 59414
Implement CSS border width and related properties in CSSStyleApplyProperty.
https://bugs.webkit.org/show_bug.cgi?id=59414
Summary Implement CSS border width and related properties in CSSStyleApplyProperty.
Luke Macpherson
Reported 2011-04-25 20:58:07 PDT
Implement CSS border width and related properties in CSSStyleApplyProperty.
Attachments
Patch (10.59 KB, patch)
2011-04-25 21:05 PDT, Luke Macpherson
no flags
Patch (14.27 KB, patch)
2011-04-26 20:16 PDT, Luke Macpherson
no flags
Patch (11.41 KB, patch)
2011-04-27 17:44 PDT, Luke Macpherson
no flags
Patch (12.60 KB, patch)
2011-04-27 18:35 PDT, Luke Macpherson
no flags
Luke Macpherson
Comment 1 2011-04-25 21:05:21 PDT
Luke Macpherson
Comment 2 2011-04-25 21:07:07 PDT
May require a manual merge after https://bugs.webkit.org/show_bug.cgi?id=59314 goes in.
Eric Seidel (no email)
Comment 3 2011-04-25 21:13:32 PDT
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
Comment 4 2011-04-25 21:25:29 PDT
CSSStyleSelector was actually big in terms of generated binary code size (i.e. .o file size) too.
Luke Macpherson
Comment 5 2011-04-25 21:29:52 PDT
(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
Comment 6 2011-04-25 21:32:28 PDT
HTMLTokenizer::nextToken is the entire tokenizer flattened to a single symbol. (We use goto instead of normal functions calls for performance.)
Darin Adler
Comment 7 2011-04-26 09:43:34 PDT
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
Comment 8 2011-04-26 10:51:00 PDT
(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 9 2011-04-26 19:27:39 PDT
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
Comment 10 2011-04-26 20:16:44 PDT
Nikolas Zimmermann
Comment 11 2011-04-27 01:42:20 PDT
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 12 2011-04-27 01:48:03 PDT
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
Comment 13 2011-04-27 01:50:23 PDT
(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)
Comment 14 2011-04-27 07:02:34 PDT
(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
Comment 15 2011-04-27 15:25:42 PDT
(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
Comment 16 2011-04-27 16:04:37 PDT
(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
Comment 17 2011-04-27 17:44:11 PDT
Luke Macpherson
Comment 18 2011-04-27 17:45:16 PDT
Last patch is just a merge required after https://bugs.webkit.org/show_bug.cgi?id=59623 went in.
Luke Macpherson
Comment 19 2011-04-27 18:35:27 PDT
Luke Macpherson
Comment 20 2011-04-27 19:18:21 PDT
(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
Comment 21 2011-04-27 23:40:35 PDT
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
Comment 22 2011-04-28 16:46:21 PDT
Who, if anyone, is actually going to r+/cq+ this patch?
Eric Seidel (no email)
Comment 23 2011-04-28 16:53:52 PDT
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
Comment 24 2011-04-28 19:23:33 PDT
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.
WebKit Commit Bot
Comment 25 2011-04-28 19:25:26 PDT
Comment on attachment 91402 [details] Patch Clearing flags on attachment: 91402 Committed r85289: <http://trac.webkit.org/changeset/85289>
WebKit Commit Bot
Comment 26 2011-04-28 19:25:33 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.