Joe Drew (not getting mail) Assignee
	Description • 13 years ago

This is on top of the patch in bug 694351.

Due to the fact that Cairo doesn't have a path object, this is a lot more complicated than it would otherwise be. PathCairo and PathBuilderCairo hold an object called CairoPathContext, which represents a path set on a cairo_t along with a transformation. On construction, it registers with a DrawTargetCairo, and it gets updates whenever the DrawTarget's path or transformation changes. In the optimal case, no copies of the path need to happen, but if divergent paths need to be created, the path is copied out into a new context. Any path can be copied to another context; in this case both the path and the transformation are applied to that cairo_t.

	Jeff Muizelaar [:jrmuizel]
	Comment 1 • 13 years ago

Comment on attachment 579177 [details] [diff] [review]
paths

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

This all seems reasonable.

::: gfx/2d/DrawTargetCairo.cpp
@@ +68,5 @@
> +
> +}
> +
> +#define PREPARE_FOR_DRAWING(x) PrepareForDrawing(x); AutoSaveRestore asr(x);
> +#define PREPARE_FOR_DRAWING_PATH(x, p) PrepareForDrawing(x,p); AutoSaveRestore asr(x);

As I've said before, I don't really like using a macro here.

@@ +571,4 @@
>  DrawTargetCairo::PopClip()
>  {
>  }
>  

These still need to be implemented right?

::: gfx/2d/PathCairo.cpp
@@ +87,5 @@
> +  // Duplicate the context.
> +  cairo_surface_t* surf = cairo_get_target(mContext);
> +  cairo_destroy(mContext);
> +  mContext = cairo_create(surf);
> +  cairo_set_matrix(mContext, &origmatrix);

It's probably worth adding a comment about how we could potentially do manual path construction to avoid having to copy the path out of the context.

@@ +231,5 @@
> +  // going to add an intermediate control point, and recompute control point 1.
> +  // The first and last control points remain the same.
> +  Point CP0 = CurrentPoint();
> +  Point CP1 = (CP0 + aCP1 * 2.0) / 3.0;
> +  Point CP2 = (aCP2 + aCP1 * 2.0) / 3.0;

Having a reference for this formula would be nice.

	Joe Drew (not getting mail) Assignee
	Comment 2 • 13 years ago

(In reply to Jeff Muizelaar [:jrmuizel] from comment #1)
> @@ +571,4 @@
> >  DrawTargetCairo::PopClip()
> >  {
> >  }
> >  
> 
> These still need to be implemented right?

Yes, in a future patch/bug.

	Joe Drew (not getting mail) Assignee
	Comment 3 • 13 years ago

Ready for a real review, I think.

	Jeff Muizelaar [:jrmuizel]
	Comment 4 • 13 years ago

Comment on attachment 586167 [details] [diff] [review]
paths v2

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

::: gfx/2d/PathCairo.cpp
@@ +331,5 @@
> +  double x1, y1, x2, y2;
> +
> +  SetCairoStrokeOptions(*mPathContext, aStrokeOptions);
> +
> +  cairo_stroke_extents(*mPathContext, &x1, &y1, &x2, &y2);

It might be worth adding a comment about the extents that you're actually computing here. Check the cairo docs for details.

	Joe Drew (not getting mail) Assignee
	Comment 5 • 13 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/14930a83054b

	Ed Morley [:emorley]
	Comment 6 • 13 years ago

https://hg.mozilla.org/mozilla-central/rev/14930a83054b