[webkit-reviews] review granted: [Bug 12013] REGRESSION: <pattern>
in objectBoundingBox mode are broken. : [Attachment 12153]
Updated patch III
bugzilla-request-daemon at macosforge.org
bugzilla-request-daemon at macosforge.org
Mon Jan 1 19:13:08 PST 2007
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.)
More information about the webkit-reviews
mailing list