Created attachment 122240 [details] Testcase A group of perpendicular paths returns a bounding box of 0,0,0,0 (see attached testcase). (bug was originally reported at: http://code.google.com/p/chromium/issues/detail?id=109805)
Created attachment 122241 [details] Fix getBBox for perpendicular paths
This changes platform/graphics and is not SVG specific so I've added darin@apple.com
Comment on attachment 122241 [details] Fix getBBox for perpendicular paths View in context: https://bugs.webkit.org/attachment.cgi?id=122241&action=review > Source/WebCore/platform/graphics/FloatRect.cpp:114 > + float left = min(x(), other.x()); > + float top = min(y(), other.y()); > + float right = max(maxX(), other.maxX()); > + float bottom = max(maxY(), other.maxY()); The "l, t, r, b" or "left, top, right, bottom" naming is obsolete for IntRect and FloatRect. In new code we should name these things like x, y, maxX, and maxY. > Source/WebCore/platform/graphics/FloatRect.cpp:119 > void FloatRect::uniteIfNonZero(const FloatRect& other) Not new to this patch: It would be clearer if the uniteIfNonZero function used the isZero function rather than having a hand-written distinct implementation of “non-zero”. I am concerned that this function’s definition of 0 is !x, while FloatSize’s definition of 0 is fabs(x) < epsilon. It looks like Levi made that change to FloatSize in July and I am not sure why it’s not needed here as well.
(In reply to comment #3) > It would be clearer if the uniteIfNonZero function used the isZero function rather than having a hand-written distinct implementation of “non-zero”. I am concerned that this function’s definition of 0 is !x, while FloatSize’s definition of 0 is fabs(x) < epsilon. It looks like Levi made that change to FloatSize in July and I am not sure why it’s not needed here as well. It was definitely an oversight that this still uses zero instead of considering epsilon, let's correct that now. Of course, epsilon is not perfect either...
(In reply to comment #4) > (In reply to comment #3) > > It would be clearer if the uniteIfNonZero function used the isZero function rather than having a hand-written distinct implementation of “non-zero”. I am concerned that this function’s definition of 0 is !x, while FloatSize’s definition of 0 is fabs(x) < epsilon. It looks like Levi made that change to FloatSize in July and I am not sure why it’s not needed here as well. > > It was definitely an oversight that this still uses zero instead of considering epsilon, let's correct that now. Of course, epsilon is not perfect either... I agree and can do this. Should I do that in this review, or as a followup patch?
(In reply to comment #5) > I agree and can do this. Should I do that in this review, or as a followup patch? Seems good to do it separately. Might even be worth spending a short time on little research to see if we can find any case where we know it affects behavior.
Comment on attachment 122241 [details] Fix getBBox for perpendicular paths Attachment 122241 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11227184 New failing tests: svg/batik/filters/filterRegions.svg svg/clip-path/deep-nested-clip-in-mask-panning.svg svg/filters/filterRes.svg svg/dynamic-updates/SVGFEBlendElement-dom-in-attr.html svg/W3C-SVG-1.1/filters-morph-01-f.svg svg/filters/filter-refresh.svg svg/dynamic-updates/SVGFEBlendElement-dom-in2-attr.html svg/dynamic-updates/SVGFEBlendElement-dom-mode-attr.html svg/dynamic-updates/SVGFEBlendElement-svgdom-in-prop.html svg/filters/feConvolveFilter-y-bounds.svg svg/dynamic-updates/SVGFEBlendElement-svgdom-in2-prop.html svg/W3C-SVG-1.1/filters-blend-01-b.svg svg/animations/animate-mpath-insert.html svg/clip-path/deep-nested-clip-in-mask.svg svg/dynamic-updates/SVGFEBlendElement-svgdom-mode-prop.html
(In reply to comment #7) > (From update of attachment 122241 [details]) > Attachment 122241 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/11227184 > > New failing tests: > svg/batik/filters/filterRegions.svg > svg/clip-path/deep-nested-clip-in-mask-panning.svg > svg/filters/filterRes.svg > svg/dynamic-updates/SVGFEBlendElement-dom-in-attr.html > svg/W3C-SVG-1.1/filters-morph-01-f.svg > svg/filters/filter-refresh.svg > svg/dynamic-updates/SVGFEBlendElement-dom-in2-attr.html > svg/dynamic-updates/SVGFEBlendElement-dom-mode-attr.html > svg/dynamic-updates/SVGFEBlendElement-svgdom-in-prop.html > svg/filters/feConvolveFilter-y-bounds.svg > svg/dynamic-updates/SVGFEBlendElement-svgdom-in2-prop.html > svg/W3C-SVG-1.1/filters-blend-01-b.svg > svg/animations/animate-mpath-insert.html > svg/clip-path/deep-nested-clip-in-mask.svg > svg/dynamic-updates/SVGFEBlendElement-svgdom-mode-prop.html I think a couple of these are real failures (failed on my machine w/o patch, pass with it). Sorry for the churn guys, I'll have a new patch up soon.
Created attachment 122548 [details] Fix getBBox for perpendicular paths This patch contains two fixes for failures exposed in my previous patch: 1) We need to special-case the first bounding box in SVGRendererSupport::computeContainerBoundingBoxes. What's happening is we start with a bounding box of 0,0,0,0 and we add the union of all our child bounding boxes. The problem is that the resulting bounding box will always start at x=0, y=0, due to the initial bounding box of 0,0,0,0, so we need to special-case the first child and set the initial bounding box to our first child's bounding box. 2) My previous patch had a mistake where I failed to transform a child's bounding box using transform.mapRect().
Comment on attachment 122548 [details] Fix getBBox for perpendicular paths View in context: https://bugs.webkit.org/attachment.cgi?id=122548&action=review LGTM, but need someone else to sign off on it :) > Source/WebCore/platform/graphics/FloatRect.cpp:114 > + float left = min(x(), other.x()); > + float top = min(y(), other.y()); > + float right = max(maxX(), other.maxX()); > + float bottom = max(maxY(), other.maxY()); As Darin pointed out, we should avoid using these directional names in favor of maxX and maxY to not confuse things in vertical text.
Created attachment 122806 [details] Fix getBBox for perpendicular paths Small rename: l,t,r,b -> minX, minY, maxX, maxY per a Darin's comment.
Comment on attachment 122806 [details] Fix getBBox for perpendicular paths View in context: https://bugs.webkit.org/attachment.cgi?id=122806&action=review Does the test cover the “united with 0,0,0,0” issue? > Source/WebCore/rendering/svg/SVGRenderSupport.cpp:194 > + if (isFirstChild) > + isFirstChild = false; No need for the if here. An unconditional assignment does the same thing.
Thank you for the second review. (In reply to comment #12) > (From update of attachment 122806 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=122806&action=review > > Does the test cover the “united with 0,0,0,0” issue? It does. The test draws two perpendicular lines starting at (5,5) and extending to (5,105) and (105,5). Including (0,0) would have resulted in a bounding box of size 105*105, but starting at the first child results in the expected 100*100 bounding box. > > > Source/WebCore/rendering/svg/SVGRenderSupport.cpp:194 > > + if (isFirstChild) > > + isFirstChild = false; > > No need for the if here. An unconditional assignment does the same thing. I have a followup patch for the !width() issue and I'll tack on a fix for this unnecessary check.
Comment on attachment 122806 [details] Fix getBBox for perpendicular paths Clearing flags on attachment: 122806 Committed r105231: <http://trac.webkit.org/changeset/105231>
All reviewed patches have been landed. Closing bug.
Just wanted to leave a note: great findings Philip! Thanks for fixing quickly!