Last Comment Bug 830734 - Implement Path primitives
: Implement Path primitives
Status: RESOLVED FIXED
[Shumway:p1]
: dev-doc-complete
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: mozilla31
Assigned To: Rik Cabanier
:
Mentors:
Depends on: 929723 978980 1009685 1135244
Blocks: html5test 885526 931587 1119470 927828 985178 985257 985801
  Show dependency treegraph
 
Reported: 2013-01-15 06:24 PST by Tobias Schneider [:tobytailor]
Modified: 2015-02-20 17:11 PST (History)
17 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Initial implementation of Path primitives. (33.51 KB, patch)
2013-01-15 08:00 PST, Tobias Schneider [:tobytailor]
no flags Details | Diff | Splinter Review
Initial implementation of Path primitives. v2 (31.42 KB, patch)
2013-01-24 10:41 PST, Tobias Schneider [:tobytailor]
jmuizelaar: review-
Details | Diff | Splinter Review
Refreshed patch that incorporates jeff's comment (25.75 KB, patch)
2013-10-24 21:11 PDT, Rik Cabanier
no flags Details | Diff | Splinter Review
New patch for path objects (41.25 KB, patch)
2013-10-29 22:31 PDT, Rik Cabanier
no flags Details | Diff | Splinter Review
New patch for path objects (15.60 KB, patch)
2013-11-01 16:32 PDT, Rik Cabanier
roc: review-
jmuizelaar: review-
Details | Diff | Splinter Review
1. update IDL + implementation of path primitives (19.21 KB, patch)
2014-01-31 09:00 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
2. Recreate path object if backends don't match (4.28 KB, patch)
2014-02-27 10:59 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
2. Recreate path object if backends don't match + add runtime flag + remove creation of drawtarget from path constructor (6.18 KB, patch)
2014-02-27 16:35 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
3. Add tests for path objects (5.17 KB, patch)
2014-02-27 17:29 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
1. update IDL + implementation of path primitives (19.22 KB, patch)
2014-02-27 20:01 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
1. update IDL + implementation of path primitives (19.22 KB, patch)
2014-02-27 21:26 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
1. update IDL + implementation of path primitives (19.22 KB, patch)
2014-02-27 21:29 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
1. update IDL + implementation of path primitives (18.12 KB, patch)
2014-02-27 21:34 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
1. update IDL + implementation of path primitives (18.12 KB, patch)
2014-02-27 22:38 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
1. update IDL + implementation of path primitives (18.73 KB, patch)
2014-02-28 15:12 PST, Rik Cabanier
no flags Details | Diff | Splinter Review
1. update IDL + implementation of path primitives (15.15 KB, patch)
2014-02-28 16:50 PST, Rik Cabanier
jmuizelaar: review-
Details | Diff | Splinter Review
1. update IDL + implementation of path primitives ( (14.95 KB, patch)
2014-03-11 14:56 PDT, Rik Cabanier
roc: review-
Details | Diff | Splinter Review
2. Recreate path object if backends don't match + add runtime flag + remove creation of drawtarget from path constructor (8.83 KB, patch)
2014-03-11 14:57 PDT, Rik Cabanier
no flags Details | Diff | Splinter Review
2. Add tests for path objects (6.35 KB, patch)
2014-03-11 14:58 PDT, Rik Cabanier
roc: review+
Details | Diff | Splinter Review
2. Recreate path object if backends don't match + add runtime flag + remove creation of drawtarget from path constructor (7.33 KB, patch)
2014-03-11 15:03 PDT, Rik Cabanier
roc: review-
Details | Diff | Splinter Review
1. Add implementation + idl of path primitives (1.90 KB, patch)
2014-03-17 17:18 PDT, Rik Cabanier
no flags Details | Diff | Splinter Review
1. Add implementation + idl of path primitives (16.79 KB, patch)
2014-03-17 18:12 PDT, Rik Cabanier
roc: review+
Details | Diff | Splinter Review

Description Tobias Schneider [:tobytailor] 2013-01-15 06:24:52 PST
Canvas v5 introduces a bunch of useful API additions. We are working on Shumway, a SWF runtime written in JavaScript, that uses Canvas for rendering. One of the new features we (and other apps like Pdf.js and Games in general) would profit a lot from is support for Path primitives (http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#path-objects). At the moment we use a shim for exactly that API and implementing it native definitely makes sense for us.
Comment 1 Tobias Schneider [:tobytailor] 2013-01-15 08:00:17 PST
Created attachment 702330 [details] [diff] [review]
Initial implementation of Path primitives.
Comment 2 Rik Cabanier 2013-01-16 10:29:21 PST
Do you have any comments on: http://lists.w3.org/Archives/Public/public-canvas-api/2013JanMar/0002.html
Basically, your implementation would be the pathSink (naming is TBD)
Comment 3 Robert Longson 2013-01-17 00:12:04 PST
Can we not share more code with our SVG SVGPathData::ConstructPath implementation rather than writing yet another path constructor?
Comment 4 Tobias Schneider [:tobytailor] 2013-01-24 10:41:36 PST
Created attachment 705958 [details] [diff] [review]
Initial implementation of Path primitives. v2
Comment 5 Rik Cabanier 2013-01-24 11:01:35 PST
We are also working on adding the path object to WebKit: https://bugs.webkit.org/show_bug.cgi?id=97333
This will live behind an experimental flag for now. Please note that it will be different from the current WhatWG proposal. It would be good if we can coordinate this.
Comment 6 :Ms2ger 2013-01-24 11:10:15 PST
(In reply to Rik Cabanier from comment #5)
> We are also working on adding the path object to WebKit:
> https://bugs.webkit.org/show_bug.cgi?id=97333
> This will live behind an experimental flag for now. Please note that it will
> be different from the current WhatWG proposal. It would be good if we can
> coordinate this.

Did you send use cases to the list? If not, I suggest we implement the spec and you fix your implementation.
Comment 7 Rik Cabanier 2013-01-24 11:18:01 PST
I did. There was no substantial feedback so we're trying to fix it in the implementation. Ian indicated in the bug that he would update the spec.

The current path syntax is broken. Simply aggregating path segments is the wrong way to do path logic and will give bad results.
Comment 8 Tobias Schneider [:tobytailor] 2013-01-24 11:33:57 PST
We already use a shim for the Path API in Shumway (Flash VM in JavaScript) and we haven't encountered any major problems with the current spec so far. Not exactly sure what your concerns are.
Comment 9 Rik Cabanier 2013-01-24 11:38:15 PST
That's because flash's edge lists already separate the paths into non-intersecting regions. In addition, Flash Authoring (which is the biggest producer of SWFs) will remove shared lines which also fixes even-odd weirdness for you.
This is not something that an author could do.

I would be very surprised if you can use this to implement stroking reliably.
Comment 10 Tobias Schneider [:tobytailor] 2013-01-25 04:03:42 PST
Any real world examples that proof your point?
Comment 11 Tobias Schneider [:tobytailor] 2013-01-25 04:07:37 PST
s/proof/prove/
Comment 12 Rik Cabanier 2013-01-25 08:36:42 PST
(In reply to Tobias Schneider from comment #10)
> Any real world examples that proof your point?

I have a blog post that shows the problem. I'm waiting for my team to sign off on it.
Comment 13 Tobias Schneider [:tobytailor] 2013-01-25 08:51:00 PST
> I have a blog post that shows the problem. I'm waiting for my team to sign
> off on it.

Cool.

For now I've implemented just as much of the API as WebKit does (plus the ability to parse an optional given SVG path data string). That's what you named PathSink. I'm not going to implement any of the Path interface methods (addPath, addPathByStrokingPath...) in this step. So no conflicts to API's that might change.
Comment 14 Jeff Muizelaar [:jrmuizel] 2013-01-26 15:37:30 PST
Comment on attachment 705958 [details] [diff] [review]
Initial implementation of Path primitives. v2

Review of attachment 705958 [details] [diff] [review]:
-----------------------------------------------------------------

This should be building an Azure Path object directly instead of building a temporary Canvas path object and building a Path from that during Stroke/Fill etc.
Comment 15 Rik Cabanier 2013-10-17 11:26:28 PDT
Let's make this happen!
Comment 16 Rik Cabanier 2013-10-21 22:04:17 PDT
(In reply to Jeff Muizelaar [:jrmuizel] on PTO till October 21 from comment #14)
> Comment on attachment 705958 [details] [diff] [review]
> Initial implementation of Path primitives. v2
> 
> Review of attachment 705958 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This should be building an Azure Path object directly instead of building a
> temporary Canvas path object and building a Path from that during
> Stroke/Fill etc.

I think this was done because the Path object doesn't know what type of backend the canvas object will have.
I've revived this patch and will try to land it.

Should I update it to create an azure path?
Comment 17 Rik Cabanier 2013-10-24 21:11:26 PDT
Created attachment 822115 [details] [diff] [review]
Refreshed patch that incorporates jeff's comment
Comment 18 Rik Cabanier 2013-10-24 23:29:08 PDT
Comment on attachment 822115 [details] [diff] [review]
Refreshed patch that incorporates jeff's comment

Try build: https://tbpl.mozilla.org/?tree=Try&rev=0879372fd3ec

Need to patch the test that finds the new global class. maybe add more tests?
Comment 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-10-29 17:57:58 PDT
Comment on attachment 822115 [details] [diff] [review]
Refreshed patch that incorporates jeff's comment

Review of attachment 822115 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +1838,5 @@
>  
> +already_AddRefed<CanvasPath>
> +CanvasRenderingContext2D::CurrentPath()
> +{
> +  RefPtr<PathBuilder> CopyPathBuilder;

copyPathBuilder

@@ +1851,5 @@
> +    mDSPathBuilder = mTempPath->CopyToBuilder(CurrentState().fillRule);
> +  }
> +
> +  nsRefPtr<CanvasPath> NewPathObject(new CanvasPath(this, CopyPathBuilder));
> +  return NewPathObject.forget();

newPathObject

::: content/canvas/src/CanvasRenderingContext2D.h
@@ +24,5 @@
>  #include "gfx2DGlue.h"
>  #include "imgIEncoder.h"
>  
> +#define NS_CANVASPATHAZURE_PRIVATE_IID \
> +    {0xaf712aac, 0xfd0b, 0x4cea, {0x9f, 0xa4, 0x83, 0x02, 0x87, 0x4f, 0x7a, 0x18}}

We shouldn't need this.

@@ +45,5 @@
>  
>  template<typename T> class Optional;
>  
> +class CanvasPath :
> +  public nsIDOMPath,

We shouldn't need to inherit from nsIDOMPath here. We shouldn't need nsIDOMPath at all.

@@ +52,5 @@
> +public:
> +  NS_DECLARE_STATIC_IID_ACCESSOR(NS_CANVASPATHAZURE_PRIVATE_IID)
> +
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_CLASS(CanvasPath)

You don't need to implement nsISupports. Follow CanvasPattern and use   NS_INLINE_DECL_CYCLE_COLLECTING_NATIVE_REFCOUNTING(CanvasPattern).

@@ +76,5 @@
> +             mozilla::ErrorResult& error);
> +  void Rect(double x, double y, double w, double h);
> +  void Arc(double x, double y, double radius,
> +           double startAngle, double endAngle, bool anticlockwise,
> +           mozilla::ErrorResult& error);

You should already be in the mozilla namespace so don't use the mozilla prefix here.

@@ +83,5 @@
> +  void BezierTo(const mozilla::gfx::Point& aCP1,
> +                const mozilla::gfx::Point& aCP2,
> +                const mozilla::gfx::Point& aCP3);
> +
> +  CanvasPath(nsISupports* aParent, mozilla::RefPtr<mozilla::gfx::PathBuilder> mPathBuilder);

Use typedefs in the class to avoid using prefixes here. E.g.
  typedef mozilla::gfx::PathBuilder PathBuilder;
  typedef mozilla::gfx::Float Float;
You don't need the mozilla prefix in any case.

@@ +376,5 @@
>    void Rect(double x, double y, double w, double h);
>    void Arc(double x, double y, double radius, double startAngle,
>             double endAngle, bool anticlockwise, mozilla::ErrorResult& error);
>  
> +  already_AddRefed<CanvasPath > CurrentPath();

Remove space before >

::: dom/base/nsDOMClassInfo.cpp
@@ +87,5 @@
>  // Event related includes
>  #include "nsIDOMEventTarget.h"
>  
> +// Canvas
> +#include "nsIDOMCanvasRenderingContext2D.h"

No changes to nsDOMClassInfo should be needed. nsDOMClassInfo is obsolete.

::: dom/base/nsDOMClassInfoClasses.h
@@ +25,5 @@
>  
>  // Range classes
>  DOMCI_CLASS(Selection)
>  
> +DOMCI_CLASS(CanvasPath)

This shouldn't be needed. nsDOMClassInfoClasses is obsolete.

::: dom/interfaces/canvas/nsIDOMCanvasRenderingContext2D.idl
@@ +32,5 @@
>    const unsigned long DRAWWINDOW_ASYNC_DECODE_IMAGES = 0x10;
>  };
> +
> +[scriptable, uuid(1146b8a4-45b4-4c96-8817-d55647f31e15)]
> +interface nsIDOMPath : nsISupports

I don't think we want to add this. nsIDOMCanvasRenderingContext2D.idl is obsolete.

::: dom/webidl/CanvasRenderingContext2D.webidl
@@ +123,5 @@
>    [Throws]
>    void putImageData(ImageData imagedata, double dx, double dy, double dirtyX, double dirtyY, double dirtyWidth, double dirtyHeight);
>  
> +  // current path 
> +  attribute Path currentPath;

Is this specced somewhere? Because I don't see it in http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html.

::: js/xpconnect/src/dom_quickstubs.qsconf
@@ +52,5 @@
>      # dom/interfaces/core
>      'nsIDOMDOMStringList.*',
>  
> +    #
> +    'nsIDOMPath.*',

You shouldn't be adding anything to dom_quickstubs.qsconf. This is all obseleted by .webidl.
Comment 20 Rik Cabanier 2013-10-29 19:30:47 PDT
Thanks for the review.
Most of that code came from the old patch. I will update per your comments.
Comment 21 Rik Cabanier 2013-10-29 22:31:43 PDT
Created attachment 824467 [details] [diff] [review]
New patch for path objects
Comment 22 Rik Cabanier 2013-10-29 22:34:50 PDT
Comment on attachment 822115 [details] [diff] [review]
Refreshed patch that incorporates jeff's comment

Review of attachment 822115 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +1838,5 @@
>  
> +already_AddRefed<CanvasPath>
> +CanvasRenderingContext2D::CurrentPath()
> +{
> +  RefPtr<PathBuilder> CopyPathBuilder;

Fixed.

@@ +1851,5 @@
> +    mDSPathBuilder = mTempPath->CopyToBuilder(CurrentState().fillRule);
> +  }
> +
> +  nsRefPtr<CanvasPath> NewPathObject(new CanvasPath(this, CopyPathBuilder));
> +  return NewPathObject.forget();

Fixed

::: content/canvas/src/CanvasRenderingContext2D.h
@@ +24,5 @@
>  #include "gfx2DGlue.h"
>  #include "imgIEncoder.h"
>  
> +#define NS_CANVASPATHAZURE_PRIVATE_IID \
> +    {0xaf712aac, 0xfd0b, 0x4cea, {0x9f, 0xa4, 0x83, 0x02, 0x87, 0x4f, 0x7a, 0x18}}

You were right.
Removed this + all other obsolete code

@@ +76,5 @@
> +             mozilla::ErrorResult& error);
> +  void Rect(double x, double y, double w, double h);
> +  void Arc(double x, double y, double radius,
> +           double startAngle, double endAngle, bool anticlockwise,
> +           mozilla::ErrorResult& error);

removed the prefix everywhere in the file.

@@ +376,5 @@
>    void Rect(double x, double y, double w, double h);
>    void Arc(double x, double y, double radius, double startAngle,
>             double endAngle, bool anticlockwise, mozilla::ErrorResult& error);
>  
> +  already_AddRefed<CanvasPath > CurrentPath();

done
Comment 23 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-10-31 18:46:19 PDT
Comment on attachment 824467 [details] [diff] [review]
New patch for path objects

Review of attachment 824467 [details] [diff] [review]:
-----------------------------------------------------------------

I still don't see a spec for currentPath anywhere!

::: content/canvas/src/CanvasRenderingContext2D.h
@@ +42,5 @@
>  
>  template<typename T> class Optional;
>  
> +class BaseCanvasPath
> +{

I don't understand why you've separated things into this base class. Can't we just merge this into CanvasPath?

::: dom/bindings/Bindings.conf
@@ +1876,5 @@
>  addExternalIface('CameraReleaseCallback', nativeType='nsICameraReleaseCallback', headerFile='nsIDOMCameraManager.h')
>  addExternalIface('CameraStartRecordingCallback', nativeType='nsICameraStartRecordingCallback', headerFile='nsIDOMCameraManager.h')
>  addExternalIface('CameraPreviewStateChange', nativeType='nsICameraPreviewStateChange', headerFile='nsIDOMCameraManager.h')
>  addExternalIface('CameraPreviewStreamCallback', nativeType='nsICameraPreviewStreamCallback', headerFile='nsIDOMCameraManager.h')
> +addExternalIface('CameraRecorderStateChange', nativeType='nsICameraRecorderStateChange', headerFile='nsIDOMCameraManager.h')

There should be no change here.

::: dom/tests/mochitest/general/test_interfaces.html
@@ +104,5 @@
>      "CameraCapabilities",
>      "CameraControl",
>      "CameraManager",
>      "CanvasGradient",
> +    "CanvasPath",

This should not be here.
Comment 24 Rik Cabanier 2013-10-31 21:25:37 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #23)
> Comment on attachment 824467 [details] [diff] [review]
> New patch for path objects
> 
> Review of attachment 824467 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I still don't see a spec for currentPath anywhere!

True. Blink and WebKit implemented it but it's not in the spec yet.
I'll follow up.

> 
> ::: content/canvas/src/CanvasRenderingContext2D.h
> @@ +42,5 @@
> >  
> >  template<typename T> class Optional;
> >  
> > +class BaseCanvasPath
> > +{
> 
> I don't understand why you've separated things into this base class. Can't
> we just merge this into CanvasPath?

I did that because the cleanup patch uses the same base functionality but not the JS exposed logic.
If that one doesn't go anywhere, I can put it back together.

> 
> ::: dom/bindings/Bindings.conf
> @@ +1876,5 @@
> >  addExternalIface('CameraReleaseCallback', nativeType='nsICameraReleaseCallback', headerFile='nsIDOMCameraManager.h')
> >  addExternalIface('CameraStartRecordingCallback', nativeType='nsICameraStartRecordingCallback', headerFile='nsIDOMCameraManager.h')
> >  addExternalIface('CameraPreviewStateChange', nativeType='nsICameraPreviewStateChange', headerFile='nsIDOMCameraManager.h')
> >  addExternalIface('CameraPreviewStreamCallback', nativeType='nsICameraPreviewStreamCallback', headerFile='nsIDOMCameraManager.h')
> > +addExternalIface('CameraRecorderStateChange', nativeType='nsICameraRecorderStateChange', headerFile='nsIDOMCameraManager.h')
> 
> There should be no change here.
> 
> ::: dom/tests/mochitest/general/test_interfaces.html
> @@ +104,5 @@
> >      "CameraCapabilities",
> >      "CameraControl",
> >      "CameraManager",
> >      "CanvasGradient",
> > +    "CanvasPath",
> 
> This should not be here.
Will do
Comment 25 :Ms2ger 2013-11-01 01:41:09 PDT
Comment on attachment 824467 [details] [diff] [review]
New patch for path objects

Review of attachment 824467 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +4034,5 @@
> +  return PathBinding::Wrap(cx, scope, this);
> +}
> +
> +already_AddRefed<CanvasPath>
> +CanvasPath::Constructor(const GlobalObject& aGlobal, ErrorResult& rv)

aRv, also elsewhere

@@ +4084,5 @@
> +}
> +
> +void
> +BaseCanvasPath::BezierCurveTo(double cp1x, double cp1y,
> +                          double cp2x, double cp2y,

Indentation

@@ +4088,5 @@
> +                          double cp2x, double cp2y,
> +                          double x, double y)
> +{
> +  BezierTo(mozilla::gfx::Point(ToFloat(cp1x), ToFloat(cp1y)),
> +             mozilla::gfx::Point(ToFloat(cp2x), ToFloat(cp2y)),

Indentation

@@ +4202,5 @@
> +
> +  return mTempPath->CopyToBuilder();
> +}
> +
> +}

} // namespace foo

::: content/canvas/src/CanvasRenderingContext2D.h
@@ +73,5 @@
> +
> +  RefPtr<gfx::PathBuilder> mPathBuilder;
> +};
> +
> +class CanvasPath :

Why is this not called mozilla::dom::Path?

@@ +83,5 @@
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_CLASS(CanvasPath)
> +
> +  nsISupports* GetParentObject() { return mParent; }
> +
> +  JSObject* WrapObject(JSContext *cx, JS::Handle<JSObject*> scope);

* goes to the left, argument names start with a

@@ +91,5 @@
> +  static already_AddRefed<CanvasPath> Constructor(const GlobalObject& aGlobal,
> +                                                  CanvasPath& aCanvasPath,
> +                                                  ErrorResult& rv);
> +
> +  CanvasPath(nsISupports* aParent, RefPtr<gfx::PathBuilder> mPathBuilder);

Passing refptrs as arguments isn't done.

@@ +97,5 @@
> +  CanvasPath(nsISupports* aParent);
> +
> +  virtual ~CanvasPath() {}
> +
> +  nsISupports* mParent;

What keeps this alive?

@@ +629,5 @@
>    /**
>      * Returns the surface format this canvas should be allocated using. Takes
>      * into account mOpaque, platform requirements, etc.
>      */
> +  gfx::SurfaceFormat GetSurfaceFormat() const;

Put all those changes in a separate patch, please.

::: content/canvas/test/test_canvas.html
@@ +24990,5 @@
>    throw e;
>    ok(false, "unexpected exception thrown in: test_linedash");
>   }
>   try {
> +  test_path

You're not running the test. As I asked you before, though, please put this in a separate file.

::: dom/bindings/Bindings.conf
@@ +877,5 @@
>  },
>  
> +'Path': {
> +    'nativeType': 'mozilla::dom::CanvasPath',
> +    'headerFile': 'nsIDOMCanvasRenderingContext2D.h'

That's not true.
Comment 26 Rik Cabanier 2013-11-01 15:41:19 PDT
Comment on attachment 824467 [details] [diff] [review]
New patch for path objects

Review of attachment 824467 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/CanvasRenderingContext2D.h
@@ +73,5 @@
> +
> +  RefPtr<gfx::PathBuilder> mPathBuilder;
> +};
> +
> +class CanvasPath :

To avoid clashes with Path

@@ +83,5 @@
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_CLASS(CanvasPath)
> +
> +  nsISupports* GetParentObject() { return mParent; }
> +
> +  JSObject* WrapObject(JSContext *cx, JS::Handle<JSObject*> scope);

fixed

@@ +91,5 @@
> +  static already_AddRefed<CanvasPath> Constructor(const GlobalObject& aGlobal,
> +                                                  CanvasPath& aCanvasPath,
> +                                                  ErrorResult& rv);
> +
> +  CanvasPath(nsISupports* aParent, RefPtr<gfx::PathBuilder> mPathBuilder);

fixed

@@ +97,5 @@
> +  CanvasPath(nsISupports* aParent);
> +
> +  virtual ~CanvasPath() {}
> +
> +  nsISupports* mParent;

fixed by wrapping

@@ +629,5 @@
>    /**
>      * Returns the surface format this canvas should be allocated using. Takes
>      * into account mOpaque, platform requirements, etc.
>      */
> +  gfx::SurfaceFormat GetSurfaceFormat() const;

made these changes because roc said to do so. I can remove if needed.
Comment 27 Rik Cabanier 2013-11-01 15:41:53 PDT
Comment on attachment 824467 [details] [diff] [review]
New patch for path objects

Review of attachment 824467 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/CanvasRenderingContext2D.h
@@ +42,5 @@
>  
>  template<typename T> class Optional;
>  
> +class BaseCanvasPath
> +{

I collapsed them again
Comment 28 :Ms2ger 2013-11-01 15:43:42 PDT
(In reply to Rik Cabanier from comment #26)
> @@ +629,5 @@
> >    /**
> >      * Returns the surface format this canvas should be allocated using. Takes
> >      * into account mOpaque, platform requirements, etc.
> >      */
> > +  gfx::SurfaceFormat GetSurfaceFormat() const;
> 
> made these changes because roc said to do so. I can remove if needed.

Don't revert them, but create two patches: one that just removes the prefixes, and another on top of that that contains the functional changes.
Comment 29 Rik Cabanier 2013-11-01 16:32:31 PDT
Created attachment 826170 [details] [diff] [review]
New patch for path objects
Comment 30 Rik Cabanier 2013-11-01 16:50:05 PDT
(In reply to :Ms2ger from comment #28)
> (In reply to Rik Cabanier from comment #26)
> > @@ +629,5 @@
> > >    /**
> > >      * Returns the surface format this canvas should be allocated using. Takes
> > >      * into account mOpaque, platform requirements, etc.
> > >      */
> > > +  gfx::SurfaceFormat GetSurfaceFormat() const;
> > 
> > made these changes because roc said to do so. I can remove if needed.
> 
> Don't revert them, but create two patches: one that just removes the
> prefixes, and another on top of that that contains the functional changes.

OK. Latest patch doesn't have the changes. Will add another patch with them when I get r+ on this one.
Comment 31 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-11-03 22:12:23 PST
Comment on attachment 826170 [details] [diff] [review]
New patch for path objects

Review of attachment 826170 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/test/test_canvas.html
@@ +24909,5 @@
>    ok(false, "unexpected exception thrown in: test_linedash");
>   }
>   try {
> +  test_path();
> +  } catch(e) {

Fix indent.

::: dom/webidl/CanvasRenderingContext2D.webidl
@@ +122,5 @@
>    [Throws]
>    void putImageData(ImageData imagedata, double dx, double dy, double dirtyX, double dirtyY, double dirtyWidth, double dirtyHeight);
>  
> +  // current path
> +  attribute Path currentPath;

I thought you were going to take this out?
Comment 32 Rik Cabanier 2013-11-03 22:44:48 PST
Comment on attachment 826170 [details] [diff] [review]
New patch for path objects

Review of attachment 826170 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/test/test_canvas.html
@@ +24909,5 @@
>    ok(false, "unexpected exception thrown in: test_linedash");
>   }
>   try {
> +  test_path();
> +  } catch(e) {

will do.

::: dom/webidl/CanvasRenderingContext2D.webidl
@@ +122,5 @@
>    [Throws]
>    void putImageData(ImageData imagedata, double dx, double dy, double dirtyX, double dirtyY, double dirtyWidth, double dirtyHeight);
>  
> +  // current path
> +  attribute Path currentPath;

The tests won't work without it. Basically, the path object is useless without a connection to canvas.

I will see if the mailing list converges this week.
If not, I will take it out and just test if the path APIs work.
It it does, I will put the new API behind a runtime flag.
Comment 33 Bas Schouten (:bas.schouten) 2013-11-05 16:16:57 PST
FWIW, for other work I'm implementing code that allows streaming the contents of a Path object to an arbitrary PathSink.
Comment 34 Rik Cabanier 2013-11-05 16:19:10 PST
(In reply to Bas Schouten (:bas.schouten) from comment #33)
> FWIW, for other work I'm implementing code that allows streaming the
> contents of a Path object to an arbitrary PathSink.

Like this: https://bugzilla.mozilla.org/show_bug.cgi?id=929723
Comment 35 Jeff Muizelaar [:jrmuizel] 2013-12-02 09:50:24 PST
Comment on attachment 826170 [details] [diff] [review]
New patch for path objects

Review of attachment 826170 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +4020,5 @@
> +{
> +  SetIsDOMBinding();
> +
> +  BackendType aBackend = gfxPlatform::GetPlatform()->GetPreferredCanvasBackend();
> +  RefPtr<DrawTarget> aDrawTarget = Factory::CreateDrawTarget(aBackend, IntSize(8, 8), FORMAT_B8G8R8X8);

I believe we don't have need for creating a draw target to create a path builder anymore. That should be fixed as we don't want to create a DrawTarget for every path.

Also, does the code handle the cases where DrawTarget backends don't match?(i.e. Direct2D vs software when the canvas is too large) I didn't see any tests for that.
Comment 36 Rik Cabanier 2013-12-20 17:02:16 PST
(In reply to Jeff Muizelaar [:jrmuizel] from comment #35)
> Comment on attachment 826170 [details] [diff] [review]
> New patch for path objects
> 
> Review of attachment 826170 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/CanvasRenderingContext2D.cpp
> @@ +4020,5 @@
> > +{
> > +  SetIsDOMBinding();
> > +
> > +  BackendType aBackend = gfxPlatform::GetPlatform()->GetPreferredCanvasBackend();
> > +  RefPtr<DrawTarget> aDrawTarget = Factory::CreateDrawTarget(aBackend, IntSize(8, 8), FORMAT_B8G8R8X8);
> 
> I believe we don't have need for creating a draw target to create a path
> builder anymore. That should be fixed as we don't want to create a
> DrawTarget for every path.

Can you point me to the API that allows this?

> 
> Also, does the code handle the cases where DrawTarget backends don't
> match?(i.e. Direct2D vs software when the canvas is too large) I didn't see
> any tests for that.

Did Bas land that code? If so, can you point me to it?
Comment 37 Rik Cabanier 2013-12-20 17:05:56 PST
I'm in the process of:
- renaming path to path2d
- changing currentPath to set/getCurrentPath

Looking at the architecture of mozilla's canvas implementation, I think it would really benefit from having the Shape class (http://blogs.adobe.com/webplatform/2013/01/31/revised-canvas-paths/) since it allows caching of the mPathBuilder object.
What should I do to implement this? It could be behind a runtime flag for now.
Comment 38 Jeff Muizelaar [:jrmuizel] 2014-01-21 14:16:43 PST
(In reply to Rik Cabanier from comment #36)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #35)
> > 
> > I believe we don't have need for creating a draw target to create a path
> > builder anymore. That should be fixed as we don't want to create a
> > DrawTarget for every path.
> 
> Can you point me to the API that allows this?

There's no current API for this. It would need to be created but should be relatively easy now.

> > 
> > Also, does the code handle the cases where DrawTarget backends don't
> > match?(i.e. Direct2D vs software when the canvas is too large) I didn't see
> > any tests for that.
> 
> Did Bas land that code? If so, can you point me to it?

http://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPlatform.cpp?from=CreateOffscreenCanvasDrawTarget#1004
Comment 39 Rik Cabanier 2014-01-31 08:59:10 PST
(In reply to Jeff Muizelaar [:jrmuizel] from comment #38)
> (In reply to Rik Cabanier from comment #36)
> > (In reply to Jeff Muizelaar [:jrmuizel] from comment #35)
> > > 
> > > I believe we don't have need for creating a draw target to create a path
> > > builder anymore. That should be fixed as we don't want to create a
> > > DrawTarget for every path.
> > 
> > Can you point me to the API that allows this?
> 
> There's no current API for this. It would need to be created but should be
> relatively easy now.

Could you tell me how I could create such an API?
 
> > > 
> > > Also, does the code handle the cases where DrawTarget backends don't
> > > match?(i.e. Direct2D vs software when the canvas is too large) I didn't see
> > > any tests for that.
> > 
> > Did Bas land that code? If so, can you point me to it?
> 
> http://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPlatform.
> cpp?from=CreateOffscreenCanvasDrawTarget#1004

I saw other code for streaming to any pathsink. (StreamToSink)
Were you pointing to the code that creates a reusable backend?
Comment 40 Rik Cabanier 2014-01-31 09:00:28 PST
Created attachment 8368633 [details] [diff] [review]
1. update IDL +  implementation of path primitives
Comment 41 Rik Cabanier 2014-02-27 10:59:40 PST
Created attachment 8383190 [details] [diff] [review]
2. Recreate path object if backends don't match
Comment 42 Rik Cabanier 2014-02-27 16:35:37 PST
Created attachment 8383387 [details] [diff] [review]
2. Recreate path object if backends don't match + add runtime flag + remove creation of drawtarget from path constructor
Comment 43 Rik Cabanier 2014-02-27 17:29:03 PST
Created attachment 8383416 [details] [diff] [review]
3. Add tests for path objects
Comment 44 Rik Cabanier 2014-02-27 20:01:04 PST
Created attachment 8383462 [details] [diff] [review]
1. update IDL + implementation of path primitives
Comment 45 Rik Cabanier 2014-02-27 21:26:33 PST
Created attachment 8383499 [details] [diff] [review]
1. update IDL + implementation of path primitives
Comment 46 Rik Cabanier 2014-02-27 21:29:00 PST
Created attachment 8383500 [details] [diff] [review]
1. update IDL + implementation of path primitives
Comment 47 Rik Cabanier 2014-02-27 21:34:18 PST
Created attachment 8383501 [details] [diff] [review]
1. update IDL + implementation of path primitives

Sorry about the noise. Was updating the wrong patch.

try run: https://tbpl.mozilla.org/?tree=Try&rev=18fb9ed6533f
Comment 48 Rik Cabanier 2014-02-27 22:38:38 PST
Created attachment 8383514 [details] [diff] [review]
1. update IDL + implementation of path primitives

fixed issue with test file
Comment 49 Rik Cabanier 2014-02-28 15:12:15 PST
Created attachment 8383982 [details] [diff] [review]
1. update IDL + implementation of path primitives
Comment 50 Rik Cabanier 2014-02-28 16:50:59 PST
Created attachment 8384034 [details] [diff] [review]
1. update IDL + implementation of path primitives
Comment 51 Jeff Muizelaar [:jrmuizel] 2014-03-03 13:16:02 PST
Comment on attachment 8384034 [details] [diff] [review]
1. update IDL + implementation of path primitives

Review of attachment 8384034 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +4298,5 @@
> +  SetIsDOMBinding();
> +
> +  RefPtr<DrawTarget> aDrawTarget = gfxPlatform::GetPlatform()->CreateOffscreenCanvasDrawTarget(IntSize(8, 8), gfx::SurfaceFormat::B8G8R8X8);
> +  //RefPtr<DrawTarget> aDrawTarget = Factory::CreateDrawTarget(aBackend, IntSize(8, 8), FORMAT_B8G8R8X8);
> +  mPathBuilder = aDrawTarget->CreatePathBuilder();

There's currently no reason that we need mTarget for CreatePathBuilder anymore. Afaict it can just live off of the Factory. Can you fix this first before this patch? It should make creating this draw target unnecessary.

@@ +4363,5 @@
> +}
> +
> +void
> +CanvasPath::ArcTo(double x1, double y1, double x2, double y2, double radius,
> +                  ErrorResult& error)

This seems to duplicate CanvasRenderingContext2D::ArcTo. Is that necessary?
Comment 52 Rik Cabanier 2014-03-03 13:38:17 PST
(In reply to Jeff Muizelaar [:jrmuizel] from comment #51)
> Comment on attachment 8384034 [details] [diff] [review]
> 1. update IDL + implementation of path primitives
> 
> Review of attachment 8384034 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/CanvasRenderingContext2D.cpp
> @@ +4298,5 @@
> > +  SetIsDOMBinding();
> > +
> > +  RefPtr<DrawTarget> aDrawTarget = gfxPlatform::GetPlatform()->CreateOffscreenCanvasDrawTarget(IntSize(8, 8), gfx::SurfaceFormat::B8G8R8X8);
> > +  //RefPtr<DrawTarget> aDrawTarget = Factory::CreateDrawTarget(aBackend, IntSize(8, 8), FORMAT_B8G8R8X8);
> > +  mPathBuilder = aDrawTarget->CreatePathBuilder();
> 
> There's currently no reason that we need mTarget for CreatePathBuilder
> anymore. Afaict it can just live off of the Factory. Can you fix this first
> before this patch? It should make creating this draw target unnecessary.

sorry, I addressed that in the second patch: 
   RefPtr<DrawTarget> aDrawTarget = gfxPlatform::ScreenReferenceDrawTarget()->CreatePathBuilder();

> @@ +4363,5 @@
> > +}
> > +
> > +void
> > +CanvasPath::ArcTo(double x1, double y1, double x2, double y2, double radius,
> > +                  ErrorResult& error)
> 
> This seems to duplicate CanvasRenderingContext2D::ArcTo. Is that necessary?

I didn't see an easy way to simplify this.
I have a proposed patch to simplify the path handling for canvas which will address that problem. See https://bugzilla.mozilla.org/show_bug.cgi?id=931587
Comment 53 Jeff Muizelaar [:jrmuizel] 2014-03-03 13:42:40 PST
(In reply to Rik Cabanier from comment #52)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #51)
> > Comment on attachment 8384034 [details] [diff] [review]
> > 1. update IDL + implementation of path primitives
> > 
> > Review of attachment 8384034 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: content/canvas/src/CanvasRenderingContext2D.cpp
> > @@ +4298,5 @@
> > > +  SetIsDOMBinding();
> > > +
> > > +  RefPtr<DrawTarget> aDrawTarget = gfxPlatform::GetPlatform()->CreateOffscreenCanvasDrawTarget(IntSize(8, 8), gfx::SurfaceFormat::B8G8R8X8);
> > > +  //RefPtr<DrawTarget> aDrawTarget = Factory::CreateDrawTarget(aBackend, IntSize(8, 8), FORMAT_B8G8R8X8);
> > > +  mPathBuilder = aDrawTarget->CreatePathBuilder();
> > 
> > There's currently no reason that we need mTarget for CreatePathBuilder
> > anymore. Afaict it can just live off of the Factory. Can you fix this first
> > before this patch? It should make creating this draw target unnecessary.
> 
> sorry, I addressed that in the second patch: 
>    RefPtr<DrawTarget> aDrawTarget =
> gfxPlatform::ScreenReferenceDrawTarget()->CreatePathBuilder();

This is still using a DrawTarget to create the path builder. There should be no reason for that.
Comment 54 Rik Cabanier 2014-03-11 14:56:33 PDT
Created attachment 8389441 [details] [diff] [review]
1. update IDL + implementation of path primitives (
Comment 55 Rik Cabanier 2014-03-11 14:57:22 PDT
Created attachment 8389444 [details] [diff] [review]
2. Recreate path object if backends don't match + add runtime flag + remove creation of drawtarget from path constructor
Comment 56 Rik Cabanier 2014-03-11 14:58:01 PDT
Created attachment 8389445 [details] [diff] [review]
2. Add tests for path objects
Comment 57 Rik Cabanier 2014-03-11 15:03:00 PDT
Created attachment 8389450 [details] [diff] [review]
2. Recreate path object if backends don't match + add runtime flag + remove creation of drawtarget from path constructor
Comment 58 Rik Cabanier 2014-03-11 15:05:01 PDT
try run: https://tbpl.mozilla.org/?tree=Try&rev=378a61b62519
Comment 59 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-03-16 22:24:29 PDT
Comment on attachment 8389441 [details] [diff] [review]
1. update IDL + implementation of path primitives (

Review of attachment 8389441 [details] [diff] [review]:
-----------------------------------------------------------------

How does this deal with a mismatch between the canvas DrawTarget backend and the backend used to construct the CanvasPath?

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +4332,5 @@
> +void
> +CanvasPath::ArcTo(double x1, double y1, double x2, double y2, double radius,
> +                  ErrorResult& error)
> +{
> +  if (radius < 0) {

Any way to share the code copied into this function?

@@ +4436,5 @@
> +  RefPtr<Path> mTempPath = mPathBuilder->Finish();
> +
> +  FillRule fillRule = FillRule::FILL_WINDING;
> +  if(winding == CanvasWindingRule::Evenodd)
> +    fillRule = FillRule::FILL_EVEN_ODD;

space before (, and {} around body

::: content/canvas/src/CanvasRenderingContext2D.h
@@ +87,5 @@
> +  CanvasPath(nsCOMPtr<nsISupports> aParent, RefPtr<gfx::PathBuilder> mPathBuilder);
> +  virtual ~CanvasPath() {}
> +
> +private:
> +  

delete this line
Comment 60 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-03-16 22:29:00 PDT
Comment on attachment 8389450 [details] [diff] [review]
2. Recreate path object if backends don't match + add runtime flag + remove creation of drawtarget from path constructor

Review of attachment 8389450 [details] [diff] [review]:
-----------------------------------------------------------------

Fold this into part 1!
Comment 61 Rik Cabanier 2014-03-17 13:26:19 PDT
Comment on attachment 8389441 [details] [diff] [review]
1. update IDL + implementation of path primitives (

Review of attachment 8389441 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +4332,5 @@
> +void
> +CanvasPath::ArcTo(double x1, double y1, double x2, double y2, double radius,
> +                  ErrorResult& error)
> +{
> +  if (radius < 0) {

Yes,
I opened a "cleanup" bug that will collapse the logic: https://bugzilla.mozilla.org/show_bug.cgi?id=931587
I will tackle that after this goes in. (If you remember, we talked about not keeping 2 paths in the convas context)
Comment 62 Rik Cabanier 2014-03-17 17:18:44 PDT
Created attachment 8392595 [details] [diff] [review]
1. Add implementation + idl of path primitives
Comment 63 Rik Cabanier 2014-03-17 18:12:58 PDT
Created attachment 8392615 [details] [diff] [review]
1. Add implementation + idl of path primitives
Comment 64 Rik Cabanier 2014-03-17 20:36:43 PDT
successful try run: https://tbpl.mozilla.org/?tree=Try&rev=1c1d11d3d9eb

Note You need to log in before you can comment on or make changes to this bug.