[Webkit-unassigned] [Bug 54707] Introduce lookup-table based approach for applying CSS properties.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=54707





--- Comment #60 from Luke Macpherson <macpherson at chromium.org>  2011-04-10 16:50:07 PST ---
I didn't want to reply earlier because I didn't really know what to say - I'm pretty sure it will come off as excuses to avoid work, but let me assure you that isn't the case, and humbly offer the reasons why I didn't follow all of these suggestions.

(In reply to comment #52)
> (From update of attachment 85506 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85506&action=review
> 
> As the idea is to make the code more readable, I have a few suggestions:
> 
> > Source/WebCore/css/CSSStyleApplyProperty.cpp:48
> > +    ApplyPropertyDefault(T (RenderStyle::*getter)() const, void (RenderStyle::*setter)(T), T (*initial)())
> 
> Use typedefs to avoid repeating the function pointers.
> 
> typedef T (RenderStyle::*GetterMethod)() const;
> typedef T void (RenderStyle::*SetterMethod)(T);
> typedef T (*initialMethod)();
> 
> ..
> protected:
> GetterMethod m_getter;
> ..

I don't think this buys much other than obfuscating the types, since these typedefs would only be used approximately twice, and need to be defined in every class. I acknowledge this as an aesthetic preference, however my previous experience has been that proliferation of typedefs was a net negative in code readability.

> > Source/WebCore/css/CSSStyleApplyProperty.cpp:57
> > +    {
> > +        (selector->style()->*m_setter)((selector->parentStyle()->*m_getter)());
> 
> While you're at it: I don't think ASSERTIONS() will hurt here.

It is not clear what exactly you would want to assert here. Assertions are useful to early-out if an unexpected condition occurs, however in this case the only unexpected condition will immediately result in a null pointer dereference, so no additional assertion is necessary.

> > Source/WebCore/css/CSSStyleApplyProperty.cpp:65
> > +    virtual void value(CSSStyleSelector* selector, CSSValue* value) const
> 
> Why is this named, value, instead of setValue? Or even better, I think, you should have used:
> applyInheritValue() (maybe s/Value//)
> applyInitialValue() (ditto)
> applyValue()

Ok, expect a patch for this shortly.

> > Source/WebCore/css/CSSStyleApplyProperty.cpp:67
> > +        if (value->isPrimitiveValue())
> 
> Early return style is prefereed:
> if (!value->isPrimitiveValue())
>     return;

I had discussed this with two other reviewers who both agreed that early return was not preferred for functions that contain a single short if statement.

> > Source/WebCore/css/CSSStyleApplyProperty.cpp:78
> > +// CSSPropertyColor
> > +class ApplyPropertyColorBase : public ApplyPropertyBase {
> 
> I don't think this is a good idea at all. It should go into a new file. Otherwhise your CSSStyleApplyProperty file will look just as cluttered as CSSStyleSelector is now.

My thoughts were along the lines of smfr's comment - Having this logic spread over a large number of files may well make it less readable, especially since this classes are effectively private to CSSStyleApplyProperty. There may not be one class per property, but there will be quite a few when this refactoring is complete. I would be in favor of doing the refactoring with all the private command pattern classes in a single file, at which point we will know exactly how many there are, and how interrelated they are, and will be in a better position to decide on whether they should be split into separate files or not.

> > Source/WebCore/css/CSSStyleApplyProperty.cpp:80
> > +    ApplyPropertyColorBase(const Color& (RenderStyle::*getter)() const, const Color& (RenderStyle::*defaultValue)() const, void (RenderStyle::*setter)(const Color&))
> 
> Use typedefs for the function pointers.

See comment above.

> > Source/WebCore/css/CSSStyleApplyProperty.cpp:88
> > +        const Color& color = (selector->parentStyle()->*m_getter)();
> 
> I'd think ASSERTIONS would be helpful as well here.

See comment above.

> > Source/WebCore/css/CSSStyleApplyProperty.cpp:97
> > +        (selector->style()->*m_setter)(color);
> 
> Ditto.
> 
> > Source/WebCore/css/CSSStyleApplyProperty.cpp:102
> > +        if (value->isPrimitiveValue())
> > +            (selector->style()->*m_setter)(selector->getColorFromPrimitiveValue(static_cast<CSSPrimitiveValue*>(value)));
> 
> Ditto + Early return.
> 
> > Source/WebCore/css/CSSStyleApplyProperty.cpp:110
> > +class ApplyPropertyColor : public ApplyPropertyColorBase {
> 
> Why is this seperation needed ApplyPropertyColor and base? It's highly confusing IMHO.
> 
> Imagine how many classes we'd end up creating, when converting everything into your new approach.
> We should rather minimize the need of new classes.

All I can say is that this needs to be evaluated on a case-by-case basis. ApplyPropertyColor and ApplyPropertyColorBase only share applyInheritValue, the logic of this isn't something I came up with, it's just that the color css property has different behavior to other properties that take color values.

> > Source/WebCore/css/CSSStyleApplyProperty.cpp:149
> > +    setPropertyValue(CSSPropertyColor, new ApplyPropertyColor(&RenderStyle::color, &RenderStyle::setColor, RenderStyle::initialColor));
> 
> I'd rather prefer to move the setPropertyValue() calls in a new CSSStyleApplyPropertyColor.cpp file, in a static function.
> So that CSSStyleApplyProperty only ever has to look like this:
> CSSStyleApplyPropertyColor::initialize(this);
> CSSStyleApplyPropertyXXX::initialize(this);
> ...
> 
> For each of the groups of properties.

The reason not to do this is that CSSStyleSelector and RenderStyle, and possibly others would need to friend a ton of classes. By passing parameters from CSSStyleApplyProperty you keep all the friend nastiness confined to a single place.

> > Source/WebCore/css/CSSStyleApplyProperty.h:47
> > +class CSSStyleApplyProperty : public RefCounted<CSSStyleApplyProperty> {
> 
> Why is this refcounted if you plan to share a static single instance??

Fixed.

> > Source/WebCore/css/CSSStyleApplyProperty.h:51
> > +    void inherit(CSSPropertyID property, CSSStyleSelector* selector) const
> 
> Same renamings would be nice.

Will do, expect a CL soon.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.