Bug 59414 - Implement CSS border width and related properties in CSSStyleApplyProperty.
Summary: Implement CSS border width and related properties in CSSStyleApplyProperty.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 59314
  Show dependency treegraph
 
Reported: 2011-04-25 20:58 PDT by Luke Macpherson
Modified: 2011-04-28 19:25 PDT (History)
9 users (show)

See Also:


Attachments
Patch (10.59 KB, patch)
2011-04-25 21:05 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch (14.27 KB, patch)
2011-04-26 20:16 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch (11.41 KB, patch)
2011-04-27 17:44 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch (12.60 KB, patch)
2011-04-27 18:35 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Luke Macpherson 2011-04-25 20:58:07 PDT
Implement CSS border width and related properties in CSSStyleApplyProperty.
Comment 1 Luke Macpherson 2011-04-25 21:05:21 PDT
Created attachment 91049 [details]
Patch
Comment 2 Luke Macpherson 2011-04-25 21:07:07 PDT
May require a manual merge after https://bugs.webkit.org/show_bug.cgi?id=59314 goes in.
Comment 3 Eric Seidel (no email) 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. :)
Comment 4 Nico Weber 2011-04-25 21:25:29 PDT
CSSStyleSelector was actually big in terms of generated binary code size (i.e. .o file size) too.
Comment 5 Luke Macpherson 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.
Comment 6 Adam Barth 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.)
Comment 7 Darin Adler 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.
Comment 8 Nikolas Zimmermann 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...
Comment 9 Luke Macpherson 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.
Comment 10 Luke Macpherson 2011-04-26 20:16:44 PDT
Created attachment 91221 [details]
Patch
Comment 11 Nikolas Zimmermann 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...
Comment 12 Maciej Stachowiak 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.
Comment 13 Maciej Stachowiak 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.
Comment 14 Dimitri Glazkov (Google) 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.
Comment 15 Luke Macpherson 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.
Comment 16 Luke Macpherson 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
Comment 17 Luke Macpherson 2011-04-27 17:44:11 PDT
Created attachment 91391 [details]
Patch
Comment 18 Luke Macpherson 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.
Comment 19 Luke Macpherson 2011-04-27 18:35:27 PDT
Created attachment 91402 [details]
Patch
Comment 20 Luke Macpherson 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.
Comment 21 Luke Macpherson 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.
Comment 22 Luke Macpherson 2011-04-28 16:46:21 PDT
Who, if anyone, is actually going to r+/cq+ this patch?
Comment 23 Eric Seidel (no email) 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.
Comment 24 WebKit Commit Bot 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.
Comment 25 WebKit Commit Bot 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>
Comment 26 WebKit Commit Bot 2011-04-28 19:25:33 PDT
All reviewed patches have been landed.  Closing bug.