[Azure] Implement paths in Cairo backend

RESOLVED FIXED in mozilla12

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Joe Drew (not getting mail), Assigned: Joe Drew (not getting mail))

Tracking

(Blocks: 1 bug)

Trunk
mozilla12
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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.
Attachment #579177 - Flags: feedback?(jmuizelaar)
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.
Attachment #579177 - Flags: feedback?(jmuizelaar) → feedback+
(Assignee)

Comment 2

5 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.
(Assignee)

Comment 3

5 years ago
Created attachment 586167 [details] [diff] [review]
paths v2

Ready for a real review, I think.
Assignee: nobody → joe
Attachment #579177 - Attachment is obsolete: true
Attachment #586167 - Flags: review?(jmuizelaar)
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.
Attachment #586167 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 5

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

Comment 6

5 years ago
https://hg.mozilla.org/mozilla-central/rev/14930a83054b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.