exo: Add support for subcompositor interface.

exo: Add support for sub-surfaces.

Sub-surfaces can be arbitrarily nested and create a tree of
sub-surfaces. They can be used to offload some of the compositing work
within a window from clients to the compositor.

BUG=549781
TEST=exo_unittests --gtest_filter=DisplayTest.GetShellSurface:SubSurfaceTest.*

Committed: https://crrev.com/27fe264d915a51c00667209d36c0e6287f3abb4e
Cr-Commit-Position: refs/heads/master@{#360766}

[reveman, 2015-11-07 02:54:11]

wip

[reveman, 2015-11-19 00:00:09]

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

[reveman, 2015-11-19 00:00:09]

please take a look.

[piman, 2015-11-19 21:59:10]

LGTM overall. I think it would be useful to add tests to check the views
hierarchy after surface operations + commits.

https://codereview.chromium.org/1419373013/diff/140001/components/exo/display.cc
File components/exo/display.cc (right):

https://codereview.chromium.org/1419373013/diff/140001/components/exo/display...
components/exo/display.cc:45: LOG(ERROR) << "Surface has already been assigned a
role";
nit: DLOG here and below

https://codereview.chromium.org/1419373013/diff/140001/components/exo/display...
components/exo/display.cc:57: if (surface == parent) {
Is there a possibility that indirect cycles might be created? What would happen?

https://codereview.chromium.org/1419373013/diff/140001/components/exo/surface.cc
File components/exo/surface.cc (right):

https://codereview.chromium.org/1419373013/diff/140001/components/exo/surface...
components/exo/surface.cc:88:
pending_sub_surfaces_.push_back(std::make_pair(sub_surface, gfx::Point()));
nit: DCHECK that the sub_surface is not already in the list? I think it would
cause issues otherwise. I think this is ensured by the caller because you can
only create one SubSurface per Surface, right?

https://codereview.chromium.org/1419373013/diff/140001/components/exo/surface...
components/exo/surface.cc:101: });
nit: worth a helper function SubSurfaceEntryList::iterator
FindSubSurfaceEntry(Surface* subsurface);

https://codereview.chromium.org/1419373013/diff/140001/components/exo/surface...
components/exo/surface.cc:123: reference->AsTracedValue());
nit: can you add a DCHECK_NE(sub_surface, reference);? This is ensured by the
caller, but it would be good to repeat here, otherwise the splice is undefined I
think.

https://codereview.chromium.org/1419373013/diff/140001/components/exo/surface...
components/exo/surface.cc:136: "surface that is neither a parent nor s sibling";
nit: typo 's sibling'->'a sibling'

https://codereview.chromium.org/1419373013/diff/140001/components/exo/surface...
components/exo/surface.cc:142: std::advance(position_it, 1);
nit: ++position_it ?

[reveman, 2015-11-20 05:19:48]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-11-20 05:19:49]

On 2015/11/19 at 21:59:10, piman wrote:
> LGTM overall. I think it would be useful to add tests to check the views
hierarchy after surface operations + commits.
> 

I added that to latest patch as well as a missing commit behavior test.

https://codereview.chromium.org/1419373013/diff/140001/components/exo/display.cc
File components/exo/display.cc (right):

https://codereview.chromium.org/1419373013/diff/140001/components/exo/display...
components/exo/display.cc:45: LOG(ERROR) << "Surface has already been assigned a
role";
On 2015/11/19 at 21:59:10, piman (slow to review) wrote:
> nit: DLOG here and below

Done.

https://codereview.chromium.org/1419373013/diff/140001/components/exo/display...
components/exo/display.cc:57: if (surface == parent) {
On 2015/11/19 at 21:59:10, piman (slow to review) wrote:
> Is there a possibility that indirect cycles might be created? What would
happen?

Yes, that would cause bad behavior. I changed this to check if surface contains
parent in latest patch and improved the unit test to verify that this works
properly. Thanks for pointing it out.

https://codereview.chromium.org/1419373013/diff/140001/components/exo/surface.cc
File components/exo/surface.cc (right):

https://codereview.chromium.org/1419373013/diff/140001/components/exo/surface...
components/exo/surface.cc:88:
pending_sub_surfaces_.push_back(std::make_pair(sub_surface, gfx::Point()));
On 2015/11/19 at 21:59:10, piman (slow to review) wrote:
> nit: DCHECK that the sub_surface is not already in the list?

Done.

> I think it would cause issues otherwise. I think this is ensured by the caller
because you can only create one SubSurface per Surface, right?

Correct and verified by unit tests in latest patch.

https://codereview.chromium.org/1419373013/diff/140001/components/exo/surface...
components/exo/surface.cc:101: });
On 2015/11/19 at 21:59:10, piman (slow to review) wrote:
> nit: worth a helper function SubSurfaceEntryList::iterator
FindSubSurfaceEntry(Surface* subsurface);

Done.

https://codereview.chromium.org/1419373013/diff/140001/components/exo/surface...
components/exo/surface.cc:123: reference->AsTracedValue());
On 2015/11/19 at 21:59:10, piman (slow to review) wrote:
> nit: can you add a DCHECK_NE(sub_surface, reference);? This is ensured by the
caller, but it would be good to repeat here, otherwise the splice is undefined I
think.

I added proper checking here in latest patch instead of relying on the caller to
do this. Same below.

https://codereview.chromium.org/1419373013/diff/140001/components/exo/surface...
components/exo/surface.cc:136: "surface that is neither a parent nor s sibling";
On 2015/11/19 at 21:59:10, piman (slow to review) wrote:
> nit: typo 's sibling'->'a sibling'

Done.

https://codereview.chromium.org/1419373013/diff/140001/components/exo/surface...
components/exo/surface.cc:142: std::advance(position_it, 1);
On 2015/11/19 at 21:59:10, piman (slow to review) wrote:
> nit: ++position_it ?

Done.

[reveman, 2015-11-20 05:19:49]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1419373013/#ps160001
(title: "address review feedback")

[commit-bot: I haz the power, 2015-11-20 05:19:57]

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

[commit-bot: I haz the power, 2015-11-20 06:33:46]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-11-20 06:34:44]

Patchset 9 (id:??) landed as
https://crrev.com/27fe264d915a51c00667209d36c0e6287f3abb4e
Cr-Commit-Position: refs/heads/master@{#360766}