Palette tool laser prototype.

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

Index: ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc
diff --git a/ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc b/ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc
new file mode 100644
index 0000000000000000000000000000000000000000..dbcc25e72ce856d701d701381f0ecb0694324abd
--- /dev/null
+++ b/ash/common/system/chromeos/palette/tools/laser_pointer_mode.cc
@@ -0,0 +1,120 @@
+// 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/palette_delegate.h"
+#include "ash/common/system/chromeos/palette/palette_ids.h"
+#include "ash/common/wm_shell.h"
+#include "ui/wm/core/coordinate_conversion.h"
+#include "ui/wm/core/cursor_manager.h"
+
+namespace ash {
+namespace {
+const int kPointLifeDurationMs = 300;
+const int kAddStationaryPointsDelayMs = 10;
+}
+
+LaserPointerMode::LaserPointerMode(Delegate* delegate)
+    : CommonPaletteTool(delegate),
+      laser_points_(
+          base::TimeDelta::FromMilliseconds(int64_t{kPointLifeDurationMs})) {

[jdufault, 2016/08/12 19:57:58]

Drop int64_t cast.

[sammiequon, 2016/08/16 17:00:03]

Done.

+  laser_pointer_view_.reset(new LaserPointerView());
+  InitializeTimer();

[jdufault, 2016/08/12 19:57:58]

Inline this function.

[sammiequon, 2016/08/16 17:00:03]

Done.

+  WmShell::Get()->AddPointerWatcher(this, true);
+}
+
+LaserPointerMode::~LaserPointerMode() {
+  WmShell::Get()->RemovePointerWatcher(this);
+}
+
+PaletteGroup LaserPointerMode::GetGroup() const {
+  return PaletteGroup::MODE;
+}
+
+PaletteToolId LaserPointerMode::GetToolId() const {
+  return PaletteToolId::LASER_POINTER;
+}
+
+void LaserPointerMode::OnEnable() {
+  CommonPaletteTool::OnEnable();
+  if (WmShell::Get()->palette_delegate())

[jdufault, 2016/08/12 19:57:58]

newline above if

[sammiequon, 2016/08/16 17:00:03]

Done.

+    WmShell::Get()->palette_delegate()->OnLaserModeEnabled();
+  laser_points_.AddPoint(current_mouse_location_);
+  laser_pointer_view_->SetNewPoints(laser_points_);
+}
+
+void LaserPointerMode::OnDisable() {
+  CommonPaletteTool::OnDisable();
+  if (WmShell::Get()->palette_delegate())

[jdufault, 2016/08/12 19:57:58]

newline above if

[sammiequon, 2016/08/16 17:00:03]

Done.

+    WmShell::Get()->palette_delegate()->OnLaserModeDisabled();
+  StopTimer();
+  // Clear the points and then notify the view that there is nothing to be
+  // rendered.
+  laser_points_.Clear();
+  laser_pointer_view_->Stop();
+}
+
+gfx::VectorIconId LaserPointerMode::GetActiveTrayIcon() {
+  return gfx::VectorIconId::PALETTE_TRAY_ICON_LASER_POINTER;
+}
+
+gfx::VectorIconId LaserPointerMode::GetPaletteIconId() {
+  return gfx::VectorIconId::PALETTE_MODE_LASER_POINTER;
+}
+
+void LaserPointerMode::InitializeTimer() {
+  timer_.reset(new base::Timer(
+      FROM_HERE, base::TimeDelta::FromMilliseconds(kAddStationaryPointsDelayMs),
+      base::Bind(&LaserPointerMode::AddStationaryPoint, base::Unretained(this)),
+      false));
+}
+
+void LaserPointerMode::StartTimer() {

[jdufault, 2016/08/12 19:57:57]

I'd consider inlining this method since it's only called once and it's one line.

[sammiequon, 2016/08/16 17:00:04]

Done.

+  timer_->Reset();
+}
+
+void LaserPointerMode::StopTimer() {
+  points_lock_.Acquire();
+  timer_repeat_count_ = 0;
+  points_lock_.Release();
+  timer_->Stop();
+}
+
+void LaserPointerMode::AddStationaryPoint() {
+  points_lock_.Acquire();

[jdufault, 2016/08/12 19:57:57]

Instead of having a lock can we just make it so that this function is always
called on the UI thread? You can verify this by adding the following statement:

  DCHECK_CURRENTLY_ON(BrowserThread::UI)

[sammiequon, 2016/08/16 17:00:04]

Done.

+  laser_points_.AddPoint(current_mouse_location_);
+  laser_pointer_view_->SetNewPoints(laser_points_);
+  int count = timer_repeat_count_;
+  points_lock_.Release();
+  // We can stop repeating the timer once the mouse has been stationary for
+  // longer than the life of a point.
+  if (count * kAddStationaryPointsDelayMs < kPointLifeDurationMs) {
+    timer_repeat_count_++;

[jdufault, 2016/08/12 19:57:58]

This should only be modified inside of the lock. I think the variable decl also
needs to be mutable.

I'd strongly suggest staying away from making this code multithreaded - it's
very easy to introduce bugs.

[sammiequon, 2016/08/16 17:00:04]

Done.

+    timer_->Reset();
+  } else {
+    StopTimer();
+  }
+}
+
+void LaserPointerMode::OnPointerEventObserved(
+    const ui::PointerEvent& event,
+    const gfx::Point& location_in_screen,
+    views::Widget* target) {
+  if (event.type() == ui::ET_POINTER_MOVED &&
+      event.pointer_details().pointer_type ==
+          ui::EventPointerType::POINTER_TYPE_PEN) {
+    current_mouse_location_ = location_in_screen;
+    if (enabled()) {

[jdufault, 2016/08/12 19:57:58]

Can this enabled() check be moved to the top of the function?

[sammiequon, 2016/08/16 17:00:04]

We need to update current_mouse_location_ when it is disabled so when it is
enabled the dot shows up at the right place.

+      points_lock_.Acquire();
+      // Add the new point and notify the queue.
+      laser_points_.AddPoint(current_mouse_location_);

[jdufault, 2016/08/12 19:57:58]

What happens if only the timer callback adds points? Is behavior jerky/bad?

[sammiequon, 2016/08/16 17:00:04]

Yeah it lags a lot.

+      laser_pointer_view_->SetNewPoints(laser_points_);
+      timer_repeat_count_ = 0;
+      points_lock_.Release();
+      StartTimer();
+    }
+  }
+}
+}  // namespace ash