Make mus_demo_unittests work with multiple root windows

Make mus_demo_unittests work with multiple root windows

WindowServerTestImpl::EnsureClientHasDrawnWindow verifies whether one
root window of the WindowTree is drawn. This currently works well for
mus_demo_unittests because the WindowServer only has a single root
window. However, MusDemo will open multiple root windows in external
mode and hence mus_demo_unittests should really check whether all these
windows are drawn.

BUG=666958

[fwang, 2017-02-23 08:39:41]

Description was changed from

==========
Make mus_demo_unittests work with multiple root windows

WindowServerTestImpl::EnsureClientHasDrawnWindow verify whether one root
window of the WindowTree is drawn. This currently works well for
mus_demo_unittests because the WindowServer only has a single root
window. However, MusDemo will open multiple root windows in external
mode and hence mus_demo_unittests should really check whether all these
windows are drawn.

BUG=666958
==========

to

==========
Make mus_demo_unittests work with multiple root windows

WindowServerTestImpl::EnsureClientHasDrawnWindow verify whether one root
window of the WindowTree is drawn. This currently works well for
mus_demo_unittests because the WindowServer only has a single root
window. However, MusDemo will open multiple root windows in external
mode and hence mus_demo_unittests should really check whether all these
windows are drawn.

BUG=666958
==========

[fwang, 2017-02-23 11:11:44]

The CQ bit was checked by fwang@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 11:11:59]

Dry run: 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, 2017-02-23 12:21:06]

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

[commit-bot: I haz the power, 2017-02-23 12:21:07]

Dry run: This issue passed the CQ dry run.

[fwang, 2017-02-23 12:27:03]

Description was changed from

==========
Make mus_demo_unittests work with multiple root windows

WindowServerTestImpl::EnsureClientHasDrawnWindow verify whether one root
window of the WindowTree is drawn. This currently works well for
mus_demo_unittests because the WindowServer only has a single root
window. However, MusDemo will open multiple root windows in external
mode and hence mus_demo_unittests should really check whether all these
windows are drawn.

BUG=666958
==========

to

==========
Make mus_demo_unittests work with multiple root windows

WindowServerTestImpl::EnsureClientHasDrawnWindow verifies whether one
root window of the WindowTree is drawn. This currently works well for
mus_demo_unittests because the WindowServer only has a single root
window. However, MusDemo will open multiple root windows in external
mode and hence mus_demo_unittests should really check whether all these
windows are drawn.

BUG=666958
==========

[fwang, 2017-02-23 12:35:52]

fwang@igalia.com changed reviewers:
+ sky@chromium.org

[sky, 2017-02-23 20:04:49]

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

[sky, 2017-02-23 20:04:49]

The window server knows how many windows a client has, can't it figure out the
count if we want it to do that?

[fwang, 2017-02-23 20:19:12]

On 2017/02/23 20:04:49, sky wrote:
> The window server knows how many windows a client has, can't it figure out the
> count if we want it to do that?

You mean we can just avoid passing "expected_window_count" and replace
"tree->roots().size() == expected_window_count" with true? Yes, I think that
will work.

[fwang, 2017-02-23 20:24:24]

On 2017/02/23 20:19:12, fwang wrote:
> On 2017/02/23 20:04:49, sky wrote:
> > The window server knows how many windows a client has, can't it figure out
the
> > count if we want it to do that?
> 
> You mean we can just avoid passing "expected_window_count" and replace
> "tree->roots().size() == expected_window_count" with true? Yes, I think that
> will work.

Oh, but my reasoning for that was that if there is a regression somewhere and
MusDemoExternal does not open as many windows as expected, then the test "all
opened windows have been drawn" will still pass and hence is too weak.

[sky, 2017-02-23 21:33:15]

The callback could return the count. That way the test can assert on the count.

On Thu, Feb 23, 2017 at 12:24 PM,  <fwang@igalia.com> wrote:
> On 2017/02/23 20:19:12, fwang wrote:
>> On 2017/02/23 20:04:49, sky wrote:
>> > The window server knows how many windows a client has, can't it figure
>> > out
> the
>> > count if we want it to do that?
>>
>> You mean we can just avoid passing "expected_window_count" and replace
>> "tree->roots().size() == expected_window_count" with true? Yes, I think
>> that
>> will work.
>
> Oh, but my reasoning for that was that if there is a regression somewhere
> and
> MusDemoExternal does not open as many windows as expected, then the test
> "all
> opened windows have been drawn" will still pass and hence is too weak.
>
> https://codereview.chromium.org/2715533005/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[fwang, 2017-02-24 08:29:21]

On 2017/02/23 21:33:15, sky wrote:
> The callback could return the count. That way the test can assert on the
count.
> 

OK, I'll do that. That's indeed much better to use EXPECT_EQ

[fwang, 2017-02-24 08:51:15]

https://codereview.chromium.org/2715533005/diff/40001/services/ui/ws/window_s...
File services/ui/ws/window_server_test_impl.cc (right):

https://codereview.chromium.org/2715533005/diff/40001/services/ui/ws/window_s...
services/ui/ws/window_server_test_impl.cc:53: "for that client name.";
This is maybe not a big concern for now (the function is only called in two
places and in separate tests) but maybe we should:
1) execute callback with a 0 value (or the current count) here.
2) or more complex: store the callback in the map too, execute the old callback
with the current count here and start again with the new callback.
3) or even more complex: accumulate the callbacks in the map and execute all of
them when the root count is reached.

WDYT?

[sky, 2017-02-24 18:27:38]

How about waiting until you actual need this value?

On Fri, Feb 24, 2017 at 12:51 AM,  <fwang@igalia.com> wrote:
>
>
https://codereview.chromium.org/2715533005/diff/40001/services/ui/ws/window_s...
> File services/ui/ws/window_server_test_impl.cc (right):
>
>
https://codereview.chromium.org/2715533005/diff/40001/services/ui/ws/window_s...
> services/ui/ws/window_server_test_impl.cc:53: "for that client name.";
> This is maybe not a big concern for now (the function is only called in
> two places and in separate tests) but maybe we should:
> 1) execute callback with a 0 value (or the current count) here.
> 2) or more complex: store the callback in the map too, execute the old
> callback with the current count here and start again with the new
> callback.
> 3) or even more complex: accumulate the callbacks in the map and execute
> all of them when the root count is reached.
>
> WDYT?
>
> https://codereview.chromium.org/2715533005/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[fwang, 2017-02-24 18:47:57]

On 2017/02/24 18:27:38, sky wrote:
> How about waiting until you actual need this value?
> 

OK.

[fwang, 2017-03-01 17:24:34]

On 2017/02/24 18:27:38, sky wrote:
> How about waiting until you actual need this value?

OK I've rebased on top of Antonio's patches. Let's wait that they are ready
before doing the changes to mus_demo_unitests.

BTW, I've also added a test checking whether 5 "external mode" windows are
drawn, and it passes with Antonio's patches.

https://codereview.chromium.org/2715533005/diff/80001/services/ui/ws/window_s...
File services/ui/ws/window_server_test_impl.cc (right):

https://codereview.chromium.org/2715533005/diff/80001/services/ui/ws/window_s...
services/ui/ws/window_server_test_impl.cc:44: callback.Run(0);
I added this for completeness, although it's actually not needed.

[fwang, 2017-04-03 10:25:22]

On 2017/02/24 18:27:38, sky wrote:
> How about waiting until you actual need this value?

Given that review of https://codereview.chromium.org/2712203002/ is stuck for
now, we could probably merge that change now. Otherwise, this is already merged
in Igalia's chromium fork so I would just close it and let someone else upstream
it later.

[fwang, 2017-04-05 17:32:38]

Closing that since it does not seem it will be upstreamed for now.