Preliminary cleanup of ShelfButton activity/status indicator.

Preliminary cleanup of ShelfButton activity/status indicator.

This used to be a bar. A lot of the code for it is overly complicated
because of that assumption, which is no longer true. This refactors and
simplifies for the current visuals (a dot).

It also changes the code to draw the circle at the correct scale factor
instead of drawing it at a scale factor of 5 then downscaling it, which
will be necessary to add the desired 1px border.

BUG=670970
Review-Url: https://codereview.chromium.org/2727073002
Cr-Commit-Position: refs/heads/master@{#454178}
Committed: https://chromium.googlesource.com/chromium/src/+/db8636d2cf1ba306fb968a3717daf08437ca0f34

[Evan Stade, 2017-03-02 00:44:56]

estade@chromium.org changed reviewers:
+ tdanderson@chromium.org

[Evan Stade, 2017-03-02 01:14:50]

estade@chromium.org changed reviewers:
+ derat@chromium.org
- tdanderson@chromium.org

[Evan Stade, 2017-03-02 01:14:50]

-tdanderson, +derat since he has a little more context on this after our brief
convo.

[Evan Stade, 2017-03-02 01:14:57]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 01:15:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Evan Stade, 2017-03-02 01:15:44]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 01:16:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Daniel Erat, 2017-03-02 01:23:02]

lgtm

yay, i guess this code didn't have any tests so none need to be updated!

[Evan Stade, 2017-03-02 01:38:03]

The CQ bit was unchecked by estade@chromium.org

[Evan Stade, 2017-03-02 01:39:02]

On 2017/03/02 01:23:02, Daniel Erat wrote:
> lgtm
> 
> yay, i guess this code didn't have any tests so none need to be updated!

would that there were tests which I could delete to improve the +/- linecount
stats

[Evan Stade, 2017-03-02 01:39:07]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2017-03-02 01:39:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 01:57:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-02 01:57:32]

Failed to apply patch for ash/common/shelf/shelf_button.cc:
While running git apply --index -p1;
  error: patch failed: ash/common/shelf/shelf_button.cc:50
  error: ash/common/shelf/shelf_button.cc: patch does not apply

Patch:       ash/common/shelf/shelf_button.cc
Index: ash/common/shelf/shelf_button.cc
diff --git a/ash/common/shelf/shelf_button.cc b/ash/common/shelf/shelf_button.cc
index
2a9d2ad924f891fd4f3d1c8534fa4de8e0bfc548..b39fa9c2f785d3bd2c4c23ae58b1dd20de0d3ccc
100644
--- a/ash/common/shelf/shelf_button.cc
+++ b/ash/common/shelf/shelf_button.cc
@@ -21,6 +21,7 @@
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/geometry/vector2d.h"
 #include "ui/gfx/image/image_skia_operations.h"
+#include "ui/gfx/scoped_canvas.h"
 #include "ui/gfx/skbitmap_operations.h"
 #include "ui/views/animation/ink_drop_impl.h"
 #include "ui/views/animation/square_ink_drop_ripple.h"
@@ -28,18 +29,12 @@
 
 namespace {
 
-// Size of the bar. This is along the opposite axis of the shelf. For example,
-// if the shelf is aligned horizontally then this is the height of the bar.
-const int kBarSize = 3;
 const int kIconSize = 32;
 const int kAttentionThrobDurationMS = 800;
 const int kMaxAnimationSeconds = 10;
 const int kIndicatorOffsetFromBottom = 2;
-const int kIndicatorRadius = 2;
+const int kIndicatorRadiusDip = 2;
 const SkColor kIndicatorColor = SK_ColorWHITE;
-// Canvas scale to ensure that the activity indicator is not pixelated even at
-// the highest possible device scale factors.
-const int kIndicatorCanvasScale = 5;
 
 // Shelf item ripple constants.
 const int kInkDropSmallSize = 48;
@@ -50,17 +45,6 @@ const int kInkDropLargeSize = 60;
 const int kIconPaddingHorizontal = 7;
 const int kIconPaddingVertical = 8;
 
-// Paints an activity indicator on |canvas| whose |size| is specified in DIP.
-void PaintIndicatorOnCanvas(gfx::Canvas* canvas, const gfx::Size& size) {
-  cc::PaintFlags flags;
-  flags.setColor(kIndicatorColor);
-  flags.setFlags(cc::PaintFlags::kAntiAlias_Flag);
-  canvas->DrawCircle(
-      gfx::Point(size.width() / 2,
-                 size.height() - kIndicatorOffsetFromBottom -
kIndicatorRadius),
-      kIndicatorRadius, flags);
-}
-
 // Simple AnimationDelegate that owns a single ThrobAnimation instance to
 // keep all Draw Attention animations in sync.
 class ShelfButtonAnimation : public gfx::AnimationDelegate {
@@ -86,7 +70,14 @@ class ShelfButtonAnimation : public gfx::AnimationDelegate {
       animation_.Stop();
   }
 
-  int GetAlpha() { return GetThrobAnimation().CurrentValueBetween(0, 255); }
+  bool HasObserver(Observer* observer) const {
+    return observers_.HasObserver(observer);
+  }
+
+  SkAlpha GetAlpha() {
+    return GetThrobAnimation().CurrentValueBetween(SK_AlphaTRANSPARENT,
+                                                   SK_AlphaOPAQUE);
+  }
 
   double GetAnimation() { return GetThrobAnimation().GetCurrentValue(); }
 
@@ -127,111 +118,71 @@ class ShelfButtonAnimation : public
gfx::AnimationDelegate {
 namespace ash {
 

////////////////////////////////////////////////////////////////////////////////
-// ShelfButton::BarView
+// ShelfButton::AppStatusIndicatorView
 
-class ShelfButton::BarView : public views::ImageView,
-                             public ShelfButtonAnimation::Observer {
+class ShelfButton::AppStatusIndicatorView
+    : public views::View,
+      public ShelfButtonAnimation::Observer {
  public:
-  BarView(WmShelf* wm_shelf)
-      : wm_shelf_(wm_shelf),
-        show_attention_(false),
-        animation_end_time_(base::TimeTicks()),
-        animating_(false) {
+  AppStatusIndicatorView()
+      : show_attention_(false), animation_end_time_(base::TimeTicks()) {
     // Make sure the events reach the parent view for handling.
     set_can_process_events_within_subtree(false);
   }
 
-  ~BarView() override {
-    if (show_attention_)
-      ShelfButtonAnimation::GetInstance()->RemoveObserver(this);
+  ~AppStatusIndicatorView() override {
+    ShelfButtonAnimation::GetInstance()->RemoveObserver(this);
   }
 
   // views::View:
   void OnPaint(gfx::Canvas* canvas) override {
+    gfx::ScopedCanvas scoped(canvas);
     if (show_attention_) {
-      int alpha =
-          animating_ ? ShelfButtonAnimation::GetInstance()->GetAlpha() : 255;
+      SkAlpha alpha = ShelfButtonAnimation::GetInstance()->HasObserver(this)
+                          ? ShelfButtonAnimation::GetInstance()->GetAlpha()
+                          : SK_AlphaOPAQUE;
       canvas->SaveLayerAlpha(alpha);
-      views::ImageView::OnPaint(canvas);
-      canvas->Restore();
-    } else {
-      views::ImageView::OnPaint(canvas);
     }
+
+    DCHECK_EQ(width(), height());
+    DCHECK_EQ(kIndicatorRadiusDip, width() / 2);
+    cc::PaintFlags flags;
+    flags.setColor(kIndicatorColor);
+    flags.setFlags(cc::PaintFlags::kAntiAlias_Flag);
+    canvas->DrawCircle(gfx::Point(width() / 2, height() / 2),
+                       kIndicatorRadiusDip, flags);
   }
 
   // ShelfButtonAnimation::Observer
   void AnimationProgressed() override {
-    UpdateBounds();
+    UpdateAnimating();
     SchedulePaint();
   }
 
-  void SetBarBoundsRect(const gfx::Rect& bounds) {
-    base_bounds_ = bounds;
-    UpdateBounds();
-  }
-
   void ShowAttention(bool show) {
-    if (show_attention_ != show) {
-      show_attention_ = show;
-      if (show_attention_) {
-        animating_ = true;
-        animation_end_time_ =
-            base::TimeTicks::Now() +
-            base::TimeDelta::FromSeconds(kMaxAnimationSeconds);
-        ShelfButtonAnimation::GetInstance()->AddObserver(this);
-      } else {
-        animating_ = false;
-        ShelfButtonAnimation::GetInstance()->RemoveObserver(this);
-      }
-    }
-    UpdateBounds();
-  }
+    if (show_attention_ == show)
+      return;
 
- private:
-  void UpdateBounds() {
-    gfx::Rect bounds = base_bounds_;
+    show_attention_ = show;
     if (show_attention_) {
-      // Scale from .35 to 1.0 of the total width (which is wider than the
-      // visible width of the image), so the animation "rests" briefly at full
-      // visible width.  Cap bounds length at kIconSize to prevent visual
-      // flutter while centering bar within further expanding bounds.
-      double animation =
-          animating_ ? ShelfButtonAnimation::GetInstance()->GetAnimation()
-                     : 1.0;
-      double scale = .35 + .65 * animation;
-      if (wm_shelf_->IsHorizontalAlignment()) {
-        int width = base_bounds_.width() * scale;
-        bounds.set_width(std::min(width, kIconSize));
-        int x_offset = (base_bounds_.width() - bounds.width()) / 2;
-        bounds.set_x(base_bounds_.x() + x_offset);
-        UpdateAnimating(bounds.width() == kIconSize);
-      } else {
-        int height = base_bounds_.height() * scale;
-        bounds.set_height(std::min(height, kIconSize));
-        int y_offset = (base_bounds_.height() - bounds.height()) / 2;
-        bounds.set_y(base_bounds_.y() + y_offset);
-        UpdateAnimating(bounds.height() == kIconSize);
-      }
+      animation_end_time_ = base::TimeTicks::Now() +
+                            base::TimeDelta::FromSeconds(kMaxAnimationSeconds);
+      ShelfButtonAnimation::GetInstance()->AddObserver(this);
+    } else {
+      ShelfButtonAnimation::GetInstance()->RemoveObserver(this);
     }
-    SetBoundsRect(bounds);
   }
 
-  void UpdateAnimating(bool max_length) {
-    if (!max_length)
-      return;
-    if (base::TimeTicks::Now() > animation_end_time_) {
-      animating_ = false;
+ private:
+  void UpdateAnimating() {
+    if (base::TimeTicks::Now() > animation_end_time_)
       ShelfButtonAnimation::GetInstance()->RemoveObserver(this);
-    }
   }
 
-  WmShelf* wm_shelf_;
   bool show_attention_;
   base::TimeTicks animation_end_time_;  // For attention throbbing underline.
-  bool animating_;  // Is time-limited attention animation running?
-  gfx::Rect base_bounds_;
 
-  DISALLOW_COPY_AND_ASSIGN(BarView);
+  DISALLOW_COPY_AND_ASSIGN(AppStatusIndicatorView);
 };
 

////////////////////////////////////////////////////////////////////////////////
@@ -245,7 +196,7 @@ ShelfButton::ShelfButton(InkDropButtonListener* listener,
ShelfView* shelf_view)
       listener_(listener),
       shelf_view_(shelf_view),
       icon_view_(new views::ImageView()),
-      bar_(new BarView(shelf_view->wm_shelf())),
+      indicator_(new AppStatusIndicatorView()),
       state_(STATE_NORMAL),
       destroyed_flag_(nullptr) {
   SetFocusBehavior(FocusBehavior::ACCESSIBLE_ONLY);
@@ -268,7 +219,7 @@ ShelfButton::ShelfButton(InkDropButtonListener* listener,
ShelfView* shelf_view)
   // Do not make this interactive, so that events are sent to ShelfView.
   icon_view_->set_can_process_events_within_subtree(false);
 
-  AddChildView(bar_);
+  AddChildView(indicator_);
   AddChildView(icon_view_);
 }
 
@@ -317,7 +268,7 @@ void ShelfButton::AddState(State state) {
     state_ |= state;
     Layout();
     if (state & STATE_ATTENTION)
-      bar_->ShowAttention(true);
+      indicator_->ShowAttention(true);
   }
 }
 
@@ -326,7 +277,7 @@ void ShelfButton::ClearState(State state) {
     state_ &= ~state;
     Layout();
     if (state & STATE_ATTENTION)
-      bar_->ShowAttention(false);
+      indicator_->ShowAttention(false);
   }
 }
 
@@ -406,17 +357,11 @@ void ShelfButton::Layout() {
   if (SHELF_ALIGNMENT_LEFT == wm_shelf->GetAlignment())
     x_offset = button_bounds.width() - (kIconSize + icon_pad);
 
-  // Center icon with respect to the secondary axis, and ensure
-  // that the icon doesn't occlude the bar highlight.
-  if (is_horizontal_shelf) {
+  // Center icon with respect to the secondary axis.
+  if (is_horizontal_shelf)
     x_offset = std::max(0, button_bounds.width() - icon_width) / 2;
-    if (y_offset + icon_height + kBarSize > button_bounds.height())
-      icon_height = button_bounds.height() - (y_offset + kBarSize);
-  } else {
+  else
     y_offset = std::max(0, button_bounds.height() - icon_heigh…
(message too large)

[Evan Stade, 2017-03-02 03:22:58]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2017-03-02 03:22:59]

The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org
Link to the patchset: https://codereview.chromium.org/2727073002/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2017-03-02 03:23:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 04:26:02]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488424978848330,
"parent_rev": "4f8acbd89df62b20df694810ec309c1bf1e0d6c9", "commit_rev":
"db8636d2cf1ba306fb968a3717daf08437ca0f34"}

[commit-bot: I haz the power, 2017-03-02 04:26:52]

Description was changed from

==========
Preliminary cleanup of ShelfButton activity/status indicator.

This used to be a bar. A lot of the code for it is overly complicated
because of that assumption, which is no longer true. This refactors and
simplifies for the current visuals (a dot).

It also changes the code to draw the circle at the correct scale factor
instead of drawing it at a scale factor of 5 then downscaling it, which
will be necessary to add the desired 1px border.

BUG=670970
==========

to

==========
Preliminary cleanup of ShelfButton activity/status indicator.

This used to be a bar. A lot of the code for it is overly complicated
because of that assumption, which is no longer true. This refactors and
simplifies for the current visuals (a dot).

It also changes the code to draw the circle at the correct scale factor
instead of drawing it at a scale factor of 5 then downscaling it, which
will be necessary to add the desired 1px border.

BUG=670970

Review-Url: https://codereview.chromium.org/2727073002
Cr-Commit-Position: refs/heads/master@{#454178}
Committed:
https://chromium.googlesource.com/chromium/src/+/db8636d2cf1ba306fb968a3717da...
==========

[commit-bot: I haz the power, 2017-03-02 04:26:53]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/db8636d2cf1ba306fb968a3717da...