Make beginLayer() and CanvasRenderingContext2D use SkXfermode::Mode.

Make beginLayer() and CanvasRenderingContext2D use SkXfermode::Mode.

CompositeOperator and BlendMode is mutual exclusive. From the CSS
perspective, a css property is always set either, not both.[1]
For example, source-in with lighten is impossible. If source-in is needed,
BlendMode must be WebBlendNormal. Otherwise, if lighten is needed,
CompositeOperartion must be source-over.

This CL makes CanvasRenderingContext2D use SkXfermode::Mode directly. It helps
to avoid a bug of having invalid CompositeOperator and BlendMode combination.

GraphicsContext::beginLayer() gets WebBlendMode as parameter.
GraphicsContext::beginLayer() receives only CompositeOperator while
GraphicsContext::setCompositeOperation() receives both CompositeOperator and
BlendMode. It can causes a developer to be confused. For example,
CanvasRenderingContext2D::fullCanvasCompositedDraw() restores only
CompositeOperator after beginLayer(), although fortunately it doesn't cause any
bugs because BlendMode is always WebBlendNormal at the moment.

[1] http://dev.w3.org/fxtf/compositing-1

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

[dshwang, 2014-10-21 11:26:08]

dongseong.hwang@intel.com changed reviewers:
+ junov@chromium.org, senorblanco@chromium.org

[dshwang, 2014-10-21 11:26:28]

Stephen, could you review?

[Stephen Chennney, 2014-10-21 13:31:08]

schenney@chromium.org changed reviewers:
+ schenney@chromium.org

[Stephen Chennney, 2014-10-21 13:31:09]

I would prefer it if the constraint between the composite mode and blend mode
was enforced earlier, rather than in Skia utils. I see that some places
explicitly set the values so there's no need to check there, but maybe the
better fix is to enforce the constraint in the setter code. Developers are not
going to dig down to Skia utils to see the constraint.

I would also rather that a single parameter was passed since you can infer the
other. But it is not clear to me how you would do that without creating more API
complexity. So I'm not requesting it.

[Justin Novosad, 2014-10-21 15:22:58]

Could we take this opportunity to move GraphicsContext one step close to skia by
using SkXfermode::Mode and get rid of the other enums?

Also, what I read in the spec is that the are two groups: compositing modes and
blending modes. The CSS properties only accept blending modes, and the canvas
composite operation can be either a blending mode or a compositing mode.  When
you write "CompositeOperator and BlendMode is not mutual exclusive", actually, I
think the spec says that they are.  IMHO, the distinction between blending modes
and compositing modes should only exist in the API layer. In platform/graphics
there should be just one unified concept of compositing and blending.

[Stephen White, 2014-10-21 15:37:52]

+1 to moving call sites towards Skia semantics. I don't mind this patch as-is,
since it pairs up a compositing mode with a blending mode at the call site,
which is functionally equivalent to an SkXfermode. But if there's agreement that
we should go all the way to an SkXfermode, we should do that.

[dshwang, 2014-10-21 15:49:24]

I agree on you. I'll make GraphicsContext know only SkXfermode and updates call
sites.

[dshwang, 2014-10-21 18:27:46]

On 2014/10/21 15:49:24, dshwang wrote:
> I agree on you. I'll make GraphicsContext know only SkXfermode and updates
call
> sites.

I'm trying to do it, but it requires several incremental steps, because of huge
changes. This CL definitely helps GraphicsContext directly to handle SkXfermode.
So IMO this CL should be landed as small step of the project making
GraphicsContext to handle SkXfermode.
For example, crrev.com/663313005 is also needed. I'll do it incrementally.

[Justin Novosad, 2014-10-23 03:23:34]

On 2014/10/21 18:27:46, dshwang wrote:
> On 2014/10/21 15:49:24, dshwang wrote:
> > I agree on you. I'll make GraphicsContext know only SkXfermode and updates
> call
> > sites.
> 
> I'm trying to do it, but it requires several incremental steps, because of
huge
> changes. This CL definitely helps GraphicsContext directly to handle
SkXfermode.
> So IMO this CL should be landed as small step of the project making
> GraphicsContext to handle SkXfermode.
> For example, crrev.com/663313005 is also needed. I'll do it incrementally.

I see, I did not realize we had those dependencies when I suggested that.
However this change goes against the Idea of unifying blend modes and
compositing operators into a single parameter (which should eventually be
SkXfermode::Mode IMO)
Just to be clear: I don't think it is necessary to support SkXfermode (the
class) in GraphicsContext. Supporting just SkXfermode::Mode (the enum) would be
sufficient.

[dshwang, 2014-10-23 17:38:29]

On 2014/10/23 03:23:34, junov wrote:
> I see, I did not realize we had those dependencies when I suggested that.
> However this change goes against the Idea of unifying blend modes and
> compositing operators into a single parameter (which should eventually be
> SkXfermode::Mode IMO)

New patch set moves to unifying blend modes and compositing operators more
aggressively.

> Just to be clear: I don't think it is necessary to support SkXfermode (the
> class) in GraphicsContext. Supporting just SkXfermode::Mode (the enum) would
be
> sufficient.

Absolutely

[dshwang, 2014-10-23 17:39:29]

Patchset #4 (id:60001) has been deleted

[Justin Novosad, 2014-10-23 17:48:29]

Excellent! Thanks for doing this.
Assuming all the tests pass: lgtm

[dshwang, 2014-10-23 18:37:02]

On 2014/10/23 17:48:29, junov wrote:
> Excellent! Thanks for doing this.
> Assuming all the tests pass: lgtm

Thank you for review.

canvas/philip/tests/2d.composite.operation.darker.html fails because canvas
recognizes "darker" as "darken".
I hold this CL until https://codereview.chromium.org/663313005/ is resolved.

[dshwang, 2015-01-16 16:42:53]

After https://codereview.chromium.org/663313005/ is landed, I rebase it to ToT
@junov, could you review again?

[Justin Novosad, 2015-01-16 19:05:59]

On 2015/01/16 16:42:53, dshwang wrote:
> After https://codereview.chromium.org/663313005/ is landed, I rebase it to ToT
> @junov, could you review again?

lgtm

[dshwang, 2015-01-17 13:24:22]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2015-01-17 13:25:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/661053003/100001

[dshwang, 2015-01-18 17:13:56]

The CQ bit was unchecked by dongseong.hwang@intel.com

[dshwang, 2015-01-18 17:14:01]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2015-01-18 17:14:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/661053003/100001

[commit-bot: I haz the power, 2015-01-19 05:28:09]

Committed patchset #5 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188586