Update activity indicator from a light bar to a point

ash/shelf/shelf_button.cc

Index: ash/shelf/shelf_button.cc
diff --git a/ash/shelf/shelf_button.cc b/ash/shelf/shelf_button.cc
index faf0b4e8c98b5f44a8e8942969528bf88b9bc5b2..d0aa04e556adb24e80cafe8535241f22ab8cea76 100644
--- a/ash/shelf/shelf_button.cc
+++ b/ash/shelf/shelf_button.cc
@@ -8,11 +8,13 @@
 
 #include "ash/ash_constants.h"
 #include "ash/ash_switches.h"
+#include "ash/material_design/material_design_controller.h"
 #include "ash/shelf/shelf.h"
 #include "ash/shelf/shelf_view.h"
 #include "base/time/time.h"
 #include "grit/ash_resources.h"
 #include "skia/ext/image_operations.h"
+#include "third_party/skia/include/core/SkPaint.h"
 #include "ui/accessibility/ax_view_state.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/compositor/layer.h"
@@ -37,6 +39,11 @@ const int kIconPad = 5;
 const int kIconPadVertical = 6;
 const int kAttentionThrobDurationMS = 800;
 const int kMaxAnimationSeconds = 10;
+const int kShelfIconHeight = 48;

[varkha, 2016/05/02 18:13:58]

Is this defined / dependent on a constant that is defined elsewhere?

[yiyix, 2016/05/03 18:55:34]

You are right, this is defined in shelf_constants. Width = 44 and height = 47. I
will update the code to use the current constraints. When I implement BUG
595005, I will update it to use the new spec.

+const int kShelfIconWidth = 48;
+const int kIconInsideVertical = 3;

[varkha, 2016/05/02 18:13:59]

nit: rename kIndicatorOffsetFromBottom.

[yiyix, 2016/05/03 18:55:34]

Done.

+const int kIndicatorRadius = 2;
+const int kIndicatorColor = SK_ColorWHITE;
 
 // Simple AnimationDelegate that owns a single ThrobAnimation instance to
 // keep all Draw Attention animations in sync.
@@ -486,8 +493,17 @@ void ShelfButton::UpdateBar() {
 
   if (bar_id != 0) {
     Shelf* shelf = shelf_view_->shelf();
-    ResourceBundle* rb = &ResourceBundle::GetSharedInstance();
-    gfx::ImageSkia image = *rb->GetImageNamed(bar_id).ToImageSkia();
+    gfx::ImageSkia image;
+    if (MaterialDesignController::IsMaterial()) {

[tdanderson, 2016/05/02 17:58:35]

varkha@, side comment: should we consider giving the Ash MDC a different class
name? After reading line 497 I found that I had to scroll to the #includes to
check that we were using the Ash version and not the top chrome one.

[varkha, 2016/05/02 18:13:58]

Would using explicit namespace here help? I think I have seen examples like
that.

[tdanderson, 2016/05/02 18:19:55]

Yes, though since there is no way to force all future CLs to do that.

[yiyix, 2016/05/03 18:55:34]

I added ash for this cl.

+      gfx::Canvas canvas(gfx::Size(kShelfIconHeight, kShelfIconWidth),
+                         kShelfIconHeight, true /* is_opaque */);
+      GetIndicatorImage(canvas);
+      image = gfx::ImageSkia(canvas.ExtractImageRep());
+

[tdanderson, 2016/05/02 17:58:35]

nit: remove blank line

[yiyix, 2016/05/03 18:55:34]

Done.

+    } else {
+      ResourceBundle* rb = &ResourceBundle::GetSharedInstance();
+      image = *rb->GetImageNamed(bar_id).ToImageSkia();
+    }
     if (!shelf->IsHorizontalAlignment()) {
       image = gfx::ImageSkiaOperations::CreateRotatedImage(
           image, shelf->alignment() == wm::SHELF_ALIGNMENT_LEFT
@@ -503,8 +519,16 @@ void ShelfButton::UpdateBar() {
         views::ImageView::CENTER));
     bar_->SchedulePaint();
   }
-
   bar_->SetVisible(bar_id != 0 && state_ != STATE_NORMAL);
 }
 
+void ShelfButton::GetIndicatorImage(gfx::Canvas& canvas) {

[tdanderson, 2016/05/02 17:58:35]

nit: the Google C++ style guide states that reference parameters should always
be const (https://google.github.io/styleguide/cppguide.html#Reference_Arguments)

[yiyix, 2016/05/03 18:55:34]

Since I am drawing a circle on this canvas, it is same as the String *out in the
guide. I updated gfx::Canvas& canvas to gfx::Canvas *canvas.

+  SkPaint paint;
+  paint.setColor(kIndicatorColor);
+  int pad_vertical = kIconInsideVertical + kIndicatorRadius;

[tdanderson, 2016/05/02 17:58:35]

extreme nit, possibly personal preference: declare |pad_vertical| to be const
since it is a local variable which is never mutated.

[varkha, 2016/05/02 18:13:58]

nit: I would just remove the pad_vertical and inline in the invocation of
DrawCircle below.

[yiyix, 2016/05/03 18:55:34]

Done.

+  canvas.DrawCircle(
+      gfx::Point(kShelfIconWidth / 2, kShelfIconHeight - pad_vertical),

[varkha, 2016/05/02 18:13:59]

Would it be safer to use canvas properties instead of the constants? This way
the constants are only used in one place to size the canvas.

[yiyix, 2016/05/03 18:55:34]

Since it is harder to get these properties from canvas and the size does not
change from the time we create the Canvas to drawing the circle on it, I will
pass the gfx::size to this function so it does not look like random constants
from the code. 

+      kIndicatorRadius, paint);
+}
+
 }  // namespace ash