Palette tool laser prototype.

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

+// Copyright 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/tools/laser_pointer_mode.h"
+#include "ash/common/system/chromeos/palette/tools/laser_pointer_view.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 {

[jdufault, 2016/08/22 18:37:36]

Can you make this take the LaserPointerPoints instance as parameter, similar to
how LaserPointerModeTestApi works?

Also, instead of using friend class for inheritance prefer to use protected. It
communicates intent more clearly.

[sammiequon, 2016/08/23 00:20:33]

Done.

+ public:
+  LaserPointerPointsTestApi(base::TimeDelta life_time)
+      : LaserPointerPoints(life_time) {}
+  ~LaserPointerPointsTestApi() {}
+
+  void MoveForwardInTime(const base::TimeDelta& delta,
+                         const base::Time& time,
+                         const gfx::Point& location) {
+    for (LaserPointerPoints::LaserPoint& point : points_)
+      point.creation_time -= delta;
+    LaserPointerPoints::LaserPoint new_point;

[jdufault, 2016/08/22 18:37:36]

Instead of building a new_point instance just call AddPoint(location). This
means you can also remove the |time| parameter.

This will also take care of calling ClearOldPoints().

[sammiequon, 2016/08/23 00:20:33]

I did it this way since AddPoint adds it at Time::Now(), so if the computer is
running the test is super slow it may mess it up.

+    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/22 18:37:36]

The anonymous namespace should be at the top. Pull the *TestApi classes into
separate files if you need to.

[sammiequon, 2016/08/23 00:20:33]

Done.

+
+const int kTestPointsLifetime = 5;

[jdufault, 2016/08/22 18:37:36]

Can you specify the unit here?

[sammiequon, 2016/08/23 00:20:33]

Done.

+
+class LaserPointerPointsTest : public test::AshTestBase {
+ public:
+  LaserPointerPointsTest() {}
+  ~LaserPointerPointsTest() override {}
+
+  void SetUp() override {
+    AshTestBase::SetUp();
+    points_.reset(new LaserPointerPoints(
+        base::TimeDelta::FromSeconds(kTestPointsLifetime)));
+    points_test_api_.reset(new LaserPointerPointsTestApi(
+        base::TimeDelta::FromSeconds(kTestPointsLifetime)));
+  }
+
+  void TearDown() override { AshTestBase::TearDown(); }
+
+ protected:
+  std::unique_ptr<LaserPointerPoints> points_;
+  std::unique_ptr<LaserPointerPointsTestApi> points_test_api_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(LaserPointerPointsTest);
+};
+
+class LaserPointerModeTest : public test::AshTestBase {
+ public:
+  LaserPointerModeTest() {}
+  ~LaserPointerModeTest() override {}
+
+  void SetUp() override {
+    AshTestBase::SetUp();
+    mode_test_api_.reset(new LaserPointerModeTestApi(
+        base::WrapUnique<LaserPointerMode>(new LaserPointerMode(nullptr))));
+  }
+
+  void TearDown() override {
+    mode_test_api_.reset();
+    AshTestBase::TearDown();
+  }
+
+ protected:
+  std::unique_ptr<LaserPointerModeTestApi> mode_test_api_;
+
+ 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(LaserPointerPointsTest, LaserPointerInternalCollection) {
+  gfx::Point left(1, 1);
+  gfx::Point bottom(3, 9);
+  gfx::Point top_right(30, 0);
+  points_->AddPoint(left);
+  points_->AddPoint(gfx::Point(2, 2));  // Should not expand bounding box.
+  points_->AddPoint(bottom);
+  points_->AddPoint(top_right);

[jdufault, 2016/08/22 18:37:36]

I think you should also verify the bounding box when the points_ object  has 0
and 1 points.

[sammiequon, 2016/08/23 00:20:33]

Done.

+
+  // Test laser pointer points collection and helper functions.

[jdufault, 2016/08/22 18:37:36]

Replace this comment with a series of comments that give a very high-level
overview of what each section of code does. I've written an example one below
(line 131)

[sammiequon, 2016/08/23 00:20:33]

Done.

+  EXPECT_EQ(4, points_->GetNumberOfPoints());
+  EXPECT_FALSE(points_->IsEmpty());
+  EXPECT_EQ(gfx::Rect(left.x(), top_right.y(), top_right.x() - left.x(),
+                      bottom.y() - top_right.y()),
+            points_->GetBoundingBox());
+  EXPECT_EQ(left, points_->GetOldest().location);

[jdufault, 2016/08/22 18:37:36]

Instead of testing GetOldest/GetNewest here, I'd write a separate test that
verifies the oldest point changes as points are removed.

[sammiequon, 2016/08/23 00:20:33]

The time stuff is in the next test.

+  EXPECT_EQ(top_right, points_->GetNewest().location);
+
+  gfx::Point new_left_bottom(0, 40);

[jdufault, 2016/08/22 18:37:36]

// Add a new point which will expand the bounding box.

[sammiequon, 2016/08/23 00:20:33]

Done.

+  points_->AddPoint(new_left_bottom);
+  EXPECT_EQ(5, points_->GetNumberOfPoints());
+  EXPECT_EQ(gfx::Rect(new_left_bottom.x(), top_right.y(),
+                      top_right.x() - new_left_bottom.x(),
+                      new_left_bottom.y() - top_right.y()),
+            points_->GetBoundingBox());
+
+  points_->Clear();
+  EXPECT_TRUE(points_->IsEmpty());
+}
+
+// Test the laser pointer points collection to verify that old points are
+// removed.
+TEST_F(LaserPointerPointsTest, LaserPointerInternalCollectionDeletion) {
+  gfx::Point test_point(0, 0);
+  base::Time start_time = base::Time::Now();
+
+  // When a point older than kTestPointsLifetime is added, it should get
+  // removed.

[jdufault, 2016/08/22 18:37:36]

Can you add comments at the interesting points in this test? Right now it is
hard to follow.

ie, for a block of code, you're trying to simulate some behavior. What is the
behavior?

[sammiequon, 2016/08/23 00:20:33]

Done.

+  points_test_api_->MoveForwardInTime(base::TimeDelta::FromSeconds(1),
+                                      start_time, test_point);
+  EXPECT_EQ(points_test_api_->GetNumberOfPoints(), 1);
+  points_test_api_->MoveForwardInTime(base::TimeDelta::FromSeconds(1),
+                                      start_time, test_point);
+  EXPECT_EQ(points_test_api_->GetNumberOfPoints(), 2);
+  points_test_api_->MoveForwardInTime(base::TimeDelta::FromSeconds(10),
+                                      start_time, test_point);
+  EXPECT_EQ(points_test_api_->GetNumberOfPoints(), 1);
+  points_test_api_->MoveForwardInTime(base::TimeDelta::FromSeconds(1),
+                                      start_time, test_point);
+  points_test_api_->MoveForwardInTime(base::TimeDelta::FromSeconds(1),
+                                      start_time, test_point);
+  points_test_api_->MoveForwardInTime(base::TimeDelta::FromSeconds(1),
+                                      start_time, test_point);
+  EXPECT_EQ(points_test_api_->GetNumberOfPoints(), 4);
+  points_test_api_->MoveForwardInTime(base::TimeDelta::FromSeconds(3),
+                                      start_time, test_point);
+  EXPECT_EQ(points_test_api_->GetNumberOfPoints(), 3);
+}
+
+// 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, mode_test_api_->laser_points().GetNumberOfPoints());
+  mode_test_api_->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, mode_test_api_->laser_points().GetNumberOfPoints());
+
+  mode_test_api_->OnDisable();
+  GetEventGenerator().MoveMouseToInHost(gfx::Point(34, 58));
+  GetEventGenerator().MoveMouseToInHost(gfx::Point(19, 71));
+  EXPECT_EQ(0, mode_test_api_->laser_points().GetNumberOfPoints());

[jdufault, 2016/08/22 18:37:36]

I'd add another EXPECT_EQ(0, ...) right after the OnDisable call as well.


// Verify disabling the mode will clear any active points.
mode_test_api_->OnDisable()
EXPECT_EQ(0, count())

// Verify that the laser pointer ignores any input while disabled.
GetEventGenerator()...
GetEventGenerator()...
EXPECT_EQ(0, count())

// Verify that enabling the laser pointer immediately adds a point at the
current mouse location.
...

[sammiequon, 2016/08/23 00:20:33]

Done.

+
+  mode_test_api_->OnEnable();
+  EXPECT_EQ(1, mode_test_api_->laser_points().GetNumberOfPoints());
+  EXPECT_EQ(GetEventGenerator().current_location(),
+            mode_test_api_->laser_points().GetNewest().location);
+}
+}  // namespace ash