Bug 65796 – REGRESSION (r91125): Polyline tool in google docs is broken

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Nico Weber]

1. Open https://docs.google.com/drawings/d/1LJkmi18nTH4N7A1IcpDlG4nfdJFFlij4GrN2TMzY-tM/edit?hl=en_US&ndplr=1
2. Click the polyline tool (looks a bit like an arrow pointing to the bottom left, 11th icon from the left)
3. Click on the image, but don't move mouse

Expected: Nothing happens
Actual: The whole drawing disappears. Moving the cursor a bit makes it reappear.

[Nico Weber]

Created attachment 103131 [details]
reduced test case

[Nico Weber]

( This is http://crbug.com/91858 )

[Nico Weber]

Rob, ideas?

[Rob Buis]

Created attachment 104636 [details]
Patch

[WebKit Review Bot]

Comment on attachment 104636 [details]
Patch

Attachment 104636 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9462530

New failing tests:
svg/custom/zero-path-square-cap-rendering3.svg

[Dirk Schulze]

Comment on attachment 104636 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104636&action=review

> LayoutTests/ChangeLog:11
> +        * platform/mac/svg/custom/zero-path-square-cap-rendering-expected.png:

Why is this a progression? I thought we should draw a rect for zero length paths?

> LayoutTests/svg/custom/zero-path-square-cap-rendering3.svg:4
> +<?xml version="1.0" standalone="no"?>
> +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" 
> +"http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
> +<svg width="100%" height="100%" version="1.1" xmlns="http://www.w3.org/2000/svg">

We try to minimize tests as much as possible. Remove the doctype declaration, the xml notation.

> LayoutTests/svg/custom/zero-path-square-cap-rendering3.svg:9
> +<g transform="translate(200 200) scale(0.766666)">
> +<rect width="300" height="100"
> +style="fill:rgb(0,0,255);stroke-width:1;
> +stroke:rgb(0,0,0)"/>

Why is that test so complicated? Is it necessary to have different transforms in the test? Why?

> LayoutTests/svg/custom/zero-path-square-cap-rendering3.svg:12
> +<path stroke="#000000" stroke-opacity="1" stroke-width="496.95" stroke-linecap="square" stroke-linejoin="miter" stroke-miterlimit="8" shape-rendering="optimizeSpeed" d="M 0 0"></path>
> +</g>

<path d='M50,50 z' stroke='green' stroke-width='100' stroke-linecap='square'/> is the code on zero-path-square-cap-rendering.svg. Why isn't the first square drawn, while it is drawn here? The only difference is, that we close the path on the first example.

> Source/WebCore/ChangeLog:8
> +        Don't follow zero-length path code path for paths that consist of just one moveto.

path code path for paths? So the problem should be a path with just one segment? Why does the first example change than? Can you link to the part of the spec?

> Source/WebCore/rendering/svg/RenderSVGPath.cpp:158
>      // Spec(11.4): Any zero length subpath shall not be stroked if the âstroke-linecapâ property has a value of butt
>      // but shall be stroked if the âstroke-linecapâ property has a value of round or square

Why did you land your previous patch with âstroke-linecapâ? I thought the reviewer wanted you to change this first? Nevertheless, this does not explain why you don't stroke your first example.

> Source/WebCore/rendering/svg/RenderSVGPath.cpp:159
> +    return style()->svgStyle()->hasStroke() && style()->svgStyle()->capStyle() != ButtCap && !m_fillBoundingBox.width() && !m_fillBoundingBox.height() && m_path.numberOfSegments() > 1;

I might had problems to understand what you are doing here :) Your new test has just one segment in the path, but you are still drawing a rect, no?

[Rob Buis]

Created attachment 104828 [details]
Patch

[Rob Buis]

Hi Dirk,

(In reply to comment #6)
> (From update of attachment 104636 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104636&action=review
> 
> > LayoutTests/ChangeLog:11
> > +        * platform/mac/svg/custom/zero-path-square-cap-rendering-expected.png:
> 
> Why is this a progression? I thought we should draw a rect for zero length paths?

You are right, it was rather a regression, see below.

> > LayoutTests/svg/custom/zero-path-square-cap-rendering3.svg:4
> > +<?xml version="1.0" standalone="no"?>
> > +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" 
> > +"http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
> > +<svg width="100%" height="100%" version="1.1" xmlns="http://www.w3.org/2000/svg">
> 
> We try to minimize tests as much as possible. Remove the doctype declaration, the xml notation.

Fixed.

> > LayoutTests/svg/custom/zero-path-square-cap-rendering3.svg:9
> > +<g transform="translate(200 200) scale(0.766666)">
> > +<rect width="300" height="100"
> > +style="fill:rgb(0,0,255);stroke-width:1;
> > +stroke:rgb(0,0,0)"/>
> 
> Why is that test so complicated? Is it necessary to have different transforms in the test? Why?

Only the scale is needed, fixed.

> > LayoutTests/svg/custom/zero-path-square-cap-rendering3.svg:12
> > +<path stroke="#000000" stroke-opacity="1" stroke-width="496.95" stroke-linecap="square" stroke-linejoin="miter" stroke-miterlimit="8" shape-rendering="optimizeSpeed" d="M 0 0"></path>
> > +</g>
> 
> <path d='M50,50 z' stroke='green' stroke-width='100' stroke-linecap='square'/> is the code on zero-path-square-cap-rendering.svg. Why isn't the first square drawn, while it is drawn here? The only difference is, that we close the path on the first example.

Yes, I noticed the close makes it a zero-length path, whereas just a moveto is not. So it was a mistake to not draw in zero-path-square-cap-rendering.svg, while for zero-path-square-cap-rendering3.svg the path should not be drawn since it is just a moveto.

> > Source/WebCore/ChangeLog:8
> > +        Don't follow zero-length path code path for paths that consist of just one moveto.
> 
> path code path for paths? So the problem should be a path with just one segment? Why does the first example change than? Can you link to the part of the spec?

I updated the 11.4 comment to show that just a moveto is not a zero-length path.

> > Source/WebCore/rendering/svg/RenderSVGPath.cpp:158
> >      // Spec(11.4): Any zero length subpath shall not be stroked if the âstroke-linecapâ property has a value of butt
> >      // but shall be stroked if the âstroke-linecapâ property has a value of round or square
> 
> Why did you land your previous patch with âstroke-linecapâ? I thought the reviewer wanted you to change this first? Nevertheless, this does not explain why you don't stroke your first example.

I fixed the characters now.

> > Source/WebCore/rendering/svg/RenderSVGPath.cpp:159
> > +    return style()->svgStyle()->hasStroke() && style()->svgStyle()->capStyle() != ButtCap && !m_fillBoundingBox.width() && !m_fillBoundingBox.height() && m_path.numberOfSegments() > 1;
> 
> I might had problems to understand what you are doing here :) Your new test has just one segment in the path, but you are still drawing a rect, no?

The rect is there so anything is visible at all :) The path is just a moveto and should not render (see spec link), but it also should not prevent the rect from being drawn as was the case before my patch. I made the rect green and smaller to more match our normal test results.
Cheers,

Rob.

[WebKit Review Bot]

Comment on attachment 104828 [details]
Patch

Attachment 104828 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9481294

New failing tests:
svg/custom/zero-path-square-cap-rendering3.svg

[Dirk Schulze]

Comment on attachment 104828 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104828&action=review

Just one question remains. Otherwise looks great!

> Source/WebCore/ChangeLog:8
> +        Paths that consist of just one moveto are not zero-length paths, so do

Ok, what about M 0 0 M 0 0 ? Is that one subpath or two subpaths? Would that be a zero length path?

[Nico Weber]

What's the status here?

[Dirk Schulze]

Comment on attachment 104828 [details]
Patch

With renis patch, we will differ between all shapes. So we just need to know about empty paths on SVGPathElement. Maybe you can get these information from SVGPathBuilder. You'd just need another boolean value (hasZeroLengthSubPath). You'd get the necessary information during parsing the path. Of course you won't parse the path data all the time, just on changes of the path. This way we wouldn't depend on platform information that might be wrong (after internal path transformations) or not existent at all.

Also, I'd like to see more test cases with different segment combinations.

[Nico Weber]

This has been broken for over 3 months on trunk now and we had to revert r91125 on multiple chrome release branches. Should we revert r91125 in webkit too until someone has time to look at this?

[Dirk Schulze]

(In reply to comment #13)
> This has been broken for over 3 months on trunk now and we had to revert r91125 on multiple chrome release branches. Should we revert r91125 in webkit too until someone has time to look at this?

Seems like Rob is busy. I'd agree with rolling this patch out.

[Stephen Chenney]

I would like to take ownership and resolve this. Anyone object?

[Stephen Chenney]

To clarify the taregt behavior, this from the spec:

http://www.w3.org/TR/SVG/painting.html#StrokeProperties

A subpath (see Paths) consisting of a single moveto shall not be stroked. Any zero length subpath shall not be stroked if the ‘stroke-linecap’ property has a value of butt but shall be stroked if the ‘stroke-linecap’ property has a value of round or square, producing respectively a circle or a square centered at the given point. Examples of zero length subpaths include 'M 10,10 L 10,10', 'M 20,20 h 0', 'M 30,30 z' and 'M 40,40 c 0,0 0,0 0,0'.

[Stephen Chenney]

Created attachment 115175 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 115175 [details]
Patch

Looks good.

[WebKit Review Bot]

Comment on attachment 115175 [details]
Patch

Clearing flags on attachment: 115175

Committed r100291: <http://trac.webkit.org/changeset/100291>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Reopen, because bug isn't fixed ... But a failing test was landed which made bots red. :( I think the test should be landed with a proper fix.

[Stephen Chenney]

This is a CoreGraphics bug. In Skia, SkPath, we have this test which is used before attempting to draw the path (in RenderSVGPath).

199	bool SkPath::isEmpty() const {
200	    SkDEBUGCODE(this->validate();)
201	
202	    int count = fVerbs.count();
203	    return count == 0 || (count == 1 && fVerbs[0] == kMove_Verb);
204	}

I'll look for a workaround, if it's clean.

[Stephen Chenney]

CG Bug filed:
https://bugreport.apple.com/cgi-bin/WebObjects/RadarWeb.woa/43/wo/IOTksiNQcb7NdKAzfbUsbw/7.66

Still seeing if we can work around.

[Peter Kasting]

This commit was rolled back in r100312.

[Stephen Chenney]

Radar ID: 10450621

[Stephen Chenney]

Created attachment 115415 [details]
Patch

Patch to workaround the CG bug. Still checking win expectations so review only at this time

[Darin Adler]

Comment on attachment 115415 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115415&action=review

> Source/WebCore/platform/graphics/cg/PathCG.cpp:281
> +    PathIsEmptyTester tester;
> +    CGPathApply(m_path, &tester, PathIsEmptyTester::applierFunction);
> +    return CGPathIsEmpty(m_path) || tester.isEmpty();

What cases does CGPathIsEmpty cover that the tester does not?

[Darin Adler]

Comment on attachment 115415 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115415&action=review

> Source/WebCore/platform/graphics/cg/PathCG.cpp:280
> +    CGPathApply(m_path, &tester, PathIsEmptyTester::applierFunction);

Seems a shame to iterate and count all elements in the path when we only need to look at 2 maximum.

[Darin Adler]

Comment on attachment 115415 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115415&action=review

> Source/WebCore/platform/graphics/cg/PathCG.cpp:52
> +        return !m_count || (m_count == 1 && !m_seenNonMoveToElement);

What about a path with many move-to elements but no non-move-to elements?

Why do we need a count at all?

[Stephen Chenney]

(In reply to comment #27)
> (From update of attachment 115415 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115415&action=review
> 
> > Source/WebCore/platform/graphics/cg/PathCG.cpp:281
> > +    PathIsEmptyTester tester;
> > +    CGPathApply(m_path, &tester, PathIsEmptyTester::applierFunction);
> > +    return CGPathIsEmpty(m_path) || tester.isEmpty();
> 
> What cases does CGPathIsEmpty cover that the tester does not?

None. I'll remove.

[Stephen Chenney]

(In reply to comment #29)
> (From update of attachment 115415 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115415&action=review
> 
> > Source/WebCore/platform/graphics/cg/PathCG.cpp:52
> > +        return !m_count || (m_count == 1 && !m_seenNonMoveToElement);
> 
> What about a path with many move-to elements but no non-move-to elements?
> 
> Why do we need a count at all?

I test a path with 2 move-to and nothing else. It doesn't cause any problems inside CG, doesn't draw, and even has zero bounds.

The check for length 1 matches the Skia check, so I thought I would leave it that way for consistency. I will change it if we don't care about Skia consistency.

[Stephen Chenney]

(In reply to comment #28)
> (From update of attachment 115415 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115415&action=review
> 
> > Source/WebCore/platform/graphics/cg/PathCG.cpp:280
> > +    CGPathApply(m_path, &tester, PathIsEmptyTester::applierFunction);
> 
> Seems a shame to iterate and count all elements in the path when we only need to look at 2 maximum.

It is a shame to iterate when we only care about the first 2, but I could find no way to get either (a) the length, nor (b) the elements, nor (c) specific elements in a CGPath. So I couldn't see any way to avoid the iteration. Maybe a CG expert can correct me.

[WebKit Review Bot]

Comment on attachment 115415 [details]
Patch

Attachment 115415 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10361873

New failing tests:
svg/custom/subpaths-moveto-only-rendering.svg

[Stephen Chenney]

Created attachment 115455 [details]
Patch

I like this fix better. Much more efficient.

[Stephen Chenney]

Comment on attachment 115455 [details]
Patch

I think this one is ready to commit. Verified results as best I could on Win, Linux and Mac.

[Stephen Chenney]

The latest patch is also better because it gives the right size for the element. The size should be 0,0 because it is not drawn at all. The old code gave a size that assumed the path was zero length and hence included the linecap.

[Stephen Chenney]

Comment on attachment 115455 [details]
Patch

My opinion on this is changing yet again (thanks Simon). Canvas uses the same code path, and a canvas test case that generates the same sequence of CG calls (as far as I can tell) does not generate the bug.

My current theory is that this is a problem with the bounding volume. CG definitely returns a large bound for the lone move path, which is incorrect according to the SVG spec. If this is uses to clip the objects behind, then the rectangle will not get drawn. Canvas definitely does not clip in this way, which would explain why it works.

[Stephen Chenney]

Created attachment 115852 [details]
Patch

This patch addresses all the issue, while not breaking anything else. All the other things I attempted caused failures of some other kind. This fix applies at the source of the problem, which should make it easy to remove when the CG bug is fixed.

[Stephen Chenney]

And also, this will fix the canvas path too, although the bug never manifested there because canvas does not use bounds.

I wish there were a better way, but I really did try everything I could think of. For example, just fixing isEmpty causes this bug to still manifest on a path m 0 0 m 10 10. Just fixing the bound causes the zero-length line cap to fail.

[Darin Adler]

Comment on attachment 115852 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115852&action=review

I do not understand why Path::isEmpty is being changed here to return true for paths that are not considered empty on other platforms.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:46
> +// A class to provide an empty test that considers a one-element path with only a MoveTo element
> +// to be empty. This works around Radar 10450621.
> +class CGPathIsEmptyTester {

I think it would be better if the class name said more of what the comment does. I’d give it a longer name like this:

    PathEmptinessTesterWithMoveToWorkaround

Also, I would say “Using this instead of CGPathIsEmpty works around a CG bug, Radar 10450621.” This class itself doesn’t work around anything!

> Source/WebCore/platform/graphics/cg/PathCG.cpp:50
> +    bool isEmpty()

Probably better to mark this const.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:55
> +    static void applierFunction(void* info, const CGPathElement* element)

I’d name this something like testPathElement.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:59
> +        if (++tester->m_count < 2 && element->type != kCGPathElementMoveToPoint)
> +            ++tester->m_count;

This code is non-obvious. Both what it does, and why. Even though there is a why comment in the m_count below, the way auto-increment is used obscures the subtle technique used to make any non-move-to element make the result false. Here’s how I’d write it:

    if (element->type == kCGPathElementMoveToPoint)
        ++tester->m_count;
    else {
        // Any element other than a MoveTo means the path is not empty; setting the count to 2 is a simple way to achieve that.
        tester->m_count = 2;
    }

> Source/WebCore/platform/graphics/cg/PathCG.cpp:63
> +    // Count will be the number of elements + 1 if there is a non-move-to element first

If you take my suggestion then you can say:

    // If any non-move-to element is present, then the count will be >= 2.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:64
> +    unsigned m_count;

I think this would be clearer if named m_moveToCount.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:68
> +// A class to detect if a path has only move-to elements and no actual lines.
> +// This works around Radar 10450621.

I would say, "Using this instead of calling CGPathGetBoundingBox works around a CG bug, Radar 10450621".

But I don’t understand why the single bug needs two different workarounds. Why do we need one set of logic for CGPathGetBoundingBox and a different one for CGPathIsEmpty?

> Source/WebCore/platform/graphics/cg/PathCG.cpp:69
> +class CGPathHasEmptyBoundTester {

I’d name this PathEmptyBoundTester.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:73
> +    bool hasEmptyBound()

Same const comment.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:78
> +    static void applierFunction(void* info, const CGPathElement* element)

Same name comment.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:86
> +    bool m_haveSeenNonMove;

We typically try to name these boolean members based on a sentence like “tester <xxx>”.

So it would be m_hasSeenNonMove.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:321
> +    // A bug in CoreGraphics leads to an incorrect bound on paths containing a single
> +    // moveTo, and nothing else, with a linecap style that is non-butt. Such paths are effectively
> +    // empty (other platforms consider it so) and hence we enforce it here. If the bug in
> +    // CG is fixed, we could use CGPathIsEmpty(m_path);

I don’t see how returning true here is “enforcing it”. How does it help to return true for isEmpty here?

The comment mentions linecap style, but the code itself does not consider linecap style and in fact is at the wrong level to do so.

[Stephen Chenney]

Thanks for all the excellent comments on naming, comment clarity, etc. I appreciate it and I will address those when I get into work next.

The big high level question is this: "But I don’t understand why the single bug needs two different workarounds. Why do we need one set of logic for CGPathGetBoundingBox and a different one for CGPathIsEmpty?"

Say we only modify the result of Path::isEmpty to match other platforms in that a SVG d="m 0 0" is an empty path. This does not fix the case of a path d="m 0 0 m 1 1" with linecap Square/Round, which is not empty according to most interpretations. Such a path should draw nothing according to the SVG spec and have no effect on other content, yet it has a non-zero bound and hence obscures content behind it. The result is exactly as described in the original report.

You may ask why we need to consider "m 0 0" as empty? It is because linecap stroking for zero length subpaths is incompletely implemented and a hack. Specifically, it relies on a zero size bounding volume to detect zero length subpaths and draws the linecaps for such paths according to the SVG spec. However, "m 0 0" has zero sized bonds on most platforms, so it gets stroked, against the spec, if it gets past the isEmpty check that precedes any stroking. Note that the spec is explicit that "m 0 0" is not a zero length path and should not be stroked under any circumstances. So we must pretend that it's empty.

I plan to fix the zero length path stroking behavior properly so that we do not require "m 0 0" to be considered empty. I plan to do this next. See https://bugs.webkit.org/show_bug.cgi?id=71820

You may ask why empty != draws nothing? e.g. Is "m 0 0 m 1 1" empty? That would require changing more platforms and just compounds the hacks (in my opinion). Furthermore, I believe that a path "m 0 0 m 1 1" should produce markers if requested. I think this is an error in current implementations but the spec doesn't say anything about it. I will try to clarify the behavior when I get done with this bug.

Say we leave the isEmpty as it is and modify the bounding box code to work around the actual CG bug. This would give an empty bounding volume on any path that should draw nothing, such as "m 0 0" and "m 0 0 m 1 1". The problem with this fix is the aforementioned hack for zero-length paths. That code will draw linecaps for non-butt strokes even in the "m 0 0" and "m 1 1" cases, which the spec says it shouldn't.

Hence, as the code stands, we need both isEmpty and the bounding box code to be modified.

I want to fix this bug first to get a level playing field before I deal with 71820.

Should you prefer, I can check in CG-related expectations for some tests to fail and just check in the bounding box workaround.

Otherwise, I will check this in, with the tests, and then remove the isEmpty workaround when I fix 71820.

How does all that sound?

[Stephen Chenney]

Created attachment 116112 [details]
Patch

This patch addresses the naming issues and improves the commenting. See the previous comment on this bug report for detailed discussion of why the patch looks this way.

[WebKit Review Bot]

Comment on attachment 116112 [details]
Patch

Rejecting attachment 116112 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
.txt
patching file LayoutTests/svg/custom/path-moveto-only-rendering.svg
patching file LayoutTests/svg/custom/subpaths-moveto-only-rendering.svg
patching file LayoutTests/svg/custom/zero-path-square-cap-rendering2-expected.txt
Hunk #1 FAILED at 4.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/svg/custom/zero-path-square-cap-rendering2-expected.txt.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Darin Adler', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/10696742

[Stephen Chenney]

Created attachment 117473 [details]
Patch

[WebKit Review Bot]

Comment on attachment 117473 [details]
Patch

Rejecting attachment 117473 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Stephen Chenney]

Created attachment 117492 [details]
Patch

[Darin Adler]

Comment on attachment 117492 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117492&action=review

> Source/WebCore/platform/graphics/cg/PathCG.cpp:331
>  bool Path::isEmpty() const
>  {
> -    return CGPathIsEmpty(m_path);
> +    // The SVG rendering code that uses this method relies on paths with a single move-to
> +    // element, and nothing else, as being empty. Until that code is refactored to avoid
> +    // the dependence on isEmpty, we match the behavior of other platforms.
> +    // When the SVG code is refactored, we could use CGPathIsEmpty(m_path);
> +    PathIsEmptyOrSingleMoveTester tester;
> +    CGPathApply(m_path, &tester, PathIsEmptyOrSingleMoveTester::testPathElement);
> +    return tester.isEmpty();
>  }

I think the strategy here is imperfect. All callers of Path::isEmpty will be slower on platforms that use Core Graphics even though only the call sites inside SVG need this more expensive check for the bug workaround.

[Darin Adler]

Do we have Radar bug numbers for the Core Graphics problems?

[Stephen Chenney]

(In reply to comment #48)
> Do we have Radar bug numbers for the Core Graphics problems?

Yes. See comment #25.

I agree that modifying isEmpty is a bad idea - particularly as it is only necessary for some other badly implemented spec requirement. I'm modifying that bad implementations, so this change will go away when I land another patch that I'm working on. With luck no user will ever see it.

[WebKit Review Bot]

Comment on attachment 117492 [details]
Patch

Rejecting attachment 117492 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ng file LayoutTests/platform/win/svg/custom/path-moveto-only-rendering-expected.txt
patching file LayoutTests/platform/win/svg/custom/subpaths-moveto-only-rendering-expected.txt
patching file LayoutTests/svg/custom/path-moveto-only-rendering.svg
patching file LayoutTests/svg/custom/subpaths-moveto-only-rendering.svg
patching file LayoutTests/svg/custom/zero-path-square-cap-rendering2-expected.txt

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/10697835

[Stephen Chenney]

Created attachment 117618 [details]
Patch

One more attempt to get this through the queue

[WebKit Review Bot]

Comment on attachment 117618 [details]
Patch

Clearing flags on attachment: 117618

Committed r101805: <http://trac.webkit.org/changeset/101805>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

I believe this patch caused about 100 canvas regression tests to start failing on Mac.

[Darin Adler]

The isEmpty change causes many canvas regression tests to fail on Mac. Need to roll this out.

[Darin Adler]

Rolled out in <http://trac.webkit.org/changeset/101816>; we probably should try again without changing isEmpty.

[Darin Adler]

Or perhaps there is just a bug in the isEmpty implementation we landed.

[Stephen Chenney]

(In reply to comment #57)
> Or perhaps there is just a bug in the isEmpty implementation we landed.

I think I should just get rid of the isEmpty change. It was a temp thing anyway and not worth more time on.

[Stephen Chenney]

(In reply to comment #58)
> (In reply to comment #57)
> > Or perhaps there is just a bug in the isEmpty implementation we landed.
> 
> I think I should just get rid of the isEmpty change. It was a temp thing anyway and not worth more time on.

Argh, the pain. The problem was other methods using isEmpty to test things like "hasCurrentPoint" that have different behavior with the workaround isEmpty. I'm testing an updated patch to verify that I have all the cases. Then I would prefer to leave in the workaround isEmpty, as it means the least expectations modification now and in the future (i.e. I would otherwise have to check in known incorrect expectations).

[Stephen Chenney]

Created attachment 117683 [details]
Patch

THis patch address the test failures in Canvas and SVG. It has been tested and everything passes. Minimal changes were made.

[WebKit Review Bot]

Attachment 117683 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebCore/ChangeLog:27:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:28:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Stephen Chenney]

Created attachment 117687 [details]
Patch

Remove tabs in Changelogs.

[WebKit Review Bot]

Comment on attachment 117687 [details]
Patch

Clearing flags on attachment: 117687

Committed r101903: <http://trac.webkit.org/changeset/101903>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Stephen Chenney]

Turns out this is not a CG bug but a failure to correctly detect CGRectIsNull. Reopening to fix properly.

[Stephen Chenney]

Created attachment 121294 [details]
Patch

This is a much nicer fix for the issue. I also removed the isEmpty changes and just marked the tests as failing pending a fix for another bug.

[Eric Seidel (no email)]

Comment on attachment 121294 [details]
Patch

Won't this affect mac results?

[Eric Seidel (no email)]

(In reply to comment #67)
> (From update of attachment 121294 [details])
> Won't this affect mac results?

I missed the mac/ changes in my first pass reading.

I think this would be easier to review once the patch for https://bugs.webkit.org/show_bug.cgi?id=71820 is posted, no?

[Stephen Chenney]

(In reply to comment #68)
> (In reply to comment #67)
> > (From update of attachment 121294 [details] [details])
> > Won't this affect mac results?
> 
> I missed the mac/ changes in my first pass reading.
> 
> I think this would be easier to review once the patch for https://bugs.webkit.org/show_bug.cgi?id=71820 is posted, no?

Sure, I would be happy to let it sit until then.

Part of my reason for revisiting is that we need to get some fix for this into the upcoming chrome releases. We can deal with that in the chrome branch, however, and not modify trunk.

I'll clear the r? flag for now.

[Simon Fraser (smfr)]

Do we still think that there's a Core Graphics bug here?

[Stephen Chenney]

(In reply to comment #70)
> Do we still think that there's a Core Graphics bug here?

At this point I would not classify it as a bug. The decision to use CGRectNull as a return value is an odd one to me, but not a bug. Given that, all code should be checking for CGRectNull as a return value from CGPath bounding volume methods, and I think it's reasonable to consider degenerate paths as null-bounded paths.

It's documented properly, which is all I really ask. Not sure how I missed it earlier, but miss it I certainly did.

[Stephen Chenney]

Comment on attachment 121294 [details]
Patch

Will have a new patch up tomorrow.

[Stephen Chenney]

Created attachment 124108 [details]
Patch

Ready for review and commit now that WK71820 is fixed

[Nikolas Zimmermann]

Comment on attachment 124108 [details]
Patch

Impressive, r=me :-) Note, it doesn't apply, so I guess you'll need to upload a new version, if you want me to cq+.

[Nikolas Zimmermann]

Note, you didn't add expectations for Chromium, was that desired?

[Stephen Chenney]

(In reply to comment #75)
> Note, you didn't add expectations for Chromium, was that desired?

Thanks. I'll sort out the merge conflicts and re-upload.

This affects the CG graphics layer only, and the only platform that uses it is mac, so there is no impact on chromium. Chromium dropped CG support about three weeks ago, and now is skia only on all platforms.

[Stephen Chenney]

Created attachment 124116 [details]
Patch

Updated patch to resovle conflicts

[Nikolas Zimmermann]

Comment on attachment 124116 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124116&action=review

> LayoutTests/platform/mac/test_expectations.txt:203
> +BUGWK65798 : svg/stroke/zero-length-subpaths-linecap-rendering.svg = TEXT
> +BUGWK65798 : svg/stroke/zero-length-path-linecap-rendering.svg = TEXT

Oh ok, I guess you have no Lion machine around to generate the results?

[WebKit Review Bot]

Attachment 124116 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Last 3072 characters of output:
est/expectations] [2]
LayoutTests/platform/mac/test_expectations.txt:187:  Path does not exist. tables/mozilla_expected_failures/other/test4.html  [test/expectations] [2]
LayoutTests/platform/mac/test_expectations.txt:188:  Path does not exist. tables/mozilla/bugs/bug29157.html  [test/expectations] [2]
LayoutTests/platform/mac/test_expectations.txt:189:  Path does not exist. tables/mozilla/other/wa_table_thtd_rowspan.html  [test/expectations] [2]
LayoutTests/platform/mac/test_expectations.txt:190:  Path does not exist. tables/mozilla/other/wa_table_tr_align.html  [test/expectations] [2]
LayoutTests/platform/mac/test_expectations.txt:191:  Path does not exist. tables/mozilla_expected_failures/marvin/backgr_layers-show.html  [test/expectations] [2]
LayoutTests/platform/mac/test_expectations.txt:192:  Path does not exist. tables/mozilla_expected_failures/marvin/table_frame_lhs.html  [test/expectations] [2]
LayoutTests/platform/mac/test_expectations.txt:193:  Path does not exist. tables/mozilla_expected_failures/marvin/table_frame_rhs.html  [test/expectations] [2]
LayoutTests/platform/mac/test_expectations.txt:194:  Path does not exist. fast/css/bidi-override-in-anonymous-block.html  [test/expectations] [2]
LayoutTests/platform/mac/test_expectations.txt:195:  Path does not exist. fast/dom/HTMLTableElement/colSpan.html  [test/expectations] [2]
LayoutTests/platform/mac/test_expectations.txt:196:  Path does not exist. fast/dom/HTMLTableElement/createCaption.html  [test/expectations] [2]
LayoutTests/platform/mac/test_expectations.txt:197:  Path does not exist. fast/repaint/table-section-repaint.html  [test/expectations] [2]
LayoutTests/platform/mac/test_expectations.txt:198:  Path does not exist. fast/table/frame-and-rules.html  [test/expectations] [2]
LayoutTests/platform/mac/test_expectations.txt:202:  Path does not exist. svg/stroke/zero-length-subpaths-linecap-rendering.svg  [test/expectations] [2]
LayoutTests/platform/mac/test_expectations.txt:203:  Path does not exist. svg/stroke/zero-length-path-linecap-rendering.svg  [test/expectations] [2]
LayoutTests/platform/mac/test_expectations.txt:204:  Path does not exist. svg/stroke/zero-length-arc-linecaps-rendering.svg  [test/expectations] [2]
LayoutTests/platform/mac/test_expectations.txt:205:  Path does not exist. svg/hixie/intrinsic/003.html  [test/expectations] [2]
LayoutTests/platform/mac/test_expectations.txt:206:  Path does not exist. svg/W3C-SVG-1.1/animate-elem-80-t.svg  [test/expectations] [2]
LayoutTests/platform/mac/test_expectations.txt:207:  Path does not exist. svg/W3C-SVG-1.1-SE/painting-control-04-f.svg  [test/expectations] [2]
LayoutTests/platform/mac/test_expectations.txt:210:  Path does not exist. http/tests/media/video-buffering-repaints-controls.html  [test/expectations] [2]
LayoutTests/platform/mac/test_expectations.txt:213:  Path does not exist. fast/table/027.html  [test/expectations] [2]
LayoutTests/platform/mac/test_expectations.txt:214:  Path does not exist. fast/table/027-vertical.html  [test/expectations] [2]
Total errors found: 182 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Stephen Chenney]

(In reply to comment #78)
> (From update of attachment 124116 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124116&action=review
> 
> > LayoutTests/platform/mac/test_expectations.txt:203
> > +BUGWK65798 : svg/stroke/zero-length-subpaths-linecap-rendering.svg = TEXT
> > +BUGWK65798 : svg/stroke/zero-length-path-linecap-rendering.svg = TEXT
> 
> Oh ok, I guess you have no Lion machine around to generate the results?

I have no Lion machine, and I've had very bad luck getting expectations right when generated locally. So now I take the chromium recommended method, which is to mark the test as failing or flakey then pull the expectations from a build machine once it has built and tested. It's more changes, but fewer build breaks. And I always reopen bugs to make sure the expectations are updated.

Anyway, looks like I might have to do it again. :-(

[Stephen Chenney]

Created attachment 124134 [details]
Patch

[WebKit Review Bot]

Comment on attachment 124134 [details]
Patch

Clearing flags on attachment: 124134

Committed r106016: <http://trac.webkit.org/changeset/106016>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.