Bug 59774 - Add template parameter to ApplyPropertyColor to improve clarity by removing constructor parameter side effects.
Summary: Add template parameter to ApplyPropertyColor to improve clarity by removing c...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 59623
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-28 20:46 PDT by Luke Macpherson
Modified: 2011-05-03 20:36 PDT (History)
4 users (show)

See Also:


Attachments
Patch (7.41 KB, patch)
2011-04-28 20:48 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch (7.70 KB, patch)
2011-05-01 18:45 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch (7.98 KB, patch)
2011-05-02 22:12 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Luke Macpherson 2011-04-28 20:46:24 PDT
Add template parameter to ApplyPropertyColor to improve clarity by removing constructor parameter side effects.
Comment 1 Luke Macpherson 2011-04-28 20:48:28 PDT
Created attachment 91629 [details]
Patch
Comment 2 Antti Koivisto 2011-04-29 01:18:04 PDT
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.
Comment 3 Dimitri Glazkov (Google) 2011-04-29 07:55:35 PDT
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.
Comment 4 Luke Macpherson 2011-05-01 18:45:35 PDT
Created attachment 91869 [details]
Patch
Comment 5 Luke Macpherson 2011-05-01 18:53:15 PDT
Incorporated suggestions from Antti and Dimitri.
Comment 6 WebKit Commit Bot 2011-05-02 19:37:04 PDT
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
Comment 7 Luke Macpherson 2011-05-02 22:12:09 PDT
Created attachment 92045 [details]
Patch
Comment 8 Luke Macpherson 2011-05-02 22:14:18 PDT
Last patch is just a merge to resolve conflicts.
Comment 9 Eric Seidel (no email) 2011-05-03 20:35:10 PDT
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.
Comment 10 WebKit Commit Bot 2011-05-03 20:36:16 PDT
Comment on attachment 92045 [details]
Patch

Clearing flags on attachment: 92045

Committed r85711: <http://trac.webkit.org/changeset/85711>
Comment 11 WebKit Commit Bot 2011-05-03 20:36:21 PDT
All reviewed patches have been landed.  Closing bug.