Last Comment Bug 707848 - [Azure] Implement paths in Cairo backend
: [Azure] Implement paths in Cairo backend
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla12
Assigned To: Joe Drew (not getting mail)
:
Mentors:
Depends on:
Blocks: 694351
  Show dependency treegraph
 
Reported: 2011-12-05 18:01 PST by Joe Drew (not getting mail)
Modified: 2012-01-11 18:09 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
paths (45.80 KB, patch)
2011-12-05 18:01 PST, Joe Drew (not getting mail)
jmuizelaar: feedback+
Details | Diff | Review
paths v2 (46.96 KB, patch)
2012-01-05 11:50 PST, Joe Drew (not getting mail)
jmuizelaar: review+
Details | Diff | Review

Description Joe Drew (not getting mail) 2011-12-05 18:01:08 PST
Created attachment 579177 [details] [diff] [review]
paths

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.
Comment 1 Jeff Muizelaar [:jrmuizel] 2011-12-14 14:52:09 PST
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.
Comment 2 Joe Drew (not getting mail) 2012-01-05 11:42:44 PST
(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.
Comment 3 Joe Drew (not getting mail) 2012-01-05 11:50:58 PST
Created attachment 586167 [details] [diff] [review]
paths v2

Ready for a real review, I think.
Comment 4 Jeff Muizelaar [:jrmuizel] 2012-01-06 14:11:55 PST
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.
Comment 5 Joe Drew (not getting mail) 2012-01-10 19:42:55 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/14930a83054b
Comment 6 Ed Morley [:emorley] 2012-01-11 18:09:39 PST
https://hg.mozilla.org/mozilla-central/rev/14930a83054b

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