[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 #20 from Eric Seidel <eric at webkit.org>  2011-02-24 16:26:34 PST ---
(From update of attachment 83735)
View in context: https://bugs.webkit.org/attachment.cgi?id=83735&action=review

> Source/WebCore/ChangeLog:9
> +
> +        No new tests. (OOPS!)
> +

You should talk more about why you're doing this change in the ChangeLog.  What's the benifit?  Is there a perf win here?  Are we just making CSSStyleSelector more readable?

You noted over IRC that this is only the first part of the changes.  You should mention that in the ChangeLog.

You should also replace the "no new tests" boiler plate with something like "This is already covered by many LayoutTests." or similar.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:58
> +    int property() const { return m_property; }

I know I mentioned this elsewhere, but it would be great if this could be an enum instead of an int.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:85
> +        (instance->style()->*m_setter)((instance->parentStyle()->*m_getter)());

I'm not sure I understand why we're calling the function pointers directly here.  Why wouldn't these be wrapped as functions on CSSStyleSelector.  So we could just call something like:
instance->styleSetter(instance->parentStyleGetter())?  I'm not sure why these are function pointers.  Maybe I'm just confused. :)  Maybe this needs more explanation in the ChangeLog. :)

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