Bug 65796 - REGRESSION (r91125): Polyline tool in google docs is broken
Summary: REGRESSION (r91125): Polyline tool in google docs is broken
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Stephen Chenney
URL:
Keywords:
Depends on: 72403 73670
Blocks: 18356 73587
  Show dependency treegraph
 
Reported: 2011-08-05 16:46 PDT by Nico Weber
Modified: 2012-01-26 11:18 PST (History)
12 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Nico Weber 2011-08-05 16:46:05 PDT
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 Nico Weber 2011-08-05 16:46:25 PDT
Created attachment 103131 [details]
reduced test case
Comment 2 Nico Weber 2011-08-05 16:47:08 PDT
( This is http://crbug.com/91858 )
Comment 3 Nico Weber 2011-08-10 10:00:29 PDT
Rob, ideas?
Comment 4 Rob Buis 2011-08-21 19:35:23 PDT
Created attachment 104636 [details]
Patch
Comment 5 WebKit Review Bot 2011-08-21 20:18:26 PDT
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
Comment 6 Dirk Schulze 2011-08-22 00:02:51 PDT
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?
Comment 7 Rob Buis 2011-08-23 05:28:33 PDT
Created attachment 104828 [details]
Patch
Comment 8 Rob Buis 2011-08-23 05:34:04 PDT
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.
Comment 9 WebKit Review Bot 2011-08-23 07:24:20 PDT
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
Comment 10 Dirk Schulze 2011-08-23 10:46:58 PDT
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?
Comment 11 Nico Weber 2011-08-30 15:53:43 PDT
What's the status here?
Comment 12 Dirk Schulze 2011-10-08 22:48:27 PDT
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.
Comment 13 Nico Weber 2011-10-21 07:03:30 PDT
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 Dirk Schulze 2011-10-21 07:05:07 PDT
(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 Stephen Chenney 2011-11-11 07:16:21 PST
I would like to take ownership and resolve this. Anyone object?
Comment 16 Stephen Chenney 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 Stephen Chenney 2011-11-15 09:03:58 PST
Created attachment 115175 [details]
Patch
Comment 18 Nikolas Zimmermann 2011-11-15 09:09:44 PST
Comment on attachment 115175 [details]
Patch

Looks good.
Comment 19 WebKit Review Bot 2011-11-15 10:21:33 PST
Comment on attachment 115175 [details]
Patch

Clearing flags on attachment: 115175

Committed r100291: <http://trac.webkit.org/changeset/100291>
Comment 20 WebKit Review Bot 2011-11-15 10:21:38 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 Csaba Osztrogonác 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 Stephen Chenney 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 Stephen Chenney 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 Peter Kasting 2011-11-15 14:56:47 PST
This commit was rolled back in r100312.
Comment 25 Stephen Chenney 2011-11-15 15:31:29 PST
Radar ID: 10450621
Comment 26 Stephen Chenney 2011-11-16 11:33:10 PST
Created attachment 115415 [details]
Patch

Patch to workaround the CG bug. Still checking win expectations so review only at this time
Comment 27 Darin Adler 2011-11-16 12:22:13 PST
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?
Comment 28 Darin Adler 2011-11-16 12:23:19 PST
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.
Comment 29 Darin Adler 2011-11-16 12:23:52 PST
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?
Comment 30 Stephen Chenney 2011-11-16 12:29:28 PST
(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.
Comment 31 Stephen Chenney 2011-11-16 12:32:08 PST
(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.
Comment 32 Stephen Chenney 2011-11-16 12:33:57 PST
(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.
Comment 33 WebKit Review Bot 2011-11-16 14:23:07 PST
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
Comment 34 Stephen Chenney 2011-11-16 14:50:37 PST
Created attachment 115455 [details]
Patch

I like this fix better. Much more efficient.
Comment 35 Stephen Chenney 2011-11-16 14:58:11 PST
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.
Comment 36 Stephen Chenney 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 Stephen Chenney 2011-11-18 07:21:37 PST
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.
Comment 38 Stephen Chenney 2011-11-18 13:01:47 PST
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.
Comment 39 Stephen Chenney 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 Darin Adler 2011-11-18 15:35:13 PST
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.
Comment 41 Stephen Chenney 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 Stephen Chenney 2011-11-21 11:42:07 PST
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.
Comment 43 WebKit Review Bot 2011-12-01 12:14:50 PST
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
Comment 44 Stephen Chenney 2011-12-01 13:14:47 PST
Created attachment 117473 [details]
Patch
Comment 45 WebKit Review Bot 2011-12-01 13:17:02 PST
Comment on attachment 117473 [details]
Patch

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 Stephen Chenney 2011-12-01 14:38:26 PST
Created attachment 117492 [details]
Patch
Comment 47 Darin Adler 2011-12-01 14:48:28 PST
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.
Comment 48 Darin Adler 2011-12-01 14:48:54 PST
Do we have Radar bug numbers for the Core Graphics problems?
Comment 49 Stephen Chenney 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 WebKit Review Bot 2011-12-01 18:37:14 PST
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
Comment 51 Stephen Chenney 2011-12-02 07:39:02 PST
Created attachment 117618 [details]
Patch

One more attempt to get this through the queue
Comment 52 WebKit Review Bot 2011-12-02 08:37:55 PST
Comment on attachment 117618 [details]
Patch

Clearing flags on attachment: 117618

Committed r101805: <http://trac.webkit.org/changeset/101805>
Comment 53 WebKit Review Bot 2011-12-02 08:38:03 PST
All reviewed patches have been landed.  Closing bug.
Comment 54 Darin Adler 2011-12-02 09:19:49 PST
I believe this patch caused about 100 canvas regression tests to start failing on Mac.
Comment 55 Darin Adler 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 Darin Adler 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 Darin Adler 2011-12-02 09:50:35 PST
Or perhaps there is just a bug in the isEmpty implementation we landed.
Comment 58 Stephen Chenney 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 Stephen Chenney 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 Stephen Chenney 2011-12-02 13:50:21 PST
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.
Comment 61 WebKit Review Bot 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 Stephen Chenney 2011-12-02 14:01:28 PST
Created attachment 117687 [details]
Patch

Remove tabs in Changelogs.
Comment 63 WebKit Review Bot 2011-12-02 21:34:58 PST
Comment on attachment 117687 [details]
Patch

Clearing flags on attachment: 117687

Committed r101903: <http://trac.webkit.org/changeset/101903>
Comment 64 WebKit Review Bot 2011-12-02 21:35:07 PST
All reviewed patches have been landed.  Closing bug.
Comment 65 Stephen Chenney 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 Stephen Chenney 2012-01-05 10:17:16 PST
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.
Comment 67 Eric Seidel (no email) 2012-01-05 10:20:25 PST
Comment on attachment 121294 [details]
Patch

Won't this affect mac results?
Comment 68 Eric Seidel (no email) 2012-01-05 10:21:55 PST
(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?
Comment 69 Stephen Chenney 2012-01-05 10:25:58 PST
(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.
Comment 70 Simon Fraser (smfr) 2012-01-05 10:28:33 PST
Do we still think that there's a Core Graphics bug here?
Comment 71 Stephen Chenney 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 Stephen Chenney 2012-01-25 20:10:03 PST
Comment on attachment 121294 [details]
Patch

Will have a new patch up tomorrow.
Comment 73 Stephen Chenney 2012-01-26 05:48:49 PST
Created attachment 124108 [details]
Patch

Ready for review and commit now that WK71820 is fixed
Comment 74 Nikolas Zimmermann 2012-01-26 06:43:05 PST
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+.
Comment 75 Nikolas Zimmermann 2012-01-26 06:43:33 PST
Note, you didn't add expectations for Chromium, was that desired?
Comment 76 Stephen Chenney 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 Stephen Chenney 2012-01-26 07:20:12 PST
Created attachment 124116 [details]
Patch

Updated patch to resovle conflicts
Comment 78 Nikolas Zimmermann 2012-01-26 07:24:46 PST
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?
Comment 79 WebKit Review Bot 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 Stephen Chenney 2012-01-26 07:29:23 PST
(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. :-(
Comment 81 Stephen Chenney 2012-01-26 10:03:10 PST
Created attachment 124134 [details]
Patch
Comment 82 WebKit Review Bot 2012-01-26 11:18:33 PST
Comment on attachment 124134 [details]
Patch

Clearing flags on attachment: 124134

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