ozone/platform/wayland: Add some tests

ozone/platform/wayland: Add some tests

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

[Michael Forney, 2016-02-07 04:22:31]

Description was changed from

==========
ozone/platform/wayland: Add some basic tests

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

to

==========
ozone/platform/wayland: Add some tests

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

[Michael Forney, 2016-02-08 22:45:48]

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

[Michael Forney, 2016-02-08 22:45:48]

https://codereview.chromium.org/1673063002/diff/60001/ui/ozone/common/stub_cl...
File ui/ozone/common/stub_client_native_pixmap_factory.h (right):

https://codereview.chromium.org/1673063002/diff/60001/ui/ozone/common/stub_cl...
ui/ozone/common/stub_client_native_pixmap_factory.h:15: OZONE_EXPORT
ClientNativePixmapFactory* CreateStubClientNativePixmapFactory();
This should probably go in a separate CL, but I wanted to first check that it is
the correct thing to do.

https://codereview.chromium.org/1673063002/diff/60001/ui/ozone/platform/wayla...
File ui/ozone/platform/wayland/mock_platform_window_delegate.h (right):

https://codereview.chromium.org/1673063002/diff/60001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/mock_platform_window_delegate.h:11: class
MockPlatformWindowDelegate : public PlatformWindowDelegate {
Should I add move this to ui/platform_window in a new
//ui/platform_window:test_support target?

[rjkroege, 2016-02-09 01:30:40]

https://codereview.chromium.org/1673063002/diff/60001/ui/ozone/BUILD.gn
File ui/ozone/BUILD.gn (right):

https://codereview.chromium.org/1673063002/diff/60001/ui/ozone/BUILD.gn#newco...
ui/ozone/BUILD.gn:49: ozone_platform_test_deps += [
"platform/wayland:wayland_unittests" ]
I know that gn is the new way to go but you still need to update the gypi build?

https://codereview.chromium.org/1673063002/diff/60001/ui/ozone/common/stub_cl...
File ui/ozone/common/stub_client_native_pixmap_factory.h (right):

https://codereview.chromium.org/1673063002/diff/60001/ui/ozone/common/stub_cl...
ui/ozone/common/stub_client_native_pixmap_factory.h:15: OZONE_EXPORT
ClientNativePixmapFactory* CreateStubClientNativePixmapFactory();
On 2016/02/08 22:45:48, Michael Forney wrote:
> This should probably go in a separate CL, but I wanted to first check that it
is
> the correct thing to do.

There should be no need to export this? I am suspicious at any rate. :-)

https://codereview.chromium.org/1673063002/diff/60001/ui/ozone/platform/wayla...
File ui/ozone/platform/wayland/mock_platform_window_delegate.h (right):

https://codereview.chromium.org/1673063002/diff/60001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/mock_platform_window_delegate.h:11: class
MockPlatformWindowDelegate : public PlatformWindowDelegate {
On 2016/02/08 22:45:48, Michael Forney wrote:
> Should I add move this to ui/platform_window in a new
> //ui/platform_window:test_support target?

No.

https://codereview.chromium.org/1673063002/diff/60001/ui/ozone/platform/wayla...
File ui/ozone/platform/wayland/wayland_display_unittest.cc (right):

https://codereview.chromium.org/1673063002/diff/60001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/wayland_display_unittest.cc:44:
TEST_F(WaylandDisplayTest, Ping) {
can you add some comment somewhere about how to run the tests.

[Michael Forney, 2016-02-09 01:50:18]

https://codereview.chromium.org/1673063002/diff/60001/ui/ozone/common/stub_cl...
File ui/ozone/common/stub_client_native_pixmap_factory.h (right):

https://codereview.chromium.org/1673063002/diff/60001/ui/ozone/common/stub_cl...
ui/ozone/common/stub_client_native_pixmap_factory.h:15: OZONE_EXPORT
ClientNativePixmapFactory* CreateStubClientNativePixmapFactory();
On 2016/02/09 01:30:40, rjkroege wrote:
> On 2016/02/08 22:45:48, Michael Forney wrote:
> > This should probably go in a separate CL, but I wanted to first check that
it
> is
> > the correct thing to do.
> 
> There should be no need to export this? I am suspicious at any rate. :-)

Without this change, I get

../../ui/ozone/platform/wayland/client_native_pixmap_factory_wayland.cc:12:
error: undefined reference to 'ui::CreateStubClientNativePixmapFactory()'

This is because CreateClientNativePixmapFactoryWayland() in the :wayland target
calls CreateStubClientNativePixmapFactory(), which is part of //ui/ozone, not
//ui/ozone:ozone_base.

If I try to move common/stub_client_native_pixmap_factory.{cc,h} and
public/client_native_pixmap_factory.{cc,h} to ozone_base, I get

../../ui/ozone/public/client_native_pixmap_factory.cc:34: error: undefined
reference to 'ui::GetOzonePlatformName()'
../../ui/ozone/public/client_native_pixmap_factory.cc:36: error: undefined
reference to 'ui::PlatformObject<ui::ClientNativePixmapFactory>::Create()'

when linking libozone_base.so.

Maybe spang has some idea about what's going on and the best way to fix it.
https://chromium.googlesource.com/chromium/src/+/2837393c63b187eebc6207e39543...
seems closely related.

https://codereview.chromium.org/1673063002/diff/60001/ui/ozone/platform/wayla...
File ui/ozone/platform/wayland/wayland_display_unittest.cc (right):

https://codereview.chromium.org/1673063002/diff/60001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/wayland_display_unittest.cc:44:
TEST_F(WaylandDisplayTest, Ping) {
On 2016/02/09 01:30:40, rjkroege wrote:
> can you add some comment somewhere about how to run the tests.

Just ./ozone_unittests, nothing special.

[rjkroege, 2016-02-09 05:15:18]

it's fine except for the missing gypi build?

https://codereview.chromium.org/1673063002/diff/60001/ui/ozone/common/stub_cl...
File ui/ozone/common/stub_client_native_pixmap_factory.h (right):

https://codereview.chromium.org/1673063002/diff/60001/ui/ozone/common/stub_cl...
ui/ozone/common/stub_client_native_pixmap_factory.h:15: OZONE_EXPORT
ClientNativePixmapFactory* CreateStubClientNativePixmapFactory();
On 2016/02/09 01:50:18, Michael Forney wrote:
> On 2016/02/09 01:30:40, rjkroege wrote:
> > On 2016/02/08 22:45:48, Michael Forney wrote:
> > > This should probably go in a separate CL, but I wanted to first check that
> it
> > is
> > > the correct thing to do.
> > 
> > There should be no need to export this? I am suspicious at any rate. :-)
> 
> Without this change, I get
> 
> ../../ui/ozone/platform/wayland/client_native_pixmap_factory_wayland.cc:12:
> error: undefined reference to 'ui::CreateStubClientNativePixmapFactory()'
> 
> This is because CreateClientNativePixmapFactoryWayland() in the :wayland
target
> calls CreateStubClientNativePixmapFactory(), which is part of //ui/ozone, not
> //ui/ozone:ozone_base.
> 
> If I try to move common/stub_client_native_pixmap_factory.{cc,h} and
> public/client_native_pixmap_factory.{cc,h} to ozone_base, I get
> 
> ../../ui/ozone/public/client_native_pixmap_factory.cc:34: error: undefined
> reference to 'ui::GetOzonePlatformName()'
> ../../ui/ozone/public/client_native_pixmap_factory.cc:36: error: undefined
> reference to 'ui::PlatformObject<ui::ClientNativePixmapFactory>::Create()'
> 
> when linking libozone_base.so.
> 
> Maybe spang has some idea about what's going on and the best way to fix it.
>
https://chromium.googlesource.com/chromium/src/+/2837393c63b187eebc6207e39543...
> seems closely related.

ok

[spang, 2016-02-09 19:12:21]

On 2016/02/09 05:15:18, rjkroege wrote:
> it's fine except for the missing gypi build?
> 
>
https://codereview.chromium.org/1673063002/diff/60001/ui/ozone/common/stub_cl...
> File ui/ozone/common/stub_client_native_pixmap_factory.h (right):
> 
>
https://codereview.chromium.org/1673063002/diff/60001/ui/ozone/common/stub_cl...
> ui/ozone/common/stub_client_native_pixmap_factory.h:15: OZONE_EXPORT
> ClientNativePixmapFactory* CreateStubClientNativePixmapFactory();
> On 2016/02/09 01:50:18, Michael Forney wrote:
> > On 2016/02/09 01:30:40, rjkroege wrote:
> > > On 2016/02/08 22:45:48, Michael Forney wrote:
> > > > This should probably go in a separate CL, but I wanted to first check
that
> > it
> > > is
> > > > the correct thing to do.
> > > 
> > > There should be no need to export this? I am suspicious at any rate. :-)
> > 
> > Without this change, I get
> > 
> > ../../ui/ozone/platform/wayland/client_native_pixmap_factory_wayland.cc:12:
> > error: undefined reference to 'ui::CreateStubClientNativePixmapFactory()'
> > 
> > This is because CreateClientNativePixmapFactoryWayland() in the :wayland
> target
> > calls CreateStubClientNativePixmapFactory(), which is part of //ui/ozone,
not
> > //ui/ozone:ozone_base.
> > 
> > If I try to move common/stub_client_native_pixmap_factory.{cc,h} and
> > public/client_native_pixmap_factory.{cc,h} to ozone_base, I get
> > 
> > ../../ui/ozone/public/client_native_pixmap_factory.cc:34: error: undefined
> > reference to 'ui::GetOzonePlatformName()'
> > ../../ui/ozone/public/client_native_pixmap_factory.cc:36: error: undefined
> > reference to 'ui::PlatformObject<ui::ClientNativePixmapFactory>::Create()'
> > 
> > when linking libozone_base.so.
> > 
> > Maybe spang has some idea about what's going on and the best way to fix it.
> >
>
https://chromium.googlesource.com/chromium/src/+/2837393c63b187eebc6207e39543...
> > seems closely related.
> 
> ok

Remove the ":wayland" source_set dependency to avoid linking a duplicate copy of
the platform into the test. This creates a bunch of ODR violations and also your
link error.

You'll need to tag the platform classes referenced by the test with
OZONE_EXPORT.

[spang, 2016-02-09 19:14:24]

https://codereview.chromium.org/1673063002/diff/60001/ui/ozone/platform/wayla...
File ui/ozone/platform/wayland/BUILD.gn (right):

https://codereview.chromium.org/1673063002/diff/60001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/BUILD.gn:41: ":wayland",
Remove this dependency.

[Michael Forney, 2016-02-09 19:47:16]

On 2016/02/09 19:12:21, spang wrote:
> Remove the ":wayland" source_set dependency to avoid linking a duplicate copy
of
> the platform into the test. This creates a bunch of ODR violations and also
your
> link error.
> 
> You'll need to tag the platform classes referenced by the test with
> OZONE_EXPORT.

Ah, okay. Should this be fixed in the drm platform as well?

https://chromium.googlesource.com/chromium/src/+/master/ui/ozone/platform/drm...

[spang, 2016-02-09 19:50:40]

On 2016/02/09 19:47:16, Michael Forney wrote:
> On 2016/02/09 19:12:21, spang wrote:
> > Remove the ":wayland" source_set dependency to avoid linking a duplicate
copy
> of
> > the platform into the test. This creates a bunch of ODR violations and also
> your
> > link error.
> > 
> > You'll need to tag the platform classes referenced by the test with
> > OZONE_EXPORT.
> 
> Ah, okay. Should this be fixed in the drm platform as well?

Yes oops :(

[Michael Forney, 2016-02-10 02:01:00]

https://codereview.chromium.org/1673063002/diff/60001/ui/ozone/BUILD.gn
File ui/ozone/BUILD.gn (right):

https://codereview.chromium.org/1673063002/diff/60001/ui/ozone/BUILD.gn#newco...
ui/ozone/BUILD.gn:49: ozone_platform_test_deps += [
"platform/wayland:wayland_unittests" ]
On 2016/02/09 01:30:40, rjkroege wrote:
> I know that gn is the new way to go but you still need to update the gypi
build?

Sorry, slipped my mind. Done.

https://codereview.chromium.org/1673063002/diff/60001/ui/ozone/platform/wayla...
File ui/ozone/platform/wayland/BUILD.gn (right):

https://codereview.chromium.org/1673063002/diff/60001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/BUILD.gn:41: ":wayland",
On 2016/02/09 19:14:24, spang wrote:
> Remove this dependency.

Done.

https://codereview.chromium.org/1673063002/diff/60001/ui/ozone/platform/wayla...
File ui/ozone/platform/wayland/mock_platform_window_delegate.h (right):

https://codereview.chromium.org/1673063002/diff/60001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/mock_platform_window_delegate.h:11: class
MockPlatformWindowDelegate : public PlatformWindowDelegate {
On 2016/02/09 01:30:40, rjkroege wrote:
> On 2016/02/08 22:45:48, Michael Forney wrote:
> > Should I add move this to ui/platform_window in a new
> > //ui/platform_window:test_support target?
> 
> No.

Okay.

[spang, 2016-02-10 18:58:29]

https://codereview.chromium.org/1673063002/diff/80001/ui/ozone/platform/wayla...
File ui/ozone/platform/wayland/fake_server.cc (right):

https://codereview.chromium.org/1673063002/diff/80001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/fake_server.cc:18: void Delete(wl_resource* resource)
{
DeleteResourceData?

https://codereview.chromium.org/1673063002/diff/80001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/fake_server.cc:56: &DestroyResource, &Attach, &Damage,
Can you annotate all of them for consistency?

https://codereview.chromium.org/1673063002/diff/80001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/fake_server.cc:69: wl_resource_create(client,
&wl_surface_interface, 4, id);
What is 4? Maybe do

    static const int kWhatIsFour = 4;
    wl_resource_create(client, &wl_surface_interface, kWhatIsFour, id);

https://codereview.chromium.org/1673063002/diff/80001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/fake_server.cc:88: if (obj->mock)
When is this false?

https://codereview.chromium.org/1673063002/diff/80001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/fake_server.cc:302: setenv("WAYLAND_SOCKET",
std::to_string(client_fd.release()).c_str(), 1);
std::to_string not allowed yet

See https://chromium-cpp.appspot.com/

Use the functions in base/strings/string_number_conversions.h

[Michael Forney, 2016-02-11 03:38:09]

https://codereview.chromium.org/1673063002/diff/80001/ui/ozone/platform/wayla...
File ui/ozone/platform/wayland/fake_server.cc (right):

https://codereview.chromium.org/1673063002/diff/80001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/fake_server.cc:56: &DestroyResource, &Attach, &Damage,
On 2016/02/10 18:58:29, spang wrote:
> Can you annotate all of them for consistency?

Done.

https://codereview.chromium.org/1673063002/diff/80001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/fake_server.cc:69: wl_resource_create(client,
&wl_surface_interface, 4, id);
On 2016/02/10 18:58:28, spang wrote:
> What is 4? Maybe do
> 
>     static const int kWhatIsFour = 4;
>     wl_resource_create(client, &wl_surface_interface, kWhatIsFour, id);

Done.

https://codereview.chromium.org/1673063002/diff/80001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/fake_server.cc:88: if (obj->mock)
On 2016/02/10 18:58:29, spang wrote:
> When is this false?

It could be NULL if the implementation makes a request to the server before the
test has a chance to set the mock object, or if the implementation makes some
additional xdg_surface object (not associated with the PlatformWindow's
gfx::AcceleratedWidget).

https://codereview.chromium.org/1673063002/diff/80001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/fake_server.cc:302: setenv("WAYLAND_SOCKET",
std::to_string(client_fd.release()).c_str(), 1);
On 2016/02/10 18:58:29, spang wrote:
> std::to_string not allowed yet
> 
> See https://chromium-cpp.appspot.com/
> 
> Use the functions in base/strings/string_number_conversions.h

Done.

[spang, 2016-02-11 16:54:48]

https://codereview.chromium.org/1673063002/diff/80001/ui/ozone/platform/wayla...
File ui/ozone/platform/wayland/fake_server.cc (right):

https://codereview.chromium.org/1673063002/diff/80001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/fake_server.cc:88: if (obj->mock)
On 2016/02/11 03:38:09, Michael Forney wrote:
> On 2016/02/10 18:58:29, spang wrote:
> > When is this false?
> 
> It could be NULL if the implementation makes a request to the server before
the
> test has a chance to set the mock object, or if the implementation makes some
> additional xdg_surface object (not associated with the PlatformWindow's
> gfx::AcceleratedWidget).

Wouldn't forgetting to set the mock immediately be a programmer error?

I still don't really get why MockSurface is separate from FakeSurface. Why not
make FakeSurface be the mock instead of assigning an unowned pointer to some
other object?

[Michael Forney, 2016-02-12 00:49:58]

https://codereview.chromium.org/1673063002/diff/80001/ui/ozone/platform/wayla...
File ui/ozone/platform/wayland/fake_server.cc (right):

https://codereview.chromium.org/1673063002/diff/80001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/fake_server.cc:88: if (obj->mock)
On 2016/02/11 16:54:48, spang wrote:
> On 2016/02/11 03:38:09, Michael Forney wrote:
> > On 2016/02/10 18:58:29, spang wrote:
> > > When is this false?
> > 
> > It could be NULL if the implementation makes a request to the server before
> the
> > test has a chance to set the mock object, or if the implementation makes
some
> > additional xdg_surface object (not associated with the PlatformWindow's
> > gfx::AcceleratedWidget).
> 
> Wouldn't forgetting to set the mock immediately be a programmer error?

The client essentially owns the server-side FakeSurface and FakeXdgSurface
objects. The test can't control whether or not the implementation makes calls to
those objects before the test has a chance to look up the server-side resource
associated with the client object and set the mock (or if it creates and makes
calls on a separate object that the test is oblivious to).

> I still don't really get why MockSurface is separate from FakeSurface. Why not
> make FakeSurface be the mock instead of assigning an unowned pointer to some
> other object?

The reason for the separation between FakeSurface and MockSurface is ownership.
The client/implementation owns the FakeSurface (through the wl_resource), and
the test owns the MockSurface.

The client could destroy the resource (and therefore FakeSurface) at any time,
so really we should not even be storing pointers to FakeSurface or
FakeXdgSurface and expecting them to remain valid after calls to the
implementation. I don't do this in any of the current tests, but I haven't
really thought about it until now, and it is pretty subtle.

After thinking about this some more, maybe this would be a better way to handle
it:

- Remove FakeSurface and have one MockSurface object allocated by the server
when the client creates a new wl_surface object.
- Make MockSurface ref-counted, and have the wl_resource own one reference.
- Make FakeSurface::GetObject return a scoped_refptr<T>. When the test looks up
the MockSurface associated with the window's gfx::AcceleratedWidget, it would
increase the ref-count.
- Change the wl_resource destructor to set Surface->resource to nullptr and then
release its reference (rather than just deleting the Surface).
- Document that the tests must ASSERT_TRUE(surface->resource()) in between calls
to the implementation and events sent to the client. As mentioned above, we
don't have any cases where we do that now, but it would be good to document this
somewhere so we don't run into trouble in the future.

What do you think?

[spang, 2016-02-12 02:58:23]

On 2016/02/12 00:49:58, Michael Forney wrote:
>
https://codereview.chromium.org/1673063002/diff/80001/ui/ozone/platform/wayla...
> File ui/ozone/platform/wayland/fake_server.cc (right):
> 
>
https://codereview.chromium.org/1673063002/diff/80001/ui/ozone/platform/wayla...
> ui/ozone/platform/wayland/fake_server.cc:88: if (obj->mock)
> On 2016/02/11 16:54:48, spang wrote:
> > On 2016/02/11 03:38:09, Michael Forney wrote:
> > > On 2016/02/10 18:58:29, spang wrote:
> > > > When is this false?
> > > 
> > > It could be NULL if the implementation makes a request to the server
before
> > the
> > > test has a chance to set the mock object, or if the implementation makes
> some
> > > additional xdg_surface object (not associated with the PlatformWindow's
> > > gfx::AcceleratedWidget).
> > 
> > Wouldn't forgetting to set the mock immediately be a programmer error?
> 
> The client essentially owns the server-side FakeSurface and FakeXdgSurface
> objects. The test can't control whether or not the implementation makes calls
to
> those objects before the test has a chance to look up the server-side resource
> associated with the client object and set the mock (or if it creates and makes
> calls on a separate object that the test is oblivious to).
> 
> > I still don't really get why MockSurface is separate from FakeSurface. Why
not
> > make FakeSurface be the mock instead of assigning an unowned pointer to some
> > other object?
> 
> The reason for the separation between FakeSurface and MockSurface is
ownership.
> The client/implementation owns the FakeSurface (through the wl_resource), and
> the test owns the MockSurface.
> 
> The client could destroy the resource (and therefore FakeSurface) at any time,
> so really we should not even be storing pointers to FakeSurface or
> FakeXdgSurface and expecting them to remain valid after calls to the
> implementation. I don't do this in any of the current tests, but I haven't
> really thought about it until now, and it is pretty subtle.
> 
> After thinking about this some more, maybe this would be a better way to
handle
> it:
> 
> - Remove FakeSurface and have one MockSurface object allocated by the server
> when the client creates a new wl_surface object.
> - Make MockSurface ref-counted, and have the wl_resource own one reference.
> - Make FakeSurface::GetObject return a scoped_refptr<T>. When the test looks
up
> the MockSurface associated with the window's gfx::AcceleratedWidget, it would
> increase the ref-count.
> - Change the wl_resource destructor to set Surface->resource to nullptr and
then
> release its reference (rather than just deleting the Surface).
> - Document that the tests must ASSERT_TRUE(surface->resource()) in between
calls
> to the implementation and events sent to the client. As mentioned above, we
> don't have any cases where we do that now, but it would be good to document
this
> somewhere so we don't run into trouble in the future.
> 
> What do you think?

I do think you should create the mock in the server, yeah.

I'm not sure refcounting is needed. Why not just own all mock surfaces on
FakeServer and destroy them after the test ends? The resource dtor could just
call a ->Destroy() method on the mock, instead of deleting it. You can then use
the mock to test surface destruction.

More importantly, I am concerned about the use of wl_display_roundtrip() to
synchronize access to the server state from the client. Just because the server
is done processing client work doesn't mean it's done mutating its internal
state, or even that writes it did when processing those messages are visible to
the client thread. You need to use atomics for that.

One thing that would work is to just suspend the server thread before accessing
its state (PostTask + WaitableEvent can be used to implement this). Or you could
run the client part of the test to completion, stop the server thread, and only
then inspect the server's mocks.

[Michael Forney, 2016-02-12 04:14:09]

On 2016/02/12 02:58:23, spang wrote:
> I do think you should create the mock in the server, yeah.
> 
> I'm not sure refcounting is needed. Why not just own all mock surfaces on
> FakeServer and destroy them after the test ends? The resource dtor could just
> call a ->Destroy() method on the mock, instead of deleting it. You can then
use
> the mock to test surface destruction.

Okay, I will look into this.

> More importantly, I am concerned about the use of wl_display_roundtrip() to
> synchronize access to the server state from the client. Just because the
server
> is done processing client work doesn't mean it's done mutating its internal
> state, or even that writes it did when processing those messages are visible
to
> the client thread. You need to use atomics for that.

My thought process was this: if the server has finished processing the
wl_display.sync request, then it has finished processing all previous requests
(and therefore all the mock methods it has called have finished), so it is just
waiting on the connection fd to be ready for read.

Are you saying that even though the server thread has finished all calls to mock
methods, and will not interact with the mock objects again until the client
makes another request, it is unsafe to access them from the client? I do not
usually write multi-threaded programs, so my experience in this area is limited.

> One thing that would work is to just suspend the server thread before
accessing
> its state (PostTask + WaitableEvent can be used to implement this). Or you
could
> run the client part of the test to completion, stop the server thread, and
only
> then inspect the server's mocks.

Thanks, I was looking for something like WaitableEvent.

[spang, 2016-02-12 16:49:25]

On 2016/02/12 04:14:09, Michael Forney wrote:
> On 2016/02/12 02:58:23, spang wrote:
> > I do think you should create the mock in the server, yeah.
> > 
> > I'm not sure refcounting is needed. Why not just own all mock surfaces on
> > FakeServer and destroy them after the test ends? The resource dtor could
just
> > call a ->Destroy() method on the mock, instead of deleting it. You can then
> use
> > the mock to test surface destruction.
> 
> Okay, I will look into this.
> 
> > More importantly, I am concerned about the use of wl_display_roundtrip() to
> > synchronize access to the server state from the client. Just because the
> server
> > is done processing client work doesn't mean it's done mutating its internal
> > state, or even that writes it did when processing those messages are visible
> to
> > the client thread. You need to use atomics for that.
> 
> My thought process was this: if the server has finished processing the
> wl_display.sync request, then it has finished processing all previous requests
> (and therefore all the mock methods it has called have finished), so it is
just
> waiting on the connection fd to be ready for read.
> 
> Are you saying that even though the server thread has finished all calls to
mock
> methods, and will not interact with the mock objects again until the client
> makes another request, it is unsafe to access them from the client? I do not
> usually write multi-threaded programs, so my experience in this area is
limited. 

I'd be more concerned about the wl_client_get_object() call than the mock. It
does not take a lock, and is therefore obviously not intended to be used on
multiple threads. So we need to be extremely careful about using it.

There's really no guarantee that acking the sync message used internally by
roundtrip() means the server is immediately idle and won't write to some object
you're accessing from the client. The dispatcher has some code to run destructor
work after client work is done at least.

> 
> > One thing that would work is to just suspend the server thread before
> accessing
> > its state (PostTask + WaitableEvent can be used to implement this). Or you
> could
> > run the client part of the test to completion, stop the server thread, and
> only
> > then inspect the server's mocks.
> 
> Thanks, I was looking for something like WaitableEvent.

[Michael Forney, 2016-02-12 22:43:14]

On 2016/02/12 16:49:25, spang wrote:
> I'd be more concerned about the wl_client_get_object() call than the mock. It
> does not take a lock, and is therefore obviously not intended to be used on
> multiple threads. So we need to be extremely careful about using it.
> 
> There's really no guarantee that acking the sync message used internally by
> roundtrip() means the server is immediately idle and won't write to some
object
> you're accessing from the client. The dispatcher has some code to run
destructor
> work after client work is done at least.

Okay, I understand. I think the code you refer to is only for freeing client
connection event sources (not anything the test cares about) and running idle
tasks (which the tests don't use), and in practice, nothing was actually written
back to the client until wl_display_flush_clients was called after
wl_event_loop_dispatch returned.

I do like the idea of suspending the server before accessing its state, so I've
added Pause/Resume methods to FakeServer which use a WaitableEvent to
synchronize with the client thread. Let me know if this is what you had in mind,
or if you have any ideas about how to improve this further.

[spang, 2016-02-13 04:01:00]

On 2016/02/12 22:43:14, Michael Forney wrote:
> On 2016/02/12 16:49:25, spang wrote:
> > I'd be more concerned about the wl_client_get_object() call than the mock.
It
> > does not take a lock, and is therefore obviously not intended to be used on
> > multiple threads. So we need to be extremely careful about using it.
> > 
> > There's really no guarantee that acking the sync message used internally by
> > roundtrip() means the server is immediately idle and won't write to some
> object
> > you're accessing from the client. The dispatcher has some code to run
> destructor
> > work after client work is done at least.
> 
> Okay, I understand. I think the code you refer to is only for freeing client
> connection event sources (not anything the test cares about) and running idle
> tasks (which the tests don't use), and in practice, nothing was actually
written
> back to the client until wl_display_flush_clients was called after
> wl_event_loop_dispatch returned.

Yeah, I think I could be convinced that your current tests are deterministic.
But the argument would rely on undocumented properties of the library which we
should avoid relying on.

> I do like the idea of suspending the server before accessing its state, so
I've
> added Pause/Resume methods to FakeServer which use a WaitableEvent to
> synchronize with the client thread. Let me know if this is what you had in
mind,
> or if you have any ideas about how to improve this further.

Yep, that's exactly what I had in mind.

lgtm

[Michael Forney, 2016-02-13 07:28:16]

Uploaded a new patch set after one final pass, and intentionally breaking
various bits to see how others reacted.

Changes are:

- Fixed a race due to the re-use of the same WaitableEvent for pause and resume.
- Made FakeServer::GetObject return nullptr if the ID was invalid.
- Added an OnResourceDestroyed to Global to handle the destruction of a bound
resource (to prevent dangling pointer).
- Re-arranged the interface implementations to be in alphabetical order, and
some class definitions to match the header.
- Made tests call Resume unconditionally in TearDown in case an assertion failed
during SetUp while the server was Paused.
- Dropped some unneeded expectations and early mock verifies.
- Initialized the gfx::AcceleratedWidget to kNullAcceleratedWidget in case the
implementation never calls OnAcceleratedWidgetAvailable.

I think everything is pretty solid now. Thanks for the helpful suggestions!

I'm not sure what the policy is on post-approval changes (and I'm in no hurry),
so I'll wait until Tuesday to submit this.

[spang, 2016-02-13 11:34:04]

lgtm

[Michael Forney, 2016-02-14 02:15:27]

The CQ bit was checked by forney@google.com

[commit-bot: I haz the power, 2016-02-14 02:15:47]

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

[commit-bot: I haz the power, 2016-02-14 02:55:22]

Description was changed from

==========
ozone/platform/wayland: Add some tests

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

to

==========
ozone/platform/wayland: Add some tests

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

[commit-bot: I haz the power, 2016-02-14 02:55:23]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-02-16 22:48:07]

Description was changed from

==========
ozone/platform/wayland: Add some tests

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

to

==========
ozone/platform/wayland: Add some tests

BUG=578890

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

[commit-bot: I haz the power, 2016-02-16 22:48:08]

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