exo: Add support for alpha_compositing interface to wayland bindings.

exo: Add support for alpha_compositing interface to wayland bindings.

This makes it possible for a client to specify a blend equation and
an alpha value that will be applied to the whole surface.

BUG=549781, 610086
TEST=exo_unittests --gtest_filter=SurfaceTest.*

Committed: https://crrev.com/fca687e21055ccab05cdd81b3fc8cf1b1f256d8c
Cr-Commit-Position: refs/heads/master@{#392733}

[reveman, 2016-05-08 00:49:51]

reveman@chromium.org changed reviewers:
+ dcastagna@chromium.org

[Daniele Castagna, 2016-05-09 23:34:20]

https://codereview.chromium.org/1962473002/diff/1/components/exo/surface.h
File components/exo/surface.h (right):

https://codereview.chromium.org/1962473002/diff/1/components/exo/surface.h#ne...
components/exo/surface.h:93: void SetBlendMode(SkXfermode::Mode blend_mode);
nit: You can drop "This". Comments are usually imperative. I see the same "This
does" above though, so it can probably stay the same.

https://codereview.chromium.org/1962473002/diff/1/components/exo/surface.h#ne...
components/exo/surface.h:209: SkXfermode::Mode pending_blend_mode_;
Would it make sense to group all the pending state in a struct?

https://codereview.chromium.org/1962473002/diff/1/components/exo/surface_unit...
File components/exo/surface_unittest.cc (right):

https://codereview.chromium.org/1962473002/diff/1/components/exo/surface_unit...
components/exo/surface_unittest.cc:178: }
Is has_external_content true? Can you check the alpha on the layer?

https://codereview.chromium.org/1962473002/diff/1/components/exo/wayland/serv...
File components/exo/wayland/server.cc (right):

https://codereview.chromium.org/1962473002/diff/1/components/exo/wayland/serv...
components/exo/wayland/server.cc:2056: class Blending : public SurfaceObserver {
nit: comment before class declaration
(https://google.github.io/styleguide/cppguide.html#Class_Comments)

https://codereview.chromium.org/1962473002/diff/1/components/exo/wayland/serv...
components/exo/wayland/server.cc:2072: surface_->SetBlendMode(blend_mode);
Why don't you need to check the surface_ in this case?

https://codereview.chromium.org/1962473002/diff/1/components/exo/wayland/serv...
components/exo/wayland/server.cc:2111: DLOG(WARNING) << "Unsupported blending
equation: " << equation;
If you're asserting with NOTIMPLEMENTED in case of an non implemented equation,
why wouldn't you do the same in this case?

[reveman, 2016-05-10 14:05:19]

ptal

https://codereview.chromium.org/1962473002/diff/1/components/exo/surface.h
File components/exo/surface.h (right):

https://codereview.chromium.org/1962473002/diff/1/components/exo/surface.h#ne...
components/exo/surface.h:93: void SetBlendMode(SkXfermode::Mode blend_mode);
On 2016/05/09 at 23:34:19, Daniele Castagna wrote:
> nit: You can drop "This". Comments are usually imperative. I see the same
"This does" above though, so it can probably stay the same.

"This" all over place in this file so I'm going to keep it consistent and maybe
change it in a follow up.

https://codereview.chromium.org/1962473002/diff/1/components/exo/surface.h#ne...
components/exo/surface.h:209: SkXfermode::Mode pending_blend_mode_;
On 2016/05/09 at 23:34:19, Daniele Castagna wrote:
> Would it make sense to group all the pending state in a struct?

It might. I'll consider it for a follow up change.

https://codereview.chromium.org/1962473002/diff/1/components/exo/surface_unit...
File components/exo/surface_unittest.cc (right):

https://codereview.chromium.org/1962473002/diff/1/components/exo/surface_unit...
components/exo/surface_unittest.cc:178: }
On 2016/05/09 at 23:34:19, Daniele Castagna wrote:
> Is has_external_content true? Can you check the alpha on the layer?

has_external_content is true independent of this. There's no get_alpha and I'm
not sure it's worth adding as this is just going to be used until we've switched
to cc::surfaces.

Ideally we'd have a bunch of pixel tests for exo that can be used to verify this
type of thing. I think manual testing and just exercising the API in this unit
test is sufficient for now.

https://codereview.chromium.org/1962473002/diff/1/components/exo/wayland/serv...
File components/exo/wayland/server.cc (right):

https://codereview.chromium.org/1962473002/diff/1/components/exo/wayland/serv...
components/exo/wayland/server.cc:2056: class Blending : public SurfaceObserver {
On 2016/05/09 at 23:34:19, Daniele Castagna wrote:
> nit: comment before class declaration
(https://google.github.io/styleguide/cppguide.html#Class_Comments)

Done. There are 2 other classed (Viewport, Security) that does something very
similar in this file. I added consistent comments to all these classes as part
of this change.

https://codereview.chromium.org/1962473002/diff/1/components/exo/wayland/serv...
components/exo/wayland/server.cc:2072: surface_->SetBlendMode(blend_mode);
On 2016/05/09 at 23:34:20, Daniele Castagna wrote:
> Why don't you need to check the surface_ in this case?

Good catch. Fixed in latest patch.

https://codereview.chromium.org/1962473002/diff/1/components/exo/wayland/serv...
components/exo/wayland/server.cc:2111: DLOG(WARNING) << "Unsupported blending
equation: " << equation;
On 2016/05/09 at 23:34:20, Daniele Castagna wrote:
> If you're asserting with NOTIMPLEMENTED in case of an non implemented
equation, why wouldn't you do the same in this case?

The default case would be incorrect usage by the client while the NOTIMPLEMENTED
above is actually something we should implement and as discussed offline, that's
not assert. Just logs a not-implemented warning message.

[Daniele Castagna, 2016-05-10 16:17:08]

lgtm

[reveman, 2016-05-10 16:21:31]

reveman@chromium.org changed reviewers:
+ piman@chromium.org

[reveman, 2016-05-10 16:21:31]

+piman for ui/compositor

See b/27244278 and b/28178978 for more context.

[piman, 2016-05-10 19:45:51]

lgtm

[reveman, 2016-05-10 19:53:47]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2016-05-10 19:54:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1962473002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1962473002/20001

[commit-bot: I haz the power, 2016-05-10 21:44:59]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-10 21:46:41]

Description was changed from

==========
exo: Add support for alpha_compositing interface to wayland bindings.

This makes it possible for a client to specify a blend equation and
an alpha value that will be applied to the whole surface.

BUG=549781,610086
TEST=exo_unittests --gtest_filter=SurfaceTest.*
==========

to

==========
exo: Add support for alpha_compositing interface to wayland bindings.

This makes it possible for a client to specify a blend equation and
an alpha value that will be applied to the whole surface.

BUG=549781,610086
TEST=exo_unittests --gtest_filter=SurfaceTest.*

Committed: https://crrev.com/fca687e21055ccab05cdd81b3fc8cf1b1f256d8c
Cr-Commit-Position: refs/heads/master@{#392733}
==========

[commit-bot: I haz the power, 2016-05-10 21:46:43]

Patchset 2 (id:??) landed as
https://crrev.com/fca687e21055ccab05cdd81b3fc8cf1b1f256d8c
Cr-Commit-Position: refs/heads/master@{#392733}