Comply Compositing spec [1]; remove 'plus-darker'.

Comply Compositing spec [1]; remove 'plus-darker'.

Remove plus-darker which is legacy of WebKit. Even worse, 'plus-darker' is
implemented as darken, not darker.

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

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

[dshwang, 2014-10-21 18:22:18]

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

[dshwang, 2014-10-21 18:22:18]

Could you review? Before making GraphicsContext use SkXfermode, it's needed.
It's because CompositeOperator uses SkXfermode::kDarker, which WebBlendMode
uses.

Is it needed "Intent to ship" process while css composite and blend was already
accepted?

[Stephen Chennney, 2014-10-21 18:36:19]

I'm not sure that we need an intent to ship. I think the user base is tiny at
this point.

Krit or Rik should weigh in, I think.

[dshwang, 2014-10-21 18:39:35]

dongseong.hwang@intel.com changed reviewers:
+ cabanier@adobe.com, dschulze@chromium.org

[dshwang, 2014-10-21 18:50:51]

On 2014/10/21 18:36:19, Stephen Chenney wrote:
> I'm not sure that we need an intent to ship. I think the user base is tiny at
> this point.

I think so.

> Krit or Rik should weigh in, I think.

Add krit@ and Rik@ as reviewer, could you review?

[Stephen Chennney, 2014-10-21 19:15:33]

I just had a long offline discussion with krit looking at other browsers and the
spec.

We would like to land the change to remove plus-darker but do not land the
addition of destination. Destination is useless as an operator, and not
supported by CoreGraphics, so maybe the spec will be changed to remove it.

[Justin Novosad, 2014-10-21 19:16:11]

lgtm for the code.

But let's wait for others to weigh in before committing.

[dshwang, 2014-10-21 19:44:23]

On 2014/10/21 19:15:33, Stephen Chenney wrote:
> I just had a long offline discussion with krit looking at other browsers and
the
> spec.
> 
> We would like to land the change to remove plus-darker but do not land the
> addition of destination. Destination is useless as an operator, and not
> supported by CoreGraphics, so maybe the spec will be changed to remove it.

Thank you for offline discussion. New patch set doesn't add 'destination'. I
check that FF also doesn't support 'destination'
It's good idea to change the spec.

On 2014/10/21 19:16:11, junov wrote:
> lgtm for the code.
> 
> But let's wait for others to weigh in before committing.

Thank you for lgtm. I wait for others' review also.

[Stephen Chennney, 2014-10-21 20:00:43]

LGTM from me too. Thanks for cleaning things up in this space.

[dshwang, 2014-10-21 20:03:38]

On 2014/10/21 20:00:43, Stephen Chenney wrote:
> LGTM from me too. Thanks for cleaning things up in this space.

Thank you for lgtm. Can I land it?

[Stephen White, 2014-10-21 20:09:39]

I'm a little confused; does this remove the "darker" composite operation from
canvas? If so, it may be non-standard, but there are sites out there which rely
on it. E.g., before accelerated canvas supported it, we got this bug:
http://crbug.com/136830.

[dshwang, 2014-10-21 20:14:21]

On 2014/10/21 20:09:39, Stephen White wrote:
> I'm a little confused; does this remove the "darker" composite operation from
> canvas? If so, it may be non-standard, but there are sites out there which
rely
> on it. E.g., before accelerated canvas supported it, we got this bug:
> http://crbug.com/136830.

Thank you for pointing out.
What is darker composite operator? I cannot find the definition.
Currently Blink implements it as Darken blend mode;
http://dev.w3.org/fxtf/compositing-1/#blendingdarken
If the sites want to use "plus-darker", they should change it to "darken".
wdyt?

[dshwang, 2014-10-21 20:28:21]

On 2014/10/21 20:14:21, dshwang wrote:
> On 2014/10/21 20:09:39, Stephen White wrote:
> > I'm a little confused; does this remove the "darker" composite operation
from
> > canvas? If so, it may be non-standard, but there are sites out there which
> rely
> > on it. E.g., before accelerated canvas supported it, we got this bug:
> > http://crbug.com/136830.
> 
> Thank you for pointing out.
> What is darker composite operator? I cannot find the definition.
> Currently Blink implements it as Darken blend mode;
> http://dev.w3.org/fxtf/compositing-1/#blendingdarken
> If the sites want to use "plus-darker", they should change it to "darken".
> wdyt?

I found 'darker' definition:
http://helpx.adobe.com/photoshop/using/blending-modes.html
If we really want to support it, it should be supported by blend mode, not
composite operator.
So IMO how about educating web developers using 'darken' when they complain?

[Stephen White, 2014-10-21 20:39:39]

On 2014/10/21 20:28:21, dshwang wrote:
> On 2014/10/21 20:14:21, dshwang wrote:
> > On 2014/10/21 20:09:39, Stephen White wrote:
> > > I'm a little confused; does this remove the "darker" composite operation
> from
> > > canvas? If so, it may be non-standard, but there are sites out there which
> > rely
> > > on it. E.g., before accelerated canvas supported it, we got this bug:
> > > http://crbug.com/136830.
> > 
> > Thank you for pointing out.
> > What is darker composite operator? I cannot find the definition.
> > Currently Blink implements it as Darken blend mode;
> > http://dev.w3.org/fxtf/compositing-1/#blendingdarken
> > If the sites want to use "plus-darker", they should change it to "darken".
> > wdyt?
> 
> I found 'darker' definition:
> http://helpx.adobe.com/photoshop/using/blending-modes.html
> If we really want to support it, it should be supported by blend mode, not
> composite operator.
> So IMO how about educating web developers using 'darken' when they complain?

Well, we can't just go breaking sites if there a lot of them. I think we need to
get some UseCounter data first (if possible).

OTOH, you've only removed the token parsing for "plus-darker", not "darker", so 
I'm confused as to why canvas is affected at all.

[Stephen White, 2014-10-21 20:41:30]

On 2014/10/21 20:39:39, Stephen White wrote:
> On 2014/10/21 20:28:21, dshwang wrote:
> > On 2014/10/21 20:14:21, dshwang wrote:
> > > On 2014/10/21 20:09:39, Stephen White wrote:
> > > > I'm a little confused; does this remove the "darker" composite operation
> > from
> > > > canvas? If so, it may be non-standard, but there are sites out there
which
> > > rely
> > > > on it. E.g., before accelerated canvas supported it, we got this bug:
> > > > http://crbug.com/136830.
> > > 
> > > Thank you for pointing out.
> > > What is darker composite operator? I cannot find the definition.
> > > Currently Blink implements it as Darken blend mode;
> > > http://dev.w3.org/fxtf/compositing-1/#blendingdarken
> > > If the sites want to use "plus-darker", they should change it to "darken".
> > > wdyt?
> > 
> > I found 'darker' definition:
> > http://helpx.adobe.com/photoshop/using/blending-modes.html
> > If we really want to support it, it should be supported by blend mode, not
> > composite operator.
> > So IMO how about educating web developers using 'darken' when they complain?
> 
> Well, we can't just go breaking sites if there a lot of them. I think we need
to
> get some UseCounter data first (if possible).
> 
> OTOH, you've only removed the token parsing for "plus-darker", not "darker",
so 
> I'm confused as to why canvas is affected at all.

Oh, never mind. I see you've removed "plus-darker" from
CSSPrimitiveValueMappings.in, but
"darker" from GraphicsTypes.cpp. The latter is what canvas uses.

[Stephen Chennney, 2014-10-21 20:51:47]

Our testing on the canvas darker is almost non-existent. As far as I can tell we
only check that you can set it, and the test is incorrect.

LayoutTests/canvas/philip/tests/2d.composite.operation.darker.html

[Stephen White, 2014-10-21 21:07:11]

On 2014/10/21 20:51:47, Stephen Chenney wrote:
> Our testing on the canvas darker is almost non-existent.

fast/canvas/canvas-composite.html tests it as a pixel test.

>As far as I can tell we
> only check that you can set it, and the test is incorrect.
> 
> LayoutTests/canvas/philip/tests/2d.composite.operation.darker.html

Yeah, that's because the canvas/philip suite is an imported suite,
and it explicitly verifies that darker is *not* supported, as per the spec.

[krit, 2014-10-21 22:09:59]

On 2014/10/21 21:07:11, Stephen White wrote:
> On 2014/10/21 20:51:47, Stephen Chenney wrote:
> > Our testing on the canvas darker is almost non-existent.
> 
> fast/canvas/canvas-composite.html tests it as a pixel test.
> 
> >As far as I can tell we
> > only check that you can set it, and the test is incorrect.
> > 
> > LayoutTests/canvas/philip/tests/2d.composite.operation.darker.html
> 
> Yeah, that's because the canvas/philip suite is an imported suite,
> and it explicitly verifies that darker is *not* supported, as per the spec.

I do not know the algorithm for darker, but it is some kind of proprietary
combination of a blend mode and compositing mode as far as I can recall. If
darker is implemented as darken in Chromium it is not compatible to webkits
implementation anyway even though it might look similar to darken when colors
are opaque.

[krit, 2014-10-21 22:11:57]

On 2014/10/21 22:09:59, krit wrote:
> On 2014/10/21 21:07:11, Stephen White wrote:
> > On 2014/10/21 20:51:47, Stephen Chenney wrote:
> > > Our testing on the canvas darker is almost non-existent.
> > 
> > fast/canvas/canvas-composite.html tests it as a pixel test.
> > 
> > >As far as I can tell we
> > > only check that you can set it, and the test is incorrect.
> > > 
> > > LayoutTests/canvas/philip/tests/2d.composite.operation.darker.html
> > 
> > Yeah, that's because the canvas/philip suite is an imported suite,
> > and it explicitly verifies that darker is *not* supported, as per the spec.
> 
> I do not know the algorithm for darker, but it is some kind of proprietary
> combination of a blend mode and compositing mode as far as I can recall. If
> darker is implemented as darken in Chromium it is not compatible to webkits
> implementation anyway even though it might look similar to darken when colors
> are opaque.

As an additional note, Firefox doesn't support "darker".

[Rik, 2014-10-22 00:23:38]

On 2014/10/21 22:09:59, krit wrote:
> On 2014/10/21 21:07:11, Stephen White wrote:
> > On 2014/10/21 20:51:47, Stephen Chenney wrote:
> > > Our testing on the canvas darker is almost non-existent.
> > 
> > fast/canvas/canvas-composite.html tests it as a pixel test.
> > 
> > >As far as I can tell we
> > > only check that you can set it, and the test is incorrect.
> > > 
> > > LayoutTests/canvas/philip/tests/2d.composite.operation.darker.html
> > 
> > Yeah, that's because the canvas/philip suite is an imported suite,
> > and it explicitly verifies that darker is *not* supported, as per the spec.
> 
> I do not know the algorithm for darker, but it is some kind of proprietary
> combination of a blend mode and compositing mode as far as I can recall. If
> darker is implemented as darken in Chromium it is not compatible to webkits
> implementation anyway even though it might look similar to darken when colors
> are opaque.

The CG documentations says:
  kCGBlendModePlusDarker
  R = MAX(0, 1 - ((1 - D) + (1 - S)))

I agree that it should be removed from the implementation.
Destination should be removed from the canvas spec (and I will do so asap)

[dshwang, 2014-10-23 18:35:22]

On 2014/10/21 20:39:39, Stephen White wrote:
> Well, we can't just go breaking sites if there a lot of them. I think we need
to
> get some UseCounter data first (if possible).

junov@, can you check the usage counters for "darker"?
You checked the counters in https://codereview.chromium.org/408813007/#msg3 but
I don't know how to check.

On 2014/10/21 22:11:57, krit wrote:
> > > > LayoutTests/canvas/philip/tests/2d.composite.operation.darker.html
> > > 
> > > Yeah, that's because the canvas/philip suite is an imported suite,
> > > and it explicitly verifies that darker is *not* supported, as per the
spec.

Yes, famous philip's test explicitly demonstrates "darker" should be removed.

> As an additional note, Firefox doesn't support "darker".

Thank you for checking FF.

> The CG documentations says:
>   kCGBlendModePlusDarker
>   R = MAX(0, 1 - ((1 - D) + (1 - S)))

Aha, it's the formula of evil :)

> I agree that it should be removed from the implementation.
> Destination should be removed from the canvas spec (and I will do so asap)

Thank you.

[Justin Novosad, 2014-10-23 18:43:28]

On 2014/10/23 18:35:22, dshwang wrote:
> On 2014/10/21 20:39:39, Stephen White wrote:
> > Well, we can't just go breaking sites if there a lot of them. I think we
need
> to
> > get some UseCounter data first (if possible).
> 
> junov@, can you check the usage counters for "darker"?
> You checked the counters in https://codereview.chromium.org/408813007/#msg3
but
> I don't know how to check.

We do not have useCounters for that unfortunately. We would have to add one and
wait a release cycle to get real-world usage stats.

> 
> Aha, it's the formula of evil :)
> 

LOL. The dark side.

[dshwang, 2015-01-13 16:26:56]

I submitted Intent to remove, and then got 3 lgtm.
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/cHhgDH8kE7w
So I'm willing to remove "darker".

@junov, could you review again?

[Justin Novosad, 2015-01-13 18:30:17]

On 2015/01/13 16:26:56, dshwang wrote:
> I submitted Intent to remove, and then got 3 lgtm.
> https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/cHhgDH8kE7w
> So I'm willing to remove "darker".
> 
> @junov, could you review again?

There are layout test failures you need to take care of.

[Justin Novosad, 2015-01-13 18:36:44]

On 2015/01/13 18:30:17, junov wrote:
> On 2015/01/13 16:26:56, dshwang wrote:
> > @junov, could you review again?
> 
> There are layout test failures you need to take care of.

Looks like it is just some missing NeedsRebaseline lines in TestExpectations for
the virtual/display_list_2d_canvas suite.

lgtm

[dshwang, 2015-01-14 14:31:02]

On 2015/01/13 18:36:44, junov wrote:
> On 2015/01/13 18:30:17, junov wrote:
> > On 2015/01/13 16:26:56, dshwang wrote:
> > > @junov, could you review again?
> > 
> > There are layout test failures you need to take care of.
> 
> Looks like it is just some missing NeedsRebaseline lines in TestExpectations
for
> the virtual/display_list_2d_canvas suite.
> 
> lgtm

Thank you for reviewing. I marked
virtual/display_list_2d_canvas/fast/canvas/canvas-composite* to NeedsRebaseline

[dshwang, 2015-01-14 14:31:07]

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

[commit-bot: I haz the power, 2015-01-14 14:31:49]

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

[commit-bot: I haz the power, 2015-01-14 15:47:22]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188390