Bug 102844 – Refactor -webkit-filter to use StyleBuilder

• Component: CSS
• Status: RESOLVED INVALID

[Dirk Schulze]

-webkit-filter should use StyleBuilder like the other properties.

[Dirk Schulze]

Created attachment 175291 [details]
Patch

[Sam Weinig]

Comment on attachment 175291 [details]
Patch

This seems to be be quite a bit more code and harder to understand, how is that better?

[Dirk Schulze]

(In reply to comment #2)
> (From update of attachment 175291 [details])
> This seems to be be quite a bit more code and harder to understand, how is that better?

right, I tend to create smaller patches for refactoring then huge patches. It is still better because it is on the right place now and uses the same code path as the other properties. This gives a better overview.

[Sam Weinig]

Comment on attachment 175291 [details]
Patch

I don't think being "on the right place" is a good enough reason to make the code more complicated.  I would r+ just the test case though.

[Dirk Schulze]

(In reply to comment #4)
> (From update of attachment 175291 [details])
> I don't think being "on the right place" is a good enough reason to make the code more complicated.  I would r+ just the test case though.

Again, this is just one part of the refactoring. And putting things where they belong to is very well a good reason to move code. I would rather ask you why it shouldn't be on the right place?

[Tony Chang]

Comment on attachment 175291 [details]
Patch

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

> Source/WebCore/css/StyleBuilder.cpp:1721
> +template <const FilterOperations& (RenderStyle::*getterFunction)() const, void (RenderStyle::*setterFunction)(const FilterOperations&), const FilterOperations& (*initialFunction)()>
> +class ApplyPropertyFilter {

This would be easier to read without the template.  See some of the other classes like ApplyPropertyVerticalAlign.

> Source/WebCore/css/StyleBuilder.cpp:1723
> +    static void setValue(RenderStyle* style, const FilterOperations& value) { (style->*setterFunction)(value); }

You could inline setValue, there's one caller and it's only one line.

[Dirk Schulze]

(In reply to comment #6)
> (From update of attachment 175291 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175291&action=review
> 
> > Source/WebCore/css/StyleBuilder.cpp:1721
> > +template <const FilterOperations& (RenderStyle::*getterFunction)() const, void (RenderStyle::*setterFunction)(const FilterOperations&), const FilterOperations& (*initialFunction)()>
> > +class ApplyPropertyFilter {
> 
> This would be easier to read without the template.  See some of the other classes like ApplyPropertyVerticalAlign.
> 
> > Source/WebCore/css/StyleBuilder.cpp:1723
> > +    static void setValue(RenderStyle* style, const FilterOperations& value) { (style->*setterFunction)(value); }
> 
> You could inline setValue, there's one caller and it's only one line.

Sounds reasonable. I'll upload a new patch till monday.

[Dirk Schulze]

Created attachment 175484 [details]
Patch

[Antti Koivisto]

Comment on attachment 175484 [details]
Patch

I don't think we should move any more stuff to StyleBuilder until it is refactored to be sane.

[Dirk Schulze]

The original template patch was landed in bug 54707. This was 20 months ago. It had a long discussion afterwards, but was then closed more then a year ago without any further objections.

Since then more an more properties moved to this concept. Even if I don't care if we move all properties to a giant switch or to the template concept, it is a mess to have the properties spread out into two concepts. This should be fixed soon. When there was no reason to stop moving properties to the templates during the last 20 months, where do you see the reason to do it now?

It is a lot harder to read the code if there are two concepts, in two different files, then the template is on its own. Do you see any chance that the template gets cleaned up soon? Given the timeframe of the original bug, I don't. And it just looks that r- here blocks the general cleanup even more.

[Ojan Vafai]

While I'm not a huge fan of StyleBuilder looking at any single property, my experience adding new features has been that it's been easier to wrap my head around StyleBuilder since the code for the feature is more self-contained rather than spread out around many switch statements. It's easier to make sure you're covering all the necessary cases.

Also, for simple new properties that don't need special handling, StyleBuilder is quite a bit nicer. You just add one line to one place.

I agree with Dirk, we should just move everything over. It's not pretty, but it can be cleaned up later. Being half-way there is worse.

[Maciej Stachowiak]

(In reply to comment #11)
> While I'm not a huge fan of StyleBuilder looking at any single property, my experience adding new features has been that it's been easier to wrap my head around StyleBuilder since the code for the feature is more self-contained rather than spread out around many switch statements. It's easier to make sure you're covering all the necessary cases.
> 
> Also, for simple new properties that don't need special handling, StyleBuilder is quite a bit nicer. You just add one line to one place.
> 
> I agree with Dirk, we should just move everything over. It's not pretty, but it can be cleaned up later. Being half-way there is worse.

If the effects of StyleBuilder generally look like this patch, then in my opinion everything should be moved back out of it instead. And I have to admit looking now at StyleBuilder.cpp that it seems pretty hard to follow. Code should be optimized for reading, not for writing, so IMO readability is more relevant than ease adding new features. Maybe it is time to re-evaluate whether StyleBuilder has been a successful experiment in improving the code. It seems like it makes things more verbose and harder to follow, which is not a good result for a refactoring change.