Vectorize download shelf progress indicators

chrome/browser/download/download_shelf.cc

Index: chrome/browser/download/download_shelf.cc
diff --git a/chrome/browser/download/download_shelf.cc b/chrome/browser/download/download_shelf.cc
index d120fe2e3270e5a7051eb5def0e01589e0060ef3..ae8e37d70397bd3a486694d8c1aecc9eb4fb260e 100644
--- a/chrome/browser/download/download_shelf.cc
+++ b/chrome/browser/download/download_shelf.cc
@@ -14,6 +14,7 @@
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/thread_task_runner_handle.h"
+#include "base/time/time.h"
 #include "chrome/browser/download/download_item_model.h"
 #include "chrome/browser/download/download_service.h"
 #include "chrome/browser/download/download_service_factory.h"
@@ -28,11 +29,12 @@
 #include "content/public/browser/download_manager.h"
 #include "content/public/browser/web_contents.h"
 #include "grit/theme_resources.h"
+#include "third_party/skia/include/core/SkPaint.h"
+#include "third_party/skia/include/core/SkPath.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/animation/animation.h"
 #include "ui/gfx/canvas.h"
-#include "ui/gfx/image/image_skia.h"
 
 using content::DownloadItem;
 
@@ -66,115 +68,63 @@ DownloadShelf::~DownloadShelf() {}
 
 // Download progress painting --------------------------------------------------
 
-// Common images used for download progress animations. We load them once the
-// first time we do a progress paint, then reuse them as they are always the
-// same.
-gfx::ImageSkia* g_foreground_16 = NULL;
-gfx::ImageSkia* g_background_16 = NULL;
-
 // static
 void DownloadShelf::PaintDownloadProgress(
     gfx::Canvas* canvas,
-    const BoundsAdjusterCallback& rtl_mirror,
-    int origin_x,
-    int origin_y,
-    int start_angle,
+    const base::TimeTicks& progress_start_time,
     int percent_done) {
-  // Load up our common images.
-  if (!g_background_16) {
-    ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
-    g_foreground_16 = rb.GetImageSkiaNamed(IDR_DOWNLOAD_PROGRESS_FOREGROUND_16);
-    g_background_16 = rb.GetImageSkiaNamed(IDR_DOWNLOAD_PROGRESS_BACKGROUND_16);
-    DCHECK_EQ(g_foreground_16->width(), g_background_16->width());
-    DCHECK_EQ(g_foreground_16->height(), g_background_16->height());
-  }
-
-  // We start by storing the bounds of the images so that it is easy to mirror
-  // the bounds if the UI layout is RTL.
-  gfx::Rect bounds(origin_x, origin_y, g_background_16->width(),
-                   g_background_16->height());
-
-  // Mirror the positions if necessary.
-  rtl_mirror.Run(&bounds);
-
-  // Draw the background progress image.
-  canvas->DrawImageInt(*g_background_16, bounds.x(), bounds.y());
-
-  // Layer the foreground progress image in an arc proportional to the download
-  // progress. The arc grows clockwise, starting in the midnight position, as
-  // the download progresses. However, if the download does not have known total
-  // size (the server didn't give us one), then we just spin an arc around until
-  // we're done.
-  float sweep_angle = 0.0;
-  float start_pos = static_cast<float>(kStartAngleDegrees);
+  // Draw background (light blue circle).
+  SkPaint bg_paint;
+  bg_paint.setStyle(SkPaint::kFill_Style);
+  bg_paint.setColor(SkColorSetRGB(0xD7, 0xE4, 0xFA));

[Peter Kasting, 2015/07/10 23:53:34]

Any way to use an existing theme color, or compute this from a combination of
such colors?  (Maybe sgabriel could provide a suggestion on a color computation
method they'd be OK with.)

Same comment on the sweep color below.

[Evan Stade, 2015/07/11 00:18:34]

This CL certainly makes it easier to theme this color if we wanted to do so, but
that's not the main intent. For now, the goal is maintaining the status quo in
terms of appearance (aside from making the animation smoother).

[Peter Kasting, 2015/07/11 07:03:25]

I think our goal should be to Do It Right, which may mean changing the
appearance.  At the very least it probably means sticking this constant in the
theme color list somewhere; someone coming along later is not going to find this
random color constant inserted in the middle of some code.  But I think it would
be better to take the additional time and find out from the designers what we
should do regarding unifying these assets' colors in the theming constants.

[Evan Stade, 2015/07/13 23:25:04]

The overall goal is to do it right. The goal of each individual CL is to move
closer to the overall goal. This CL moves us closer.

I don't think we should put colors in a themey location if they aren't actually
shared or generated. Given that chrome/browser/ui/ has over 50 examples of
hardcoded colors, I think there's precedent for this.

[Peter Kasting, 2015/07/14 01:25:22]

That's fine, if we had a concrete plan for how we were going to clean this up
right after landing this.  But I don't think there is one, which means this is
likely going to be orphaned here forever, and I'm not OK with that.  Let's not
add to the existing 50 likely-wrong instances you mention.
I'm going to bet that at least 90% of those shouldn't be done that way.  (I'd
say 100% but there's probably some dumb reason why a couple of them really
should be hardcoded.)  It's easy for these to creep in because people are
constantly just littering hardcoded colors all over, but it's not usually
correct, and I fight against it in reviews.  So don't take the existence of
these as precedent that this is OK.

There is likely already a set of theme colors today that we could use for these,
and almost certainly in the future we'll want to unify these with other theme
colors.  Just ask Sebastien if there are some appropriate colors to change to
now, and if not if he can make sure to consider these for the MD redesign (and
in the meantime until then we can just stick these in the theme color list).

[Evan Stade, 2015/07/14 01:52:18]

You're right, I am not promising to fix it right after this CL lands. But this
color already was orphaned (inside the .png) so un-orphaning could only be made
easier by this CL, if someone cared enough to do that.
I tried to write an email to Sebastien, but I'm not sure how to word it because
I don't really know what you're asking for. Can you write it to him so I don't
misrepresent your intent?

I don't see any advantage in hardcoding the color in a far-away file instead of
here. I only see disadvantages to that.

[Peter Kasting, 2015/07/14 18:44:33]

Done.
The advantages of collecting the various different colors in our UI in one place
are:
(1) We have a reference of all the colors we use, so if we redo the palette or
feel of the UI (as the MD work is doing), we don't miss anything, but rather
know upfront everything from the previous UI.
(2) If we see we have multiple matching colors, or colors that are near to each
other in value, it clues us in that we can likely combine them.  What if, for
example, the colors in this PNG matched the colors in some PNG in the history
UI?  That would likely be an indicator that we wanted to keep them in sync, but
if both pieces of code hardcode the color locally, it's unlikely that someone
maintaining either place will notice the other.

If after this you still don't feel these are sufficient reasons, do it the way
you'd like.

+  bg_paint.setAntiAlias(true);
+  SkPath bg;
+  bg.addCircle(20, 20, 13);

[Peter Kasting, 2015/07/10 23:53:34]

Can we compute this value, the progress arc values, or both from
kSmallProgressIconSize?  Or change the constants involved here such that somehow
all three of these are described by as few constants as possible?

[Evan Stade, 2015/07/11 00:18:35]

I don't know how we would do that. We could do something like
kSmallProgressIconSize - x or kSmallProgressIconSize * x, but that doesn't lead
to any fewer constants.

The progress arc values could potentially re-use these values, but again it
would be something like

[circle radius] * 2 - 1

which doesn't actually reduce the number of constants or make more intuitive
code (why -1?). Basically, all these constants are hand-tweaked and there's no
real logic behind them. They were previously implicit in the .png asset.

[Peter Kasting, 2015/07/11 07:03:25]

Well, kSMallProgressIconSize wasn't implicit in the png, and still isn't; you're
still using it.  It seems to have some relationship to the sizes you're drawing.
 What is that relationship?

[Evan Stade, 2015/07/13 23:25:04]

Yes, it was the width/height of the png.

[Peter Kasting, 2015/07/14 01:25:22]

It seems like this is related to the (20, 20) position for the center of the
circle, then.  I would think we'd want this value to be 2x whatever offset we
use to horizontally center the circle; either that means leave the 20px center
offset and change this to effectively be 40 (if previously the padding was
unequal by 1 px), or move the center of the circle to be at 19.5 instead of 20. 
(If doing 2x-1 actually results in equal padding, then we should do that, but
with a comment about why the -1 is needed.)

In any case, we can compute one value from the other.

+  canvas->DrawPath(bg, bg_paint);
+
+  // Calculate progress.
+  SkScalar sweep_angle = 0.f;
+  // Start at 12 o'clock.
+  SkScalar start_pos = SkIntToScalar(270);
   if (percent_done < 0) {
-    sweep_angle = kUnknownAngleDegrees;
-    start_pos = static_cast<float>(start_angle);
+    // For unknown size downloads, draw a 50 degree sweep that moves at
+    // 0.08 degrees per millisecond.
+    sweep_angle = 50.f;
+    start_pos += static_cast<SkScalar>(
+        (base::TimeTicks::Now() - progress_start_time).InMilliseconds() * 0.08);
   } else if (percent_done > 0) {
-    sweep_angle = static_cast<float>(kMaxDegrees / 100.0 * percent_done);
+    sweep_angle = static_cast<SkScalar>(360 * percent_done / 100.0);
   }
 
-  // Set up an arc clipping region for the foreground image. Don't bother using
-  // a clipping region if it would round to 360 (really 0) degrees, since that
-  // would eliminate the foreground completely and be quite confusing (it would
-  // look like 0% complete when it should be almost 100%).
-  canvas->Save();
-  if (sweep_angle < static_cast<float>(kMaxDegrees - 1)) {
-    SkRect oval;
-    oval.set(SkIntToScalar(bounds.x()), SkIntToScalar(bounds.y()),
-             SkIntToScalar(bounds.x() + DownloadShelf::kSmallProgressIconSize),
-             SkIntToScalar(bounds.y() + DownloadShelf::kSmallProgressIconSize));
-    SkPath path;
-    path.arcTo(oval,
-               SkFloatToScalar(start_pos),
-               SkFloatToScalar(sweep_angle), false);
-    path.lineTo(
-        SkIntToScalar(bounds.x() + DownloadShelf::kSmallProgressIconSize / 2),
-        SkIntToScalar(bounds.y() + DownloadShelf::kSmallProgressIconSize / 2));
-
-    // gfx::Canvas::ClipPath does not provide for anti-aliasing.
-    canvas->sk_canvas()->clipPath(path, SkRegion::kIntersect_Op, true);
-  }
-
-  canvas->DrawImageInt(*g_foreground_16, bounds.x(), bounds.y());
-  canvas->Restore();
+  // Draw progress.
+  SkPath progress;
+  progress.addArc(SkRect::MakeXYWH(7, 7, 25, 25), start_pos, sweep_angle);
+  SkPaint progress_paint;
+  progress_paint.setColor(SkColorSetRGB(0x41, 0x84, 0xF4));
+  progress_paint.setStyle(SkPaint::kStroke_Style);
+  progress_paint.setStrokeWidth(1.7);
+  progress_paint.setAntiAlias(true);
+  canvas->DrawPath(progress, progress_paint);
 }
 
 // static
 void DownloadShelf::PaintDownloadComplete(
     gfx::Canvas* canvas,
-    const BoundsAdjusterCallback& rtl_mirror,
-    int origin_x,
-    int origin_y,
     double animation_progress) {
-  // Load up our common images.
-  if (!g_foreground_16) {
-    ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
-    g_foreground_16 = rb.GetImageSkiaNamed(IDR_DOWNLOAD_PROGRESS_FOREGROUND_16);
-  }
-
-  gfx::Rect complete_bounds(origin_x, origin_y, g_foreground_16->width(),
-                            g_foreground_16->height());
-  // Mirror the positions if necessary.
-  rtl_mirror.Run(&complete_bounds);
-
   // Start at full opacity, then loop back and forth five times before ending
   // at zero opacity.
-  canvas->DrawImageInt(*g_foreground_16, complete_bounds.x(),
-                       complete_bounds.y(), GetOpacity(animation_progress));
+  canvas->sk_canvas()->saveLayerAlpha(nullptr, GetOpacity(animation_progress));
+  PaintDownloadProgress(canvas, base::TimeTicks(), 100);
+  canvas->sk_canvas()->restore();
 }
 
 // static
 void DownloadShelf::PaintDownloadInterrupted(
     gfx::Canvas* canvas,
-    const BoundsAdjusterCallback& rtl_mirror,
-    int origin_x,
-    int origin_y,
     double animation_progress) {
   // Start at zero opacity, then loop back and forth five times before ending
   // at full opacity.
-  PaintDownloadComplete(canvas, rtl_mirror, origin_x, origin_y,
-                        1.0 - animation_progress);
+  PaintDownloadComplete(canvas, 1.0 - animation_progress);
 }
 
 void DownloadShelf::AddDownload(DownloadItem* download) {