[webkit-reviews] review denied: [Bug 20543] SVG should use the new Gradient support on GraphicsContext : [Attachment 23953] Move gradient spread method to GraphicsContext

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Dirk Schulze <vbs85 at gmx.de>'s request
for review:
Bug 20543: SVG should use the new Gradient support on GraphicsContext
https://bugs.webkit.org/show_bug.cgi?id=20543

Attachment 23953: Move gradient spread method to GraphicsContext
https://bugs.webkit.org/attachment.cgi?id=23953&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
I have mixed feelings about this change.  CG has no concept of "Spread Method",
so if/when we ever decide to implement that obscure part of SVG (which I've yet
to see a single SVG depend on), we'll need to write a "spread method"
implementation on top of GraphicsContext functions or on top of CG functions.

So if we have to write one generic implementation (for the currently
most-popular port of WebKit) then it makes me wonder if we'll want to use that
implementation for all other ports.

enum GradientSpreadMethod {
 129	     SPREADMETHOD_PAD = 1,
 130	     SPREADMETHOD_REFLECT = 2,
 131	     SPREADMETHOD_REPEAT = 3
 132	 };

This should follow the CamelCase naming convention of all other enums in that
file.

SpreadMethodPad
SpreadMethodRepeat
SpreadMethodReflect

 74	    GradientSpreadMethod spreadMethod;

That needs to be initialized to something.

 459	     if (spreadMethod())
That should never be false!  Since spreadMethod() == 0 is not a valid value.

No default is needed here:
167	    default:
 168		 gradient.setSpread(QGradient::PadSpread);
 169		 break;

I think this change is overall OK.  I think we'll end up crossing the "how to
write a CG implementation" bridge when we come to it.  And when we do write a
CG implementation of this we'll think about generalizing said implementation.