Bug 27187 – Match Gecko behaviour for canvas path mutation APIs on an empty path

• Component: DOM
• Status: RESOLVED FIXED

[Oliver Hunt]

Gecko treats lineTo, etc as planting a new point in the canvas path if the current path is empty.  Failure to do this results in a significant performance penalty (due to CGs incessant logging) and site breakages like the one linked in this bug.

[Oliver Hunt]

Created attachment 32623 [details]
fixeration!

[Simon Fraser (smfr)]

Comment on attachment 32623 [details]
fixeration!

> +        Simple API change, check for the mpty path and add appropriate point if necessary.

Typo here. r=me

[Oliver Hunt]

Committed r45759

[Dean McNamee]

(In reply to comment #3)
> Committed r45759

This broke a bunch of <canvas> stuff.

Part of the bug is that for quadraticCurveTo and bezierCurveTo you move to the control point, which I don't think makes a lot of sense, since the control point does not lie on the path.

I am digging into the other things that broke now.

Thanks

[Dean McNamee]

(In reply to comment #4)
> (In reply to comment #3)
> > Committed r45759
> 
> This broke a bunch of <canvas> stuff.
> 
> Part of the bug is that for quadraticCurveTo and bezierCurveTo you move to the
> control point, which I don't think makes a lot of sense, since the control
> point does not lie on the path.
> 
> I am digging into the other things that broke now.
> 
> Thanks

Ok, so here is the problem, it might be Skia specific:

From Skia:

bool SkPath::isEmpty() const {
    SkDEBUGCODE(this->validate();)

    int count = fVerbs.count();
    return count == 0 || (count == 1 && fVerbs[0] == kMove_Verb);
}

And the documentation specifically states that isEmpty returns whether there is any lines or curves, not whether there is a move to point.

There are no comments for WebCore::Path::isEmpty(), so I don't know what the expected behavior is.  I can understand going both ways with isEmpty(), and in fact it seems like there should be two separate methods, one which asks if there are any points in the path, and one which asks if there are any lines or curves in the path.

An additional option, if it is decided that isEmpty should mean there are no points at all in the path, would be to change the Skia backend to reflect this.  However, that will probably break some other things, and additionally after a brief skim I cannot see how to get that information out of Skia.  This is something I could add to Skia.

In the mean time, it would be nice not to have our <canvas> tag completely broken :(

Thanks
-- dean

[Dean McNamee]

After digging a bit more, cairo does this with cairo_has_current_point.  I think this is a more accurate name, hasCurrentPoint(), and it is not necessarily the same as the path being empty.

Does it make sense to add hasCurrentPoint() to WebCore::Path?

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > Committed r45759
> > 
> > This broke a bunch of <canvas> stuff.
> > 
> > Part of the bug is that for quadraticCurveTo and bezierCurveTo you move to the
> > control point, which I don't think makes a lot of sense, since the control
> > point does not lie on the path.
> > 
> > I am digging into the other things that broke now.
> > 
> > Thanks
> 
> Ok, so here is the problem, it might be Skia specific:
> 
> From Skia:
> 
> bool SkPath::isEmpty() const {
>     SkDEBUGCODE(this->validate();)
> 
>     int count = fVerbs.count();
>     return count == 0 || (count == 1 && fVerbs[0] == kMove_Verb);
> }
> 
> And the documentation specifically states that isEmpty returns whether there is
> any lines or curves, not whether there is a move to point.
> 
> There are no comments for WebCore::Path::isEmpty(), so I don't know what the
> expected behavior is.  I can understand going both ways with isEmpty(), and in
> fact it seems like there should be two separate methods, one which asks if
> there are any points in the path, and one which asks if there are any lines or
> curves in the path.
> 
> An additional option, if it is decided that isEmpty should mean there are no
> points at all in the path, would be to change the Skia backend to reflect this.
>  However, that will probably break some other things, and additionally after a
> brief skim I cannot see how to get that information out of Skia.  This is
> something I could add to Skia.
> 
> In the mean time, it would be nice not to have our <canvas> tag completely
> broken :(
> 
> Thanks
> -- dean

[Oliver Hunt]

(In reply to comment #6)
> After digging a bit more, cairo does this with cairo_has_current_point.  I
> think this is a more accurate name, hasCurrentPoint(), and it is not
> necessarily the same as the path being empty.
> 
> Does it make sense to add hasCurrentPoint() to WebCore::Path?
Seems reasonable -- but we'll want a different bug to track that

[Dmitry Titov]

Created separate bug to track the proposed hasCurrentPoint() addition: https://bugs.webkit.org/show_bug.cgi?id=27266

[Dean McNamee]

(In reply to comment #8)
> Created separate bug to track the proposed hasCurrentPoint() addition:
> https://bugs.webkit.org/show_bug.cgi?id=27266

On more thinking I don't actually think this patch is correct:

From http://dev.w3.org/html5/spec/Overview.html:

The lineTo(x, y) method must do nothing if the context has no subpaths. Otherwise, it must connect the last point in the subpath to the given point (x, y) using a straight line, and must then add the given point (x, y) to the subpath.

This means if I do
lineTo(10, 10); lineTo(20, 20);

I should get no lines, since I never started the path with a moveTo().  With this patch you get the second line.

[Oliver Hunt]

Yes, unfortunately firefox violates the spec in a way that people seem to depend on.  I've already emailed whatwg about this.  It's safe to move to the firefox behaviour as it's a relaxation of existing API.