Bug 43161 – 2d.path.clip.empty.html test is failing

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Matthew Delaney]

2d.path.clip.empty.html test is failing because of a difference between CG's notion of clipping against an absent path and that of the canvas spec.

The canvas spec says:
"The clip() method must create a new clipping region by calculating the intersection of the current clipping region and the area described by the current path, using the non-zero winding number rule."
[http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#dom-context-2d-clip]

Whereas under the CGContext spec, the function "CGContextClip" says:
"If the context does not have a current path, the function does nothing."
Note: This is currently what the CG port is using to clip.
[http://developer.apple.com/mac/library/documentation/GraphicsImaging/Reference/CGContext/Reference/reference.html#//apple_ref/c/func/CGContextClip]

The canvas spec appears to be using the fill rules outline in SVG: [http://www.w3.org/TR/SVG/painting.html#FillProperties]

Philip and I both agree that the canvas spec should be a little clearer about which definitions it's using and about how the null/base type bases should be handled.

[Matthew Delaney]

Created attachment 62895 [details]
Patch

[Adele Peterson]

Comment on attachment 62895 [details]
Patch

I like to put my comments after the function name.  I'm not sure there's any rule about that, but I think that's how most people do it.  Actually, I'm kind of confused by the comment.  I get that you're trying to match the spec.  What does the spec say?  Does it talk about nonexistent paths?  How is a nonexistent path different from an empty path?

[Matthew Delaney]

If the canvas spec is assumed to be using the common non-zero winding number rule (as outlined in the SVG spec), then in the case where there are no paths specified (e.g. ctx.beginPath() was called, but no other path operations were called thereafter), I'm assuming that the whole current clipping region has a winding number of zero. 
=>The area described by the current path is no area. 
=> Calling ctx.clip() with no path sets the clipping region to be nothing (the intersection of nothing and anything is nothing). 

The canvas spec doesn't define what a path is, let alone define what an empty or non-existent path is. The platforms do however have notions of empty paths. It's not clear to me if all the platforms all agree on what an empty path is (or even paths and subpaths in general). CG, for example, says that an empty path is a path that contains no elements. So, from talking with Philip (and noting Firefox and Opera also implement it this way), we agree that it makes more sense to assume the no-path case of the non-zero winding number rule like SVG does and compute the resulting clipping region from calling clip to be the null region.

[Matthew Delaney]

If the canvas spec is assumed to be using the common non-zero winding number rule (as outlined in the SVG spec), then in the case where there are no paths specified (e.g. ctx.beginPath() was called, but no other path operations were called thereafter), I'm assuming that the whole current clipping region has a winding number of zero. 
=>The area described by the current path is no area. 
=> Calling ctx.clip() with no path sets the clipping region to be nothing (the intersection of nothing and anything is nothing). 

The canvas spec doesn't define what a path is, let alone define what an empty or non-existent path is. The platforms do however have notions of empty paths. It's not clear to me if all the platforms all agree on what an empty path is (or even paths and subpaths in general). CG, for example, says that an empty path is a path that contains no elements. So, from talking with Philip (and noting Firefox and Opera also implement it this way), we agree that it makes more sense to assume the no-path case of the non-zero winding number rule like SVG does and compute the resulting clipping region from calling clip to be the null region.

[Matthew Delaney]

Oops, refresh problemo.

The last thing I forgot to add was the fact that CG happens to consider the base case of having "no paths" as one in which it will not clip anything. This differs from our interpretation of the spec (the whole thing I was saying in the last comment above) and thus this patch essentially "tricks" CG into clipping against a path defined area of nothing by passing it a path with a singular point instead of the non-existant path.

[Matthew Delaney]

Created attachment 63006 [details]
Made Changelog cleaner

Cleaned up the changelog to make a little more sense at a higher level.

[Darin Adler]

Comment on attachment 63006 [details]
Made Changelog cleaner

The MIME type needed to be set on this patch, and the "patch" check box checked.

[Matthew Delaney]

Created attachment 63008 [details]
take 2

[Darin Adler]

Comment on attachment 63006 [details]
Made Changelog cleaner

> +    if (path.isEmpty())
> +        CGContextMoveToPoint(context, 0.0, 0.0);
> +    else
> +        CGContextAddPath(context, path.platformPath());

This code needs a "why" comment. What does CoreGraphics do when you pass an empty path to CGContextAddPath? How does that differ from what CGContextMoveToPoint does? Why do we want GraphicsContext to do this different thing?

I suspect this patch is right, but the code is too mysterious without a comment. But the comment should be brief, and answer the question, "Why do we need to have a special case for empty paths?"

The answer to "why" can't be: "This is what I think the canvas spec says to do." That's too vague and this is GraphicsContext, not canvas.

[Darin Adler]

Comment on attachment 63008 [details]
take 2

Needs a “why” comment as I mentioned above.

[Matthew Delaney]

Created attachment 63026 [details]
take 3

Added in code comment. Modified description in changelog to better match what the change is actually trying to achieve.

[Darin Adler]

Comment on attachment 63026 [details]
take 3

> +    // If the path is empty, CGContextClip will do nothing.
> +    // If the path contains only a point, CGContextClip will reduce the
> +    // clipping region to nothing. Since, we want this method to reduce
> +    // the clipping region to nothing when the path is empty, swapping
> +    // a trivial path (one containing only a point) in for the empty 
> +    // path achieves our desired behavior.

Great comment. You could probably shorten it a bit, but this is exactly the kind of comment I was looking for.

An even more efficient way to get the desired result would be to check for the empty patch and call CGContextClipToRect(context, CGRectZero) instead of CGContextBeginPath/CGContextAddPath/CGContextClip in that case.

[WebKit Commit Bot]

Comment on attachment 63026 [details]
take 3

Rejecting patch 63026 from commit-queue.

Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1
Last 500 characters of output:
/usr/bin/yacc
    /bin/sh -c /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Script-5DF50887116F3077005202AB.sh
** BUILD FAILED **

The following build commands failed:
WebCore:
	Distributed-CompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/i386/GraphicsContextCG.o /Users/eseidel/Projects/CommitQueue/WebCore/platform/graphics/cg/GraphicsContextCG.cpp normal i386 c++ com.apple.compilers.gcc.4_2
(1 failure)


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

[Matthew Delaney]

Created attachment 63122 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 63122 [details]
Patch

Rejecting patch 63122 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Darin Adler', u'--force']" exit_code: 1
Parsed 4 diffs from patch file(s).
patching file WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebCore/platform/graphics/cg/GraphicsContextCG.cpp
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/platform/mac/Skipped
Hunk #1 FAILED at 164.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/mac/Skipped.rej

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

[Matthew Delaney]

Created attachment 64043 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 64043 [details]
Patch

Clearing flags on attachment: 64043

Committed r65118: <http://trac.webkit.org/changeset/65118>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.