In the CSS Shaders implementation, we frequently make a decision based on programInfo.mixSettings.enabled. It would be nice if we used a CustomFilterProgramType enum in CustomFilterProgramInfo instead. Then, we could remove mixSettings.enabled. The enum would look something like: enum CustomFilterProgramType { PROGRAM_TYPE_NO_ELEMENT_TEXTURE, PROGRAM_TYPE_BLENDS_ELEMENT_TEXTURE }
Created attachment 168083 [details] Not for review
Comment on attachment 168083 [details] Not for review Looks good! Here's my informal review- just a couple of nits. View in context: https://bugs.webkit.org/attachment.cgi?id=168083&action=review > Source/WebCore/ChangeLog:8 > + Refactor of CustomFilterProgramInfo: MixSettings and related program type Can you be more specific? For example: Remove CustomFilterProgramMixSettings.enabled and check against CustomFilterProgram.programType() instead. Add m_programType to CustomFilterProgram and CustomFilterProgramInfo. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:925 > + if (isMixEnabled) { I would put the boolean expression in here without the extra variable. > Source/WebCore/platform/graphics/filters/CustomFilterProgram.cpp:41 > +CustomFilterProgram::CustomFilterProgram(CustomFilterProgramType programType, CustomFilterProgramMixSettings mixSettings) While you're touching this line, can you make CustomFilterProgramMixSettings a const reference? > Source/WebCore/platform/graphics/filters/CustomFilterProgram.h:78 > + CustomFilterProgram(CustomFilterProgramType, CustomFilterProgramMixSettings); Ditto. > Source/WebCore/platform/graphics/filters/CustomFilterProgramInfo.cpp:84 > + m_programType == PROGRAM_TYPE_BLENDS_ELEMENT_TEXTURE, Can you factor out this expression as a local boolean since we check it multiple times? Maybe like this: bool blendsElementTexture = (m_programType == PROGRAM_TYPE_BLENDS_ELEMENT_TEXTURE); > Source/WebCore/platform/graphics/filters/CustomFilterProgramInfo.cpp:98 > && m_mixSettings == o.m_mixSettings; We should only check m_mixSettings for equality if m_programType is PROGRAM_TYPE_BLENDS_ELEMENT_TEXTURE. > Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:86 > + bool isMixEnabled = programInfo.programType() == PROGRAM_TYPE_BLENDS_ELEMENT_TEXTURE; Instead of "isMixEnabled", can we call this boolean "blendsElementTexture"? > Source/WebCore/rendering/style/StyleCustomFilterProgram.h:48 > + static PassRefPtr<StyleCustomFilterProgram> create(PassRefPtr<StyleShader> vertexShader, PassRefPtr<StyleShader> fragmentShader, CustomFilterProgramType programType, CustomFilterProgramMixSettings mixSettings) const CustomFilterProgramMixSettings& > Source/WebCore/rendering/style/StyleCustomFilterProgram.h:129 > + StyleCustomFilterProgram(PassRefPtr<StyleShader> vertexShader, PassRefPtr<StyleShader> fragmentShader, CustomFilterProgramType programType, CustomFilterProgramMixSettings mixSettings) const CustomFilterProgramMixSettings&
Created attachment 168304 [details] Patch
Comment on attachment 168304 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168304&action=review > Source/WebCore/ChangeLog:11 > + CustomFilterProgramInfo has been refactored to decouple the CustomFilterProgramType from > + the CustomFilterProgramMixSettings, therefore CustomFilterProgramSettings.enabled has been > + removed, now we explicitely check for the programType(). m_program has also been added to > + CustomFilterProgram (and CustomFilterProgramInfo). would be good if you told why, and why this is an improvement, instead of what you just did > Source/WebCore/ChangeLog:13 > + No test necessary. Why? covered by existing tests? Just refactoring? > Source/WebCore/ChangeLog:38 > + * css/CSSComputedStyleDeclaration.cpp: > + (WebCore::CSSComputedStyleDeclaration::valueForFilter): > + * css/StyleResolver.cpp: > + (WebCore::StyleResolver::createCustomFilterOperation): > + * platform/graphics/filters/CustomFilterProgram.cpp: > + (WebCore::CustomFilterProgram::CustomFilterProgram): > + (WebCore::CustomFilterProgram::programInfo): > + * platform/graphics/filters/CustomFilterProgram.h: > + * platform/graphics/filters/CustomFilterProgramInfo.cpp: > + (WebCore::CustomFilterProgramInfo::CustomFilterProgramInfo): > + (WebCore::CustomFilterProgramInfo::hash): > + (WebCore::CustomFilterProgramInfo::operator==): > + * platform/graphics/filters/CustomFilterProgramInfo.h: > + (WebCore::CustomFilterProgramMixSettings::CustomFilterProgramMixSettings): > + (WebCore::CustomFilterProgramMixSettings::operator==): > + (CustomFilterProgramInfo): > + (WebCore::CustomFilterProgramInfo::programType): > + * platform/graphics/filters/CustomFilterValidatedProgram.cpp: > + (WebCore::CustomFilterValidatedProgram::CustomFilterValidatedProgram): > + (WebCore::CustomFilterValidatedProgram::rewriteMixVertexShader): > + (WebCore::CustomFilterValidatedProgram::rewriteMixFragmentShader): > + * rendering/style/StyleCustomFilterProgram.h: > + (WebCore::StyleCustomFilterProgram::create): > + (WebCore::StyleCustomFilterProgram::StyleCustomFilterProgram): Some times it can be good adding more detailed comments here
Comment on attachment 168304 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168304&action=review > Source/WebCore/ChangeLog:10 > + removed, now we explicitely check for the programType(). m_program has also been added to Nit: I think you meant "m_programType", not "m_program".
Created attachment 168498 [details] Patch
Comment on attachment 168498 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168498&action=review LGTM but others might have comments > Source/WebCore/ChangeLog:23 > + reflect the need of setting the program type explicitely. spelling > Source/WebCore/ChangeLog:25 > + type explicitely. explicitly* > Source/WebCore/ChangeLog:30 > + (WebCore::CustomFilterProgramInfo::operator==): MixSetings' equality check is being performed only whether Settings*
(In reply to comment #7) > LGTM but others might have comments Cool, thanks a lot!
Created attachment 168729 [details] Patch for landing
Comment on attachment 168729 [details] Patch for landing Clearing flags on attachment: 168729 Committed r131357: <http://trac.webkit.org/changeset/131357>
All reviewed patches have been landed. Closing bug.