Nick Cameron [:nrc] Assignee
	Description • 12 years ago

      No description provided.

	Nick Cameron [:nrc] Assignee
	Comment 2 • 12 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.

	Bas Schouten (:bas.schouten)
	Comment 3 • 12 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?

	Joe Drew (not getting mail)
	Comment 4 • 12 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.

	Joe Drew (not getting mail)
	Comment 5 • 12 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?

	Nick Cameron [:nrc] Assignee
	Comment 6 • 12 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.

	Nick Cameron [:nrc] Assignee
	Comment 7 • 12 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.

	Joe Drew (not getting mail)
	Comment 10 • 12 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.

	Nick Cameron [:nrc] Assignee
	Comment 11 • 12 years ago

added comment as requested, carrying r=joe

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/404ef622ffd3

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 13 • 12 years ago

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

	Nick Cameron [:nrc] Assignee
	Comment 14 • 12 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?

	Ed Morley [:emorley]
	Comment 15 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/404ef622ffd3

	Mozilla RelEng Bot
	Comment 16 • 12 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.

	Nick Cameron [:nrc] Assignee
	Comment 17 • 12 years ago

Need to make some changes to the patch and rebase, then try again to land.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 18 • 12 years ago

Your patch already landed; if you need to change anything, please file a new bug.

	Nick Cameron [:nrc] Assignee
	Comment 19 • 12 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?

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 20 • 12 years ago

Your patch failed to apply, because it was already applied.