Bug 65796 - REGRESSION (r91125): Polyline tool in google docs is broken
: REGRESSION (r91125): Polyline tool in google docs is broken
Status: RESOLVED FIXED
: WebKit
SVG
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 72403 73670
: 18356 73587
  Show dependency treegraph
 
Reported: 2011-08-05 16:46 PST by
Modified: 2012-01-26 11:18 PST (History)


Attachments
reduced test case (565 bytes, image/svg+xml)
2011-08-05 16:46 PST, Nico Weber
no flags Details
Patch (13.21 KB, patch)
2011-08-21 19:35 PST, Rob Buis
no flags Review Patch | Details | Formatted Diff | Diff
Patch (11.00 KB, patch)
2011-08-23 05:28 PST, Rob Buis
no flags Review Patch | Details | Formatted Diff | Diff
Patch (6.78 KB, patch)
2011-11-15 09:03 PST, Stephen Chenney
no flags Review Patch | Details | Formatted Diff | Diff
Patch (27.37 KB, patch)
2011-11-16 11:33 PST, Stephen Chenney
no flags Review Patch | Details | Formatted Diff | Diff
Patch (28.52 KB, patch)
2011-11-16 14:50 PST, Stephen Chenney
no flags Review Patch | Details | Formatted Diff | Diff
Patch (33.23 KB, patch)
2011-11-18 13:01 PST, Stephen Chenney
no flags Review Patch | Details | Formatted Diff | Diff
Patch (33.65 KB, patch)
2011-11-21 11:42 PST, Stephen Chenney
no flags Review Patch | Details | Formatted Diff | Diff
Patch (34.39 KB, patch)
2011-12-01 13:14 PST, Stephen Chenney
no flags Review Patch | Details | Formatted Diff | Diff
Patch (35.22 KB, patch)
2011-12-01 14:38 PST, Stephen Chenney
no flags Review Patch | Details | Formatted Diff | Diff
Patch (45.12 KB, patch)
2011-12-02 07:39 PST, Stephen Chenney
no flags Review Patch | Details | Formatted Diff | Diff
Patch (24.56 KB, patch)
2011-12-02 13:50 PST, Stephen Chenney
no flags Review Patch | Details | Formatted Diff | Diff
Patch (24.58 KB, patch)
2011-12-02 14:01 PST, Stephen Chenney
no flags Review Patch | Details | Formatted Diff | Diff
Patch (9.24 KB, patch)
2012-01-05 10:17 PST, Stephen Chenney
no flags Review Patch | Details | Formatted Diff | Diff
Patch (8.88 KB, patch)
2012-01-26 05:48 PST, Stephen Chenney
no flags Review Patch | Details | Formatted Diff | Diff
Patch (8.55 KB, patch)
2012-01-26 07:20 PST, Stephen Chenney
no flags Review Patch | Details | Formatted Diff | Diff
Patch (6.78 KB, patch)
2012-01-26 10:03 PST, Stephen Chenney
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-08-05 16:46:05 PST
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.
------- Comment #1 From 2011-08-05 16:46:25 PST -------
Created an attachment (id=103131) [details]
reduced test case
------- Comment #2 From 2011-08-05 16:47:08 PST -------
( This is http://crbug.com/91858 )
------- Comment #3 From 2011-08-10 10:00:29 PST -------
Rob, ideas?
------- Comment #4 From 2011-08-21 19:35:23 PST -------
Created an attachment (id=104636) [details]
Patch
------- Comment #5 From 2011-08-21 20:18:26 PST -------
(From update of attachment 104636 [details])
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
------- Comment #6 From 2011-08-22 00:02:51 PST -------
(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?

> 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?
------- Comment #7 From 2011-08-23 05:28:33 PST -------
Created an attachment (id=104828) [details]
Patch
------- Comment #8 From 2011-08-23 05:34:04 PST -------
Hi Dirk,

(In reply to comment #6)
> (From update of attachment 104636 [details] [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.
------- Comment #9 From 2011-08-23 07:24:20 PST -------
(From update of attachment 104828 [details])
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
------- Comment #10 From 2011-08-23 10:46:58 PST -------
(From update of attachment 104828 [details])
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?
------- Comment #11 From 2011-08-30 15:53:43 PST -------
What's the status here?
------- Comment #12 From 2011-10-08 22:48:27 PST -------
(From update of attachment 104828 [details])
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.
------- Comment #13 From 2011-10-21 07:03:30 PST -------
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?
------- Comment #14 From 2011-10-21 07:05:07 PST -------
(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.
------- Comment #15 From 2011-11-11 07:16:21 PST -------
I would like to take ownership and resolve this. Anyone object?
------- Comment #16 From 2011-11-11 07:33:05 PST -------
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'.
------- Comment #17 From 2011-11-15 09:03:58 PST -------
Created an attachment (id=115175) [details]
Patch
------- Comment #18 From 2011-11-15 09:09:44 PST -------
(From update of attachment 115175 [details])
Looks good.
------- Comment #19 From 2011-11-15 10:21:33 PST -------
(From update of attachment 115175 [details])
Clearing flags on attachment: 115175

Committed r100291: <http://trac.webkit.org/changeset/100291>
------- Comment #20 From 2011-11-15 10:21:38 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #21 From 2011-11-15 11:49:36 PST -------
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.
------- Comment #22 From 2011-11-15 13:36:37 PST -------
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.
------- Comment #23 From 2011-11-15 14:24:03 PST -------
CG Bug filed:
https://bugreport.apple.com/cgi-bin/WebObjects/RadarWeb.woa/43/wo/IOTksiNQcb7NdKAzfbUsbw/7.66

Still seeing if we can work around.
------- Comment #24 From 2011-11-15 14:56:47 PST -------
This commit was rolled back in r100312.
------- Comment #25 From 2011-11-15 15:31:29 PST -------
Radar ID: 10450621
------- Comment #26 From 2011-11-16 11:33:10 PST -------
Created an attachment (id=115415) [details]
Patch

Patch to workaround the CG bug. Still checking win expectations so review only at this time
------- Comment #27 From 2011-11-16 12:22:13 PST -------
(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?
------- Comment #28 From 2011-11-16 12:23:19 PST -------
(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.
------- Comment #29 From 2011-11-16 12:23:52 PST -------
(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?
------- Comment #30 From 2011-11-16 12:29:28 PST -------
(In reply to comment #27)
> (From update of attachment 115415 [details] [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.
------- Comment #31 From 2011-11-16 12:32:08 PST -------
(In reply to comment #29)
> (From update of attachment 115415 [details] [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.
------- Comment #32 From 2011-11-16 12:33:57 PST -------
(In reply to comment #28)
> (From update of attachment 115415 [details] [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.
------- Comment #33 From 2011-11-16 14:23:07 PST -------
(From update of attachment 115415 [details])
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
------- Comment #34 From 2011-11-16 14:50:37 PST -------
Created an attachment (id=115455) [details]
Patch

I like this fix better. Much more efficient.
------- Comment #35 From 2011-11-16 14:58:11 PST -------
(From update of attachment 115455 [details])
I think this one is ready to commit. Verified results as best I could on Win, Linux and Mac.
------- Comment #36 From 2011-11-17 10:28:27 PST -------
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.
------- Comment #37 From 2011-11-18 07:21:37 PST -------
(From update of attachment 115455 [details])
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.
------- Comment #38 From 2011-11-18 13:01:47 PST -------
Created an attachment (id=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.
------- Comment #39 From 2011-11-18 13:04:59 PST -------
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.
------- Comment #40 From 2011-11-18 15:35:13 PST -------
(From update of attachment 115852 [details])
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.
------- Comment #41 From 2011-11-18 17:18:26 PST -------
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?
------- Comment #42 From 2011-11-21 11:42:07 PST -------
Created an attachment (id=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.
------- Comment #43 From 2011-12-01 12:14:50 PST -------
(From update of attachment 116112 [details])
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
------- Comment #44 From 2011-12-01 13:14:47 PST -------
Created an attachment (id=117473) [details]
Patch
------- Comment #45 From 2011-12-01 13:17:02 PST -------
(From update of attachment 117473 [details])
Rejecting attachment 117473 [details] from commit-queue.

schenney@chromium.org 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.
------- Comment #46 From 2011-12-01 14:38:26 PST -------
Created an attachment (id=117492) [details]
Patch
------- Comment #47 From 2011-12-01 14:48:28 PST -------
(From update of attachment 117492 [details])
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.
------- Comment #48 From 2011-12-01 14:48:54 PST -------
Do we have Radar bug numbers for the Core Graphics problems?
------- Comment #49 From 2011-12-01 17:30:03 PST -------
(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.
------- Comment #50 From 2011-12-01 18:37:14 PST -------
(From update of attachment 117492 [details])
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
------- Comment #51 From 2011-12-02 07:39:02 PST -------
Created an attachment (id=117618) [details]
Patch

One more attempt to get this through the queue
------- Comment #52 From 2011-12-02 08:37:55 PST -------
(From update of attachment 117618 [details])
Clearing flags on attachment: 117618

Committed r101805: <http://trac.webkit.org/changeset/101805>
------- Comment #53 From 2011-12-02 08:38:03 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #54 From 2011-12-02 09:19:49 PST -------
I believe this patch caused about 100 canvas regression tests to start failing on Mac.
------- Comment #55 From 2011-12-02 09:40:36 PST -------
The isEmpty change causes many canvas regression tests to fail on Mac. Need to roll this out.
------- Comment #56 From 2011-12-02 09:49:48 PST -------
Rolled out in <http://trac.webkit.org/changeset/101816>; we probably should try again without changing isEmpty.
------- Comment #57 From 2011-12-02 09:50:35 PST -------
Or perhaps there is just a bug in the isEmpty implementation we landed.
------- Comment #58 From 2011-12-02 10:01:16 PST -------
(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.
------- Comment #59 From 2011-12-02 12:13:49 PST -------
(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).
------- Comment #60 From 2011-12-02 13:50:21 PST -------
Created an attachment (id=117683) [details]
Patch

THis patch address the test failures in Canvas and SVG. It has been tested and everything passes. Minimal changes were made.
------- Comment #61 From 2011-12-02 13:53:36 PST -------
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.
------- Comment #62 From 2011-12-02 14:01:28 PST -------
Created an attachment (id=117687) [details]
Patch

Remove tabs in Changelogs.
------- Comment #63 From 2011-12-02 21:34:58 PST -------
(From update of attachment 117687 [details])
Clearing flags on attachment: 117687

Committed r101903: <http://trac.webkit.org/changeset/101903>
------- Comment #64 From 2011-12-02 21:35:07 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #65 From 2012-01-05 07:21:18 PST -------
Turns out this is not a CG bug but a failure to correctly detect CGRectIsNull. Reopening to fix properly.
------- Comment #66 From 2012-01-05 10:17:16 PST -------
Created an attachment (id=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.
------- Comment #67 From 2012-01-05 10:20:25 PST -------
(From update of attachment 121294 [details])
Won't this affect mac results?
------- Comment #68 From 2012-01-05 10:21:55 PST -------
(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?
------- Comment #69 From 2012-01-05 10:25:58 PST -------
(In reply to comment #68)
> (In reply to comment #67)
> > (From update of attachment 121294 [details] [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.
------- Comment #70 From 2012-01-05 10:28:33 PST -------
Do we still think that there's a Core Graphics bug here?
------- Comment #71 From 2012-01-05 10:33:05 PST -------
(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.
------- Comment #72 From 2012-01-25 20:10:03 PST -------
(From update of attachment 121294 [details])
Will have a new patch up tomorrow.
------- Comment #73 From 2012-01-26 05:48:49 PST -------
Created an attachment (id=124108) [details]
Patch

Ready for review and commit now that WK71820 is fixed
------- Comment #74 From 2012-01-26 06:43:05 PST -------
(From update of attachment 124108 [details])
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+.
------- Comment #75 From 2012-01-26 06:43:33 PST -------
Note, you didn't add expectations for Chromium, was that desired?
------- Comment #76 From 2012-01-26 07:00:34 PST -------
(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.
------- Comment #77 From 2012-01-26 07:20:12 PST -------
Created an attachment (id=124116) [details]
Patch

Updated patch to resovle conflicts
------- Comment #78 From 2012-01-26 07:24:46 PST -------
(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?
------- Comment #79 From 2012-01-26 07:26:31 PST -------
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.
------- Comment #80 From 2012-01-26 07:29:23 PST -------
(In reply to comment #78)
> (From update of attachment 124116 [details] [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. :-(
------- Comment #81 From 2012-01-26 10:03:10 PST -------
Created an attachment (id=124134) [details]
Patch
------- Comment #82 From 2012-01-26 11:18:33 PST -------
(From update of attachment 124134 [details])
Clearing flags on attachment: 124134

Committed r106016: <http://trac.webkit.org/changeset/106016>
------- Comment #83 From 2012-01-26 11:18:43 PST -------
All reviewed patches have been landed.  Closing bug.