Bug 97333 - Implement Canvas Path object
: Implement Canvas Path object
Status: RESOLVED FIXED
: WebKit
Canvas
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
: http://dev.w3.org/html5/2dcontext/#pa...
: WebExposed
: 107473 108246 108508 108587 109097 109997 131114
: 107744
  Show dependency treegraph
 
Reported: 2012-09-21 06:33 PST by
Modified: 2014-04-03 14:05 PST (History)


Attachments
Patch (43.23 KB, patch)
2012-09-21 07:24 PST, Dirk Schulze
no flags Review Patch | Details | Formatted Diff | Diff
Patch (44.61 KB, patch)
2012-09-21 08:18 PST, Dirk Schulze
no flags Review Patch | Details | Formatted Diff | Diff
Patch (43.37 KB, patch)
2013-01-22 10:44 PST, Dirk Schulze
no flags Review Patch | Details | Formatted Diff | Diff
Patch (43.41 KB, patch)
2013-01-22 12:16 PST, Dirk Schulze
no flags Review Patch | Details | Formatted Diff | Diff
Patch (43.94 KB, patch)
2013-01-22 12:50 PST, Dirk Schulze
no flags Review Patch | Details | Formatted Diff | Diff
Patch (44.85 KB, patch)
2013-01-22 16:31 PST, Dirk Schulze
no flags Review Patch | Details | Formatted Diff | Diff
Patch (44.92 KB, patch)
2013-01-22 18:54 PST, Dirk Schulze
no flags Review Patch | Details | Formatted Diff | Diff
Patch (45.58 KB, patch)
2013-01-23 12:15 PST, Dirk Schulze
dino: review+
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 2012-09-21 06:33:28 PST
The HTML Canvas specification and the WHAT WG HTML specification define the Path object[1][2]. The Path and the CanvasRenderingContext2D objects share some graphics operations like: closePath, lineTo, moveTo, arc, arcTo, ellipse (not implemented yet), quadraticCurveTo, cubicCurveTo, rect [3]. But instead of an Canvas, the operations get applied to a Path object. Furthermore the object allows adding other Paths with transform. A Path object can then be drawn on a Canvas.

[1] http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#path-objects
[2] http://dev.w3.org/html5/2dcontext/#path-objects
[3] http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#2dcontext
------- Comment #1 From 2012-09-21 07:24:36 PST -------
Created an attachment (id=165130) [details]
Patch

Test if it builds on all systems.
------- Comment #2 From 2012-09-21 08:02:22 PST -------
(From update of attachment 165130 [details])
Attachment 165130 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13916636
------- Comment #3 From 2012-09-21 08:03:58 PST -------
(From update of attachment 165130 [details])
Attachment 165130 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13951714
------- Comment #4 From 2012-09-21 08:18:47 PST -------
Created an attachment (id=165134) [details]
Patch

Test if it builds on all systems.
------- Comment #5 From 2012-09-21 08:47:45 PST -------
(From update of attachment 165134 [details])
Attachment 165134 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13954638
------- Comment #6 From 2012-09-21 09:08:25 PST -------
(From update of attachment 165134 [details])
Attachment 165134 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13954647

New failing tests:
fast/dom/constructed-objects-prototypes.html
------- Comment #7 From 2012-09-21 09:45:42 PST -------
I think we might want to wait for this API to settle down before implementing it. I don't really like the new API.
------- Comment #8 From 2012-09-21 11:53:43 PST -------
(In reply to comment #7)
> I think we might want to wait for this API to settle down before implementing it. I don't really like the new API.

As a compromise, can we put the new API changes behind a flag? SVG WG decided to reuse the concept of Path for their own use. It would be extremely helpful to experiment with it. The flag could be called CANVAS_NEW or CANVAS_NEXT :) (Hope that someone has better suggestions).
------- Comment #9 From 2012-09-21 12:29:26 PST -------
smfr: The API is entirely "settled" (there's no outstanding feedback on it, last I checked). If you would like to unsettle it, please send e-mail to the WHATWG list with your concerns! (In particular, knowing that you don't want to implement it is very valuable feedback, but knowing why not would make it much easier for us to fix it.)
------- Comment #10 From 2012-09-22 00:24:44 PST -------
To the build problems: I tried a build from scratch on Mac and it works for me. Maybe the bots need to do the same.
------- Comment #11 From 2012-09-24 06:34:13 PST -------
Can you add a comment above transformIsInvertible()? What is it here? Who would override it?
------- Comment #12 From 2013-01-08 14:28:29 PST -------
*** Bug 82790 has been marked as a duplicate of this bug. ***
------- Comment #13 From 2013-01-08 15:01:37 PST -------
(In reply to comment #11)
> Can you add a comment above transformIsInvertible()? What is it here? Who would override it?

Not sure what you mean with "Who would override it?". The problem that some libraries are focused with is the following:

ctx.scale(0)
ctx.lineTo(100,100);

freezes the library. This was in particular the case for Cairo Graphics. For this reason it is necessary to check before applying the function and stop there. This problem might be fixed in Cairo Graphics already and can finally be removed. This is in the scope of a different patch and a different investigation.
------- Comment #14 From 2013-01-15 08:45:47 PST -------
*** Bug 96826 has been marked as a duplicate of this bug. ***
------- Comment #15 From 2013-01-15 10:23:45 PST -------
What is the status of this. Are you waiting on more reviews?
------- Comment #16 From 2013-01-15 11:14:13 PST -------
(In reply to comment #15)
> What is the status of this. Are you waiting on more reviews?

We don't have a consensus on how we proceed with the proposal, see comment 7.
------- Comment #17 From 2013-01-15 11:31:12 PST -------
#7 smfr

Do you think there will be changes to the API, given the statement in #9? Should we continue to wait?
------- Comment #18 From 2013-01-15 12:36:07 PST -------
(In reply to comment #17)

> Do you think there will be changes to the API, given the statement in #9? Should we continue to wait?

Might as well go ahead, but please put this behind an ENABLE flag. It's a Web-facing feature so implementations might want to hold off on releasing it if they plan to request significant changes to the specification. I'm not aware of any, but there are other reasons such as content compatibility (e.g. the new API has a name clash with an important site) that means people might want to live on this before shipping.
------- Comment #19 From 2013-01-15 12:50:48 PST -------
(In reply to comment #18)
> (In reply to comment #17)
> 
> > Do you think there will be changes to the API, given the statement in #9? Should we continue to wait?
> 
> Might as well go ahead, but please put this behind an ENABLE flag. It's a Web-facing feature so implementations might want to hold off on releasing it if they plan to request significant changes to the specification. I'm not aware of any, but there are other reasons such as content compatibility (e.g. the new API has a name clash with an important site) that means people might want to live on this before shipping.

I think that the API should change. The current API is not all that useful and will not accomplish what people want (for instance, you can't use it to aggregate paths)
------- Comment #20 From 2013-01-15 12:56:38 PST -------
(In reply to comment #7 and comment #19)

As always, if you want the API to change, send use cases and design constraints to the WHATWG list. I'm happy to update the API to address new use cases. (Please focus on problems that authors try to address that the current spec doesn't handle, not alternative proposed solutions, in initial e-mails on any topic.)
------- Comment #21 From 2013-01-16 11:52:49 PST -------
How about landing an implementation that reflects the current state of the draft specification as "webkitPath"?
------- Comment #22 From 2013-01-16 21:36:38 PST -------
(In reply to comment #21)
> How about landing an implementation that reflects the current state of the draft specification as "webkitPath"?

I'll upload a new patch with the flag for canvas path first. Actually, Rik's request is not that far from the WHAT WG version, it just goes beyond it (with very good use cases). It should not be seen as counter proposal.

I am not so sure about prefixing this new object. I would rather not compile it by default on the bots until we are more certain about the API. That gives us a lot more freedom to change our mind.
------- Comment #23 From 2013-01-18 12:53:31 PST -------
No need to prefix. I will make sure the spec doesn't conflict with shipped implementations. If changes are desired to the spec, make sure the use cases are mailed to the WHATWG list or a bug is filed.
------- Comment #24 From 2013-01-21 13:04:57 PST -------
Making this to a master bug report.
------- Comment #25 From 2013-01-22 10:44:36 PST -------
Created an attachment (id=184013) [details]
Patch
------- Comment #26 From 2013-01-22 10:56:03 PST -------
(From update of attachment 184013 [details])
Attachment 184013 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16031730
------- Comment #27 From 2013-01-22 10:58:34 PST -------
(From update of attachment 184013 [details])
Attachment 184013 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16040699
------- Comment #28 From 2013-01-22 11:05:00 PST -------
(From update of attachment 184013 [details])
Attachment 184013 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16041740
------- Comment #29 From 2013-01-22 11:24:00 PST -------
(From update of attachment 184013 [details])
Attachment 184013 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16033762
------- Comment #30 From 2013-01-22 11:24:42 PST -------
(From update of attachment 184013 [details])
Attachment 184013 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16065018
------- Comment #31 From 2013-01-22 11:32:28 PST -------
(From update of attachment 184013 [details])
Attachment 184013 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16037735
------- Comment #32 From 2013-01-22 11:53:01 PST -------
(From update of attachment 184013 [details])
Attachment 184013 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16045725
------- Comment #33 From 2013-01-22 12:16:33 PST -------
Created an attachment (id=184032) [details]
Patch

Patch. Should build on Mac now.
------- Comment #34 From 2013-01-22 12:28:52 PST -------
(From update of attachment 184032 [details])
Attachment 184032 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16033805
------- Comment #35 From 2013-01-22 12:31:36 PST -------
(From update of attachment 184032 [details])
Attachment 184032 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16063158
------- Comment #36 From 2013-01-22 12:50:40 PST -------
Created an attachment (id=184036) [details]
Patch

Patch. Should fix more builds.
------- Comment #37 From 2013-01-22 14:01:07 PST -------
(From update of attachment 184036 [details])
Attachment 184036 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16031825

New failing tests:
platform/chromium/virtual/gpu/fast/canvas/canvas-path-object.html
fast/canvas/canvas-path-object.html
------- Comment #38 From 2013-01-22 14:43:00 PST -------
(From update of attachment 184036 [details])
Attachment 184036 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16041824

New failing tests:
platform/chromium/virtual/gpu/fast/canvas/canvas-path-object.html
fast/canvas/canvas-path-object.html
------- Comment #39 From 2013-01-22 16:31:26 PST -------
Created an attachment (id=184070) [details]
Patch

Patch. Update build files after changes from other patches.
------- Comment #40 From 2013-01-22 17:22:09 PST -------
(From update of attachment 184070 [details])
Attachment 184070 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16065192

New failing tests:
platform/chromium/virtual/gpu/fast/canvas/canvas-path-object.html
------- Comment #41 From 2013-01-22 18:54:38 PST -------
Created an attachment (id=184104) [details]
Patch

Patch.
------- Comment #42 From 2013-01-23 10:58:31 PST -------
(From update of attachment 184104 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=184104&action=review

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:17
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY

APPLE -> ADOBE ?

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:39
> +void CanvasPathMethods::closePath()
> +{
> +    if (m_path.isEmpty())
> +        return;
> +

Should you check the transform here as well?

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:47
> +    if (!isfinite(x) | !isfinite(y))

||

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:56
> +    if (!isfinite(x) | !isfinite(y))

||

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:65
> +    FloatPoint p1 = FloatPoint(x, y);
> +    if (!m_path.hasCurrentPoint())
> +        m_path.moveTo(p1);
> +    else if (p1 != m_path.currentPoint())
> +        m_path.addLineTo(FloatPoint(x, y));

Use p1 in addLineTo since you have it?

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:70
> +    if (!isfinite(cpx) | !isfinite(cpy) | !isfinite(x) | !isfinite(y))

||

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:79
> +    if (!m_path.hasCurrentPoint())
> +        m_path.moveTo(FloatPoint(cpx, cpy));
> +
> +    FloatPoint p1 = FloatPoint(x, y);
> +    if (p1 != m_path.currentPoint())
> +        m_path.addQuadCurveTo(FloatPoint(cpx, cpy), p1);

Is it defined behaviour that if (cpx, cpy) == (x, y) and you didn't have a control point then you still do the moveTo? In fact, it seems a little weird to move to the control point at all.

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:84
> +    if (!isfinite(cp1x) | !isfinite(cp1y) | !isfinite(cp2x) | !isfinite(cp2y) | !isfinite(x) | !isfinite(y))

||

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:93
> +    if (!m_path.hasCurrentPoint())
> +        m_path.moveTo(FloatPoint(cp1x, cp1y));
> +
> +    FloatPoint p1 = FloatPoint(x, y);
> +    if (p1 != m_path.currentPoint())
> +        m_path.addBezierCurveTo(FloatPoint(cp1x, cp1y), FloatPoint(cp2x, cp2y), p1);

Same question as for quad above.

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:99
> +    if (!isfinite(x1) | !isfinite(y1) | !isfinite(x2) | !isfinite(y2) | !isfinite(r))

||

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:114
> +    if (!m_path.hasCurrentPoint())
> +        m_path.moveTo(p1);

Again, is this defined? In this case you're moving to the start of the arc, then not adding the arc, so subsequent calls would not draw from the end. I have no idea what is correct, but that seems weird.

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:124
> +    if (!isfinite(x) | !isfinite(y) | !isfinite(r) | !isfinite(sa) | !isfinite(ea))

||

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:133
> +        // The arc is empty but we still need to draw the connecting line

Nit: trailing .

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:141
> +    // If 'sa' and 'ea' differ by more than 2Pi, just add a circle starting/ending at 'sa'

Nit: trailing .

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:151
> +    if (anticlockwise && sa - ea >= 2 * piFloat) {
> +        m_path.addArc(FloatPoint(x, y), r, sa, sa - 2 * piFloat, anticlockwise);
> +        return;
> +    }
> +    if (!anticlockwise && ea - sa >= 2 * piFloat) {
> +        m_path.addArc(FloatPoint(x, y), r, sa, sa + 2 * piFloat, anticlockwise);
> +        return;
> +    }
> +
> +    m_path.addArc(FloatPoint(x, y), r, sa, ea, anticlockwise);

Might as well create a variable FloatPoint(x, y)

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:159
> +    if (!isfinite(x) || !isfinite(y) || !isfinite(width) || !isfinite(height))

This one is ok!! :)

> Source/WebCore/html/canvas/CanvasPathMethods.h:17
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY

APPLE!

> Source/WebCore/html/canvas/CanvasPathMethods.h:49
> +    void arc(float x, float y, float r, float sa, float ea, bool clockwise, ExceptionCode&);

Watch out - you name this clockwise here and anticlockwise in the implementation!

> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:133
>               in [Optional=DefaultIsUndefined] boolean anticlockwise)
>          raises (DOMException);
> +
>      void fill(in [Optional] DOMString winding);

Intentional?

> Source/WebCore/html/canvas/DOMPath.h:17
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY

APPLE!

> Source/WebCore/html/canvas/DOMPath.idl:17
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY

APPLE!

> Source/WebCore/html/canvas/DOMPath.idl:63
> +             in [Optional=DefaultIsUndefined] boolean anticlockwise)

And anticlockwise here.
------- Comment #43 From 2013-01-23 12:07:55 PST -------
(From update of attachment 184104 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=184104&action=review

>> Source/WebCore/html/canvas/CanvasPathMethods.cpp:39
>> +
> 
> Should you check the transform here as well?

Very good question. I checked the spec, but couldn't find any wording what happens on a non-invertible CTM. Implementations are fairly different.

Here are two examples:
http://jsfiddle.net/Dghuh/1/
http://jsfiddle.net/Dghuh/2/

The first one does scale(0), which should make the CTM non-invertibe, we still apply lineTo and closePath for some reason. IE and FF refuse to draw anything.

The second does setTransform(0,0,0,0,0), which should reset the CTM to a zero matrix (again, not invertible). IE, Opera and FF draw a line to 0,0 and close the path afterwards (which kind of makes sense, since the universe is convoluted to one point). We refuse the lineTo command and close the path as expected.

There is clearly a need to specify this case properly. For now, I preserve our behavior.

>> Source/WebCore/html/canvas/CanvasPathMethods.cpp:79
>> +        m_path.addQuadCurveTo(FloatPoint(cpx, cpy), p1);
> 
> Is it defined behaviour that if (cpx, cpy) == (x, y) and you didn't have a control point then you still do the moveTo? In fact, it seems a little weird to move to the control point at all.

"The quadraticCurveTo(cpx, cpy, x, y) method must ensure there is a subpath for (cpx, cpy)"

and later:

"When the user agent is to ensure there is a subpath for a coordinate (x, y) on a path, the user agent must check to see if the path has any subpaths, and if it does not, then the user agent must create a new subpath with the point (x, y) as its first (and only) point, as if the moveTo() method had been called."

We are following the spec, or better, the spec is following us ;)

>> Source/WebCore/html/canvas/CanvasPathMethods.cpp:93
>> +        m_path.addBezierCurveTo(FloatPoint(cp1x, cp1y), FloatPoint(cp2x, cp2y), p1);
> 
> Same question as for quad above.

Ditto: "method must ensure there is a subpath for (cp1x, cp1y)".

>> Source/WebCore/html/canvas/CanvasPathMethods.cpp:114
>> +        m_path.moveTo(p1);
> 
> Again, is this defined? In this case you're moving to the start of the arc, then not adding the arc, so subsequent calls would not draw from the end. I have no idea what is correct, but that seems weird.

Yes: " method must first ensure there is a subpath for (x1, y1)"

Definition of "ensure there is a subpath for" is above.

>> Source/WebCore/html/canvas/CanvasPathMethods.cpp:151
>> +    m_path.addArc(FloatPoint(x, y), r, sa, ea, anticlockwise);
> 
> Might as well create a variable FloatPoint(x, y)

The point is never used twice. I wouldn't do it right now.

>> Source/WebCore/html/canvas/CanvasPathMethods.cpp:159
>> +    if (!isfinite(x) || !isfinite(y) || !isfinite(width) || !isfinite(height))
> 
> This one is ok!! :)

Hehe, true.

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:133
>>      void fill(in [Optional] DOMString winding);
> 
> Intentional?

Yes, to separate the functions that can be moved from the functions that can't.
------- Comment #44 From 2013-01-23 12:15:10 PST -------
Created an attachment (id=184279) [details]
Patch

Fixed requested changes.
------- Comment #45 From 2013-01-23 15:05:08 PST -------
Committed r140604: <http://trac.webkit.org/changeset/140604>
------- Comment #46 From 2013-01-29 16:15:58 PST -------
Reopen this bug report as meta bug.
------- Comment #47 From 2014-04-03 14:05:14 PST -------
All pending patches landed.