Palette tool laser prototype.

ash/common/system/chromeos/palette/tools/laser_pointer_unittest.cc

+// Copyright (c) 2016 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/common/system/chromeos/palette/palette_tool_manager.h"
+#include "ash/common/system/chromeos/palette/tools/laser_pointer_mode.h"
+#include "ash/common/system/chromeos/palette/tools/laser_pointer_points.h"
+#include "ash/common/wm_shell.h"
+#include "ash/test/ash_test_base.h"
+#include "ui/events/test/event_generator.h"
+
+namespace ash {
+
+class LaserPointerPointsTestApi : public LaserPointerPoints {
+ public:
+  LaserPointerPointsTestApi(base::TimeDelta life_time)
+      : LaserPointerPoints(life_time) {}
+  ~LaserPointerPointsTestApi() {}
+
+  void AddTimedPoint(const base::Time& time, const gfx::Point& location) {

[jdufault, 2016/08/19 01:02:09]

AddPointAtTime(...)

[sammiequon, 2016/08/19 20:28:47]

Done.

+    LaserPointerPoints::LaserPoint new_point;
+    new_point.creation_time = time;
+    new_point.location = location;
+    points_.push_back(new_point);
+    ClearOldPoints();
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(LaserPointerPointsTestApi);
+};
+
+class LaserPointerModeTestApi {
+ public:
+  LaserPointerModeTestApi(std::unique_ptr<LaserPointerMode> instance)
+      : instance_(std::move(instance)) {}
+  ~LaserPointerModeTestApi() {}
+
+  void OnEnable() { instance_->OnEnable(); }
+  void OnDisable() { instance_->OnDisable(); }
+
+  const LaserPointerPoints& laser_points() {
+    return instance_->laser_pointer_view_->laser_points_;
+  }
+
+  gfx::Point current_mouse_location() {
+    return instance_->current_mouse_location_;
+  }
+
+ private:
+  std::unique_ptr<LaserPointerMode> instance_;
+
+  DISALLOW_COPY_AND_ASSIGN(LaserPointerModeTestApi);
+};
+
+namespace {

[jdufault, 2016/08/19 01:02:09]

Move anonymous namespace contents to the top of the file.

[sammiequon, 2016/08/19 20:28:47]

Done.

+const int kTestPointsLifetime = 5;
+
+class LaserPointerModeTest : public test::AshTestBase {
+ public:
+  LaserPointerModeTest()
+      : laser_pointer_points_(
+            base::TimeDelta::FromSeconds(kTestPointsLifetime)),
+        laser_pointer_points_delete_(
+            base::TimeDelta::FromSeconds(kTestPointsLifetime)) {}
+  ~LaserPointerModeTest() override {}
+
+  void SetUp() override {
+    AshTestBase::SetUp();
+    laser_pointer_mode_.reset(new LaserPointerModeTestApi(
+        base::WrapUnique<LaserPointerMode>(new LaserPointerMode(nullptr))));
+  }
+
+  void TearDown() override {
+    laser_pointer_mode_.reset();
+    AshTestBase::TearDown();
+  }
+
+ protected:
+  std::unique_ptr<LaserPointerModeTestApi> laser_pointer_mode_;
+  LaserPointerPoints laser_pointer_points_;

[jdufault, 2016/08/19 01:02:09]

What about just points_?

[sammiequon, 2016/08/19 20:28:47]

Done.

+  LaserPointerPointsTestApi laser_pointer_points_delete_;

[jdufault, 2016/08/19 01:02:08]

points_test_api_?

[jdufault, 2016/08/19 01:02:09]

If this test api instance is separate from laser_pointer_mode_ you should have
separate fixtures, one for LaserPointerMode tests and another for
LaserPointerPoints tests.

[sammiequon, 2016/08/19 20:28:46]

Done.

[sammiequon, 2016/08/19 20:28:47]

Done.

+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(LaserPointerModeTest);
+};
+}  // namespace
+
+// Tests that the laser pointers internal collection handles receiving points
+// and that the functions are returning the expected output.
+TEST_F(LaserPointerModeTest, LaserPointerInternalCollection) {
+  gfx::Point point1(1, 1);
+  gfx::Point point2(2, 2);
+  gfx::Point point3(3, 9);
+  gfx::Point point4(30, 0);
+  gfx::Point point5(0, 40);
+  laser_pointer_points_.AddPoint(point1);
+  laser_pointer_points_.AddPoint(point2);
+  laser_pointer_points_.AddPoint(point3);
+  laser_pointer_points_.AddPoint(point4);
+
+  // Test laser pointer points collection and helper functions.
+  EXPECT_EQ(4, laser_pointer_points_.GetNumberOfPoints());
+  EXPECT_FALSE(laser_pointer_points_.IsEmpty());
+  EXPECT_EQ(gfx::Rect(1, 0, 29, 9), laser_pointer_points_.GetBoundingBox());

[jdufault, 2016/08/19 01:02:09]

I think having renaming some of the points to top_left and bottom_right (or
something similar), and then adding some unnamed points in between them (with a
comment saying that the added points are between the top left and bottom right)
will make this a bit more readable. You can then also check the bbox using named
constants, ie,

EXPECT_EQ(gfx::Rect(top_left.x, top_left.y, bottom_right.x - top_left.x,
bottom_right.y - top_left.y), ...);

I think that's much easier to read and understand instead of just the magic
numbers.

[sammiequon, 2016/08/19 20:28:46]

Done.

+  EXPECT_EQ(point1, laser_pointer_points_.GetOldest().location);
+  EXPECT_EQ(point4, laser_pointer_points_.GetMostRecent().location);
+
+  laser_pointer_points_.AddPoint(point5);

[jdufault, 2016/08/19 01:02:09]

Declare point here, ie,

gfx::Point expanded_bottom_left (or the right name) = ...

And do the EXPECT_EQ check using that named constant.

[sammiequon, 2016/08/19 20:28:46]

Done.

+  EXPECT_EQ(5, laser_pointer_points_.GetNumberOfPoints());
+  EXPECT_EQ(gfx::Rect(0, 0, 30, 40), laser_pointer_points_.GetBoundingBox());
+
+  laser_pointer_points_.Clear();
+  EXPECT_TRUE(laser_pointer_points_.IsEmpty());
+}
+
+// Test the laser pointer points collection removes old points as expected.
+TEST_F(LaserPointerModeTest, LaserPointerInternalCollectionDeletion) {
+  gfx::Point test_point(0, 0);
+  base::Time start_time = base::Time::FromJsTime(0);
+  base::Time three_second_time = base::Time::FromJsTime(3000);

[jdufault, 2016/08/19 01:02:08]

Use FromSeconds

[sammiequon, 2016/08/19 20:28:47]

As per offline conv this is fine.

+  base::Time six_second_time = base::Time::FromJsTime(6000);
+  base::Time six_half_second_time = base::Time::FromJsTime(6500);

[jdufault, 2016/08/19 01:02:09]

I don't think these constants aid in readability.

[sammiequon, 2016/08/19 20:28:47]

Done.

+  base::Time seven_second_time = base::Time::FromJsTime(7000);
+  base::Time seven_half_second_time = base::Time::FromJsTime(7500);
+  base::Time twenty_second_time = base::Time::FromJsTime(20000);
+
+  // When a new point is added points older than 5 seconds (kTestPointsLifetime)
+  // // ago should be removed.

[jdufault, 2016/08/19 01:02:08]

What about something like this for the comment?

  When a point older than kTestPointsLifetime is added, it should 
  get removed.

[sammiequon, 2016/08/19 20:28:47]

Done.

+  laser_pointer_points_delete_.AddTimedPoint(start_time, test_point);
+  EXPECT_EQ(laser_pointer_points_delete_.GetNumberOfPoints(), 1);
+  laser_pointer_points_delete_.AddTimedPoint(three_second_time, test_point);
+  EXPECT_EQ(laser_pointer_points_delete_.GetNumberOfPoints(), 2);
+  laser_pointer_points_delete_.AddTimedPoint(six_second_time, test_point);
+  EXPECT_EQ(laser_pointer_points_delete_.GetNumberOfPoints(), 2);
+
+  laser_pointer_points_delete_.AddTimedPoint(six_half_second_time, test_point);
+  laser_pointer_points_delete_.AddTimedPoint(seven_second_time, test_point);
+  laser_pointer_points_delete_.AddTimedPoint(seven_half_second_time,
+                                             test_point);
+  EXPECT_EQ(laser_pointer_points_delete_.GetNumberOfPoints(), 5);
+  laser_pointer_points_delete_.AddTimedPoint(twenty_second_time, test_point);
+  EXPECT_EQ(laser_pointer_points_delete_.GetNumberOfPoints(), 1);

[jdufault, 2016/08/19 01:02:08]

This seems strange to me. Why does adding a 20 second old point clear all of the
other points? Is the test adding points to the front of the queue so it's
actually in reverse order?

I think a better approach here is instead of adding a point at a specific time,
have the test API iterate over all of the existing points and move their time
backwards. This mirrors the real environment a bit more closely.

So the test would look like this

  AddPoint()
  EXPECT_EQ(1, PointCount())
  MoveForwardInTime(1s)
  EXPECT_EQ(1, PointCount())
  MoveForwardInTime(20s)
  EXPECT_EQ(0, PointCount())

I think this easier to read, too.

[sammiequon, 2016/08/19 20:28:47]

Done.

+}
+
+// Test to ensure the class responsible for drawing the laser pointer receives
+// points from mouse movements as expected.
+TEST_F(LaserPointerModeTest, LaserPointerRenderer) {
+  // TODO(sammiequon): Update these tests once event generator with stylus
+  // events cl lands.
+  GetEventGenerator().MoveMouseToInHost(gfx::Point(10, 40));
+  EXPECT_EQ(0, laser_pointer_mode_->laser_points().GetNumberOfPoints());
+  laser_pointer_mode_->OnEnable();
+
+  GetEventGenerator().MoveMouseToInHost(gfx::Point(25, 66));
+  GetEventGenerator().MoveMouseToInHost(gfx::Point(91, 38));
+  GetEventGenerator().MoveMouseToInHost(gfx::Point(34, 58));
+  GetEventGenerator().MoveMouseToInHost(gfx::Point(19, 71));
+
+  EXPECT_EQ(5, laser_pointer_mode_->laser_points().GetNumberOfPoints());
+
+  laser_pointer_mode_->OnDisable();
+  GetEventGenerator().MoveMouseToInHost(gfx::Point(34, 58));
+  GetEventGenerator().MoveMouseToInHost(gfx::Point(19, 71));
+  EXPECT_EQ(0, laser_pointer_mode_->laser_points().GetNumberOfPoints());
+
+  laser_pointer_mode_->OnEnable();
+  EXPECT_EQ(1, laser_pointer_mode_->laser_points().GetNumberOfPoints());
+  EXPECT_EQ(gfx::Point(19, 71),

[jdufault, 2016/08/19 01:02:09]

GetEventGenerator().current_location()

[sammiequon, 2016/08/19 20:28:46]

Done.

+            laser_pointer_mode_->laser_points().GetMostRecent().location);
+}
+}  // namespace ash