Bug 59774 – Add template parameter to ApplyPropertyColor to improve clarity by removing constructor parameter side effects.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Luke Macpherson]

Add template parameter to ApplyPropertyColor to improve clarity by removing constructor parameter side effects.

[Luke Macpherson]

Created attachment 91629 [details]
Patch

[Antti Koivisto]

It would be better to use enum instead of a boolean as the parameter. ApplyPropertyColor<true> is pretty magical. I assume the same enum would be useful for other non-color properties too.

[Dimitri Glazkov (Google)]

Comment on attachment 91629 [details]
Patch

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

I like it, but a few nits:

> Source/WebCore/css/CSSStyleApplyProperty.cpp:132
> +template <bool inheritColorFromParent = false>

Can a template parameter be an enum, like enum InheritColor { InheritColorFromParent, WhateverTheOtherValue };

> Source/WebCore/css/CSSStyleApplyProperty.cpp:343
> +    setPropertyValue(CSSPropertyColor, new ApplyPropertyColor<true>(&RenderStyle::color, &RenderStyle::setColor, 0, RenderStyle::initialColor));

This way, you could write this as new ApplyPropertyColor<IneritColorFromParent> -- much more readable.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:404
> +    setPropertyValue(CSSPropertyBackgroundColor, new ApplyPropertyColor<>(&RenderStyle::backgroundColor, &RenderStyle::setBackgroundColor, 0));

And without the default value, this could look prettier too, explicitly explaining what's happening.

[Luke Macpherson]

Created attachment 91869 [details]
Patch

[Luke Macpherson]

Incorporated suggestions from Antti and Dimitri.

[WebKit Commit Bot]

Comment on attachment 91869 [details]
Patch

Rejecting attachment 91869 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'apply-..." exit_code: 2

Last 500 characters of output:
iffs from patch file(s).
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/css/CSSStyleApplyProperty.cpp
Hunk #3 succeeded at 386 (offset 45 lines).
Hunk #4 succeeded at 447 (offset 45 lines).
Hunk #5 FAILED at 465.
1 out of 5 hunks FAILED -- saving rejects to file Source/WebCore/css/CSSStyleApplyProperty.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Dimitri Glazkov', u'--..." exit_code: 1

Full output: http://queues.webkit.org/results/8531848

[Luke Macpherson]

Created attachment 92045 [details]
Patch

[Luke Macpherson]

Last patch is just a merge to resolve conflicts.

[Eric Seidel (no email)]

Comment on attachment 92045 [details]
Patch

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

I have some rather nebulous random nits/questions:

> Source/WebCore/css/CSSStyleApplyProperty.cpp:173
>              (selector->style()->*m_setter)(selector->getColorFromPrimitiveValue(primitiveValue));

I'm not a big fan of the direct use of function pointers.  But if this is the only use of m_setter, than maybe this is best. If we're using it more than once we should consider wrapping it in a helper to call m_setter for us.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:450
> +    setPropertyValue(CSSPropertyBackgroundColor, new ApplyPropertyColor<NoInheritFromParent>(&RenderStyle::backgroundColor, &RenderStyle::setBackgroundColor, 0));

I think we should consider giving the template a default value to inherit or not inherit, whichever is more common.

We should consider moving to the ::create() style as well at some point.

Are these OwnPtr's?  If so, we'll need to adopt() them to comply with strict own ptr rules.

We seem to have a bunch of repeated information between these lines.  Perhaps we can find a way to reduce the copy/paste so as to highlight the differences more?

> Source/WebCore/css/CSSStyleApplyProperty.cpp:468
> +    setPropertyValue(CSSPropertyOutlineColor, new ApplyPropertyColor<InheritFromParent>(&RenderStyle::outlineColor, &RenderStyle::setOutlineColor, &RenderStyle::color));

I should probably know this by now, but I assume we have a single table for all of these?  Should we be concerned about the extra heap allocations moving this out of static data?  I suspect that given how huge the previous macro-based solution was, this is probably still a memory size win.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:474
> +    setPropertyValue(CSSPropertyWebkitColumnRuleColor, new ApplyPropertyColor<NoInheritFromParent>(&RenderStyle::columnRuleColor, &RenderStyle::setColumnRuleColor, &RenderStyle::color));

Maybe we should consider moving the creation of these onto RenderStyle so we could get rid of the RenderStyle:: copy/paste.

I'm trying to envison what the ideal code line would look like.  We're trying to map from a CSSPropertyName to a set of functions to apply it to the RenderStyle, right?  Obviously there is a little bit of logic as well, but that's encapsulated in the Apply* type.

Do these ever have more than 4 bits of data?  Should we do this using structs of some form?
{CSSPropertyWebkitColumnRuleColor, ApplyPropertyColor, columnRuleColor, setColumnRuleColor, color}

I'm not sure I'm a big enough fan of macros to suggest we macro some of this.  I'm not sure that would help.

Should we autogenerate this from some DSL?

This is probably all thinking too far ahead.

I think we seem to have general agreement that a function-pointer based solution is better than the previous huge switch statement with macros.

The question is what that solution looks like in the end.  One of my goals would be to make it as easily hackable/readable as possible in the final product.

Obviously you're not at the final product yet.  We have a lot of room to iterate.

[WebKit Commit Bot]

Comment on attachment 92045 [details]
Patch

Clearing flags on attachment: 92045

Committed r85711: <http://trac.webkit.org/changeset/85711>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.