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

RESOLVED FIXED in mozilla15

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nrc, Assigned: nrc)

Tracking

15 Branch
mozilla15
x86_64
All
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

1.46 KB, patch
Joe Drew (not getting mail)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Follow up https://bugzilla.mozilla.org/show_bug.cgi?id=747822#c12
(Assignee)

Comment 1

5 years ago
Created attachment 624667 [details] [diff] [review]
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.
Attachment #624667 - Flags: review?
Attachment #624667 - Flags: feedback?(roc)
(Assignee)

Updated

5 years ago
Attachment #624667 - Flags: review? → review?(joe)
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.
Attachment #624667 - Flags: feedback?(roc) → feedback-
(Assignee)

Comment 3

5 years ago
Created attachment 624693 [details] [diff] [review]
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.
Attachment #624667 - Attachment is obsolete: true
Attachment #624667 - Flags: review?(joe)
Attachment #624693 - Flags: review?(joe)
Attachment #624693 - Flags: feedback?(roc)
Attachment #624693 - Flags: feedback?(roc) → feedback+
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! :)
Attachment #624693 - Flags: review?(joe) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/b21638e66135
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/b21638e66135
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.