Paths don't draw correctly using Azure/Cairo if they need to move between user space and device space. This shows up on asteroidbench test 4 as a bunch of unwanted lines across the canvas when drawing enemy bullets.

Comment on attachment 654845[details][diff][review]
Part 4: Added reftest and removed todo v2
Review of attachment 654845[details][diff][review]:
-----------------------------------------------------------------
This should be part n+1 because it depends on the fixes in the previous n patches.
::: layout/reftests/canvas/784573-1.html
@@ +1,1 @@
> +<!DOCTYPE html>
Can you add a comment saying what this is testing (that is, device space vs user space paths). Also, <!-- Any copyright is dedicated to the public domain. -->

Comment on attachment 655515[details][diff][review]
Part 0: Fix cairo path transforms v2
Review of attachment 655515[details][diff][review]:
-----------------------------------------------------------------
Some unanswered questions (some left over from previous reviews) lead me to still need to see a follow-up before r+.
::: gfx/2d/DrawTargetCairo.h
@@ +168,5 @@
> cairo_t* mContext;
> cairo_surface_t* mSurface;
> IntSize mSize;
> std::vector<SourceSurfaceCairo*> mSnapshots;
> + mutable CairoPathContext* mPathObserver;
Ownership rules deserve a comment here or on SetPathObserver. Seeing a bare pointer makes my tummy hurt.
::: gfx/2d/PathCairo.cpp
@@ +22,5 @@
> {
> cairo_reference(mContext);
> cairo_set_fill_rule(mContext, GfxFillRuleToCairoFillRule(mFillRule));
>
> + aDrawTarget->SetPathObserver(this);
Note: You didn't answer my earlier question, "How do we get around needing to have a separate context from the draw target, as below?", but I think I have figured it out: the non-copy CairoPathContext constructor is only ever called when creating a new path, correct?
@@ +35,5 @@
> + , mFillRule(aPathContext->mFillRule)
> +{
> + cairo_reference(mContext);
> +
> + // Canvas is telling us what transform we would apply from device space to
Don't talk about canvas here - it is one of the users of the DrawTarget API, but not the only one.
@@ +75,5 @@
> cairo_path_destroy(path);
> +
> + // Transform the context.
> + GfxMatrixToCairoMatrix(mTransform, matrix);
> + cairo_set_matrix(mContext, &matrix);
You set the cairo context's matrix to mContext's old matrix, and then set it to mTransform. Why? When do those two get out of sync?
@@ +86,5 @@
> +{
> + if (mDrawTarget) {
> + // null the draw target so that we don't try to copy the path when we get
> + // a PathWillChange notification back.
> + DrawTargetCairo* drawTarget = mDrawTarget;
Seems like most of this can just be elided, with some comments explaining it and an unconditional mDrawTarget = nullptr;
@@ +250,5 @@
> inverse.Invert();
> Point transformed = inverse * aPoint;
>
> + // ContainsPoint can be called after the DrawTarget has been notified about
> + // a change in transform but before the path has been notified by Canvas. In
s/Canvas/DrawTarget/ perhaps? References to canvas don't really make sense in backend code. Perhaps s/Canvas/user code/?
@@ +300,5 @@
>
> void
> PathCairo::CopyPathTo(cairo_t* aContext, DrawTargetCairo* aDrawTarget)
> {
> + mPathContext->CopyPathTo(aContext);
Why do you do this unconditionally? Was the previous optimization incorrect? And, if mPathContext->GetContext() == aContext, isn't this going to do useless work?
::: gfx/2d/PathCairo.h
@@ +37,5 @@
> class CairoPathContext : public RefCounted<CairoPathContext>
> {
> public:
> + // Construct a new empty CairoPathContext that uses the given draw target and
> + // it's cairo context. Using the existing context may save having to copy the
its
@@ +62,5 @@
> // our context.
> void ForgetDrawTarget();
>
> // Create a duplicate context, and copy this path to that context. Optionally,
> // the new context can be transformed.
Please fix this comment.
@@ +98,5 @@
>
> // This constructor, called with a CairoPathContext*, implicitly takes
> // ownership of the path, and therefore makes aPathContext copy out its path
> // regardless of whether it has a pointer to a DrawTargetCairo.
> // The path currently set on aPathContext is not changed.
s/aPathContext/aContext/g

(In reply to Joe Drew (:JOEDREW!) from comment #15)
> @@ +300,5 @@
> >
> > void
> > PathCairo::CopyPathTo(cairo_t* aContext, DrawTargetCairo* aDrawTarget)
> > {
> > + mPathContext->CopyPathTo(aContext);
>
> Why do you do this unconditionally? Was the previous optimization incorrect?
> And, if mPathContext->GetContext() == aContext, isn't this going to do
> useless work?
CairoPathContext::CopyPathTo() does this check anyway so it's duplicated. The previous code was deciding whether to subscribe to the DrawTarget more than to skip the call. Subscribing to the DrawTarget is a coin toss as to whether it'll save time or create the need for another copy.

(In reply to Joe Drew (:JOEDREW! \o/) from comment #26)
> ::: gfx/2d/PathCairo.cpp
> @@ +61,5 @@
> > mContext = cairo_create(surf);
> >
> > + // Set the matrix to match the source context so that the path is copied in
> > + // device space.
> > + cairo_set_matrix(mContext, &matrix);
>
> Ooh, better use a CairoTempMatrix here, so we're reset to identity
> afterwards.
We swap out the matrix every time so it doesn't matter what we set it to. We could set it to identity but it would serve no purpose.