Mus: Fixes rendering of transparent containers.

Mus: Fixes rendering of transparent containers.

Pre-fix, setting opacity on the parent window did not affect how the child
windows were rendered.
Effective opacity of window contents when rendered needs to be a product of the
opacities of the window and all its ancestors.
This CL fixes the issue and adds a unit test.

BUG=NONE

Committed: https://crrev.com/0c2f515ccb40f6ecb9c2a04acf6187cd32f00715
Cr-Commit-Position: refs/heads/master@{#404171}

[mfomitchev, 2016-06-25 02:28:49]

mfomitchev@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2016-06-27 15:17:08]

https://codereview.chromium.org/2099893002/diff/1/components/mus/ws/platform_...
File components/mus/ws/platform_display.h (right):

https://codereview.chromium.org/2099893002/diff/1/components/mus/ws/platform_...
components/mus/ws/platform_display.h:149: friend class
mus::ws::test::PlatformDisplayTest;
If you're going to friend this, then don't bother with the set function.

https://codereview.chromium.org/2099893002/diff/1/components/mus/ws/platform_...
File components/mus/ws/platform_display_unittest.cc (right):

https://codereview.chromium.org/2099893002/diff/1/components/mus/ws/platform_...
components/mus/ws/platform_display_unittest.cc:39: EXPECT_TRUE(frame_data);
If frame_data is null then line 40 crashes. I recommend no assertions in this
function and the caller can assert on the return value.

https://codereview.chromium.org/2099893002/diff/1/components/mus/ws/platform_...
components/mus/ws/platform_display_unittest.cc:116: EXPECT_EQ(2u,
quad_state_list->size());
How fragile is this code? By that I mean it seems as though it would be easy to
make slight changes to DrawWindowTree() that would effect what gets generated
and have to update this function.

I'm also wondering if we should make it so a test can directly call
DrawWindowTree().

https://codereview.chromium.org/2099893002/diff/1/components/mus/ws/test_utils.h
File components/mus/ws/test_utils.h (right):

https://codereview.chromium.org/2099893002/diff/1/components/mus/ws/test_util...
components/mus/ws/test_utils.h:223:
TestPlatformDisplayDelegate(std::unique_ptr<ServerWindow> root);
explicit

[Fady Samuel, 2016-06-27 15:20:03]

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

[Fady Samuel, 2016-06-27 15:20:03]

https://codereview.chromium.org/2099893002/diff/1/components/mus/ws/platform_...
File components/mus/ws/platform_display_unittest.cc (right):

https://codereview.chromium.org/2099893002/diff/1/components/mus/ws/platform_...
components/mus/ws/platform_display_unittest.cc:116: EXPECT_EQ(2u,
quad_state_list->size());
On 2016/06/27 15:17:08, sky wrote:
> How fragile is this code? By that I mean it seems as though it would be easy
to
> make slight changes to DrawWindowTree() that would effect what gets generated
> and have to update this function.
> 
> I'm also wondering if we should make it so a test can directly call
> DrawWindowTree().

drive-by +1 I think DrawWindowTree and associated methods should move out into a
separate class. WindowTreeFrameGenerator?

[mfomitchev, 2016-06-30 22:06:03]

Rewrote this on top of the FrameGenerator refactor, PTAL.

https://codereview.chromium.org/2099893002/diff/1/components/mus/ws/platform_...
File components/mus/ws/platform_display.h (right):

https://codereview.chromium.org/2099893002/diff/1/components/mus/ws/platform_...
components/mus/ws/platform_display.h:149: friend class
mus::ws::test::PlatformDisplayTest;
On 2016/06/27 15:17:08, sky wrote:
> If you're going to friend this, then don't bother with the set function.

Done.

https://codereview.chromium.org/2099893002/diff/1/components/mus/ws/platform_...
File components/mus/ws/platform_display_unittest.cc (right):

https://codereview.chromium.org/2099893002/diff/1/components/mus/ws/platform_...
components/mus/ws/platform_display_unittest.cc:39: EXPECT_TRUE(frame_data);
On 2016/06/27 15:17:08, sky wrote:
> If frame_data is null then line 40 crashes. I recommend no assertions in this
> function and the caller can assert on the return value.

Done.

https://codereview.chromium.org/2099893002/diff/1/components/mus/ws/platform_...
components/mus/ws/platform_display_unittest.cc:116: EXPECT_EQ(2u,
quad_state_list->size());
On 2016/06/27 15:20:03, Fady Samuel wrote:
> On 2016/06/27 15:17:08, sky wrote:
> > How fragile is this code? By that I mean it seems as though it would be easy
> to
> > make slight changes to DrawWindowTree() that would effect what gets
generated
> > and have to update this function.
> > 
> > I'm also wondering if we should make it so a test can directly call
> > DrawWindowTree().
> 
> drive-by +1 I think DrawWindowTree and associated methods should move out into
a
> separate class. WindowTreeFrameGenerator?

Switched to testing DrawWindowTree directly. I am still checking the number of
quads that gets generated. I think it should be reasonably stable, since it
should match the number of surfaces per window, which I assume shouldn't change
too much?

https://codereview.chromium.org/2099893002/diff/1/components/mus/ws/test_utils.h
File components/mus/ws/test_utils.h (right):

https://codereview.chromium.org/2099893002/diff/1/components/mus/ws/test_util...
components/mus/ws/test_utils.h:223:
TestPlatformDisplayDelegate(std::unique_ptr<ServerWindow> root);
On 2016/06/27 15:17:08, sky wrote:
> explicit

Done.

[sky, 2016-07-01 16:04:14]

LGTM

https://codereview.chromium.org/2099893002/diff/20001/components/mus/ws/frame...
File components/mus/ws/frame_generator_unittest.cc (right):

https://codereview.chromium.org/2099893002/diff/20001/components/mus/ws/frame...
components/mus/ws/frame_generator_unittest.cc:11: #include
"components/mus/ws/frame_generator.h"
nit: this should be your first include, just like frame_generator.cc (see style
guide).

[mfomitchev, 2016-07-07 02:01:59]

The CQ bit was checked by mfomitchev@chromium.org

[mfomitchev, 2016-07-07 02:02:00]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2099893002/#ps40001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-07-07 02:02:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mfomitchev, 2016-07-07 02:03:52]

The CQ bit was unchecked by mfomitchev@chromium.org

[mfomitchev, 2016-07-07 14:49:56]

https://codereview.chromium.org/2099893002/diff/20001/components/mus/ws/frame...
File components/mus/ws/frame_generator_unittest.cc (right):

https://codereview.chromium.org/2099893002/diff/20001/components/mus/ws/frame...
components/mus/ws/frame_generator_unittest.cc:11: #include
"components/mus/ws/frame_generator.h"
On 2016/07/01 16:04:13, sky wrote:
> nit: this should be your first include, just like frame_generator.cc (see
style
> guide).

Done.

[mfomitchev, 2016-07-07 14:50:02]

The CQ bit was checked by mfomitchev@chromium.org

[mfomitchev, 2016-07-07 14:50:03]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2099893002/#ps60001
(title: "Moving an include.")

[commit-bot: I haz the power, 2016-07-07 14:50:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-07 14:56:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-07 14:56:14]

Try jobs failed on following builders:
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)

[Fady Samuel, 2016-07-07 15:12:33]

https://codereview.chromium.org/2099893002/diff/60001/services/ui/ws/frame_ge...
File services/ui/ws/frame_generator_unittest.cc (right):

https://codereview.chromium.org/2099893002/diff/60001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator_unittest.cc:130: }  // namespace ui
nit: ws

https://codereview.chromium.org/2099893002/diff/60001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator_unittest.cc:131: }  // namespace mus
nit: ui

[mfomitchev, 2016-07-07 15:16:41]

https://codereview.chromium.org/2099893002/diff/60001/services/ui/ws/frame_ge...
File services/ui/ws/frame_generator_unittest.cc (right):

https://codereview.chromium.org/2099893002/diff/60001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator_unittest.cc:130: }  // namespace ui
On 2016/07/07 15:12:33, Fady Samuel wrote:
> nit: ws

Done.

https://codereview.chromium.org/2099893002/diff/60001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator_unittest.cc:131: }  // namespace mus
On 2016/07/07 15:12:33, Fady Samuel wrote:
> nit: ui

Done.

[mfomitchev, 2016-07-07 15:19:44]

The CQ bit was checked by mfomitchev@chromium.org

[mfomitchev, 2016-07-07 15:19:44]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2099893002/#ps80001
(title: "Fix namespace comments")

[commit-bot: I haz the power, 2016-07-07 15:20:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-07 15:25:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-07 15:25:19]

Try jobs failed on following builders:
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)

[mfomitchev, 2016-07-07 15:35:19]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2016-07-07 15:36:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-07 17:42:23]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-07 17:42:45]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-07 17:43:29]

Description was changed from

==========
Mus: Fixes rendering of transparent containers.

Pre-fix, setting opacity on the parent window did not affect how the child
windows were rendered.
Effective opacity of window contents when rendered needs to be a product of the
opacities of the window and all its ancestors.
This CL fixes the issue and adds a unit test.

BUG=NONE
==========

to

==========
Mus: Fixes rendering of transparent containers.

Pre-fix, setting opacity on the parent window did not affect how the child
windows were rendered.
Effective opacity of window contents when rendered needs to be a product of the
opacities of the window and all its ancestors.
This CL fixes the issue and adds a unit test.

BUG=NONE

Committed: https://crrev.com/0c2f515ccb40f6ecb9c2a04acf6187cd32f00715
Cr-Commit-Position: refs/heads/master@{#404171}
==========

[commit-bot: I haz the power, 2016-07-07 17:43:30]

Patchset 5 (id:??) landed as
https://crrev.com/0c2f515ccb40f6ecb9c2a04acf6187cd32f00715
Cr-Commit-Position: refs/heads/master@{#404171}