cros: Laser pointer fades out on release, do not cover palette.

ash/laser/laser_pointer_points.cc

Index: ash/laser/laser_pointer_points.cc
diff --git a/ash/laser/laser_pointer_points.cc b/ash/laser/laser_pointer_points.cc
index 8f2176ed8dbdfe42a5a5d5d2e78c3425e5797c5b..bf13b06cce2b4731ae3717b43ab1a615ee47ff27 100644
--- a/ash/laser/laser_pointer_points.cc
+++ b/ash/laser/laser_pointer_points.cc
@@ -17,9 +17,27 @@ LaserPointerPoints::~LaserPointerPoints() {}
 void LaserPointerPoints::AddPoint(const gfx::Point& point) {
   LaserPoint new_point;
   new_point.location = point;
-  new_point.creation_time = base::Time::Now();
+  new_point.age = 0.0;
+  MoveForwardToTime(base::Time::Now());

[jdufault, 2016/10/05 21:36:05]

Call this at the start of the function. It's needlessly splitting up the
initialization of new_point.

[sammiequon, 2016/10/06 00:19:15]

Done.

   points_.push_back(new_point);
+}
+
+void LaserPointerPoints::MoveForwardToCurrentTime() {
+  MoveForwardToTime(base::Time::Now());
+}
+
+void LaserPointerPoints::MoveForwardToTime(const base::Time& new_latest_time) {

[jdufault, 2016/10/05 21:36:05]

Drop new_ on new_latest_time_, it is redundant.

[sammiequon, 2016/10/06 00:19:15]

Done.

+  if (collection_latest_time_.is_null())
+    collection_latest_time_ = new_latest_time;

[jdufault, 2016/10/05 21:36:05]

If this is true then points_ must be empty, right?

I would add

  DCHECK_IMPLIES(collection_latest_time_.is_null(), !points_.length());

The function doesn't really make a ton of sense with this if as well, since it
then doesn't do anything. I would instead do

if (!collection_latest_time_.is_null()) {
  // update point ages
}

collection_latest_time_ = latest_time_;

[sammiequon, 2016/10/06 00:19:15]

It seems DCHECK_IMPLIES is only used in v8 stuff. I think the way it's being
used null time implies empty collection but thats becaused the laser pointer
view gets reset every time the collection becomes empty.

+
+  // Update the ages of the points based on the change in new latest time.
+  base::TimeDelta delta = new_latest_time - collection_latest_time_;
+  double lifespan_change =
+      delta.InMillisecondsF() / life_duration_.InMillisecondsF();
+  for (LaserPoint& point : points_)
+    point.age += lifespan_change;
   ClearOldPoints();

[jdufault, 2016/10/05 21:36:05]

Call ClearOldPoints after updating collection_latest_time_ to keep related logic
together.

[sammiequon, 2016/10/06 00:19:15]

Done.

+  collection_latest_time_ = new_latest_time;
 }
 
 void LaserPointerPoints::Clear() {
@@ -63,12 +81,9 @@ LaserPointerPoints::laser_points() {
 }
 
 void LaserPointerPoints::ClearOldPoints() {

[jdufault, 2016/10/05 21:36:05]

It looks like the only caller of this method is MoveForwardToTime. I would
inline the method.

[sammiequon, 2016/10/06 00:19:15]

Done.

-  DCHECK(!IsEmpty());
   auto first_alive_point =
-      std::find_if(points_.begin(), points_.end(), [this](LaserPoint& p) {
-        return GetNewest().creation_time - p.creation_time < life_duration_;
-      });
+      std::find_if(points_.begin(), points_.end(),
+                   [](LaserPoint& p) { return p.age < 1.0; });
   points_.erase(points_.begin(), first_alive_point);
 }
-
 }  // namespace ash