Bug 71820

Created attachment 116133 [details] Test case for all zero-length-path rendering cases. This test case covers all of the zero-length single path cases. Currently we fail on "m 0 0 z". Firefox fails on all but the "m 0 0" case. It looks like Qt fails on all of the round cap cases, but works for all of the square cap cases, which is really odd.

Created attachment 116145 [details] An even more thorough test.

Created attachment 122336 [details] Patch DO NOT COMMIT pending Skia changes rolling in

Nudge. If the review is too large, I can break out the initial refactoring changes and then add the actual feature fix in a second patch.

Comment on attachment 122336 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122336&action=review Sorry it took so long, this is a really nice patch - it only needs some style tweaks I think. Do you see any negative performance impact for common-cases? I think it doesn't impose any burden for non-zero-length subpath. > Source/WebCore/rendering/svg/RenderSVGShape.cpp:140 > + Vector<FloatPoint>::iterator iter = m_zeroLengthLinecapLocations.begin(); > + while (iter != m_zeroLengthLinecapLocations.end()) { vectors should be accessed by index, this is preferred. > Source/WebCore/rendering/svg/RenderSVGShape.cpp:141 > + float strokeWidth = this->strokeWidth(); This shouldn't be inside the loop, or directly use strokeWidth(). > Source/WebCore/rendering/svg/RenderSVGShape.cpp:272 > + const Color& fallbackColor, const bool nonScalingStroke, const AffineTransform& nonScalingStrokeTransform, We don't constify POD types usually. > Source/WebCore/rendering/svg/RenderSVGShape.cpp:317 > + Vector<FloatPoint>::iterator iter = m_zeroLengthLinecapLocations.begin(); > + while (iter != m_zeroLengthLinecapLocations.end()) { Should walk vector by index. > Source/WebCore/rendering/svg/RenderSVGShape.cpp:437 > + Vector<FloatPoint>::iterator iter = m_zeroLengthLinecapLocations.begin(); Ditto. > Source/WebCore/rendering/svg/RenderSVGShape.cpp:475 > +class ZeroLengthSubpathFinder { This needs to go into its own file, similar to SVGMarkerFoobar.cpp etc. > Source/WebCore/rendering/svg/RenderSVGShape.cpp:489 > + switch (element->type) { Good job! > Source/WebCore/rendering/svg/RenderSVGShape.cpp:506 > + if (subpathFinder->m_lastPoint != element->points[0] > + || element->points[0] != element->points[1]) I'd rather not wrap lines here. > Source/WebCore/rendering/svg/RenderSVGShape.cpp:515 > + if (subpathFinder->m_lastPoint != element->points[0] > + || element->points[0] != element->points[1] ditto. > Source/WebCore/svg/SVGPathSegListBuilder.h:53 > + // Used in UnalteredParsing/NormalizedParsing modes. You could land this seperated, to make the patch a bit smaller.

Created attachment 123159 [details] Patch DO NOT COMMIT pending Skia changes rolling in

(In reply to comment #5) > (From update of attachment 122336 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=122336&action=review > > Sorry it took so long, this is a really nice patch - it only needs some style tweaks I think. > Do you see any negative performance impact for common-cases? I think it doesn't impose any burden for non-zero-length subpath. Thanks for the review. I know you are very busy with all the additional traffic coming your way. I did not do any significant performance checking, but for the most common case (non-degenerate paths) the only significant cost is an iteration over the path at the time it is created from the element. Given everything else that goes on in that situation, such as constructing parsers and path builders, the cost seems very reasonable. Let me know if you have ideas on how to be more concrete about the performance cost. > > > Source/WebCore/rendering/svg/RenderSVGShape.cpp:140 > > + Vector<FloatPoint>::iterator iter = m_zeroLengthLinecapLocations.begin(); > > + while (iter != m_zeroLengthLinecapLocations.end()) { > > vectors should be accessed by index, this is preferred. Done, everywhere. > > Source/WebCore/rendering/svg/RenderSVGShape.cpp:141 > > + float strokeWidth = this->strokeWidth(); > > This shouldn't be inside the loop, or directly use strokeWidth(). Done > > Source/WebCore/rendering/svg/RenderSVGShape.cpp:272 > > + const Color& fallbackColor, const bool nonScalingStroke, const AffineTransform& nonScalingStrokeTransform, > > We don't constify POD types usually. I think I understood your comment. Please check and let me know if I did what you expected. > > Source/WebCore/rendering/svg/RenderSVGShape.cpp:475 > > +class ZeroLengthSubpathFinder { > > This needs to go into its own file, similar to SVGMarkerFoobar.cpp etc. Done. Create RenderSVGSubpathData.h > > Source/WebCore/rendering/svg/RenderSVGShape.cpp:506 > > + if (subpathFinder->m_lastPoint != element->points[0] > > + || element->points[0] != element->points[1]) > > I'd rather not wrap lines here. Done. > > Source/WebCore/svg/SVGPathSegListBuilder.h:53 > > + // Used in UnalteredParsing/NormalizedParsing modes. > > You could land this seperated, to make the patch a bit smaller. I would rather not, just to avoid the overhead. Let me know if you would rather them be separate. There are also some extra expectations changes. There has been some thrashing in that area of the expectations, so the patch may not apply. Again, please do not CQ+ because we need to get a Skia roll in for chromium, then a chromium deps update for WebKit.

Created attachment 123745 [details] Patch Updated patch. Ready for commit.

Comment on attachment 123745 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123745&action=review r=me with additional comments: > Source/WebCore/rendering/svg/RenderSVGShape.cpp:139 > + // FIXME: This is not correct for round linecaps Missing period. I'd prefer if you'd link to a bug report here, can you fix that before landing? > Source/WebCore/rendering/svg/RenderSVGShape.cpp:432 > + for (size_t i = 0; i < m_zeroLengthLinecapLocations.size(); ++i) { No need for the braces, they should be removed. > Source/WebCore/rendering/svg/RenderSVGSubpathData.h:29 > +class RenderSVGSubpathData { Oh, this should be named SVGSubpathData, or SVGZeroLengthSubpathHandler, but no Render* prefix. For SVG, everything starting with Render is a RenderObject dervied object. Stuff like SVGMarkerData, etc. is not.

Created attachment 123788 [details] Patch Review issues addressed. Commit this.

Thanks for the comments Nikolas. Now on to the related bugs. :-)

Comment on attachment 123788 [details] Patch Attachment 123788 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11344049

Comment on attachment 123788 [details] Patch Attachment 123788 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11159045

Comment on attachment 123788 [details] Patch Attachment 123788 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11342049

Created attachment 123831 [details] Patch Hate it when I forget to add a file

Comment on attachment 123831 [details] Patch Attachment 123831 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11177104

Comment on attachment 123831 [details] Patch Attachment 123831 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11294109

Comment on attachment 123831 [details] Patch Attachment 123831 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11264126

Created attachment 123847 [details] Patch Argh. Third time is a charm

Comment on attachment 123847 [details] Patch Clearing flags on attachment: 123847 Committed r105878: <http://trac.webkit.org/changeset/105878>

All reviewed patches have been landed. Closing bug.

it made some tests fail on Qt and I'm not sure how to rebase them. Could you take a look and check if the results are actually correct: http://build.webkit.org/results/Qt%20Linux%20Release/r105879%20(42498)/results.html? Thanks!

(In reply to comment #22) > it made some tests fail on Qt and I'm not sure how to rebase them. Could you take a look and check if the results are actually correct: http://build.webkit.org/results/Qt%20Linux%20Release/r105879%20(42498)/results.html? Thanks! There are text differences in the bounding boxes that I believe are due to a different problem. I've filed https://bugs.webkit.org/show_bug.cgi?id=77020 to track it. You can either skip the test for now, or accept the new results pending further investigation. For Chromium we will be accepting then fixing.

(In reply to comment #23) > (In reply to comment #22) > > it made some tests fail on Qt and I'm not sure how to rebase them. Could you take a look and check if the results are actually correct: http://build.webkit.org/results/Qt%20Linux%20Release/r105879%20(42498)/results.html? Thanks! > > There are text differences in the bounding boxes that I believe are due to a different problem. I've filed https://bugs.webkit.org/show_bug.cgi?id=77020 to track it. > > You can either skip the test for now, or accept the new results pending further investigation. For Chromium we will be accepting then fixing. I have skipped them for now: http://trac.webkit.org/changeset/105889