Created attachment 88372 [details] Patch

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?

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.

(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.

Comment on attachment 88372 [details] Patch Clearing flags on attachment: 88372 Committed r83241: <http://trac.webkit.org/changeset/83241>

All reviewed patches have been landed. Closing bug.

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

The commit-queue encountered the following flaky tests while processing attachment 88372 [details]: fast/history/history-subframe-with-name.html bug 51039 (author: mihaip@chromium.org) The commit-queue is continuing to process your patch.