Spec: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#dom-path-addpath This was also implemented in WebKit and Blink.

This should be the last addition for a while :-)

Comment on attachment 8393918 [details] [diff] [review] Bug 985801 - Add implementation for Path2D::AddPath Review of attachment 8393918 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/CanvasRenderingContext2D.cpp @@ +4375,5 @@ > + > + if (!transform.IsIdentity()) { > + if (!transform.Invert()) { > + NS_WARNING("Could not invert transform"); > + return; Can you add a comment explaining why this satisfies the specced behavior for singular transforms? @@ +4377,5 @@ > + if (!transform.Invert()) { > + NS_WARNING("Could not invert transform"); > + return; > + } > + Trailing whitespace ::: dom/webidl/CanvasRenderingContext2D.webidl @@ +318,5 @@ > Constructor, > Constructor(Path2D other)] > interface Path2D > +{ > + void addPath(Path2D path, optional SVGMatrix transformation); I don't understand why this isn't part of CanvasPathMethods. Can you explain why?

Comment on attachment 8393918 [details] [diff] [review] Bug 985801 - Add implementation for Path2D::AddPath Review of attachment 8393918 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/CanvasRenderingContext2D.cpp @@ +4375,5 @@ > + > + if (!transform.IsIdentity()) { > + if (!transform.Invert()) { > + NS_WARNING("Could not invert transform"); > + return; yes, it doesn't match the spec. I will hold off on submitting this until there's more clarity. WK and Blink both exit on non-invertible matrices. ::: dom/webidl/CanvasRenderingContext2D.webidl @@ +318,5 @@ > Constructor, > Constructor(Path2D other)] > interface Path2D > +{ > + void addPath(Path2D path, optional SVGMatrix transformation); No, there's no good reason why this is absent from the 2d context.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2) > Can you add a comment explaining why this satisfies the specced behavior for singular transforms? The spec does not say how to transform a path with a singular matrix. In WebKit we never perform a current path or Path2D action on the context if the CTM or transformation matrix is singular. This basically follows the concept of SVG. > I don't understand why this isn't part of CanvasPathMethods. Can you explain > why? I think this is a valid question and I would really like to have CanvasPathMethods and am willing to do the change in WebKit. In general, the interface would just move from Path2D.idl to CanvasPathMethods.idl. Path2D would have the same support. Given the concerns from Blink, I think it is ok to implement addPath incrementally. Should we get to an agreement, we could just move addPath to CanvasPathMethods.

The IDL changed to optional SVGMatrix? transformation = null but is still part of Path2D. IIRC the concerns from Blink stay. Roc, what do you think how we should proceed?

(In reply to Dirk Schulze from comment #4) > In general, the interface would just move from Path2D.idl to > CanvasPathMethods.idl. Path2D would have the same support. Given the > concerns from Blink, I think it is ok to implement addPath incrementally. > Should we get to an agreement, we could just move addPath to > CanvasPathMethods. What are these "concerns from Blink"?

(In reply to Dirk Schulze from comment #4) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2) > > > Can you add a comment explaining why this satisfies the specced behavior for singular transforms? > > The spec does not say how to transform a path with a singular matrix. In > WebKit we never perform a current path or Path2D action on the context if > the CTM or transformation matrix is singular. This basically follows the > concept of SVG. Can we get the spec fixed to specify this properly?

There was a thread on WhatWG on this: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2014-March/296553.html There was another one where you were leaning towards not changing the spec.

(In reply to Dirk Schulze from comment #4) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2) > > I don't understand why this isn't part of CanvasPathMethods. Can you explain > > why? > > I think this is a valid question and I would really like to have > CanvasPathMethods and am willing to do the change in WebKit. > > In general, the interface would just move from Path2D.idl to > CanvasPathMethods.idl. Path2D would have the same support. Given the > concerns from Blink, I think it is ok to implement addPath incrementally. > Should we get to an agreement, we could just move addPath to > CanvasPathMethods. Has anything happened regarding this? Having addPath on the context would make it much easier to reuse paths, as it enables separating path geometry transforms from stroke style transforms. This demonstrates an issue where a path is supposed to be reused in a transformed context, but the stroke should not be affected by the transform (i.e., the effective lineWidth should be 10px at all points on the path): http://jsbin.com/bufuqani/1/edit It's possible to work around the lack of this capability by reverting the transform on the context and supplying it as an argument to Path2D#addPath on a temporarily-created path: http://jsbin.com/bufuqani/3/edit However, that seems unnecessarily complicated and wasteful.

(In reply to Till Schneidereit [:till] from comment #9) > > This demonstrates an issue where a path is supposed to be reused in a > transformed context, but the stroke should not be affected by the transform > (i.e., the effective lineWidth should be 10px at all points on the path): > http://jsbin.com/bufuqani/1/edit > > It's possible to work around the lack of this capability by reverting the > transform on the context and supplying it as an argument to Path2D#addPath > on a temporarily-created path: > http://jsbin.com/bufuqani/3/edit > > However, that seems unnecessarily complicated and wasteful. It sounds that what you really want is a way to transform a path and the addPath API lets you do it as a side effect. Maybe you should ask for a 'transform' method on the path2d object that returns a new transformed path.

(In reply to Rik Cabanier from comment #10) > Maybe you should ask for a 'transform' method on the path2d object that > returns a new transformed path. It seems like that would be functionally identical to the workaround in the second link: creating a new path and adding the to-be-transformed on to it with the transform given. That works, but requires creating temporary path instances. Which isn't the end of the world, to be sure.

IIUC, the remaining open issue here is whether addPath should be moved to CanvasPathMethods, right? If so, can we land it as a member of Path2D for now? In Shumway, we currently have to polyfill the entirety of Path2D if addPath is missing, severely impacting performance. If the issue is driving this into the tree, I'm happy to do the required work.

(In reply to Till Schneidereit [:till] from comment #12) > IIUC, the remaining open issue here is whether addPath should be moved to > CanvasPathMethods, right? If so, can we land it as a member of Path2D for > now? In Shumway, we currently have to polyfill the entirety of Path2D if > addPath is missing, severely impacting performance. The spec does not clarify if a path argument should still be applied if the second argument describes a singular transformation matrix. IIRC Blink is fine with not applying the path in this cases. Speaking for WebKit, I am fine too. The patch from Rik returns earlier on singular matrices either. Just Hixie doesn't like it though. Since browser vendors seem to agree, I don't see an issue with both... adding addPath to Path2D for now and returning a singular matrix. If Gecko would go this way, I would remove the compiler flag in the WebKit implementation.

(In reply to Dirk Schulze from comment #13) > (In reply to Till Schneidereit [:till] from comment #12) > > IIUC, the remaining open issue here is whether addPath should be moved to > > CanvasPathMethods, right? If so, can we land it as a member of Path2D for > > now? In Shumway, we currently have to polyfill the entirety of Path2D if > > addPath is missing, severely impacting performance. > > The spec does not clarify if a path argument should still be applied if the > second argument describes a singular transformation matrix. > > IIRC Blink is fine with not applying the path in this cases. Speaking for > WebKit, I am fine too. The patch from Rik returns earlier on singular > matrices either. Just Hixie doesn't like it though. > > Since browser vendors seem to agree, I don't see an issue with both... > adding addPath to Path2D for now and returning a singular matrix. Right, I had assumed that that was the accepted resolution, as I didn't see Hixie's opposition. Can you give a link? I only saw http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2014-March/thread.html#296569

(In reply to Till Schneidereit [:till] from comment #14) > > Right, I had assumed that that was the accepted resolution, as I didn't see > Hixie's opposition. Can you give a link? I only saw > http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2014-March/thread. > html#296569 There was no update since then.

(In reply to Dirk Schulze from comment #15) > (In reply to Till Schneidereit [:till] from comment #14) > > > > Right, I had assumed that that was the accepted resolution, as I didn't see > > Hixie's opposition. Can you give a link? I only saw > > http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2014-March/thread. > > html#296569 > > There was no update since then. So Hixie voiced is opposition on IRC or IRL?

(In reply to Till Schneidereit [:till] from comment #12) > IIUC, the remaining open issue here is whether addPath should be moved to > CanvasPathMethods, right? If so, can we land it as a member of Path2D for > now? In Shumway, we currently have to polyfill the entirety of Path2D if > addPath is missing, severely impacting performance. This patch was held up because if the issue that Dirk brought up: what to do with singular matrices. I think we can address that issue later. If I have time, I will update my patch and submit it.

(In reply to Rik Cabanier from comment #17) > I think we can address that issue later. If I have time, I will update my > patch and submit it. Great. If it turns out you don't have time, let me know and I'll drive it in.

try run: https://tbpl.mozilla.org/?tree=Try&rev=9996933c1176

The webidl change needs review from a DOM peer.

Please review the WebIDL change to this patch?

Comment on attachment 8477385 [details] [diff] [review] Add implementation + tests for Path2D::AddPath This doesn't match the spec's IDL. Why not?

(In reply to Boris Zbarsky [:bz] from comment #23) > Comment on attachment 8477385 [details] [diff] [review] > Add implementation + tests for Path2D::AddPath > > This doesn't match the spec's IDL. Why not? We had an agreement on the mailing list to make it optional and this is also how Blink implemented it. See: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2014-March/296579.html The spec has not been updated yet.

Comment on attachment 8477385 [details] [diff] [review] Add implementation + tests for Path2D::AddPath > The spec has not been updated yet. OK. Please make sure a spec issue is filed (and possibly link to the spec issue from the IDL file, if you don't think it will get updated in the very immediate future). >+ const Optional<NonNull<SVGMatrix> >& matrix); Please drop the space in "> >" and the extra space after the '&'. Also, either both arguments should be named aSomething or neither should have the 'a' prefix. Having it for one but not the other is just bizarre. Same in the .cpp. >+ } catch(e) { >+ throw e; >+ ok(false, "unexpected exception thrown in: test_pathconstructor_canvas"); This code makes no sense. Did you mean to have those two lines in the opposite order? Of course this same pattern occurs in other places in the test file.... Those should be fixed too. r=me with those issues dealt with.

Comment on attachment 8478306 [details] [diff] [review] Add implementation + tests for Path2D::AddPath That's fine for the test changes, but please do address my other review comments too. r=me with that.

(In reply to Boris Zbarsky [:bz] from comment #27) > Comment on attachment 8478306 [details] [diff] [review] > Add implementation + tests for Path2D::AddPath > > That's fine for the test changes, but please do address my other review > comments too. > > r=me with that. Done. Saw you comments after I had updated the test so I submitted another patch

You probably want to file an actual w3c bug on the spec...

And also, that last attachment doesn't address all the review comments.

Sorry for missing that. Could you take a quick view to make sure I didn't miss something again?

Comment on attachment 8478341 [details] [diff] [review] Add implementation + tests for Path2D::AddPath Much better, thanks.

Mentioned on Firefox 34 for developers: https://developer.mozilla.org/en-US/Firefox/Releases/34#Interfaces.2FAPIs.2FDOM Reference page: https://developer.mozilla.org/en-US/docs/Web/API/Path2D.addPath Probably doesn't matter, but I stumbled upon the use of ctx.currentTransform in one of the test cases in the patch. It is not implemented yet (bug 928150).