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

ash/laser/laser_pointer_controller.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/laser/laser_pointer_controller.h"
 
 #include "ash/common/system/chromeos/palette/palette_utils.h"
 #include "ash/laser/laser_pointer_view.h"
 #include "ash/shell.h"
 #include "ui/aura/window_event_dispatcher.h"
@@ skipping 55 matching lines @@
 
   if (event->pointer_details().pointer_type !=
       ui::EventPointerType::POINTER_TYPE_PEN)
     return;
 
   if (event->type() != ui::ET_MOUSE_DRAGGED &&
       event->type() != ui::ET_MOUSE_PRESSED &&
       event->type() != ui::ET_MOUSE_RELEASED)
     return;
 
-  // Delete the LaserPointerView instance if mouse is released.
-  if (event->type() == ui::ET_MOUSE_RELEASED) {
-    stationary_timer_->Stop();
-    laser_pointer_view_->Stop();
-    laser_pointer_view_.reset();
+  aura::Window* current_window = GetCurrentRootWindow();
+
+  // Check if the current window is valid. Destroy the laser pointer view if
+  // not.
+  if (!current_window) {
+    DestroyLaserPointerView();
     return;
   }
 
-  // This will handle creating the initial laser pointer view on
-  // ET_MOUSE_PRESSED events.
-  SwitchTargetRootWindowIfNeeded(GetCurrentRootWindow());
+  gfx::Point event_location = event->root_location();
+  aura::Window* target = static_cast<aura::Window*>(event->target());
+  aura::Window* event_root = target->GetRootWindow();
+  // Remap point from where it was captured to the display it is actually on.

[jdufault, 2016/10/04 22:04:12]

Above line 85, what about this comment instead:

// Compute the event coordinate relative to the display it is currently on (and
not the one the event was captured on).

[sammiequon, 2016/10/05 17:42:22]

Done.

+  aura::Window::ConvertPointToTarget(event_root, current_window,
+                                     &event_location);
+
+  // If the stylus is over the palette icon or widget do not display the laser.

[jdufault, 2016/10/04 22:04:12]

This comment seems misleading because the laser pointer can be visible here if
it is fading away.

[sammiequon, 2016/10/05 17:42:22]

Done.

+  if ((!laser_pointer_view_ || is_fading_away_) &&
+      PaletteContainsPointInScreen(event_location)) {
+    return;
+  }
+
+  // If the stylus is pressed and the laser pointer view is currently active,
+  // create a new one and stop the timer.

[jdufault, 2016/10/04 22:04:12]

nit: replace `one` with `view`

Why the comment about stopping the timer?

[sammiequon, 2016/10/05 17:42:22]

Done.

+  if (event->type() == ui::ET_MOUSE_PRESSED)
+    DestroyLaserPointerView();
+
+  SwitchTargetRootWindowIfNeeded(current_window);
 
   if (laser_pointer_view_) {
-    // Remap point from where it was captured to the display it is actually on.
-    gfx::Point event_location = event->root_location();
-    aura::Window* target = static_cast<aura::Window*>(event->target());
-    aura::Window* event_root = target->GetRootWindow();
-    aura::Window::ConvertPointToTarget(
-        event_root, laser_pointer_view_->GetRootWindow(), &event_location);
-
     current_mouse_location_ = event_location;
-    laser_pointer_view_->AddNewPoint(current_mouse_location_);
+    laser_pointer_view_->AddNewPoint(current_mouse_location_, is_fading_away_);
 
     stationary_timer_repeat_count_ = 0;
-    if (event->type() == ui::ET_MOUSE_DRAGGED) {
-      // Start the timer to add stationary points if dragged.
+    if (event->type() == ui::ET_MOUSE_DRAGGED ||
+        event->type() == ui::ET_MOUSE_RELEASED) {
+      // Signal the view to start fading away if the mouse has been released.
+      is_fading_away_ = event->type() == ui::ET_MOUSE_RELEASED;

[jdufault, 2016/10/04 22:04:12]

Shouldn't this check happen before we add the point?

[sammiequon, 2016/10/05 17:42:22]

Done.

+      // Start the timer to add stationary points if dragged, or too advance the

[jdufault, 2016/10/04 22:04:12]

too => to

[sammiequon, 2016/10/05 17:42:22]

Done.

+      // internal collections timestamps if released.
       if (!stationary_timer_->IsRunning())
         stationary_timer_->Reset();
     }
-
-    // If the stylus is over the palette icon or widget, do not consume the
-    // event.
-    if (!PaletteContainsPointInScreen(current_mouse_location_))
-      event->StopPropagation();
   }
+  event->StopPropagation();
 }
 
 void LaserPointerController::OnWindowDestroying(aura::Window* window) {
   SwitchTargetRootWindowIfNeeded(window);
 }
 
 void LaserPointerController::SwitchTargetRootWindowIfNeeded(
     aura::Window* root_window) {
   if (!root_window) {
-    stationary_timer_->Stop();
-    laser_pointer_view_.reset();
+    DestroyLaserPointerView();
   } else if (laser_pointer_view_) {
     laser_pointer_view_->ReparentWidget(root_window);
   } else if (enabled_) {
     laser_pointer_view_.reset(new LaserPointerView(
         base::TimeDelta::FromMilliseconds(kPointLifeDurationMs), root_window));
   }
 }
 
+void LaserPointerController::DestroyLaserPointerView() {
+  // |stationary_timer_| should also be stopped so that it does not attempt to
+  // add points when |laser_pointer_view_| is nullptr.

[jdufault, 2016/10/04 22:04:12]

nit: nullptr => null

[sammiequon, 2016/10/05 17:42:23]

Done.

+  stationary_timer_->Stop();
+  if (laser_pointer_view_) {
+    is_fading_away_ = false;
+    laser_pointer_view_->Stop();

[jdufault, 2016/10/04 22:04:12]

If we are destroying the view, why do we need to call Stop?

[sammiequon, 2016/10/05 17:42:22]

Done.

+    laser_pointer_view_.reset();
+  }
+}
+
 void LaserPointerController::AddStationaryPoint() {
-  laser_pointer_view_->AddNewPoint(current_mouse_location_);
+  laser_pointer_view_->AddNewPoint(current_mouse_location_, is_fading_away_);
   // We can stop repeating the timer once the mouse has been stationary for
   // longer than the life of a point.
   if (stationary_timer_repeat_count_ * kAddStationaryPointsDelayMs >=
       kPointLifeDurationMs) {
     stationary_timer_->Stop();
+    // Reset the view if the timer expires and the view was in process of fading
+    // away.
+    if (is_fading_away_)
+      DestroyLaserPointerView();
   }
   stationary_timer_repeat_count_++;
 }
 }  // namespace ash