Add touch events to the protocol, the stub layer, and to the client plugin.

remoting/client/plugin/touch_input_scaler_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 "remoting/client/plugin/touch_input_scaler.h"
+
+#include <cmath>
+
+#include "base/logging.h"

[Wez, 2015/02/05 02:09:06]

If you remove the CHECKs you shouldn't need this include.

[Rintaro Kuroiwa, 2015/02/06 23:35:01]

Done.

+#include "remoting/protocol/protocol_mock_objects.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace remoting {
+
+using ::testing::_;
+using protocol::MockInputStub;
+using protocol::TouchEvent;
+using protocol::TouchEventPoint;
+
+namespace {
+
+const float kDefaultRadius = 30.0f;

[Wez, 2015/02/05 02:09:07]

nit: Suggest moving this down to near the PointInfo type, since that is where
it's used.

[Rintaro Kuroiwa, 2015/02/06 23:35:01]

Done.

+
+// If the rounding error for the coordinates checked in
+// TouchPointCoordinateEqual is within 1 pixel diff, it should be acceptable.

[Wez, 2015/02/05 02:09:07]

nit: Suggest "Require touch point coordinates to be correct to within one
pixel."

Note that the way you're asserting that, if you expect a value of e.g. x=4.9,
then you'll allow x=5.5, which will resolve to a different physical pixel when
rounded by the host - is that desirable?

[Rintaro Kuroiwa, 2015/02/06 23:35:01]

I think off by 1 pixel is acceptable.

[Wez, 2015/02/13 16:51:45]

Acknowledged.

+const float kEpsilon = 1.0f;
+
+MATCHER_P(TouchPointCoordinateEqual,
+          expected_event,
+          "Expect point coordinates equal.") {
+  EXPECT_EQ(expected_event.touch_points().size(), arg.touch_points().size());
+  if (arg.touch_points().size() != expected_event.touch_points().size())
+    return false;
+
+  for (int i = 0; i < expected_event.touch_points().size(); ++i) {
+    const TouchEventPoint& arg_point = arg.touch_points(i);
+    const TouchEventPoint& expected_point = expected_event.touch_points(i);
+    EXPECT_NEAR(expected_point.x(), arg_point.x(), kEpsilon);
+    if (std::abs(expected_point.x() - arg_point.x()) >= kEpsilon)
+      return false;
+
+    EXPECT_NEAR(expected_point.y(), arg_point.y(), kEpsilon);
+    if (std::abs(expected_point.y() - arg_point.y()) >= kEpsilon)
+      return false;
+  }
+  return true;
+}
+
+MATCHER_P(TouchPointRadiiEqual, expected_event, "Expected point radii equal.") {
+  EXPECT_EQ(expected_event.touch_points().size(), arg.touch_points().size());
+  if (arg.touch_points().size() != expected_event.touch_points().size())
+    return false;
+
+  for (int i = 0; i < expected_event.touch_points().size(); ++i) {
+    const TouchEventPoint& arg_point = arg.touch_points(i);
+    const TouchEventPoint& expected_point = expected_event.touch_points(i);
+    EXPECT_NEAR(expected_point.radius_x(), arg_point.radius_x(), kEpsilon);
+    if (std::abs(expected_point.radius_x() - arg_point.radius_x()) >= kEpsilon)
+      return false;
+
+    EXPECT_NEAR(expected_point.radius_y(), arg_point.radius_y(), kEpsilon);
+    if (std::abs(expected_point.radius_y() - arg_point.radius_y()) >= kEpsilon)
+      return false;
+  }
+  return true;
+}
+
+struct PointInfo {
+  PointInfo(float x, float y)
+      : PointInfo(x, y, kDefaultRadius, kDefaultRadius) {}
+  PointInfo(float x, float y, float radius_x, float radius_y)
+      : x(x), y(y), radius_x(radius_x), radius_y(radius_y) {}
+
+  float x;
+  float y;
+  float radius_x;
+  float radius_y;
+};
+
+}  // namespace
+
+class TouchInputScalerTest : public ::testing::Test {
+ protected:
+  TouchInputScalerTest() : touch_input_scaler_(&mock_stub_) {}
+
+  void AddInputCoordinate(const PointInfo& point_info) {
+    point_infos_.push_back(point_info);
+  }
+
+  void AddDefaultTestCoordinate() { point_infos_.push_back({1.0f, 1.0f}); }

[Wez, 2015/02/05 02:09:07]

Earlier you defined kDefaultRadius; consider defining kDefaultPointInfo or
kDefaultXCoord and kDefaultYCoord, say.

[Rintaro Kuroiwa, 2015/02/06 23:35:01]

Done.

+
+  void InjectTestTouchEvent() {
+    CHECK(!point_infos_.empty());

[Wez, 2015/02/05 02:09:06]

In gtest we use ASSERT rather than CHECK.

[Rintaro Kuroiwa, 2015/02/06 23:35:00]

I thought CHECK is more appropriate here because it is a bug in how the test is
written. But the test will fail somewhere else so I'll remove this.

+    TouchEvent e;
+    e.set_event_type(TouchEvent::TOUCH_POINT_MOVE);
+
+    uint32_t id = 1;
+    for (const PointInfo& point_info : point_infos_) {
+      TouchEventPoint* point = e.add_touch_points();
+      point->set_id(id++);
+      point->set_x(point_info.x);
+      point->set_y(point_info.y);
+      point->set_radius_x(point_info.radius_x);
+      point->set_radius_y(point_info.radius_y);
+    }
+
+    touch_input_scaler_.InjectTouchEvent(e);
+  }
+
+  void SetInputDimensions(int width, int height) {
+    touch_input_scaler_.set_input_size(webrtc::DesktopSize(width, height));
+  }
+
+  void SetOutputDimensions(int width, int height) {
+    touch_input_scaler_.set_output_size(webrtc::DesktopSize(width, height));
+  }
+
+  MockInputStub mock_stub_;
+  TouchInputScaler touch_input_scaler_;
+
+ private:
+  std::vector<PointInfo> point_infos_;
+};

[Wez, 2015/02/05 02:09:07]

DISALLOW_COPY_AND_ASSIGN()

[Rintaro Kuroiwa, 2015/02/06 23:35:01]

Done.

+
+// TouchInputFilter require both input and output dimensions.
+// These test verify that no events are forwarded to the next InputStub if
+// either dimensions are not set.

[Wez, 2015/02/05 02:09:07]

Suggest explicitly setting the dimensions to zero, since that's what the test
name suggests, or changing the test name from zero to unset, or ZeroOrUnset, for
example.

[Rintaro Kuroiwa, 2015/02/06 23:35:01]

Done.

+TEST_F(TouchInputScalerTest, BothDimensionsZero) {
+  AddDefaultTestCoordinate();
+  EXPECT_CALL(mock_stub_, InjectTouchEvent(_)).Times(0);
+  InjectTestTouchEvent();
+}
+
+TEST_F(TouchInputScalerTest, SetOnlyInputDimensions) {
+  SetInputDimensions(50, 60);
+  AddDefaultTestCoordinate();
+  EXPECT_CALL(mock_stub_, InjectTouchEvent(_)).Times(0);
+  InjectTestTouchEvent();
+}
+
+TEST_F(TouchInputScalerTest, SetOnlyOutputDimensions) {
+  SetOutputDimensions(50, 60);
+  AddDefaultTestCoordinate();
+  EXPECT_CALL(mock_stub_, InjectTouchEvent(_)).Times(0);
+  InjectTestTouchEvent();
+}
+
+// The x,y coordinate fall in the desktop size.
+TEST_F(TouchInputScalerTest, NoClampingNoScaling) {
+  SetInputDimensions(50, 60);
+  SetOutputDimensions(50, 60);
+
+  AddInputCoordinate({10.0f, 15.0f});
+  TouchEvent expected_out;
+  TouchEventPoint* point = expected_out.add_touch_points();
+  point->set_x(10.0f);
+  point->set_y(15.0f);
+
+  EXPECT_CALL(mock_stub_,
+              InjectTouchEvent(TouchPointCoordinateEqual(expected_out)));
+  InjectTestTouchEvent();
+}
+
+// Make sure clamping works.

[Wez, 2015/02/05 02:09:07]

nit: Suggest rewording this to be similar to the wording above, e.g. "the
coordinates fall outside the desktop area".

[Rintaro Kuroiwa, 2015/02/06 23:35:01]

Done.

+TEST_F(TouchInputScalerTest, ClampingNoScaling) {
+  SetInputDimensions(50, 60);
+  SetOutputDimensions(50, 60);
+
+  // Note that this could happen if touch started in the chromoting window but
+  // the finger moved off the windows.
+  AddInputCoordinate({-1.0f, 1.0f});
+  TouchEvent expected_out;
+  TouchEventPoint* point = expected_out.add_touch_points();
+  point->set_x(0.0f);
+  point->set_y(1.0f);

[Wez, 2015/02/05 02:09:06]

Does it matter that the radius of the event may fall partly outside the desktop?
(I'd guess not, but...)

[Rintaro Kuroiwa, 2015/02/06 23:35:01]

This test does not due to the matcher that is used.

I added a TODO in TouchInputScaler. I know that Windows touch injection API does
not like any touch point area falling outside the screen. Honestly I'm not sure
about how other platforms work.

But obviously if the ellipse encloses the entire desktop, then that's  a case
where we can make the size (radii) smaller.

+
+  EXPECT_CALL(mock_stub_,
+              InjectTouchEvent(TouchPointCoordinateEqual(expected_out)));
+  InjectTestTouchEvent();
+}
+
+TEST_F(TouchInputScalerTest, ClampingMultiplePointsNoScaling) {

[Wez, 2015/02/05 02:09:07]

Suggest including an un-clamped point in this test, too. :)

[Rintaro Kuroiwa, 2015/02/06 23:35:01]

Done.

+  SetInputDimensions(50, 60);
+  SetOutputDimensions(50, 60);
+
+  AddInputCoordinate({-1.0f, 1.0f});
+  TouchEvent expected_out;
+  TouchEventPoint* point = expected_out.add_touch_points();
+  point->set_x(0.0f);
+  point->set_y(1.0f);
+
+  AddInputCoordinate({-2.0f, 1.0f});
+  point = expected_out.add_touch_points();
+  point->set_x(0.0f);
+  point->set_y(1.0f);
+
+  AddInputCoordinate({-3.0f, -1.0f});
+  point = expected_out.add_touch_points();
+  point->set_x(0.0f);
+  point->set_y(0.0f);
+
+  AddInputCoordinate({100.0f, 100.0f});
+  point = expected_out.add_touch_points();
+  // 1 less than max width and height.
+  point->set_x(49.0f);
+  point->set_y(59.0f);
+
+  EXPECT_CALL(mock_stub_,
+              InjectTouchEvent(TouchPointCoordinateEqual(expected_out)));
+  InjectTestTouchEvent();
+}
+
+// Verify up scaling works. All coordinates should fall inside the output

[Wez, 2015/02/05 02:09:07]

nit: up-scaling

[Rintaro Kuroiwa, 2015/02/06 23:35:01]

Done.

+// dimensions, after scaling, i.e. no clamping testing.

[Wez, 2015/02/05 02:09:06]

not sure what "no clamping testing" means

[Rintaro Kuroiwa, 2015/02/06 23:35:01]

Meaning this should only test scaling and should not test the combination of
scaling and clamping.
Reworded.

+TEST_F(TouchInputScalerTest, UpScalingNoClamp) {
+  const int kInputDimension = 20;
+  const int kUpScalingFactor = 2;
+  SetInputDimensions(kInputDimension, kInputDimension);
+  SetOutputDimensions(kInputDimension * kUpScalingFactor,
+                      kInputDimension * kUpScalingFactor);
+
+  AddInputCoordinate({1.0f, 1.0f});
+  TouchEvent expected_out;
+  TouchEventPoint* point = expected_out.add_touch_points();
+  point->set_x(1.0f * kUpScalingFactor);
+  point->set_y(1.0f * kUpScalingFactor);

[Wez, 2015/02/05 02:09:07]

You're calculating the expected output from the same values as the input and
scale factor, which is bad ju-ju - suggest putting the expected values here
explicitly, and getting rid of the kUpScalingFactor constant

[Rintaro Kuroiwa, 2015/02/06 23:35:01]

Done.

+
+  AddInputCoordinate({5.0f, 3.0f});
+  point = expected_out.add_touch_points();
+  point->set_x(5.0f * kUpScalingFactor);
+  point->set_y(3.0f * kUpScalingFactor);
+  EXPECT_CALL(mock_stub_,
+              InjectTouchEvent(TouchPointCoordinateEqual(expected_out)));
+  InjectTestTouchEvent();
+}
+
+// Verify up scaling works.

[Wez, 2015/02/05 02:09:06]

You mean works, with clamping?

[Rintaro Kuroiwa, 2015/02/06 23:35:01]

Yes. Done.

+TEST_F(TouchInputScalerTest, UpScaling) {
+  const int kInputDimension = 20;
+  const int kUpScalingFactor = 2;
+  SetInputDimensions(kInputDimension, kInputDimension);
+  SetOutputDimensions(kInputDimension * kUpScalingFactor,
+                      kInputDimension * kUpScalingFactor);
+
+  AddInputCoordinate({25.0f, 25.0f});
+  TouchEvent expected_out;
+  TouchEventPoint* point = expected_out.add_touch_points();
+  point->set_x(39.0f);
+  point->set_y(39.0f);
+
+  EXPECT_CALL(mock_stub_,
+              InjectTouchEvent(TouchPointCoordinateEqual(expected_out)));
+  InjectTestTouchEvent();
+}
+
+// Verify down scaling works. All coordinates should fall inside the output
+// dimensions, after scaling, i.e. no clamping testing.
+TEST_F(TouchInputScalerTest, DownScalingNoClamp) {

[Wez, 2015/02/05 02:09:07]

NoClamping

[Rintaro Kuroiwa, 2015/02/06 23:35:01]

Done. Renamed to UpScalingNoClamping as well.

+  const int kOutputDimension = 20;
+  const int kDownScalingFactor = 2;
+  SetInputDimensions(kOutputDimension * kDownScalingFactor,
+                     kOutputDimension * kDownScalingFactor);
+  SetOutputDimensions(kOutputDimension, kOutputDimension);
+
+  AddInputCoordinate({2.0f, 2.0f});
+  TouchEvent expected_out;
+  TouchEventPoint* point = expected_out.add_touch_points();
+  point->set_x(2.0f / kDownScalingFactor);
+  point->set_y(2.0f / kDownScalingFactor);
+
+  AddInputCoordinate({6.0f, 3.0f});
+  point = expected_out.add_touch_points();
+  point->set_x(6.0f / kDownScalingFactor);
+  point->set_y(3.0f / kDownScalingFactor);
+  EXPECT_CALL(mock_stub_,
+              InjectTouchEvent(TouchPointCoordinateEqual(expected_out)));
+  InjectTestTouchEvent();
+}
+
+// Verify down scaling works.

[Wez, 2015/02/05 02:09:07]

As above

[Rintaro Kuroiwa, 2015/02/06 23:35:01]

Done.

+TEST_F(TouchInputScalerTest, DownScaling) {
+  const int kOutputDimension = 20;
+  const int kDownScalingFactor = 2;
+  SetInputDimensions(kOutputDimension * kDownScalingFactor,
+                     kOutputDimension * kDownScalingFactor);
+  SetOutputDimensions(kOutputDimension, kOutputDimension);
+
+  AddInputCoordinate({-20.0f, 10.0f});
+  TouchEvent expected_out;
+  TouchEventPoint* point = expected_out.add_touch_points();
+  point->set_x(0.0f);
+  point->set_y(10.0f / kDownScalingFactor);
+
+  AddInputCoordinate({10.0f, -20.0f});
+  point = expected_out.add_touch_points();
+  point->set_x(10.0f / kDownScalingFactor);
+  point->set_y(0.0f);
+
+  AddInputCoordinate({6.0f, 80.0f});
+  point = expected_out.add_touch_points();
+  point->set_x(6.0f / kDownScalingFactor);
+  point->set_y(kOutputDimension - 1);
+
+  AddInputCoordinate({80.0f, 6.0f});
+  point = expected_out.add_touch_points();
+  point->set_x(kOutputDimension - 1);
+  point->set_y(6.0f / kDownScalingFactor);
+
+  EXPECT_CALL(mock_stub_,
+              InjectTouchEvent(TouchPointCoordinateEqual(expected_out)));
+  InjectTestTouchEvent();
+}
+
+// Scaling up by factor of 2.

[Wez, 2015/02/05 02:09:06]

This comment doesn't seem to relate to the test?

Shouldn't you test this as part of the up-scaling test earlier on?

[Rintaro Kuroiwa, 2015/02/06 23:35:01]

Yes, the comment is a test detail, not for explaining the test. Changed.

This uses a different matcher that only tests the radii scaling, I wanted to do
test this independently from x,y scaling.

I'll add tests that do both.

+TEST_F(TouchInputScalerTest, UpScaleRadii) {
+  SetInputDimensions(20, 20);
+  SetOutputDimensions(40, 40);
+
+  AddInputCoordinate({0.0f, 0.0f, 1.0f, 2.0f});
+  TouchEvent expected_out;
+  TouchEventPoint* point = expected_out.add_touch_points();
+  point->set_radius_x(2.0f);
+  point->set_radius_y(4.0f);
+
+  EXPECT_CALL(mock_stub_, InjectTouchEvent(TouchPointRadiiEqual(expected_out)));
+  InjectTestTouchEvent();
+}
+
+// Scaling down by factor of 2.

[Wez, 2015/02/05 02:09:07]

As above.

[Rintaro Kuroiwa, 2015/02/06 23:35:01]

same.

+TEST_F(TouchInputScalerTest, DownScaleRadii) {
+  SetInputDimensions(20, 20);
+  SetOutputDimensions(10, 10);
+
+  AddInputCoordinate({0.0f, 0.0f, 5.0f, 4.0f});
+  TouchEvent expected_out;
+  TouchEventPoint* point = expected_out.add_touch_points();
+  point->set_radius_x(2.5f);
+  point->set_radius_y(2.0f);
+
+  EXPECT_CALL(mock_stub_, InjectTouchEvent(TouchPointRadiiEqual(expected_out)));
+  InjectTestTouchEvent();
+}
+
+}  // namespace remoting