Last Comment Bug 756007 - [Azure] Cairo backend - push/pop clip, check and fix path invariants
: [Azure] Cairo backend - push/pop clip, check and fix path invariants
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: 15 Branch
: x86_64 All
-- normal (vote)
: mozilla15
Assigned To: Nick Cameron [:nrc]
: Milan Sreckovic [:milan]
Depends on: 747822
  Show dependency treegraph
Reported: 2012-05-16 23:05 PDT by Nick Cameron [:nrc]
Modified: 2012-05-18 13:20 PDT (History)
4 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (1.54 KB, patch)
2012-05-17 00:11 PDT, Nick Cameron [:nrc]
roc: feedback-
Details | Diff | Splinter Review
revised patch (1.46 KB, patch)
2012-05-17 03:46 PDT, Nick Cameron [:nrc]
joe: review+
roc: feedback+
Details | Diff | Splinter Review

Description User image Nick Cameron [:nrc] 2012-05-16 23:05:11 PDT
Follow up
Comment 1 User image Nick Cameron [:nrc] 2012-05-17 00:11:31 PDT
Created attachment 624667 [details] [diff] [review]

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 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-17 02:30:30 PDT
Comment on attachment 624667 [details] [diff] [review]

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 User image Nick Cameron [:nrc] 2012-05-17 03:46:35 PDT
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.
Comment 4 User image Joe Drew (not getting mail) 2012-05-17 13:13:33 PDT
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 User image Ryan VanderMeulen [:RyanVM] 2012-05-17 16:37:49 PDT
Comment 6 User image :Ms2ger (⌚ UTC+1/+2) 2012-05-18 13:20:48 PDT

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