To support the canvas path object, it is necessary to copy paths between different backend. This patch will add support to the gfx layer so a pathbuilder can play into any pathsink.

I'm unsure how I can write patches for this code.

Sorry, I meant that I don't know how to write tests for this code since it's not used yet.

Comment on attachment 821213 [details] [diff] [review] First proposal for replaying paths into pathsinks try build: https://tbpl.mozilla.org/?tree=Try&rev=48620c5a64fe Any suggestions how I can test this?

Comment on attachment 821213 [details] [diff] [review] First proposal for replaying paths into pathsinks Review of attachment 821213 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/2D.h @@ +467,5 @@ > virtual TemporaryRef<Path> Finish() = 0; > + > + /* This plays the path back into a PathSink > + */ > + virtual void ReplayPath(PathSink& sink) = 0; I'm a bit confused as to why this is a method on PathBuilder instead of Path. Can you explain why? ::: gfx/2d/PathCG.cpp @@ +136,5 @@ > + > +void > +PathBuilderCG::ReplayPath(PathSink& sink) > +{ > + CGPathApply(mCGPath, this, PathApplierFunction); Shouldn't you be passing "sink" somewhere in here? :-)

Comment on attachment 821213 [details] [diff] [review] First proposal for replaying paths into pathsinks Review of attachment 821213 [details] [diff] [review]: ----------------------------------------------------------------- Now that I'm further with the Path object, I'm unsure if this is needed. It would still be nice to have though... ::: gfx/2d/2D.h @@ +467,5 @@ > virtual TemporaryRef<Path> Finish() = 0; > + > + /* This plays the path back into a PathSink > + */ > + virtual void ReplayPath(PathSink& sink) = 0; In order to have a Path, you have to call "Finish" on PathBuilder which makes it immutable. This seems like a big limitation since you might want to create a copy of a path while you are still drawing. ::: gfx/2d/PathCG.cpp @@ +136,5 @@ > + > +void > +PathBuilderCG::ReplayPath(PathSink& sink) > +{ > + CGPathApply(mCGPath, this, PathApplierFunction); Yes, good catch!

Comment on attachment 821213 [details] [diff] [review] First proposal for replaying paths into pathsinks Review of attachment 821213 [details] [diff] [review]: ----------------------------------------------------------------- The general idea here seems okay, but a couple of changes to the patch are required. ::: gfx/2d/2D.h @@ +467,5 @@ > virtual TemporaryRef<Path> Finish() = 0; > + > + /* This plays the path back into a PathSink > + */ > + virtual void ReplayPath(PathSink& sink) = 0; You can always still do that. I believe the way we should implement this is by putting it on Path, and I'd prefer something along the lines of 'AppendPathToBuilder'. I'm not sure how this would add any limitations. ::: gfx/2d/PathD2D.cpp @@ +241,5 @@ > > return new PathD2D(mGeometry, mFigureActive, mCurrentPoint, mFillRule); > } > > +class SpecializedSink : public ID2D1GeometrySink This should be implemented with a code style similar to OpeningGeometrySink for consistency.

Comment on attachment 821213 [details] [diff] [review] First proposal for replaying paths into pathsinks Review of attachment 821213 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/2D.h @@ +467,5 @@ > virtual TemporaryRef<Path> Finish() = 0; > + > + /* This plays the path back into a PathSink > + */ > + virtual void ReplayPath(PathSink& sink) = 0; After calling Finish, you would have to create another copy if you want to continue using the original path. ie c = new path(); c.moveto().... d = new path(c); c.moveto(); <= you would need to create a new pathbuilder here I will rename to function. ::: gfx/2d/PathD2D.cpp @@ +241,5 @@ > > return new PathD2D(mGeometry, mFigureActive, mCurrentPoint, mFillRule); > } > > +class SpecializedSink : public ID2D1GeometrySink I saw that class. Will do.

(In reply to Bas Schouten (:bas.schouten) from comment #9) > Comment on attachment 821213 [details] [diff] [review] > First proposal for replaying paths into pathsinks > > Review of attachment 821213 [details] [diff] [review]: > ----------------------------------------------------------------- > > The general idea here seems okay, but a couple of changes to the patch are > required. > > ::: gfx/2d/2D.h > @@ +467,5 @@ > > virtual TemporaryRef<Path> Finish() = 0; > > + > > + /* This plays the path back into a PathSink > > + */ > > + virtual void ReplayPath(PathSink& sink) = 0; > > You can always still do that. I believe the way we should implement this is > by putting it on Path, and I'd prefer something along the lines of > 'AppendPathToBuilder'. > > I'm not sure how this would add any limitations. The limitation is that you need to call Finish to get a Path. After calling Finish, the pathBuilder becomes immutable so you can't add to it anymore. So from my last example, c = new Path(); c.moveTo();... d = new Path(c); c.lineTo(); By moving the method to Path, the code for line 2 becomes: f = c.Finish; f.AppendPathToBuilder(d); c = new PathSink(); f.AppendPathToBuilder(c); By putting it on pathSink it is: c.ReplayPath(d); > > ::: gfx/2d/PathD2D.cpp > @@ +241,5 @@ > > > > return new PathD2D(mGeometry, mFigureActive, mCurrentPoint, mFillRule); > > } > > > > +class SpecializedSink : public ID2D1GeometrySink > > This should be implemented with a code style similar to OpeningGeometrySink > for consistency. I tried to copy the style from that class. Note that SpecializedSink implements a different interface than ID2D1GeometrySink

Isn't there code for this already on Central?

Comment on attachment 8366199 [details] [diff] [review] Replay paths into a pathsink Review of attachment 8366199 [details] [diff] [review]: ----------------------------------------------------------------- Duplication of functionality provided by Path::StreamToSink.

(In reply to Bas Schouten (:bas.schouten) from comment #14) > Comment on attachment 8366199 [details] [diff] [review] > Replay paths into a pathsink > > Review of attachment 8366199 [details] [diff] [review]: > ----------------------------------------------------------------- > > Duplication of functionality provided by Path::StreamToSink. Great to see that something similar has landed. I will close this bug.