Closed
Bug 747822
Opened 13 years ago
Closed 13 years ago
[Azure] Implement push/pop clip in Cairo backend
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: nrc, Assigned: nrc)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 3 obsolete files)
2.31 KB,
patch
|
nrc
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•13 years ago
|
||
Attachment #620171 -
Flags: review?(joe)
Assignee | ||
Comment 2•13 years ago
|
||
Push to try: https://tbpl.mozilla.org/?tree=Try&rev=19591f43adcf
Reference push: https://tbpl.mozilla.org/?tree=Try&rev=14eeb79a91ca
Both have pretty much the same fails, so I assume I haven't broken anything, but I haven't checked which tests pass/fail. I've tested manually and clipping on Azure/Cairo canvas works properly.
Comment 3•13 years ago
|
||
Comment on attachment 620171 [details] [diff] [review]
push/pop clip patch
Review of attachment 620171 [details] [diff] [review]:
-----------------------------------------------------------------
This might be a naive question as I don't know the cairo backend that well, but is there no other state that we need to ensure is not affected by this save/restore pair?
Comment 4•13 years ago
|
||
Comment on attachment 620171 [details] [diff] [review]
push/pop clip patch
Review of attachment 620171 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/2d/DrawTargetCairo.cpp
@@ +625,5 @@
> + if (aPath->GetBackendType() != BACKEND_CAIRO) {
> + return;
> + }
> +
> + PathCairo* path = const_cast<PathCairo*>(static_cast<const PathCairo*>(aPath));
You need to call WillChange(aPath) here, because we're going to set aPath to be our current path.
@@ +635,5 @@
>
> void
> DrawTargetCairo::PushClipRect(const Rect& aRect)
> {
> + cairo_save(mContext);
Similarly, you need to call WillChange() here. However!
We actually run into a problem in WillChange(), in that we're going to change the path from under the PathObserver, but we don't have code that handles the case of aPath = NULL.
So, you should do one of two things:
1) implement aPath = NULL meaning "forget about your current path". If you do this, you'll need to check all the callers of WillChange (remember implicit callers!) to make sure that it won't cause unintended problems.
2) Create a new function called something like PathWillChange() to do the above, and only call it in the relevant areas.
I leave it up to you to decide which makes most sense.
Attachment #620171 -
Flags: review?(joe) → review-
Comment 5•13 years ago
|
||
(In reply to Bas Schouten (:bas) from comment #3)
> Comment on attachment 620171 [details] [diff] [review]
> push/pop clip patch
>
> Review of attachment 620171 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> This might be a naive question as I don't know the cairo backend that well,
> but is there no other state that we need to ensure is not affected by this
> save/restore pair?
The Cairo path object handles things for us pretty well. What did you have in mind?
Assignee | ||
Comment 6•13 years ago
|
||
(In reply to Bas Schouten (:bas) from comment #3)
> Comment on attachment 620171 [details] [diff] [review]
> push/pop clip patch
>
> Review of attachment 620171 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> This might be a naive question as I don't know the cairo backend that well,
> but is there no other state that we need to ensure is not affected by this
> save/restore pair?
Save/restore also affects the transform, but the way Cairo is used here won't affect us.
Assignee | ||
Comment 7•13 years ago
|
||
(In reply to Joe Drew (:JOEDREW!) from comment #4)
> Comment on attachment 620171 [details] [diff] [review]
> push/pop clip patch
>
> Review of attachment 620171 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: gfx/2d/DrawTargetCairo.cpp
> @@ +625,5 @@
> > + if (aPath->GetBackendType() != BACKEND_CAIRO) {
> > + return;
> > + }
> > +
> > + PathCairo* path = const_cast<PathCairo*>(static_cast<const PathCairo*>(aPath));
>
> You need to call WillChange(aPath) here, because we're going to set aPath to
> be our current path.
>
> @@ +635,5 @@
> >
> > void
> > DrawTargetCairo::PushClipRect(const Rect& aRect)
> > {
> > + cairo_save(mContext);
>
> Similarly, you need to call WillChange() here. However!
>
> We actually run into a problem in WillChange(), in that we're going to
> change the path from under the PathObserver, but we don't have code that
> handles the case of aPath = NULL.
>
> So, you should do one of two things:
>
> 1) implement aPath = NULL meaning "forget about your current path". If you
> do this, you'll need to check all the callers of WillChange (remember
> implicit callers!) to make sure that it won't cause unintended problems.
> 2) Create a new function called something like PathWillChange() to do the
> above, and only call it in the relevant areas.
>
> I leave it up to you to decide which makes most sense.
I'm not exactly clear, but I think this has already been implemented. If so, then I just need to make a minor change to WillChange() (in the new patch) and it's all good. If I'm wrong, I'll have another go.
Assignee | ||
Comment 8•13 years ago
|
||
Attachment #620171 -
Attachment is obsolete: true
Attachment #620972 -
Flags: review?
Assignee | ||
Updated•13 years ago
|
Attachment #620972 -
Flags: review? → review?(joe)
Assignee | ||
Comment 9•13 years ago
|
||
Attachment #620972 -
Attachment is obsolete: true
Attachment #620972 -
Flags: review?(joe)
Attachment #620981 -
Flags: review?(joe)
Comment 10•13 years ago
|
||
Comment on attachment 620981 [details] [diff] [review]
push/pop clip patch
Review of attachment 620981 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/2d/DrawTargetCairo.cpp
@@ +645,5 @@
>
> void
> DrawTargetCairo::PopClip()
> {
> + cairo_restore(mContext);
Add a comment above this saying "cairo_restore doesn't affect the path, so we don't need to do a WillChange()" or something to that effect.
Attachment #620981 -
Flags: review?(joe) → review+
Assignee | ||
Comment 11•13 years ago
|
||
added comment as requested, carrying r=joe
Attachment #620981 -
Attachment is obsolete: true
Attachment #621784 -
Flags: review+
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Updated•13 years ago
|
Whiteboard: [autoland:-p all -u all]
Updated•13 years ago
|
Whiteboard: [autoland:-p all -u all] → [autoland-try:-p all -u all]
Updated•13 years ago
|
Whiteboard: [autoland-try:-p all -u all] → [autoland-in-queue]
Comment 12•13 years ago
|
||
Flags: in-testsuite-
Keywords: checkin-needed
Whiteboard: [autoland-in-queue]
Target Milestone: --- → mozilla15
Comment on attachment 621784 [details] [diff] [review]
push/pop clip patch
Review of attachment 621784 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/2d/DrawTargetCairo.cpp
@@ +639,5 @@
> {
> + WillChange();
> + cairo_save(mContext);
> + cairo_rectangle(mContext, aRect.X(), aRect.Y(), aRect.Width(), aRect.Height());
> + cairo_clip_preserve(mContext);
Seems to me that you need a cairo_new_path(mContext) here
Assignee | ||
Comment 14•13 years ago
|
||
Should I just change the cairo_clip_preserve to cairo_clip?
Or do you mean before this code?
And now I realise I am not so sure of the semantics here - do we leave the cairo path (in the context) clear after an operation, or do we clear it before we start an operation?
Comment 15•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 16•13 years ago
|
||
Autoland Patchset:
Patches: 621784
Branch: mozilla-central => try
Patch 621784 could not be applied to mozilla-central.
patching file gfx/2d/DrawTargetCairo.cpp
Hunk #1 FAILED at 610
Hunk #2 FAILED at 762
2 out of 2 hunks FAILED -- saving rejects to file gfx/2d/DrawTargetCairo.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
Patchset could not be applied and pushed.
Assignee | ||
Comment 17•13 years ago
|
||
Need to make some changes to the patch and rebase, then try again to land.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 18•13 years ago
|
||
Your patch already landed; if you need to change anything, please file a new bug.
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 19•13 years ago
|
||
Perhaps I misinterpreted comment 16, I thought that meant the patch could not be applied. Has hunk 2 of the patch landed or not? And if not, should it be a new bug?
Comment 20•13 years ago
|
||
Your patch failed to apply, because it was already applied.
You need to log in
before you can comment on or make changes to this bug.
Description
•