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

Wed Apr 27 01:48:04 PDT 2011

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

--- Comment #12 from Maciej Stachowiak <mjs at apple.com>  2011-04-27 01:48:03 PST ---
(From update of attachment 91221)
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.

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