Separate projection mode from rest of touch HUD

Separate projection mode from rest of touch HUD

Projection mode is separated from the rest of the touch HUD. It now can
be toggled on/off using Ctrl+Alt+9 key combination and no command line
flag is needed. The rest of the touch HUD is still behind the
--ash-touch-log flag.

BUG=233567
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=209523

[mohsen, 2013-06-21 22:22:48]

Please take a look...

[sadrul, 2013-06-26 17:26:21]

Looks reasonable. Please add an owner reviewer for the next patchset.

https://codereview.chromium.org/17063013/diff/22001/ash/touch/touch_hud_debug...
File ash/touch/touch_hud_debugging.h (right):

https://codereview.chromium.org/17063013/diff/22001/ash/touch/touch_hud_debug...
ash/touch/touch_hud_debugging.h:5: #ifndef ASH_TOUCH_TOUCH_HUD_DEBUGGING_H_
I would call this TouchHudDebug

https://codereview.chromium.org/17063013/diff/22001/ash/touch/touch_hud_debug...
ash/touch/touch_hud_debugging.h:49: virtual void Clear() OVERRIDE;
Replace the comment with: // Overridden from ..., and move at the end of
non-override methods

https://codereview.chromium.org/17063013/diff/22001/ash/touch/touch_hud_proje...
File ash/touch/touch_hud_projection.cc (right):

https://codereview.chromium.org/17063013/diff/22001/ash/touch/touch_hud_proje...
ash/touch/touch_hud_projection.cc:147: iter->second->Remove();
You need to do points_.clear() here, right?

https://codereview.chromium.org/17063013/diff/22001/ash/touch/touch_observer_...
File ash/touch/touch_observer_hud.cc (right):

https://codereview.chromium.org/17063013/diff/22001/ash/touch/touch_observer_...
ash/touch/touch_observer_hud.cc:39: params.bounds = display.bounds();
This fixes positioning the HUD in the right display?

https://codereview.chromium.org/17063013/diff/22001/ash/touch/touch_observer_...
File ash/touch/touch_observer_hud.h (right):

https://codereview.chromium.org/17063013/diff/22001/ash/touch/touch_observer_...
ash/touch/touch_observer_hud.h:70: virtual void SetHudForRootWindowController(
Doc. Move these above the overridden methods

[mohsen, 2013-06-26 19:15:48]

sky@: for OWNERS in /ash/, please.

https://codereview.chromium.org/17063013/diff/22001/ash/touch/touch_hud_debug...
File ash/touch/touch_hud_debugging.h (right):

https://codereview.chromium.org/17063013/diff/22001/ash/touch/touch_hud_debug...
ash/touch/touch_hud_debugging.h:5: #ifndef ASH_TOUCH_TOUCH_HUD_DEBUGGING_H_
On 2013/06/26 17:26:21, sadrul wrote:
> I would call this TouchHudDebug

Done.

https://codereview.chromium.org/17063013/diff/22001/ash/touch/touch_hud_debug...
ash/touch/touch_hud_debugging.h:49: virtual void Clear() OVERRIDE;
On 2013/06/26 17:26:21, sadrul wrote:
> Replace the comment with: // Overridden from ..., and move at the end of
> non-override methods

Done.

https://codereview.chromium.org/17063013/diff/22001/ash/touch/touch_hud_proje...
File ash/touch/touch_hud_projection.cc (right):

https://codereview.chromium.org/17063013/diff/22001/ash/touch/touch_hud_proje...
ash/touch/touch_hud_projection.cc:147: iter->second->Remove();
On 2013/06/26 17:26:21, sadrul wrote:
> You need to do points_.clear() here, right?

Yes. Done.

https://codereview.chromium.org/17063013/diff/22001/ash/touch/touch_observer_...
File ash/touch/touch_observer_hud.cc (right):

https://codereview.chromium.org/17063013/diff/22001/ash/touch/touch_observer_...
ash/touch/touch_observer_hud.cc:39: params.bounds = display.bounds();
On 2013/06/26 17:26:21, sadrul wrote:
> This fixes positioning the HUD in the right display?

Yes. The bounds for "overlay container" need to be in screen coordinates.

https://codereview.chromium.org/17063013/diff/22001/ash/touch/touch_observer_...
File ash/touch/touch_observer_hud.h (right):

https://codereview.chromium.org/17063013/diff/22001/ash/touch/touch_observer_...
ash/touch/touch_observer_hud.h:70: virtual void SetHudForRootWindowController(
On 2013/06/26 17:26:21, sadrul wrote:
> Doc. Move these above the overridden methods

Done.

[sky, 2013-06-26 21:07:43]

https://codereview.chromium.org/17063013/diff/43001/ash/root_window_controller.h
File ash/root_window_controller.h (right):

https://codereview.chromium.org/17063013/diff/43001/ash/root_window_controlle...
ash/root_window_controller.h:110: void
set_touch_hud_projection(TouchHudProjection* hud) {
Who owns hud and what happens to existing hud? Same question on
set_touch_hud_debug.

https://codereview.chromium.org/17063013/diff/43001/ash/root_window_controlle...
ash/root_window_controller.h:195: void InitTouchHuds();
Shouldn't this be private?

https://codereview.chromium.org/17063013/diff/43001/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/17063013/diff/43001/ash/shell.h#newcode467
ash/shell.h:467: void ToggleTouchHudProjection();
Can't the caller trivially implement this rather than another method?

https://codereview.chromium.org/17063013/diff/43001/ash/shell.h#newcode469
ash/shell.h:469: bool is_touch_hud_projection_enabled() const {
Style guide says function should match member. So either rename member to
'is_touch_...' or nuke the is_ from this. I tend to think is_tou... is cleaner.

https://codereview.chromium.org/17063013/diff/43001/ash/touch/touch_hud_debug.cc
File ash/touch/touch_hud_debug.cc (right):

https://codereview.chromium.org/17063013/diff/43001/ash/touch/touch_hud_debug...
ash/touch/touch_hud_debug.cc:336: touch_log_(new TouchLog()) {
Please member initialize all primitives/pointers (canvas_ and label_container_).

[mohsen, 2013-06-27 00:44:31]

https://codereview.chromium.org/17063013/diff/43001/ash/root_window_controller.h
File ash/root_window_controller.h (right):

https://codereview.chromium.org/17063013/diff/43001/ash/root_window_controlle...
ash/root_window_controller.h:110: void
set_touch_hud_projection(TouchHudProjection* hud) {
On 2013/06/26 21:07:44, sky wrote:
> Who owns hud and what happens to existing hud? Same question on
> set_touch_hud_debug.

The hud manages its own lifetime. When the widget showing the hud is being
destroyed (e.g. because of detaching a display or shutting down chromeos), the
hud deletes itself. Here we only have a pointer to the hud associated with this
root window so we can have access to it (e.g. in TouchLogSource or
AcceleratorController).

https://codereview.chromium.org/17063013/diff/43001/ash/root_window_controlle...
ash/root_window_controller.h:195: void InitTouchHuds();
On 2013/06/26 21:07:44, sky wrote:
> Shouldn't this be private?

This function is called from Shell::InitRootWindowController(). That's why it is
public.

https://codereview.chromium.org/17063013/diff/43001/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/17063013/diff/43001/ash/shell.h#newcode467
ash/shell.h:467: void ToggleTouchHudProjection();
On 2013/06/26 21:07:44, sky wrote:
> Can't the caller trivially implement this rather than another method?

Yes, it can. Removed this function.

https://codereview.chromium.org/17063013/diff/43001/ash/shell.h#newcode469
ash/shell.h:469: bool is_touch_hud_projection_enabled() const {
On 2013/06/26 21:07:44, sky wrote:
> Style guide says function should match member. So either rename member to
> 'is_touch_...' or nuke the is_ from this. I tend to think is_tou... is
cleaner.

Done.

https://codereview.chromium.org/17063013/diff/43001/ash/touch/touch_hud_debug.cc
File ash/touch/touch_hud_debug.cc (right):

https://codereview.chromium.org/17063013/diff/43001/ash/touch/touch_hud_debug...
ash/touch/touch_hud_debug.cc:336: touch_log_(new TouchLog()) {
On 2013/06/26 21:07:44, sky wrote:
> Please member initialize all primitives/pointers (canvas_ and
label_container_).

Done.
(Isn't it enough that they're initialized in the constructor body?)

[sky, 2013-06-28 16:09:00]

LGTM

https://codereview.chromium.org/17063013/diff/43001/ash/root_window_controller.h
File ash/root_window_controller.h (right):

https://codereview.chromium.org/17063013/diff/43001/ash/root_window_controlle...
ash/root_window_controller.h:110: void
set_touch_hud_projection(TouchHudProjection* hud) {
On 2013/06/27 00:44:31, mohsen wrote:
> On 2013/06/26 21:07:44, sky wrote:
> > Who owns hud and what happens to existing hud? Same question on
> > set_touch_hud_debug.
> 
> The hud manages its own lifetime. When the widget showing the hud is being
> destroyed (e.g. because of detaching a display or shutting down chromeos), the
> hud deletes itself. Here we only have a pointer to the hud associated with
this
> root window so we can have access to it (e.g. in TouchLogSource or
> AcceleratorController).

Document ownership here.

https://codereview.chromium.org/17063013/diff/43001/ash/touch/touch_hud_debug.cc
File ash/touch/touch_hud_debug.cc (right):

https://codereview.chromium.org/17063013/diff/43001/ash/touch/touch_hud_debug...
ash/touch/touch_hud_debug.cc:336: touch_log_(new TouchLog()) {
On 2013/06/27 00:44:31, mohsen wrote:
> On 2013/06/26 21:07:44, sky wrote:
> > Please member initialize all primitives/pointers (canvas_ and
> label_container_).
> 
> Done.
> (Isn't it enough that they're initialized in the constructor body?)

Your argument equally applies to whether you should use scoped_ptr inside
functions for heap allocated objects that are destroyed at the end of the
function. None-the-less the main reason is there is nothing stopping someone
from early returning in your constructor.

[commit-bot: I haz the power, 2013-06-28 17:36:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mohsen@chromium.org/17063013/70001

[commit-bot: I haz the power, 2013-06-28 17:36:29]

Failed to apply patch for ash/touch/touch_hud_debug.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  A         ash/touch/touch_hud_debug.cc
  Copied ash/touch/touch_observer_hud.cc -> ash/touch/touch_hud_debug.cc
  patching file ash/touch/touch_hud_debug.cc
  Hunk #5 FAILED at 364.
  1 out of 8 hunks FAILED -- saving rejects to file
ash/touch/touch_hud_debug.cc.rej

Patch:   NR  ash/touch/touch_observer_hud.cc->ash/touch/touch_hud_debug.cc
Index: ash/touch/touch_hud_debug.cc
diff --git a/ash/touch/touch_observer_hud.cc b/ash/touch/touch_hud_debug.cc
similarity index 56%
copy from ash/touch/touch_observer_hud.cc
copy to ash/touch/touch_hud_debug.cc
index
a9ac307be1efe86d11fd39d9bdb40f8966f3665d..da69aa33042cac01919e97e397c954216a301730
100644
--- a/ash/touch/touch_observer_hud.cc
+++ b/ash/touch/touch_hud_debug.cc
@@ -1,33 +1,25 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright (c) 2013 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/touch/touch_observer_hud.h"
+#include "ash/touch/touch_hud_debug.h"
 
-#include "ash/display/display_controller.h"
 #include "ash/display/display_manager.h"
 #include "ash/root_window_controller.h"
-#include "ash/shell_window_ids.h"
+#include "ash/shell.h"
 #include "ash/wm/property_util.h"
 #include "base/json/json_string_value_serializer.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "third_party/skia/include/core/SkPath.h"
-#include "third_party/skia/include/core/SkXfermode.h"
-#include "third_party/skia/include/effects/SkGradientShader.h"
 #include "ui/aura/root_window.h"
-#include "ui/aura/window.h"
 #include "ui/base/animation/animation_delegate.h"
-#include "ui/base/animation/linear_animation.h"
 #include "ui/base/events/event.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/display.h"
-#include "ui/gfx/rect.h"
-#include "ui/gfx/screen.h"
 #include "ui/gfx/size.h"
 #include "ui/gfx/transform.h"
-#include "ui/views/background.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/layout/box_layout.h"
 #include "ui/views/widget/widget.h"
@@ -58,13 +50,8 @@ const SkColor kColors[] = {
   SkColorSetRGB(0xFF, 0xDE, 0xAD),
 };
 const int kAlpha = 0x60;
-const SkColor kProjectionFillColor = SkColorSetRGB(0xF5, 0xF5, 0xDC);
-const SkColor kProjectionStrokeColor = SK_ColorGRAY;
-const int kProjectionAlpha = 0xB0;
 const int kMaxPaths = arraysize(kColors);
 const int kReducedScale = 10;
-const int kFadeoutDurationInMs = 250;
-const int kFadeoutFrameRate = 60;
 
 const char* GetTouchEventLabel(ui::EventType type) {
   switch (type) {
@@ -154,7 +141,7 @@ struct TouchPointLog {
 };
 
 // A TouchTrace keeps track of all the touch events of a single touch point
-// (starting from a touch-press and ending at touch-release).
+// (starting from a touch-press and ending at a touch-release or touch-cancel).
 class TouchTrace {
  public:
   typedef std::vector<TouchPointLog>::iterator iterator;
@@ -343,131 +330,22 @@ class TouchHudCanvas : public views::View {
   DISALLOW_COPY_AND_ASSIGN(TouchHudCanvas);
 };
 
-// TouchPointView draws a single touch point in |PROJECTION| mode. This object
-// manages its own lifetime and deletes itself upon fade-out completion or
-// whenever |Remove()| is explicitly called.
-class TouchPointView : public views::View,
-                       public ui::AnimationDelegate,
-                       public views::WidgetObserver {
- public:
-  explicit TouchPointView(views::Widget* parent_widget)
-      : circle_center_(kPointRadius + 1, kPointRadius + 1),
-        gradient_center_(SkPoint::Make(kPointRadius + 1,
-                                       kPointRadius + 1)) {
-    SetPaintToLayer(true);
-    SetFillsBoundsOpaquely(false);
-
-    SetSize(gfx::Size(2 * kPointRadius + 2, 2 * kPointRadius + 2));
-
-    stroke_paint_.setStyle(SkPaint::kStroke_Style);
-    stroke_paint_.setColor(kProjectionStrokeColor);
-
-    gradient_colors_[0] = kProjectionFillColor;
-    gradient_colors_[1] = kProjectionStrokeColor;
-
-    gradient_pos_[0] = SkFloatToScalar(0.9f);
-    gradient_pos_[1] = SkFloatToScalar(1.0f);
-
-    parent_widget->GetContentsView()->AddChildView(this);
-
-    parent_widget->AddObserver(this);
-  }
-
-  void UpdateTouch(const ui::TouchEvent& touch) {
-    if (touch.type() == ui::ET_TOUCH_RELEASED ||
-        touch.type() == ui::ET_TOUCH_CANCELLED) {
-      fadeout_.reset(new ui::LinearAnimation(kFadeoutDurationInMs,
-                                             kFadeoutFrameRate,
-                                             this));
-      fadeout_->Start();
-    } else {
-      SetX(touch.root_location().x() - kPointRadius - 1);
-      SetY(touch.root_location().y() - kPointRadius - 1);
-    }
-  }
-
-  void Remove() {
-    delete this;
-  }
-
- private:
-  virtual ~TouchPointView() {
-    GetWidget()->RemoveObserver(this);
-    parent()->RemoveChildView(this);
-  }
-
-  // Overridden from views::View.
-  virtual void OnPaint(gfx::Canvas* canvas) OVERRIDE {
-    int alpha = kProjectionAlpha;
-    if (fadeout_)
-      alpha = static_cast<int>(fadeout_->CurrentValueBetween(alpha, 0));
-    fill_paint_.setAlpha(alpha);
-    stroke_paint_.setAlpha(alpha);
-    SkShader* shader = SkGradientShader::CreateRadial(
-        gradient_center_,
-        SkIntToScalar(kPointRadius),
-        gradient_colors_,
-        gradient_pos_,
-        arraysize(gradient_colors_),
-        SkShader::kMirror_TileMode,
-        NULL);
-    fill_paint_.setShader(shader);
-    shader->unref();
-    canvas->DrawCircle(circle_center_, SkIntToScalar(kPointRadius),
-                       fill_paint_);
-    canvas->DrawCircle(circle_center_, SkIntToScalar(kPointRadius),
-                       stroke_paint_);
-  }
-
-  // Overridden from ui::AnimationDelegate.
-  virtual void AnimationEnded(const ui::Animation* animation) OVERRIDE {
-    DCHECK_EQ(fadeout_.get(), animation);
-    delete this;
-  }
-
-  virtual void AnimationProgressed(const ui::Animation* animation) OVERRIDE {
-    DCHECK_EQ(fadeout_.get(), animation);
-    SchedulePaint();
-  }
-
-  virtual void AnimationCanceled(const ui::Animation* animation) OVERRIDE {
-    AnimationEnded(animation);
-  }
-
-  // Overridden from views::WidgetObserver.
-  virtual void OnWidgetDestroying(views::Widget* widget) OVERRIDE {
-    fadeout_->Stop();
-  }
-
-  const gfx::Point circle_center_;
-  const SkPoint gradient_center_;
-
-  SkPaint fill_paint_;
-  SkPaint stroke_paint_;
-  SkColor gradient_colors_[2];
-  SkScalar gradient_pos_[2];
-
-  scoped_ptr<ui::Animation> fadeout_;
-
-  DISALLOW_COPY_AND_ASSIGN(TouchPointView);
-};
-
-TouchObserverHUD::TouchObserverHUD(aura::RootWindow* initial_root)
-    : display_id_(initial_root->GetProperty(kDisplayIdKey)),
-      root_window_(initial_root),
+TouchHudDebug::TouchHudDebug(aura::RootWindow* initial_root)
+    : TouchObserverHUD(initial_root),
       mode_(FULLSCREEN),
-      touch_log_(new TouchLog()) {
+      touch_log_(new TouchLog()),
+      canvas_(NULL),
+      label_container_(NULL) {
   const gfx::Display& display =
-      Shell::GetInstance()->display_manager()->GetDisplayForId(display_id_);
+      Shell::GetInstance()->display_manager()->GetDisplayForId(display_id());
 
-  views::View* content = new views::View;
+  views::View* content = widget()->GetContentsView();
 
   canvas_ = new TouchHudCanvas(*touch_log_);
   content->AddChildView(canvas_);
 
   const gfx::Size& display_size = display.size();
   canvas_->SetSize(display_size);
-  content->SetSize(display_size);
 
   label_container_ = new views::View;
   label_container_->SetLayoutManager(new views::BoxLayout(
@@ -486,72 +364,34 @@ TouchObserverHUD::TouchObserverHUD(aura::RootWindow*
initial_root)
   label_container_->SetSize(label_container_->GetPreferredSize());
   label_container_->SetVisible(false);
   content->AddChildView(label_container_);
-
-  widget_ = new views::Widget();
-  views::Widget::InitParams
-      params(views::Widget::InitParams::TYPE_WINDOW_FRAMELESS);
-  params.transparent = true;
-  params.can_activate = false;
-  params.accept_events = false;
-  params.bounds = gfx::Rect(display_size);
-  params.parent = Shell::GetContainer(
-      root_window_,
-      internal::kShellWindowId_OverlayContainer);
-  widget_->Init(params);
-  widget_->SetContentsView(content);
-  widget_->StackAtTop();
-  widget_->Show();
-
-  widget_->AddObserver(this);
-
-  // Observe changes in display size and mode to update touch HUD.
-  Shell::GetScreen()->AddObserver(this);
-#if defined(OS_CHROMEOS)
-  Shell::GetInstance()->output_configurator()->AddObserver(this);
-#endif  // defined(OS_CHROMEOS)
-
-  Shell::GetInstance()->display_controller()->AddObserver(this);
-  root_window_->AddPreTargetHandler(this);
 }
 
-TouchObserverHUD::~TouchObserverHUD() {
-  Shell::GetInstance()->display_controller()->RemoveObserver(this);
-
-#if defined(OS_CHROMEOS)
-  Shell::GetInstance()->output_configurator()->RemoveObserver(this);
-#endif  // defined(OS_CHROMEOS)
-  Shell::GetScreen()->RemoveObserver(this);
-
-  widget_->RemoveObserver(this);
+TouchHudDebug::~TouchHudDebug() {
 }
 
 // static
-scoped_ptr<DictionaryValue> TouchObserverHUD::GetAllAsDictionary() {
+scoped_ptr<DictionaryValue> TouchHudDebug::GetAllAsDictionary() {
   scoped_ptr<DictionaryValue> value(new DictionaryValue());
   Shell::RootWindowList roots = Shell::GetInstance()->GetAllRootWindows();
   for (Shell::RootWindowList::iterator iter = roots.begin();
       iter != roots.end(); ++iter) {
     internal::RootWindowController* controller =
GetRootWindowController(*iter);
-    if (controller->touch_observer_hud()) {
-      int64 display_id = (*iter)->GetProperty(kDisplayIdKey);
-      scoped_ptr<ListValue> list =
-          controller->touch_observer_hud()->GetLogAsList…
(message too large)

[commit-bot: I haz the power, 2013-07-01 20:49:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mohsen@chromium.org/17063013/74001

[commit-bot: I haz the power, 2013-07-01 22:43:42]

Change committed as 209523