GC::saveLayer() may taint the current clip.

GC::saveLayer() may taint the current clip.

SaveLayer() can optionally apply a clip. In that case we need to make
sure that we're saving the current clip, to avoid tainting it.

This was discovered while experimenting with SaveFlags removal and
requires a minor test rebaseline.

R=schenney@chormium.org,senorblanco@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170096

[reed1, 2014-03-26 16:16:38]

lgtm

[Stephen White, 2014-03-26 16:29:15]

(I actually had another patch for this, but I can't find it right now .)

Is there a heavy cost to simply saving the clip (passing this flag) in all
cases? It is in the default set of flags for saveLayer(), and would protect us
in case someone inside the rendering code sets the clip without restoring it.

[Stephen White, 2014-03-26 16:32:55]

On 2014/03/26 16:29:15, Stephen White wrote:
> (I actually had another patch for this, but I can't find it right now .)
> 
> Is there a heavy cost to simply saving the clip (passing this flag) in all
> cases? It is in the default set of flags for saveLayer(), and would protect us
> in case someone inside the rendering code sets the clip without restoring it.

Also, when looking at this before, I wasn't sure why we call clipLayer() here
explicitly, rather than just passing kClipToLayer_SaveFlag. Isn't that the same
thing?

[Stephen White, 2014-03-26 16:36:17]

On 2014/03/26 16:32:55, Stephen White wrote:
> On 2014/03/26 16:29:15, Stephen White wrote:
> > (I actually had another patch for this, but I can't find it right now .)
> > 
> > Is there a heavy cost to simply saving the clip (passing this flag) in all
> > cases? It is in the default set of flags for saveLayer(), and would protect
us
> > in case someone inside the rendering code sets the clip without restoring
it.
> 
> Also, when looking at this before, I wasn't sure why we call clipLayer() here

s/clipLayer/clipRect/

> explicitly, rather than just passing kClipToLayer_SaveFlag. Isn't that the
same
> thing?

[f(malita), 2014-03-26 16:56:50]

On 2014/03/26 16:29:15, Stephen White wrote:
> (I actually had another patch for this, but I can't find it right now .)
> 
> Is there a heavy cost to simply saving the clip (passing this flag) in all
> cases? It is in the default set of flags for saveLayer(), and would protect us
> in case someone inside the rendering code sets the clip without restoring it.

Crossed my mind. The cost should be minimal in comparison with the actual layer
overhead so we could do that. I'd actually prefer to always save the clip *and*
matrix - which is more in line with future Skia behavior.

Will update.

> Also, when looking at this before, I wasn't sure why we call clipLayer() here
> explicitly, rather than just passing kClipToLayer_SaveFlag. Isn't that the
same
> thing?

I think it's replaceable, but I recall some chatter about kClipToLayer_SaveFlag
going away (or always being enforced?). Mike?

A quick test yields quite a few extra rebaselines (they look better - soft clip
vs. hard clip), so I'd rather do this in a follow-up since it's unrelated.

[f(malita), 2014-03-26 16:59:34]

Updated.

[Stephen White, 2014-03-26 17:38:27]

On 2014/03/26 16:56:50, Florin Malita wrote:
> On 2014/03/26 16:29:15, Stephen White wrote:
> > (I actually had another patch for this, but I can't find it right now .)
> > 
> > Is there a heavy cost to simply saving the clip (passing this flag) in all
> > cases? It is in the default set of flags for saveLayer(), and would protect
us
> > in case someone inside the rendering code sets the clip without restoring
it.
> 
> Crossed my mind. The cost should be minimal in comparison with the actual
layer
> overhead so we could do that. I'd actually prefer to always save the clip
*and*
> matrix - which is more in line with future Skia behavior.
> 
> Will update.
> 
> > Also, when looking at this before, I wasn't sure why we call clipLayer()
here
> > explicitly, rather than just passing kClipToLayer_SaveFlag. Isn't that the
> same
> > thing?
> 
> I think it's replaceable, but I recall some chatter about
kClipToLayer_SaveFlag
> going away (or always being enforced?). Mike?

Yes, I think it's always going to be present (which means the clipRect will be
redundant, I think).

> A quick test yields quite a few extra rebaselines (they look better - soft
clip
> vs. hard clip), so I'd rather do this in a follow-up since it's unrelated.

Sure, sounds good. When we do that, I think we can simply use the default flags
(ARGB_ClipLayer) in all cases. beginLayer() is passing
RGB + A, you've added matrix + clip, so that + kClipToLayer = kARGB_ClipLayer.

[Stephen White, 2014-03-26 17:45:52]

LGTM

https://codereview.chromium.org/212993004/diff/20001/Source/platform/graphics...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/212993004/diff/20001/Source/platform/graphics...
Source/platform/graphics/GraphicsContext.cpp:192: SkCanvas::kMatrixClip_SaveFlag
| extraFlags);
Just an idea, but we could probably set these in the flags passed by
beginLayer() instead of here. The only calls to saveLayer() I can see other than
beginLayer() are from FEGaussianBlur and FEMorphology, which use the default arg
of kARGB_ClipLayer_SaveFlag anyway, which already includes matrix & clip. (Those
two should probably be calling beginLayer() anyway, then we can make saveLayer()
private.) Anyway, all that can be done in another patch.

[f(malita), 2014-03-26 19:31:28]

https://codereview.chromium.org/212993004/diff/20001/Source/platform/graphics...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/212993004/diff/20001/Source/platform/graphics...
Source/platform/graphics/GraphicsContext.cpp:192: SkCanvas::kMatrixClip_SaveFlag
| extraFlags);
On 2014/03/26 17:45:52, Stephen White wrote:
> Just an idea, but we could probably set these in the flags passed by
> beginLayer() instead of here. The only calls to saveLayer() I can see other
than
> beginLayer() are from FEGaussianBlur and FEMorphology, which use the default
arg
> of kARGB_ClipLayer_SaveFlag anyway, which already includes matrix & clip.
(Those
> two should probably be calling beginLayer() anyway, then we can make
saveLayer()
> private.) Anyway, all that can be done in another patch.

Done.

[f(malita), 2014-03-26 19:31:31]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2014-03-26 19:31:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fmalita@chromium.org/212993004/40001

[commit-bot: I haz the power, 2014-03-26 20:41:13]

Change committed as 170096