[webkit-reviews] review denied: [Bug 72378] [CSSShaders] Implement the computed style for vertex and fragment shader : [Attachment 115163] Patch V1

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has denied  review:
Bug 72378: [CSSShaders] Implement the computed style for vertex and fragment
shader
https://bugs.webkit.org/show_bug.cgi?id=72378

Attachment 115163: Patch V1
https://bugs.webkit.org/attachment.cgi?id=115163&action=review

------- Additional Comments from Dean Jackson <dino at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=115163&action=review


r- up front for the chromium build issue, otherwise looks good. I wonder if we
should retitle the bug because it now does a lot more than just computed style.


>
LayoutTests/css3/filters/script-tests/custom-filter-property-computed-style.js:
1
> +description("Test the parsing of the custom() function of the -webkit-filter
property.");

This needs updating.

>
LayoutTests/css3/filters/script-tests/custom-filter-property-computed-style.js:
82
> +testFilterRule("Custom with vertex shader",
> +		  "custom(url(vertex.shader))", "custom(url(vertex.shader))");
> +
> +testFilterRule("Custom with fragment shader",
> +		 "custom(none url(fragment.shader))", "custom(none
url(fragment.shader))");
> +
> +testFilterRule("Custom with both shaders",
> +	       "custom(url(vertex.shader) url(fragment.shader))",
"custom(url(vertex.shader) url(fragment.shader))");
> +
> +testFilterRule("Multiple with fragment shader",
> +	       "grayscale() custom(none url(fragment.shader)) sepia()",
"grayscale(1) custom(none url(fragment.shader)) sepia(1)",
> +	       ["WebKitCSSFilterValue.CSS_FILTER_GRAYSCALE",
> +	       "WebKitCSSFilterValue.CSS_FILTER_CUSTOM",
> +	       "WebKitCSSFilterValue.CSS_FILTER_SEPIA"],
> +	       ["grayscale(1)",
> +	       "custom(none url(fragment.shader))",
> +	       "sepia(1)"]);

Nit: some indentation differences up there.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:66
> +#include <algorithm>

Is there a reason for this?

> Source/WebCore/css/CSSStyleSelector.h:211
> +    StyleShader* styleShader(CSSValue*);
> +    StyleShader* cachedOrPendingFromValue(WebKitCSSShaderValue*);

I wonder if this should be named cachedOrPendingStyleShaderFromValue ?

> Source/WebCore/rendering/style/StyleShader.h:57
> +    bool m_isCachedShader:1;
> +    bool m_isPendingShader:1;

I think we typically separate the : and num with spaces.