[webkit-reviews] review denied: [Bug 71406] [CSS Shaders] Implement CSS Animations and Transitions for CSS Shaders : [Attachment 139094] Patch V3

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has denied Chiculita Alexandru
<achicu at adobe.com>'s request for review:
Bug 71406: [CSS Shaders] Implement CSS Animations and Transitions for CSS
Shaders
https://bugs.webkit.org/show_bug.cgi?id=71406

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

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


Minor comments.

> LayoutTests/css3/filters/custom/custom-filter-animation.html:114
> +	       // CSS Shaders do not get good FPS in debug mode, so the
tolerance needs to be higher.

I think we should have a bug open to reduce the tolerance in the future.

> LayoutTests/css3/filters/resources/custom-filter-parser.js:2
> +(function() {
> +

We don't normally follow this style in WebKit, as far as I know. I understand
it's common on the Web. This isn't a big deal to me.

In general I'd like to see some comments in this file to explain what the
various parts do. I expect other tests might want to copy this for their own
use.

> LayoutTests/css3/filters/resources/custom-filter-parser.js:127
> +	       else if (m.ahead().isA(",", endToken))
> +		   result.push({
> +		       type: "keyword", 
> +		       value: m.skip().value
> +		   });

suggest {} here

> Source/WebCore/ChangeLog:11
> +	    between the "from" and "to" states.

indentation.

> Source/WebCore/platform/graphics/filters/CustomFilterOperation.cpp:43
> +bool equalCustomFilterParameters(const CustomFilterParameterList& listA,
const CustomFilterParameterList& listB)

I suggest customFilterParametersEqual

> Source/WebCore/platform/graphics/filters/CustomFilterOperation.cpp:49
> +	   if (listA.at(i).get() != listB.at(i).get() 
> +	       && !(*listA.at(i).get() == *listB.at(i).get()))

Any reason to not use != here?

> Source/WebCore/platform/graphics/filters/CustomFilterOperation.cpp:62
> +    for (unsigned i = 1; i < parameters.size(); ++i) {
> +	   // Break for equal or not-sorted parameters.
> +	   if (!codePointCompareLessThan(parameters.at(i - i)->name(),
parameters.at(i)->name()))
> +	       return false;
> +    }

I'm confused. at(i - i) ?

> Source/WebCore/platform/graphics/filters/CustomFilterOperation.cpp:71
> +    ASSERT(checkCustomFilterParametersOrder(fromList));
> +    ASSERT(checkCustomFilterParametersOrder(toList));

Doesn't this need guarding? It's #ifndef above.

> Source/WebCore/platform/graphics/filters/CustomFilterOperation.cpp:107
> +    ASSERT(checkCustomFilterParametersOrder(m_parameters));

ditto

> Source/WebCore/platform/graphics/filters/CustomFilterOperation.cpp:117
> +    // FIXME: There's no way to decide what is the "passthrough filter" for
shaders using the current CSS Syntax.
> +    // https://bugs.webkit.org/show_bug.cgi?id=84903

Link to spec bug on w3.org too?

> Source/WebCore/platform/graphics/filters/CustomFilterOperation.cpp:122
> +    if (!(*m_program.get() == *fromOp->m_program.get())

Ditto on the != question

> Source/WebCore/platform/graphics/filters/CustomFilterOperation.h:46
> +bool equalCustomFilterParameters(const CustomFilterParameterList& listA,
const CustomFilterParameterList& listB);

I guess we don't really need the parameter names here, following WK style.

> Source/WebCore/rendering/style/StyleCustomFilterProgram.h:125
> +	   return cachedVertexShader() == other->cachedVertexShader()
> +	       && cachedFragmentShader() == other->cachedFragmentShader();

Suggest just one line here.