756007 - [Azure] Cairo backend - push/pop clip, check and fix path invariants

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Nick Cameron [:nrc]]

Follow up https://bugzilla.mozilla.org/show_bug.cgi?id=747822#c12

Comment 1

[Nick Cameron [:nrc]]

Attachment: patch

I'm assuming that the path needs to be cleared before and after clipping, this should implement that semantics.

Canvas semantics are that the clip should not be cleared when a second clip is added, but I assume this is implemented by multiple calls to PushClip, rather than requiring the path is preserved after clipping.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 624667 [details] [diff] [review]
patch

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

::: gfx/2d/DrawTargetCairo.cpp
@@ +633,1 @@
>    PathCairo* path = const_cast<PathCairo*>(static_cast<const PathCairo*>(aPath));

Calling cairo_new_path here seems to make no difference since CopyPathTo calls it anyway. In fact, it's wrong, since WillChange is optimized so that if aPath is already the current path for this DrawTarget, it won't do anything. So clearing the path here would actually destroy aPath!

@@ +634,2 @@
>    path->CopyPathTo(mContext, this);
> +  cairo_clip(mContext);

I don't think this is right. 'path' is now the current path for this DrawTarget. That means we need to preserve mContext's current path.

@@ +645,2 @@
>    cairo_rectangle(mContext, aRect.X(), aRect.Y(), aRect.Width(), aRect.Height());
> +  cairo_clip(mContext);

These changes in PushClipRect make sense. WillChange will ensure no PathCairo is the current path for this DrawTarget, so clearing the path in order to set up our rectangle is safe. I don't think it matter whether we preserve the path after clipping or not; since there is no current PathCairo for this target, the next operation that uses a Path will will reset the cairo path. However, cairo_clip calls cairo_clip_preserve and then cairo_new_path, so I think it's slightly more efficient to call cairo_clip_preserve here.

Comment 3

[Nick Cameron [:nrc]]

Attachment: revised patch

Fixed roc's comments. I've had a look at the source, and I think I agree with everything. So, here, is another go. Annoyingly, the few tests I have work in every flavour. Who knew a few lines of code could go wrong in so many ways.

Comment 4

[Joe Drew (not getting mail)]

Comment on attachment 624693 [details] [diff] [review]
revised patch

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

roc got to my review comments before I could! :)

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b21638e66135

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/b21638e66135