Palette tool laser prototype.

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

+// Copyright (c) 2016 The Chromium Authors. All rights reserved.

[jdufault, 2016/08/19 21:05:56]

No (c)

[sammiequon, 2016/08/22 18:07:20]

Done.

+// 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_unittest.h"
+
+#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 {
+namespace {
+
+const int kTestPointsLifetime = 5;
+const double kTestTimeMs = 100000.0;

[jdufault, 2016/08/19 21:05:56]

Add comment for how long this is in a more readable unit, ie,

 const double kTestTimeMs = 100000.0; // 100 seconds

[sammiequon, 2016/08/22 18:07:20]

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 {
+    points_.reset();

[jdufault, 2016/08/19 21:05:56]

reset() should be called for you by the dtor

[sammiequon, 2016/08/22 18:07:19]

Done.

+    points_test_api_.reset();
+    AshTestBase::TearDown();

[jdufault, 2016/08/19 21:05:56]

TearDown should be the first fn call.

[sammiequon, 2016/08/22 18:07:20]

Done.

+  }
+
+ 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();
+    laser_pointer_mode_.reset(new LaserPointerModeTestApi(
+        base::WrapUnique<LaserPointerMode>(new LaserPointerMode(nullptr))));
+    points_.reset(new LaserPointerPoints(
+        base::TimeDelta::FromSeconds(kTestPointsLifetime)));
+    points_test_api_.reset(new LaserPointerPointsTestApi(
+        base::TimeDelta::FromSeconds(kTestPointsLifetime)));
+  }
+
+  void TearDown() override {
+    points_.reset();

[jdufault, 2016/08/19 21:05:56]

I don't think you need to reset these instances; the entire class should get
destroyed so the dtor will do it for you.

[sammiequon, 2016/08/22 18:07:20]

Laser pointer mode dtor removes the pointer watcher and this has to happen
before TearDown otherwise tear down will complain there is still an pointer
watcher observer when it expects none.

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

Please add a comment describing this requirement.

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

Done.

+    points_test_api_.reset();
+    laser_pointer_mode_.reset();
+    AshTestBase::TearDown();

[jdufault, 2016/08/19 21:05:56]

AshTestBase::TearDown should be the first function call

[sammiequon, 2016/08/22 18:07:19]

See above.

+  }
+
+ protected:
+  std::unique_ptr<LaserPointerModeTestApi> laser_pointer_mode_;

[jdufault, 2016/08/19 21:05:56]

mode_test_api_?

[sammiequon, 2016/08/22 18:07:20]

Done.

+  std::unique_ptr<LaserPointerPoints> points_;
+  std::unique_ptr<LaserPointerPointsTestApi> points_test_api_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(LaserPointerModeTest);
+};
+}  // namespace
+
+LaserPointerPointsTestApi::LaserPointerPointsTestApi(base::TimeDelta life_time)
+    : LaserPointerPoints(life_time) {}
+LaserPointerPointsTestApi::~LaserPointerPointsTestApi() {}
+
+void LaserPointerPointsTestApi::AddPointAtTime(const base::Time& time,
+                                               const gfx::Point& location) {
+  LaserPointerPoints::LaserPoint new_point;
+  new_point.creation_time = time;
+  new_point.location = location;
+  points_.push_back(new_point);
+  ClearOldPoints();
+}
+
+void LaserPointerPointsTestApi::MoveForwardInTime(int seconds) {

[jdufault, 2016/08/19 21:05:56]

What about eliminating AddPointAtTime and making this function assume its
functionality, so have it also take a gfx::Point location?

So the test API function will just move the existing points back in time by some
given amount (|seconds|), and then will call the normal add point API that will
add a new point at the given location (with the time set to base::Now()).

[jdufault, 2016/08/19 21:05:56]

TimeDelta delta

[sammiequon, 2016/08/22 18:07:20]

Done.

[sammiequon, 2016/08/22 18:07:20]

Done.

+  for (LaserPointerPoints::LaserPoint& point : points_)
+    point.creation_time -= base::TimeDelta::FromSeconds(seconds);
+  AddPointAtTime(base::Time::FromJsTime(kTestTimeMs), gfx::Point());
+}
+
+LaserPointerModeTestApi::LaserPointerModeTestApi(

[jdufault, 2016/08/19 21:05:56]

Where is the declaration for this class?

If it is a separate h file, then this should also be in a separate cc file.

[sammiequon, 2016/08/22 18:07:20]

Done.

+    std::unique_ptr<LaserPointerMode> instance)
+    : instance_(std::move(instance)) {}
+
+LaserPointerModeTestApi::~LaserPointerModeTestApi() {}
+
+void LaserPointerModeTestApi::OnEnable() {
+  instance_->OnEnable();
+}
+void LaserPointerModeTestApi::OnDisable() {
+  instance_->OnDisable();
+}
+
+const LaserPointerPoints& LaserPointerModeTestApi::laser_points() {
+  return instance_->laser_pointer_view_->laser_points_;
+}
+
+gfx::Point LaserPointerModeTestApi::current_mouse_location() {
+  return instance_->current_mouse_location_;
+}
+
+// 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 point2(2, 2);
+  gfx::Point bottom(3, 9);
+  gfx::Point top_right(30, 0);
+  points_->AddPoint(left);
+  points_->AddPoint(point2);

[jdufault, 2016/08/19 21:05:56]

Inline point2 since we don't need to name it. Add a comment saying it does not
expand the bounding box.

[sammiequon, 2016/08/22 18:07:20]

Done.

+  points_->AddPoint(bottom);
+  points_->AddPoint(top_right);
+
+  // Test laser pointer points collection and helper functions.
+  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);
+  EXPECT_EQ(top_right, points_->GetNewest().location);
+
+  gfx::Point new_left_bottom(0, 40);
+  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 removes old points as expected.

[jdufault, 2016/08/19 21:05:56]

What about a comment like:

  // Verify that old points are removed.

[sammiequon, 2016/08/22 18:07:20]

Done.

+TEST_F(LaserPointerPointsTest, LaserPointerInternalCollectionDeletion) {
+  gfx::Point test_point(0, 0);
+  base::Time start_time = base::Time::FromJsTime(kTestTimeMs);
+
+  // When a point older than kTestPointsLifetime is added, it should get
+  // removed.
+  points_test_api_->AddPointAtTime(start_time, test_point);
+  EXPECT_EQ(points_test_api_->GetNumberOfPoints(), 1);
+  points_test_api_->MoveForwardInTime(1);
+  EXPECT_EQ(points_test_api_->GetNumberOfPoints(), 2);
+  points_test_api_->MoveForwardInTime(10);
+  EXPECT_EQ(points_test_api_->GetNumberOfPoints(), 1);
+  points_test_api_->MoveForwardInTime(1);
+  points_test_api_->MoveForwardInTime(1);
+  points_test_api_->MoveForwardInTime(1);
+  EXPECT_EQ(points_test_api_->GetNumberOfPoints(), 4);
+  points_test_api_->MoveForwardInTime(3);
+  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, 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(GetEventGenerator().current_location(),
+            laser_pointer_mode_->laser_points().GetNewest().location);
+}
+}  // namespace ash