[Webkit-unassigned] [Bug 38683] SVG FilterEffects need more detailed DRT information

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=38683


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #55484|review?                     |review-
               Flag|                            |




--- Comment #12 from Nikolas Zimmermann <zimmermann at kde.org>  2010-05-09 01:52:54 PST ---
(From update of attachment 55484)
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!

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.