Implement Background and Mask based properties in CSSStyleApplyProperty

• Component: New Bugs
• Status: RESOLVED FIXED

[Luke Macpherson]

Implement CSSPropertyColor, CSSPropertFillLayer based properties in CSSStyleApplyProperty

[Luke Macpherson]

Created attachment 88372 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 88372 [details]
Patch

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

This seems sane.  Are you sure we can't delete the macros?  It's feels awkward having two (possibly divergent) ways to do this same thing.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:226
> +            (selector->*m_mapFill)(m_propertyId, currChild, value);

I find this calling convention very ugly.  We may want to write wrappers which take a "selector" argument and know how to do the ->*m_mapfill call.  Or maybe I'll just get used to the convention.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:275
> +    setPropertyValue(CSSPropertyWebkitBackgroundSize, CSSPropertyBackgroundSize);

Seems we may eventually rename this to "setValueApplier?" or similar.  setPropertyValue makes it sound like we're some css property on an element.

> Source/WebCore/css/CSSStyleSelector.cpp:-3655
> -        HANDLE_BACKGROUND_VALUE(clip, Clip, value)

Can we delete these macros now?

[Dimitri Glazkov (Google)]

Comment on attachment 88372 [details]
Patch

ok, but please listen to and consider following up on Eric's suggestions. His sense of code smell is usually very good.

[Luke Macpherson]

(In reply to comment #2)

Thanks Eric. I pretty much agree with you on all points.

> (From update of attachment 88372 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88372&action=review
> 
> This seems sane.  Are you sure we can't delete the macros?  It's feels awkward having two (possibly divergent) ways to do this same thing.

Almost there, just a few properties left to remove before this can happen.

> > Source/WebCore/css/CSSStyleApplyProperty.cpp:226
> > +            (selector->*m_mapFill)(m_propertyId, currChild, value);
> 
> I find this calling convention very ugly.  We may want to write wrappers which take a "selector" argument and know how to do the ->*m_mapfill call.  Or maybe I'll just get used to the convention.

I think CSSStyleSelector::mapFill* should be moved into CSSStyleApplyProperty, but I haven't been brave enough to do that refactoring yet. These look like they need re-writing anyway as there appears to be a lot of common code between them. I'll file a bug for future cleanup.

> > Source/WebCore/css/CSSStyleApplyProperty.cpp:275
> > +    setPropertyValue(CSSPropertyWebkitBackgroundSize, CSSPropertyBackgroundSize);
> 
> Seems we may eventually rename this to "setValueApplier?" or similar.  setPropertyValue makes it sound like we're some css property on an element.

Agreed. Will do.

> > Source/WebCore/css/CSSStyleSelector.cpp:-3655
> > -        HANDLE_BACKGROUND_VALUE(clip, Clip, value)
> 
> Can we delete these macros now?

Not quite yet, but soon - there are some expanding properties that need to move from CSSStyleSelector to CSSStyleApplyProperty before we can do that. I plan to do it shortly but in the interests of making small incremental changes left it out of this CL.

[WebKit Commit Bot]

Comment on attachment 88372 [details]
Patch

Clearing flags on attachment: 88372

Committed r83241: <http://trac.webkit.org/changeset/83241>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/83241 might have broken Qt Linux Release minimal

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 88372 [details]:

fast/history/history-subframe-with-name.html bug 51039 (author: [e-mail])
The commit-queue is continuing to process your patch.