[webkit-reviews] review denied: [Bug 76253] CSS Shaders: Parse float parameters for the custom() filter syntax : [Attachment 122751] Patch V2

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Chiculita Alexandru
<achicu at adobe.com>'s request for review:
Bug 76253: CSS Shaders: Parse float parameters for the custom() filter syntax
https://bugs.webkit.org/show_bug.cgi?id=76253

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

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=122751&action=review


This is completly fine, but it needs refactoring! The code easily grows to be
unreadable, this way. Please use smaller helper functions, to gain readability.
r- for now:

> LayoutTests/css3/filters/effect-custom-combined-missing.html:18
> +		   // We need to run the tests after the downloading succeeded.

> +		   // Using a timer is not ideal, but there seems to be no
better options.
> +		   setTimeout(function() {

As discussed on IRC, this is not a good idea. But there are already existing
tests exhibing the same problem.
http://trac.webkit.org/changeset/101858 fixed the same issue for SVG Fonts, we
may reuse the idea behind this for shaders as well, and ilsten to the documents
load event here.

> LayoutTests/css3/filters/resources/vertex-offset-parameters.vs:13
> +

One newline too much.

> LayoutTests/css3/filters/resources/vertex-offset-parameters.vs:15
> +    float perspective = - 1.0 / p;

I'd avoid the local.

> LayoutTests/css3/filters/resources/vertex-offset-parameters.vs:25
> +    return mat4(
> +	1.0, 0.0, 0.0, 0.0, 

Indentation.

> LayoutTests/css3/filters/resources/vertex-offset-parameters.vs:63
> +    gl_Position = u_projectionMatrix * perspectiveMatrix(perspective) *
translateMatrix(vertex_offset) * rotateMatrix(rotate * 3.14 / 180.0) *
a_position;

Hm, no M_PI available.... Is 2 digit precision enough? I guess it doesn't
matter.

> Source/WebCore/ChangeLog:16
> +	   * WebCore.xcodeproj/project.pbxproj:

What about all the other build systems? You still should add the headers, where
needed, even if there's no cpp file to add.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:835
> +	       if (parameters.size()) {

Early exit, if parameters.size is 0.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:838
> +		   for (CustomFilterParameterList::const_iterator iter =
parameters.begin(), end = parameters.end(); iter != end; ++iter) {

It's faster to traverse a vector by index, instead of using the iterators.
size_t parametersSize = parameter.size();
for (size_t i = 0; i < parametersSize; ++i) {
     CustomFilterParameter* parameter = parameters[i].get();
...

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:846
> +		       RefPtr<CSSValue> parameterCSSValue;
> +		       
> +		       switch (parameter->parameterType()) {
> +		       case CustomFilterParameter::NUMBER: {
> +			   CustomFilterNumberParameter* numberParameter =
static_cast<CustomFilterNumberParameter*>(parameter);
> +			   RefPtr<CSSValueList> numberParameterValue =
CSSValueList::createSpaceSeparated();

That cries for a helper function, that returns PassRefPtr<CSSValue> to
construct the "parameterCSSValue" out of a CustomFilterParameter.
If you add more cases to the enum, it will get unreadable otherwise.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:852
> +		       default:

I'd rather avoid the default: here, so any missing value from the enum can be
detected at compile-time.

> Source/WebCore/css/CSSStyleSelector.cpp:5228
> +	       // having just two big values: list of shaders and list of
parameters.

two big values? big?

> Source/WebCore/css/CSSStyleSelector.cpp:5240
> +    if (parametersValue) {

Early exit, if parametersValue is 0.

> Source/WebCore/css/CSSStyleSelector.cpp:5246
> +	   CSSValueListIterator parameterIterator(parametersValue);
> +	   for (; parameterIterator.hasMore(); parameterIterator.advance()) {
> +	       if (!parameterIterator.value()->isValueList())
> +		   return 0;
> +	       CSSValueListIterator iterator(parameterIterator.value());
> +	       if (!iterator.isPrimitiveValue())

This whole procedure could be moved somewhere else, in a static inline helper
function.

> Source/WebCore/css/CSSStyleSelector.cpp:5283
> +		   if (firstPrimitiveValue->primitiveType() ==
CSSPrimitiveValue::CSS_NUMBER) {
> +		       RefPtr<CustomFilterNumberParameter> numberParameter =
CustomFilterNumberParameter::create(name);

Same here, the creation of CustomFilterXXXParameters should be in a seperated
method, to make this code more readable.

> Source/WebCore/css/CSSValueList.h:98
> +    bool hasAdvanced() { return m_position; }

Why not just use index() ?

> Source/WebCore/platform/graphics/filters/CustomFilterParameter.h:44
> +	   NUMBER

Maybe add a FIXME here about the still missing types

> Source/WebCore/platform/graphics/filters/CustomFilterParameter.h:60
> +    

Newline too much.

> Source/WebCore/platform/graphics/filters/CustomFilterParameter.h:65
> +class CustomFilterNumberParameter : public CustomFilterParameter {

We normally only use one class per file.

> Source/WebCore/platform/graphics/filters/CustomFilterShader.h:67
> +    int uniformLocationByName(const String& name);

s/ name//

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:207
> +    for (CustomFilterParameterList::iterator iter = m_parameters.begin(),
end = m_parameters.end(); iter != end; ++iter) {

Traverse Vector by index.

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:213
> +	   switch (parameter->parameterType()) {
> +	   case CustomFilterParameter::NUMBER: {

This could again, go into another function, that takes both a
CustomFilterNumberParameter* and operates on m_context.