ozone/platform/wayland: Use more realistic event processing and request flushing in tests

ozone/platform/wayland: Use more realistic event processing and request flushing in tests

Now, we use a MessageLoop and wait for idle to determine when the client has
processed pending events.

This uncovered a number of situations where Wayland platform was not flushing
the connection. To resolve this, add a ScheduleFlush method to flush at some
later time, and call it when we send requests.

BUG=578890
Committed: https://crrev.com/5fb40e71b18c6c32dce4451821c7866fe80ea5e9
Cr-Commit-Position: refs/heads/master@{#380419}

[Michael Forney, 2016-02-27 00:02:54]

forney@google.com changed reviewers:
+ spang@chromium.org

[spang, 2016-03-01 21:40:49]

https://codereview.chromium.org/1739193004/diff/1/ui/ozone/platform/wayland/w...
File ui/ozone/platform/wayland/wayland_display.cc (right):

https://codereview.chromium.org/1739193004/diff/1/ui/ozone/platform/wayland/w...
ui/ozone/platform/wayland/wayland_display.cc:82: if (scheduled_flush_ ||
!base::MessageLoopForUI::IsCurrent())
I strongly disagree with making use of base::MessageLoopForUI::IsCurrent() for
flow control. Functions that care about the message loop should have it as a
precondition and write DCHECK() instead.

Why do we need to do this?

[Michael Forney, 2016-03-03 09:47:16]

https://codereview.chromium.org/1739193004/diff/1/ui/ozone/platform/wayland/w...
File ui/ozone/platform/wayland/wayland_display.cc (right):

https://codereview.chromium.org/1739193004/diff/1/ui/ozone/platform/wayland/w...
ui/ozone/platform/wayland/wayland_display.cc:82: if (scheduled_flush_ ||
!base::MessageLoopForUI::IsCurrent())
On 2016/03/01 21:40:49, spang wrote:
> I strongly disagree with making use of base::MessageLoopForUI::IsCurrent() for
> flow control. Functions that care about the message loop should have it as a
> precondition and write DCHECK() instead.
> 
> Why do we need to do this?

The reason is that we process events and sometimes make requests in response to
these events (for example, the wl_registry.global event which triggers
wl_registry.bind requests) while WaylandDisplay::Initialize is running, which
happens before the MessageLoop is created. We also may process the same types of
events after it is created (for which we should schedule the flush in the
message loop).

If we cannot use base::MessageLoopForUI::IsCurrent() to determine whether or not
to post a flush task, an alternative would be to keep a bool variable to track
when Initialize has finished (though, I think this doesn't guarantee that the
message loop is created, which is out of control of the platform).

[spang, 2016-03-03 21:05:23]

On 2016/03/03 09:47:16, Michael Forney wrote:
>
https://codereview.chromium.org/1739193004/diff/1/ui/ozone/platform/wayland/w...
> File ui/ozone/platform/wayland/wayland_display.cc (right):
> 
>
https://codereview.chromium.org/1739193004/diff/1/ui/ozone/platform/wayland/w...
> ui/ozone/platform/wayland/wayland_display.cc:82: if (scheduled_flush_ ||
> !base::MessageLoopForUI::IsCurrent())
> On 2016/03/01 21:40:49, spang wrote:
> > I strongly disagree with making use of base::MessageLoopForUI::IsCurrent()
for
> > flow control. Functions that care about the message loop should have it as a
> > precondition and write DCHECK() instead.
> > 
> > Why do we need to do this?
> 
> The reason is that we process events and sometimes make requests in response
to
> these events (for example, the wl_registry.global event which triggers
> wl_registry.bind requests) while WaylandDisplay::Initialize is running, which
> happens before the MessageLoop is created. We also may process the same types
of
> events after it is created (for which we should schedule the flush in the
> message loop).
> 
> If we cannot use base::MessageLoopForUI::IsCurrent() to determine whether or
not
> to post a flush task, an alternative would be to keep a bool variable to track
> when Initialize has finished (though, I think this doesn't guarantee that the
> message loop is created, which is out of control of the platform).

The fact that initialize is called without a loop is a longstanding defect. It
doesn't mean we should check for loop presence on every flush.

The right use of IsCurrent() is a precondition check:

  DCHECK(base::MessageLoopForUI::IsCurrent()) << "This function must run on the
UI thread";

It's not intended for checking how far along browser initialization has gotten.

Checking a boolean instead SGTM. You already have watching_.

[Michael Forney, 2016-03-10 15:23:05]

On 2016/03/03 21:05:23, spang wrote:
> Checking a boolean instead SGTM. You already have watching_.

Done.

[spang, 2016-03-10 15:59:39]

lgtm

[Michael Forney, 2016-03-10 16:07:10]

The CQ bit was checked by forney@google.com

[commit-bot: I haz the power, 2016-03-10 16:07:26]

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

[commit-bot: I haz the power, 2016-03-10 16:41:34]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-10 16:44:02]

Description was changed from

==========
ozone/platform/wayland: Use more realistic event processing and request flushing
in tests

Now, we use a MessageLoop and wait for idle to determine when the client has
processed pending events.

This uncovered a number of situations where Wayland platform was not flushing
the connection. To resolve this, add a ScheduleFlush method to flush at some
later time, and call it when we send requests.

BUG=578890
==========

to

==========
ozone/platform/wayland: Use more realistic event processing and request flushing
in tests

Now, we use a MessageLoop and wait for idle to determine when the client has
processed pending events.

This uncovered a number of situations where Wayland platform was not flushing
the connection. To resolve this, add a ScheduleFlush method to flush at some
later time, and call it when we send requests.

BUG=578890

Committed: https://crrev.com/5fb40e71b18c6c32dce4451821c7866fe80ea5e9
Cr-Commit-Position: refs/heads/master@{#380419}
==========

[commit-bot: I haz the power, 2016-03-10 16:44:03]

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