[webkit-reviews] review granted: [Bug 12013] REGRESSION: <pattern> in objectBoundingBox mode are broken. : [Attachment 12153] Updated patch III

[bugzilla-request-daemon at macosforge.org]

Eric Seidel <macdome at opendarwin.org> has granted 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 12153: Updated patch III
http://bugs.webkit.org/attachment.cgi?id=12153&action=edit

------- Additional Comments from Eric Seidel <macdome at opendarwin.org>
This assert seems needless:
+	     SVGElement* svgElement =
static_cast<SVGElement*>(item->element());
+	     ASSERT(svgElement && svgElement->document() &&
svgElement->isStyled());
+	    
svgElement->document()->accessSVGExtensions()->addPendingResource(id,
static_cast<SVGStyledElement*>

I'm surprised m_pendingResources would be a pointer.  I figure it's used often
enough it should just be a member variable.  (That would also reduce the number
of lines of code, and possibilities of bugs relating to its creation)

The first check can be removed if you change m_pendingResources to be a stack
object.  but if you don't this ASSERT shoudl still be moved:

+    if (!m_pendingResources)
+	 return HashSet<SVGStyledElement*>();
+
+    ASSERT(m_pendingResources->contains(id));

+    // For instance, dynamically build gradients / patterns / clippers...
typo: build instead of built.

no real need to call empty constructors:
+	     , m_patternTransform()

Again, an unnecessary ASSERT:
+void SVGGradientElement::insertedIntoDocument()
+{
+    SVGElement::insertedIntoDocument();
+
+    ASSERT(document());
+    SVGDocumentExtensions* extensions = document()->accessSVGExtensions();

We'd crash anyway.  If that ASSERT ever failed, we'd see much much bigger bugs
anyways ;)
Same ASSERT is found in the pattern code too.

Still not needed:
+    ASSERT(m_ownerElement);
+    m_ownerElement->buildGradient();
especially now that you have the ASSERT in the constructor.
(there are a couple of these)

This is a fabulous patch (as always).  The code looks good to land.  Please
remove the unnecessary ASSERTS as mentioned above.  I'll look at the tests now.


I wonder how much faster this makes the SVG PLT.  It should cause some
improvement (invalidating gradients fewer times.)