Closed
Bug 756007
Opened 13 years ago
Closed 13 years ago
[Azure] Cairo backend - push/pop clip, check and fix path invariants
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: nrc, Assigned: nrc)
References
Details
Attachments
(1 file, 1 obsolete file)
1.46 KB,
patch
|
joe
:
review+
roc
:
feedback+
|
Details | Diff | Splinter Review |
Assignee | ||
Comment 1•13 years ago
|
||
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•13 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•13 years ago
|
||
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 4•13 years ago
|
||
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•13 years ago
|
Keywords: checkin-needed
Comment 5•13 years ago
|
||
Comment 6•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•