[webkit-reviews] review denied: [Bug 38683] SVG FilterEffects need more detailed DRT information : [Attachment 55484] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 38683: SVG FilterEffects need more detailed DRT information
https://bugs.webkit.org/show_bug.cgi?id=38683

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

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
Looks almost perfect, though some comments again. As FilterEffect::externalRep
doesn't dump anything for now, but will do in future, make it future-proof.
Move away from:
ts << "[feFoo ";
FilterEffect::externalRepresentation(ts);
ts << "mode";

to
ts << "[feFoo";
FilterEffect::externalRepresentation(ts);
ts << " mode";

Whenever you change the externalRepresentation() you don't need to touch the
other callsites.

In detail:
WebCore/platform/graphics/filters/FEBlend.cpp:176
 +	ts << "[feBlend ";
Here in FEBlend.

WebCore/platform/graphics/filters/FEColorMatrix.cpp:223
 +	ts << "[feColorMatrix ";
Ditto.

WebCore/platform/graphics/filters/FEComponentTransfer.cpp:227
 +	ts << "[feComponentTransfer ";
Ditto.

WebCore/platform/graphics/filters/FEComposite.cpp:207
 +	ts << "[feComposite ";
Ditto.

WebCore/platform/graphics/filters/FEGaussianBlur.cpp:145
 +	ts << "[feGaussianBlur ";
Ditto.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:164
 +	ts << "[feConvolveMatrix ";
Ditto.

WebCore/svg/graphics/filters/SVGFEDiffuseLighting.cpp:127
 +	ts << "[feDiffuseLighting ";
Ditto.

WebCore/svg/graphics/filters/SVGFEDisplacementMap.cpp:161
 +	ts << "[feDisplacementMap ";
Ditto

WebCore/svg/graphics/filters/SVGFEFlood.cpp:83
 +	ts << "[feFlood ";
Ditto.

WebCore/svg/graphics/filters/SVGFEImage.cpp:74
 +	ts << "[feImage ";
Ditto.

WebCore/svg/graphics/filters/SVGFEMerge.cpp:93
 +	ts << "[feMerge ";
Ditto.

WebCore/svg/graphics/filters/SVGFEMorphology.cpp:179
 +	ts << "[feMorphology ";
Ditto.

WebCore/svg/graphics/filters/SVGFEOffset.cpp:104
 +	ts << "[feOffset "; 
Ditto.

WebCore/svg/graphics/filters/SVGFESpecularLighting.cpp:139
 +	ts << "[feSpecularLighting ";
Ditto.

WebCore/svg/graphics/filters/SVGFETurbulence.cpp:136
 +	ts << "[feTurbulence ";
Ditto.

General style comments:
WebCore/platform/graphics/filters/FEColorMatrix.cpp:198
 +  static TextStream& operator<<(TextStream& ts, const ColorMatrixType& t)
Rename 't' to 'type'.

WebCore/platform/graphics/filters/FEComponentTransfer.cpp:213
 +  static TextStream& operator<<(TextStream& ts, const
ComponentTransferFunction& f)
Rename 'f' to 'function'.

WebCore/platform/graphics/filters/SourceAlpha.cpp:81
 +  
Omit the newline.

WebCore/platform/graphics/filters/SourceGraphic.cpp:74
 +  
Ditto.

Comments regarding the DRT results:

LayoutTests/platform/mac/svg/W3C-SVG-1.1/filters-color-01-b-expected.txt:14
 +		[feColorMatrix type="MATRIX" values="0.33 0.33 0.33 0.00 0.00
0.33 0.33 0.33 0.00 0.00 0.33 0.33 0.33 0.00 0.00 0.33 0.33 0.33 0.00 0.00 "]
Here's a trailing space in feColorMatrix value dumping that should be removed.

LayoutTests/platform/mac/svg/W3C-SVG-1.1/filters-composite-02-b-expected.txt:13

 +		[feImage image-size="150x150"]
Why not rename it to size? feImage image-size looks odd.

LayoutTests/platform/mac/svg/batik/filters/filterRegions-expected.txt:26
 +	    [feFlood flood-color="#FF0000" flood-opacity="1.00"]
feFlood flood-color/opacity, same issue. maybe just dump color & oapcity here.

Just small issues remaining, the patch looks great as well as the results!