[Webkit-unassigned] [Bug 59414] Implement CSS border width and related properties in CSSStyleApplyProperty.

[bugzilla-daemon at webkit.org]

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





--- Comment #20 from Luke Macpherson <macpherson at chromium.org>  2011-04-27 19:18:21 PST ---
(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.

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