[webkit-reviews] review denied: [Bug 12013] REGRESSION: <pattern> in objectBoundingBox mode are broken. : [Attachment 12147] Updated patch II

[bugzilla-request-daemon at macosforge.org]

Eric Seidel <macdome at opendarwin.org> has denied Eric Seidel
<macdome at opendarwin.org>'s request for review:
Bug 12013: REGRESSION: <pattern> in objectBoundingBox mode are broken.
http://bugs.webkit.org/show_bug.cgi?id=12013

Attachment 12147: Updated patch II
http://bugs.webkit.org/attachment.cgi?id=12147&action=edit

------- Additional Comments from Eric Seidel <macdome at opendarwin.org>
No need to change WebCore/ksvg2/misc/KCanvasRenderingStyle.h (unless you just
meant to update your copyright)

+	     , m_spreadMethodSet(false)
+	     , m_boundingBoxModeSet(false)
+	     , m_gradientTransformSet(false)
+	     , m_paintServerSet(0)
one of these things is not like the other... 3 of these things are kinda the
same. :)

I wonder if the various gradient attribute subclasses shouldn't have their own
headers.  Generally we do one header-per class in WebCore.

This boggles my mind:
 SVGDocument::SVGDocument(DOMImplementation* i, FrameView* view)
     : Document(i, view)
 {
+    // Clear pending resources cache
+    SVGResource::clearPendingResources();
 }

Why would a createDocument() call in JS cause all pending resources to be
cleared?  Or is this a constructor which is only used in a different context?

You should probably update my "manual style resolution is a hack" comment to be
something like "gradients do not create renders, elements without renderers do
not participate in style resolution.  This hack does not work if the gradient's
parent does does not have a renderer (e.g. if it's a <defs>)."	Or,
alternatively we could just fix the hack (by adding an small method to resolve
style for the stops, which walks back up the DOM tree until it finds a renderer
to resolve from.  If you do that, we should add a test.

kinda an odd comments:
+    // If we didn't find any gradient containing stop elements, ignore the
request.
+    if (!attributes.paintServer())
+	 return;

I guess paint server creation depends on there being stops?

+    if (linearGradient->gradientStops().isEmpty())
+	 linearGradient->setGradientStops(attributes.paintServer());

Shouldn't the proper stops already be on the attributes?  I would think
attributes would be filled with values from "this" first, before walking
backwards?  It seems odd to have this one exception to the rule.

attributes could also have a applyToGradient() call.  That might be better
encapsulation than doing all the setting of properties on the linearGradient
from outside of outside of attributes.

I'm going to commit these comments to the bug, and keep reading from
SVGLinearGradientElement::collectGradientProperties()