Bug 12013 – REGRESSION: <pattern> in objectBoundingBox mode are broken.

• Component: SVG
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

I fixed this r18433, and it's broken again in ToT. Just investigating.

[Nikolas Zimmermann]

Mistake! I fixed it in r18425. Before r18433, it was already broken.

[Nikolas Zimmermann]

Created attachment 12090 [details]
Initial patch

Attaching patch which reworks pattern handling to actually work in all patternUnits/patternContentUnits combinations. Fixes quite some testcases:
coords-units-01-b.svg (also work with run-safari now, under any zoom level)
pattern-in-defs.svg (pattern buffer allocation now clipped against target bbox)
js-update-pattern.svg (live update really works now! all repainting is done properly)

Also get rid of SVGResourceListener, and the whole resourceNotification() logic.
This also fixes gradients causing objects to redraw twice.

[Eric Seidel (no email)]

Comment on attachment 12090 [details]
Initial patch

A couple things:

-        return valueInSpecifiedUnits();
-    return valueInSpecifiedUnits() * 100.f;
+        return valueInSpecifiedUnits() / 100.0;
+
+    return valueInSpecifiedUnits();

So the only reasons I didn't do that initially, is:  a. division is slow.  b.  foo / 100.0 * 100.0 (divided across method calls) just looses significant digits (and introduces rounding error).

-            // FIXME: maskImage could be NULL
+            if (!maskImage)
+                return false;

Isn't that dangerous?  Won't that leave things in an inconsistent state?  (not that crashing is any better... but it's easier to debug a crash *there* than one which is later and random)

I didn't look at the rest super-closely.  We can chat about it more tomorrow.

[Maciej Stachowiak]

Comment on attachment 12090 [details]
Initial patch

Marking r- based on Eric's commens to get this out of the review queue.

[Nikolas Zimmermann]

Hi Eric,

> -    return valueInSpecifiedUnits() * 100.f;
> +        return valueInSpecifiedUnits() / 100.0;
> +
> +    return valueInSpecifiedUnits();
> 
> So the only reasons I didn't do that initially, is:  a. division is slow.  b. 
> foo / 100.0 * 100.0 (divided across method calls) just looses significant
> digits (and introduces rounding error).

Errm, that's the only reason I actually converted this, to avoid using valueAsPercentage() / 100.0?

> 
> -            // FIXME: maskImage could be NULL
> +            if (!maskImage)
> +                return false;
> 
> Isn't that dangerous?  Won't that leave things in an inconsistent state?  (not
> that crashing is any better... but it's easier to debug a crash *there* than
> one which is later and random)
You're right, a context->restore() is missing, that's all. The state isn't inconsitent then.

Uploading new patch to fix this issue.

[Nikolas Zimmermann]

Created attachment 12099 [details]
Updated patch

Fix the issue Eric mentioned.

[Nikolas Zimmermann]

Comment on attachment 12099 [details]
Updated patch

Please ignore the changes to RenderSVGContainer.cpp - didn't want to include them here.

[Oliver Hunt]

Comment on attachment 12099 [details]
Updated patch

Remove the svgcontainer changes and it looks good to me... 

It fixes all the issues eric pointed to.  

Eric: If you have no other problems this can be r+, however i'd like a second set of eyes to check it

[Eric Seidel (no email)]

Comment on attachment 12099 [details]
Updated patch

Ok, the code looks good.

1.  Need to remove the 1x1 pruning in SVGRenderContainer
2. Need to file a bug about the /100 issue
3.  You said you were going to fix gradients to be lazy
4. Need to add some test cases, including one for failed mask allocations.  You said you created a bunch of crazy tests, but I don't see them here.  The test cases are almost more important than the code changes as once we have the tests, we'll never break them.
5.  
+        IntRect textBoundary = const_cast<RenderObject*>(object)->absoluteBoundingBoxRect();
+        targetRect = object->absoluteTransform().inverse().mapRect(textBoundary);
seems kinda hacky.  I'm surprise there isn't a method to get you what you want there.

Otherwise looks great!

[Nikolas Zimmermann]

Created attachment 12147 [details]
Updated patch II

Revised patch, rewriting/fixing even more of patterns/gradients.

[Nikolas Zimmermann]

Created attachment 12148 [details]
LayoutTest changes

[Eric Seidel (no email)]

Comment on attachment 12147 [details]
Updated patch II

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()

[Eric Seidel (no email)]

Comment on attachment 12147 [details]
Updated patch II

+            isLinear = current->gradientType() == LinearGradientPaintServer;

seems a little odd, if 
+            if (processedGradients.contains(current))
+                return LinearGradientAttributes();
is the next line returned.

I guess it doesn't matter which is returned if it's a cycle, or?

-        SVGElement* parentElement = static_cast<SVGElement*>(parent);
-        if (parentElement->isStyledLocatable()) {
+        SVGElement* parentElement = svg_dynamic_cast(parent);
+        if (parentElement && parentElement->isStyledLocatable()) {


This code change reminds me of another bug where SVG elements (i.e. <svg>) don't behave properly in terms of % when inside HTML elements.  But that's really for another time...

Seems a little odd to put this check at the end:
isRadial = current->gradientType() == RadialGradientPaintServer;
but I guess that makes it so we don't have to check for the first gradient "this".

+            currentServer = WTF::static_pointer_cast<SVGPaintServerGradient>(current->m_resource);

will be null if the referenced gradient isn't built yet.

that will cause:
+        if (!attributes.hasPaintServer() && !currentServer->gradientStops().isEmpty())
+            attributes.setPaintServer(currentServer.get());
to crash.

If that currently crashes on TOT, we should have a P1 bug covering it with a test case.  If it doesn't, than this patch introduces a P1 regression.

Shouldn't this ASSERT simply be in the constructor?
+    ASSERT(m_ownerElement);
+    m_ownerElement->buildGradient();

it seems of little use in the externalRepresentation function... the next line will crash anyway if the ASSERT fails.

the text "owner" isn't very helpful here (IMO):
+        SVGPaintServerGradient(const SVGGradientElement* owner);
(this is done in several headers)

it seems odd that
 void SVGResource::invalidate()
 defaults to empty.  Are subclasses required to implement it?  should it be = 0?  I guess they aren't really required to...

I don't understand this comment... but I really want to. :(
+        // These HashMap contains the list of pending resources. Pending resources, are such
+        // which are referenced by any object in the SVG document, but do NOT exist that.
+        // For instance, dynamically builded gradients / patterns / clippers...

IMO this is not any cleaner (and likely slower, since I doubt pow is inlined:
+    if (sqrt(pow(fdx, 2.0) + pow(fdy, 2.0)) > radius) { 

Also, that should be sqrtf if fdx and radius, etc are floats.

I guess there are a couple of these:
+    ASSERT(m_ownerElement);

they should all just be removed and one added to teh constructor (IMO).

My final concern is if we have enough test cases for all this.  I'm happy to help you make some.  Lets chat a bit on IRC and then get a final patch to land posted.

[Nikolas Zimmermann]

Created attachment 12153 [details]
Updated patch III

Fixed all of Eric's comments.

[Nikolas Zimmermann]

Created attachment 12154 [details]
LayoutTest changes

Updated baseline with new testcases.

[Eric Seidel (no email)]

Comment on attachment 12153 [details]
Updated patch III

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.)

[Eric Seidel (no email)]

Comment on attachment 12154 [details]
LayoutTest changes

The tests look fine.  Except it looks like gradient-cycle-detection.svg still has the wrong xlink url.

[Nikolas Zimmermann]

Landed in r18521.