exo: Implement wayland gamepad support

components/exo/gamepad_unittest.cc

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ash/shell.h"
+#include "base/command_line.h"
+#include "components/exo/buffer.h"
+#include "components/exo/gamepad.h"
+#include "components/exo/gamepad_delegate.h"
+#include "components/exo/shell_surface.h"
+#include "components/exo/surface.h"
+#include "components/exo/test/exo_test_base.h"
+#include "components/exo/test/exo_test_helper.h"
+#include "device/gamepad/gamepad_test_helpers.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/aura/client/focus_client.h"
+
+namespace exo {
+namespace {
+
+class MockGamepadDelegate : public GamepadDelegate {
+ public:
+  MockGamepadDelegate() {}
+
+  // Overridden from GamepadDelegate:
+  MOCK_METHOD1(OnGamepadDestroying, void(Gamepad*));
+  MOCK_CONST_METHOD1(CanAcceptGamepadEventsForSurface, bool(Surface*));
+  MOCK_METHOD1(OnStateChange, void(bool));
+  MOCK_METHOD2(OnAxis, void(int, double));
+  MOCK_METHOD3(OnButton, void(int, bool, double));
+  MOCK_METHOD0(OnFrame, void());
+};
+
+class GamepadTest : public test::ExoTestBase {

[reveman, 2016/06/30 18:06:12]

Consider avoiding this test fixture to be consistent with other input device
unit tests. The down side is of course a lot of copy and paste but that's
something we might want to address in a clean consistent way across all exo unit
tests.

[denniskempin, 2016/07/01 02:34:36]

I am not quite sure I understand your reasoning to do so. What is the benefit of
copy and pasting the test fixture methods into the test cases below other than
consistency?
Even when this is being addressed later, it will ease the refactoring since for
this test case there is only one place to adjust the common code. 

[reveman, 2016/07/07 22:14:02]

Consistency would be the benefit and imo a sufficient one. When someone does the
work to make all unit test code less copy-n-paste-y then this new gamepad code
is not going to require any additional work. I'm not going to push back on this
as it's my fault that the current amount of code-n-paste is needed for these
tests.

+ public:
+  GamepadTest() {}
+
+  void FocusNonExo() {

[reveman, 2016/06/30 18:06:12]

nit: FocusNonClientSurface. However, I think it's even better to remove this
function and move the code into the test with a comment.

[denniskempin, 2016/07/01 02:34:35]

no longer needed.

+    aura::client::FocusClient* focus_client =
+        aura::client::GetFocusClient(ash::Shell::GetPrimaryRootWindow());
+    focus_client->FocusWindow(nullptr);
+  }
+
+  void FocusExo() {

[reveman, 2016/06/30 18:06:12]

ditto

[denniskempin, 2016/07/01 02:34:36]

no longer needed.

+    aura::client::FocusClient* focus_client =
+        aura::client::GetFocusClient(ash::Shell::GetPrimaryRootWindow());
+    focus_client->FocusWindow(surface_->window());
+  }
+
+  void SetUp() override {
+    test::ExoTestBase::SetUp();
+    surface_.reset(new Surface);
+    shell_surface_.reset(new ShellSurface(surface_.get()));
+    gfx::Size buffer_size(10, 10);
+    buffer_.reset(
+        new Buffer(exo_test_helper()->CreateGpuMemoryBuffer(buffer_size)));
+    surface_->Attach(buffer_.get());
+    surface_->Commit();
+  }
+
+  void TearDown() override {
+    buffer_.reset();
+    shell_surface_.reset();
+    surface_.reset();
+    test::ExoTestBase::TearDown();
+  }
+
+  void InitializeGamepad(MockGamepadDelegate* delegate) {
+    blink::WebGamepads initial_data;
+    mock_data_fetcher_ = new device::MockGamepadDataFetcher(initial_data);

[reveman, 2016/06/30 18:06:11]

nit: can you refactor the Gamepad code so it doesn't look like we're leaking
memory here? ie. have the Gamepad ctor take a raw pointer and keep the ownership
here?

[denniskempin, 2016/07/01 02:34:36]

Done.

+
+    gamepad_.reset(new Gamepad(
+        delegate,
+        std::unique_ptr<device::GamepadDataFetcher>(mock_data_fetcher_)));
+  }
+
+  void DestroyGamepad(MockGamepadDelegate* delegate) {
+    EXPECT_CALL(*delegate, OnGamepadDestroying(testing::_)).Times(1);
+    gamepad_.reset();
+  }
+
+  void SetDataAndWaitForDelegate(const blink::WebGamepads& new_data) {
+    mock_data_fetcher_->SetTestData(new_data);
+    // Wait for 2 polling cycles for data to be read and callbacks
+    // to be processed.

[reveman, 2016/06/30 18:06:11]

It's strongly discouraged to have tests that depend timers firing. Can the
polling thread be instrumented in such a way that we better control it for
testing purposes. E.g. have the polling thread implemented outside the gamepad
code and just have Gamepad class operate on a task runner? This would allow us
to use a  TestSimpleTaskRunner here which is the recommended way of testing
multi-threaded code like this.

[denniskempin, 2016/07/01 02:34:36]

This is not based on timers, but based on the polling. The mock data fetcher
receives a signal when polling happened, so we can wait for that to happen and
do not rely on timing.

[denniskempin, 2016/07/01 17:16:50]

I've been looking into just providing a TaskRunner to the class. This would add
a large amount of complexity to the Gamepad class. Right now the polling thread
is owned by the Gamepad class and forced to shut down before Gamepad is
destroyed. 
If we move the thread out, the thread will outlive the gamepad class which opens
up the possibility that Gamepad is destroyed while the polling thread is
executing. 
Nothing that can't be solved.. but it adds quite complex synchronization logic
that I would like to prevent as it might open up possibilities for missing weird
edge cases.
Right now the relationship between the origin and polling thread is very clear
and easy to synchronize.

[reveman, 2016/07/07 22:14:02]

Does changing the polling frequency to 16 seconds instead of 16ms affect how
long it takes to run these tests? That's what I meant by rely on timing. 
Is it just not a matter of using a weak ptr as we're already doing for messages
to the origin thread? Anyhow, if this is indeed complicated then avoiding it in
this patch is fine as long as we're not adding tests that take 16ms to run.

+    for (int i = 0; i < 2; ++i) {
+      mock_data_fetcher_->WaitForDataRead();
+      base::MessageLoop::current()->RunUntilIdle();
+    }
+  }
+
+ protected:
+  std::unique_ptr<Gamepad> gamepad_;
+
+  // Pointer owned by the gamepad instance.
+  device::MockGamepadDataFetcher* mock_data_fetcher_;
+
+  std::unique_ptr<Surface> surface_;
+  std::unique_ptr<ShellSurface> shell_surface_;
+  std::unique_ptr<Buffer> buffer_;
+
+  DISALLOW_COPY_AND_ASSIGN(GamepadTest);
+};
+
+TEST_F(GamepadTest, ConnectDisconnect) {

[reveman, 2016/06/30 18:06:12]

Can you name the tests based on the delegate function they are testing? That's
the convention used for other input device classes. In this case I think it
would be s/ConnectDisconnect/OnStateChange/

[denniskempin, 2016/07/01 02:34:35]

Done.

+  testing::StrictMock<MockGamepadDelegate> delegate;
+  EXPECT_CALL(delegate, CanAcceptGamepadEventsForSurface(testing::_))
+      .WillOnce(testing::Return(true));
+
+  InitializeGamepad(&delegate);
+
+  // Gamepad connected.
+  EXPECT_CALL(delegate, OnStateChange(true)).Times(1);
+  blink::WebGamepads gamepad_connected;
+  gamepad_connected.length = 1;
+  gamepad_connected.items[0].connected = true;
+  gamepad_connected.items[0].timestamp = 1;
+  SetDataAndWaitForDelegate(gamepad_connected);
+
+  // Gamepad disconnected.
+  blink::WebGamepads all_disconnected;
+  EXPECT_CALL(delegate, OnStateChange(false)).Times(1);
+  SetDataAndWaitForDelegate(all_disconnected);
+
+  DestroyGamepad(&delegate);
+}
+
+TEST_F(GamepadTest, AxisMove) {

[reveman, 2016/06/30 18:06:12]

s/AxisMove/OnAxis/?

[denniskempin, 2016/07/01 02:34:35]

Done.

+  testing::StrictMock<MockGamepadDelegate> delegate;
+  EXPECT_CALL(delegate, CanAcceptGamepadEventsForSurface(testing::_))
+      .WillOnce(testing::Return(true));
+
+  InitializeGamepad(&delegate);
+
+  blink::WebGamepads axis_moved;
+  axis_moved.length = 1;
+  axis_moved.items[0].connected = true;
+  axis_moved.items[0].timestamp = 1;
+  axis_moved.items[0].axesLength = 1;
+  axis_moved.items[0].axes[0] = 1.0;
+
+  // Gamepad connected.
+  EXPECT_CALL(delegate, OnStateChange(true)).Times(1);
+  EXPECT_CALL(delegate, OnAxis(0, 1.0)).Times(1);
+  EXPECT_CALL(delegate, OnFrame()).Times(1);
+  SetDataAndWaitForDelegate(axis_moved);
+
+  DestroyGamepad(&delegate);
+}
+
+TEST_F(GamepadTest, FocusHandling) {

[reveman, 2016/06/30 18:06:12]

s/FocusHandling/CanAcceptGamepadEventsForSurface/ ?

[denniskempin, 2016/07/01 02:34:35]

I had to remove the focus handling test since I am not sure how to test this
properly without any race conditions.

+  testing::StrictMock<MockGamepadDelegate> delegate;
+  EXPECT_CALL(delegate, CanAcceptGamepadEventsForSurface(testing::_))
+      .WillOnce(testing::Return(true));
+
+  InitializeGamepad(&delegate);
+  FocusNonExo();
+
+  blink::WebGamepads axis_moved;
+  axis_moved.length = 1;
+  axis_moved.items[0].connected = true;
+  axis_moved.items[0].timestamp = 1;
+  axis_moved.items[0].axesLength = 2;
+  axis_moved.items[0].axes[0] = 1.0;
+  axis_moved.items[0].axes[1] = 0;
+
+  // Exo is not in focus.. should not cause any delegate calls.
+  SetDataAndWaitForDelegate(axis_moved);
+  testing::Mock::VerifyAndClearExpectations(&delegate);
+
+  EXPECT_CALL(delegate, CanAcceptGamepadEventsForSurface(testing::_))
+      .WillOnce(testing::Return(true));
+
+  // Exo becomes focused, update delegate with full state on next gamepad state.
+  FocusExo();
+  blink::WebGamepads axis_moved_2 = axis_moved;
+  axis_moved_2.items[0].timestamp = 2;
+  axis_moved_2.items[0].axes[1] = 1.0;
+  EXPECT_CALL(delegate, OnStateChange(true)).Times(1);
+  EXPECT_CALL(delegate, OnAxis(0, 1.0)).Times(1);
+  EXPECT_CALL(delegate, OnAxis(1, 1.0)).Times(1);
+  EXPECT_CALL(delegate, OnFrame()).Times(1);
+  SetDataAndWaitForDelegate(axis_moved_2);
+
+  DestroyGamepad(&delegate);
+}
+
+}  // namespace

[reveman, 2016/06/30 18:06:12]

Can you add a simple test for OnButton too?

[denniskempin, 2016/07/01 02:34:35]

Done.

+}  // namespace exo