[webkit-reviews] review granted: [Bug 68475] Implement filter shorthands : [Attachment 115443] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Dean Jackson
<dino at apple.com>'s request for review:
Bug 68475: Implement filter shorthands
https://bugs.webkit.org/show_bug.cgi?id=68475

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=115443&action=review


> Source/WebCore/rendering/FilterEffectRenderer.cpp:79
> +    for (Vector<RefPtr<FilterOperation> >::const_iterator it =
operations.operations().begin(); it != end; ++it) {

We prefer index access for vectors, rather than using enumerators.

> Source/WebCore/rendering/FilterEffectRenderer.cpp:90
> +	       float oneMinusAmount = clampTo(1.0 -
narrowPrecisionToFloat(colorMatrixOperation->amount()), 0.0, 1.0);

1.0 -> 1, 0.0 -> 0

> Source/WebCore/rendering/FilterEffectRenderer.cpp:104
> +	       inputParameters.append(0.2126 + 0.7874 * oneMinusAmount);
> +	       inputParameters.append(0.7152 - 0.7152 * oneMinusAmount);
> +	       inputParameters.append(0.0722 - 0.0722 * oneMinusAmount);
> +	       endMatrixRow(inputParameters);
> +
> +	       inputParameters.append(0.2126 - 0.2126 * oneMinusAmount);
> +	       inputParameters.append(0.7152 + 0.2848 * oneMinusAmount);
> +	       inputParameters.append(0.0722 - 0.0722 * oneMinusAmount);
> +	       endMatrixRow(inputParameters);
> +
> +	       inputParameters.append(0.2126 - 0.2126 * oneMinusAmount);
> +	       inputParameters.append(0.7152 - 0.7152 * oneMinusAmount);
> +	       inputParameters.append(0.0722 + 0.9278 * oneMinusAmount);

Would be nice to see a URL for the magic numbers.

> Source/WebCore/rendering/FilterEffectRenderer.cpp:129
> +	       inputParameters.append(0.393 + 0.607 * oneMinusAmount);
> +	       inputParameters.append(0.769 - 0.769 * oneMinusAmount);
> +	       inputParameters.append(0.189 - 0.189 * oneMinusAmount);
> +	       endMatrixRow(inputParameters);
> +
> +	       inputParameters.append(0.349 - 0.349 * oneMinusAmount);
> +	       inputParameters.append(0.686 + 0.314 * oneMinusAmount);
> +	       inputParameters.append(0.168 - 0.168 * oneMinusAmount);
> +	       endMatrixRow(inputParameters);
> +
> +	       inputParameters.append(0.272 - 0.272 * oneMinusAmount);
> +	       inputParameters.append(0.534 - 0.534 * oneMinusAmount);
> +	       inputParameters.append(0.131 + 0.869 * oneMinusAmount);

Ditto.

> Source/WebCore/rendering/FilterEffectRenderer.cpp:192
> +	       float stdDeviationX =
blurOperation->stdDeviationX().calcFloatValue(borderBox.width());
> +	       float stdDeviationY =
blurOperation->stdDeviationY().calcFloatValue(borderBox.height());

Huh, why does the stdDev depend on box size?

> Source/WebCore/rendering/FilterEffectRenderer.h:66
> +    GraphicsContext* inputContext();

Can this be const?

> Source/WebCore/rendering/FilterEffectRenderer.h:67
> +    ImageBuffer* output() { return lastEffect()->asImageBuffer(); }

Ditto.

> Source/WebCore/rendering/FilterEffectRenderer.h:78
> +	   Vector<RefPtr<FilterEffect> >::const_iterator end = m_effects.end();

> +	   for (Vector<RefPtr<FilterEffect> >::const_iterator it =
m_effects.begin(); it != end; ++it) {

Use index access.

> Source/WebCore/rendering/FilterEffectRenderer.h:96
> +    Vector<RefPtr<FilterEffect> > m_effects;

A typedef for  Vector<RefPtr<FilterEffect> > may make the code easier to read.

> LayoutTests/ChangeLog:62
> +2011-11-16  Dean Jackson  <dino at apple.com>
> +
> +	   Need a short description and bug URL (OOPS!)
> +
> +	   Reviewed by NOBODY (OOPS!).
> +

Double changelog.

> LayoutTests/css3/filters/effect-blur-expected.txt:2
> +  RenderView at (0,0) size 800x600

You should make these all dumpAsText(true).