Vectorize download shelf progress indicators

Vectorize download shelf progress indicators

BUG=505953
TBR=oshima@chromium.org

Committed: https://crrev.com/6ef02ec8fdd093a5c993c93b6938287eced4772b
Cr-Commit-Position: refs/heads/master@{#340328}

[Evan Stade, 2015-07-10 22:20:55]

estade@chromium.org changed reviewers:
+ asanka@chromium.org, sadrul@chromium.org

[Evan Stade, 2015-07-10 22:22:17]

+sadrul for views
+asanka for cocoa

Note that I haven't actually tried this on mac, but it does work on views in RTL
and LTR.

The appearance is nearly identical to the pngs, and has been {sgabriel,
andrestubbe}-approved.

[Evan Stade, 2015-07-10 22:24:02]

estade@chromium.org changed reviewers:
+ pkasting@chromium.org
- sadrul@chromium.org

[Evan Stade, 2015-07-10 22:24:02]

got the owners files wrong. -sadrul, +pkasting for views/

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

LGTM

https://codereview.chromium.org/1236463002/diff/80001/chrome/browser/download...
File chrome/browser/download/download_shelf.cc (right):

https://codereview.chromium.org/1236463002/diff/80001/chrome/browser/download...
chrome/browser/download/download_shelf.cc:79:
bg_paint.setColor(SkColorSetRGB(0xD7, 0xE4, 0xFA));
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.

https://codereview.chromium.org/1236463002/diff/80001/chrome/browser/download...
chrome/browser/download/download_shelf.cc:82: bg.addCircle(20, 20, 13);
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]

https://codereview.chromium.org/1236463002/diff/80001/chrome/browser/download...
File chrome/browser/download/download_shelf.cc (right):

https://codereview.chromium.org/1236463002/diff/80001/chrome/browser/download...
chrome/browser/download/download_shelf.cc:79:
bg_paint.setColor(SkColorSetRGB(0xD7, 0xE4, 0xFA));
On 2015/07/10 23:53:34, Peter Kasting wrote:
> 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.

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).

https://codereview.chromium.org/1236463002/diff/80001/chrome/browser/download...
chrome/browser/download/download_shelf.cc:82: bg.addCircle(20, 20, 13);
On 2015/07/10 23:53:34, Peter Kasting wrote:
> 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?

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]

https://codereview.chromium.org/1236463002/diff/80001/chrome/browser/download...
File chrome/browser/download/download_shelf.cc (right):

https://codereview.chromium.org/1236463002/diff/80001/chrome/browser/download...
chrome/browser/download/download_shelf.cc:79:
bg_paint.setColor(SkColorSetRGB(0xD7, 0xE4, 0xFA));
On 2015/07/11 at 00:18:34, Evan Stade wrote:
> On 2015/07/10 23:53:34, Peter Kasting wrote:
> > 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.
> 
> 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).

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.

https://codereview.chromium.org/1236463002/diff/80001/chrome/browser/download...
chrome/browser/download/download_shelf.cc:82: bg.addCircle(20, 20, 13);
On 2015/07/11 at 00:18:35, Evan Stade wrote:
> On 2015/07/10 23:53:34, Peter Kasting wrote:
> > 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?
> 
> 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.

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:05]

ping asanka

https://codereview.chromium.org/1236463002/diff/80001/chrome/browser/download...
File chrome/browser/download/download_shelf.cc (right):

https://codereview.chromium.org/1236463002/diff/80001/chrome/browser/download...
chrome/browser/download/download_shelf.cc:79:
bg_paint.setColor(SkColorSetRGB(0xD7, 0xE4, 0xFA));
On 2015/07/11 07:03:25, Peter Kasting wrote:
> On 2015/07/11 at 00:18:34, Evan Stade wrote:
> > On 2015/07/10 23:53:34, Peter Kasting wrote:
> > > 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.
> > 
> > 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).
> 
> 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.

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.

https://codereview.chromium.org/1236463002/diff/80001/chrome/browser/download...
chrome/browser/download/download_shelf.cc:82: bg.addCircle(20, 20, 13);
On 2015/07/11 07:03:25, Peter Kasting wrote:
> On 2015/07/11 at 00:18:35, Evan Stade wrote:
> > On 2015/07/10 23:53:34, Peter Kasting wrote:
> > > 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?
> > 
> > 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.
> 
> Well, kSMallProgressIconSize wasn't implicit in the png,

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

[Peter Kasting, 2015-07-14 01:25:23]

https://codereview.chromium.org/1236463002/diff/80001/chrome/browser/download...
File chrome/browser/download/download_shelf.cc (right):

https://codereview.chromium.org/1236463002/diff/80001/chrome/browser/download...
chrome/browser/download/download_shelf.cc:79:
bg_paint.setColor(SkColorSetRGB(0xD7, 0xE4, 0xFA));
On 2015/07/13 at 23:25:04, Evan Stade wrote:
> On 2015/07/11 07:03:25, Peter Kasting wrote:
> > On 2015/07/11 at 00:18:34, Evan Stade wrote:
> > > On 2015/07/10 23:53:34, Peter Kasting wrote:
> > > > 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.
> > > 
> > > 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).
> > 
> > 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.
> 
> 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.

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 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.

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).

https://codereview.chromium.org/1236463002/diff/80001/chrome/browser/download...
chrome/browser/download/download_shelf.cc:82: bg.addCircle(20, 20, 13);
On 2015/07/13 at 23:25:04, Evan Stade wrote:
> On 2015/07/11 07:03:25, Peter Kasting wrote:
> > On 2015/07/11 at 00:18:35, Evan Stade wrote:
> > > On 2015/07/10 23:53:34, Peter Kasting wrote:
> > > > 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?
> > > 
> > > 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.
> > 
> > Well, kSMallProgressIconSize wasn't implicit in the png,
> 
> Yes, it was the width/height of the png.

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.

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

https://codereview.chromium.org/1236463002/diff/80001/chrome/browser/download...
File chrome/browser/download/download_shelf.cc (right):

https://codereview.chromium.org/1236463002/diff/80001/chrome/browser/download...
chrome/browser/download/download_shelf.cc:79:
bg_paint.setColor(SkColorSetRGB(0xD7, 0xE4, 0xFA));
On 2015/07/14 01:25:22, Peter Kasting wrote:
> On 2015/07/13 at 23:25:04, Evan Stade wrote:
> > On 2015/07/11 07:03:25, Peter Kasting wrote:
> > > On 2015/07/11 at 00:18:34, Evan Stade wrote:
> > > > On 2015/07/10 23:53:34, Peter Kasting wrote:
> > > > > 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.
> > > > 
> > > > 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).
> > > 
> > > 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.
> > 
> > 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.
> 
> 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.

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 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.
> 
> 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).

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.

[Evan Stade, 2015-07-14 16:55:14]

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

[Evan Stade, 2015-07-14 16:55:15]

+thakis for cocoa

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

https://codereview.chromium.org/1236463002/diff/80001/chrome/browser/download...
File chrome/browser/download/download_shelf.cc (right):

https://codereview.chromium.org/1236463002/diff/80001/chrome/browser/download...
chrome/browser/download/download_shelf.cc:79:
bg_paint.setColor(SkColorSetRGB(0xD7, 0xE4, 0xFA));
On 2015/07/14 at 01:52:18, Evan Stade wrote:
> 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?

Done.

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

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.

[asanka, 2015-07-14 18:47:58]

Sorry about the delay. /cocoa/ lgtm

[Evan Stade, 2015-07-15 23:33:34]

estade@chromium.org changed reviewers:
+ sadrul@chromium.org
- thakis@chromium.org

[Evan Stade, 2015-07-15 23:33:35]

After discussion with Sebastien, the colors are changed slightly. It should now
match the tab loading indicator.

pkasting, could you re-review chrome/browser/ui/?
asanka, could you re-review cocoa?
Compare ps8 to ps6.

+sadrul, could you review the one line change in ui/native_theme/?

https://codereview.chromium.org/1236463002/diff/140001/ui/native_theme/common...
File ui/native_theme/common_theme.cc (right):

https://codereview.chromium.org/1236463002/diff/140001/ui/native_theme/common...
ui/native_theme/common_theme.cc:43: const SkColor kThrobberSpinningColor =
SkColorSetRGB(0x42, 0x85, 0xF4);
this is now Google Blue

[sadrul, 2015-07-15 23:38:53]

lgtm

[asanka, 2015-07-16 00:45:08]

lgtm

https://codereview.chromium.org/1236463002/diff/120001/chrome/browser/downloa...
File chrome/browser/download/download_shelf.h (right):

https://codereview.chromium.org/1236463002/diff/120001/chrome/browser/downloa...
chrome/browser/download/download_shelf.h:66: // know the total size, so we just
draw a rotating segment until we're done.
Nit: Mention that |progress_start_time| is only needed if percent is -1.

[Peter Kasting, 2015-07-16 00:52:18]

Better!

https://codereview.chromium.org/1236463002/diff/160001/chrome/browser/downloa...
File chrome/browser/download/download_shelf.cc (right):

https://codereview.chromium.org/1236463002/diff/160001/chrome/browser/downloa...
chrome/browser/download/download_shelf.cc:83:
theme_provider.GetColor(ThemeProperties::COLOR_THROBBER_SPINNING);
Seems like maybe we should rename this color constant, though I don't know if
there's a good name ("indicator stroke color"?).

https://codereview.chromium.org/1236463002/diff/160001/chrome/browser/downloa...
chrome/browser/download/download_shelf.cc:87: bg.addCircle(20, 20, 13);
Looking at all this code, it still seems like either the circle radius should be
12.5 instead of 13, or the progress arc rect should have w/h 26 instead of 25. 
Otherwise, the rect used inside SkPath::addCircle() doesn't match the rect used
for addArc() below.  I would guess the circle radius is what needs changing
rather than the progress arc rect, as if the arc rect changes that also suggests
that the "progress icon size" constant is wrong.

If we can bring these into alignment, then we can replace all the numbers here,
below, and in the "progress icon size" constant with just:

kProgressIndicatorRadius = 13;  // Or 12.5
kProgressIndicatorPadding = 7;

Then everything else can just be computed by adding those together and
multiplying by 2 where necessary.

[Evan Stade, 2015-07-16 02:16:44]

https://codereview.chromium.org/1236463002/diff/120001/chrome/browser/downloa...
File chrome/browser/download/download_shelf.h (right):

https://codereview.chromium.org/1236463002/diff/120001/chrome/browser/downloa...
chrome/browser/download/download_shelf.h:66: // know the total size, so we just
draw a rotating segment until we're done.
On 2015/07/16 00:45:08, asanka wrote:
> Nit: Mention that |progress_start_time| is only needed if percent is -1.

Done.

https://codereview.chromium.org/1236463002/diff/160001/chrome/browser/downloa...
File chrome/browser/download/download_shelf.cc (right):

https://codereview.chromium.org/1236463002/diff/160001/chrome/browser/downloa...
chrome/browser/download/download_shelf.cc:83:
theme_provider.GetColor(ThemeProperties::COLOR_THROBBER_SPINNING);
On 2015/07/16 00:52:17, Peter Kasting wrote:
> Seems like maybe we should rename this color constant, though I don't know if
> there's a good name ("indicator stroke color"?).

let me know if you think of a name you'd prefer. COLOR_PROGRESS_INDICATOR seems
the most accurate but I'm ambivalent about it.

https://codereview.chromium.org/1236463002/diff/160001/chrome/browser/downloa...
chrome/browser/download/download_shelf.cc:87: bg.addCircle(20, 20, 13);
On 2015/07/16 00:52:17, Peter Kasting wrote:
> Looking at all this code, it still seems like either the circle radius should
be
> 12.5 instead of 13, or the progress arc rect should have w/h 26 instead of 25.

> Otherwise, the rect used inside SkPath::addCircle() doesn't match the rect
used
> for addArc() below.  I would guess the circle radius is what needs changing
> rather than the progress arc rect, as if the arc rect changes that also
suggests
> that the "progress icon size" constant is wrong.
> 
> If we can bring these into alignment, then we can replace all the numbers
here,
> below, and in the "progress icon size" constant with just:
> 
> kProgressIndicatorRadius = 13;  // Or 12.5
> kProgressIndicatorPadding = 7;
> 
> Then everything else can just be computed by adding those together and
> multiplying by 2 where necessary.

constants tweaked

[Peter Kasting, 2015-07-16 20:02:24]

LGTM with some suggestions for constant names/comments

https://codereview.chromium.org/1236463002/diff/200001/chrome/browser/downloa...
File chrome/browser/download/download_shelf.h (right):

https://codereview.chromium.org/1236463002/diff/200001/chrome/browser/downloa...
chrome/browser/download/download_shelf.h:57: kSmallIconSize = 16,
Nit: kFiletypeIconSize

https://codereview.chromium.org/1236463002/diff/200001/chrome/browser/downloa...
chrome/browser/download/download_shelf.h:60: kSmallProgressIconSize = 39,
Nit:

    // Size of the overall progress indicator, including padding.
    kProgressIndicatorSize = 39,

https://codereview.chromium.org/1236463002/diff/200001/chrome/browser/downloa...
chrome/browser/download/download_shelf.h:62: kSmallProgressIconOffset =
(kSmallProgressIconSize - kSmallIconSize) / 2
Nit: If we want to keep this, I'd name it kFiletypeIconSize.  However, it may
make more sense to just compute it where needed.

[Evan Stade, 2015-07-22 16:39:46]

https://codereview.chromium.org/1236463002/diff/200001/chrome/browser/downloa...
File chrome/browser/download/download_shelf.h (right):

https://codereview.chromium.org/1236463002/diff/200001/chrome/browser/downloa...
chrome/browser/download/download_shelf.h:57: kSmallIconSize = 16,
On 2015/07/16 20:02:23, Peter Kasting wrote:
> Nit: kFiletypeIconSize

was able to delete this one

https://codereview.chromium.org/1236463002/diff/200001/chrome/browser/downloa...
chrome/browser/download/download_shelf.h:60: kSmallProgressIconSize = 39,
On 2015/07/16 20:02:23, Peter Kasting wrote:
> Nit:
> 
>     // Size of the overall progress indicator, including padding.
>     kProgressIndicatorSize = 39,

Done.

https://codereview.chromium.org/1236463002/diff/200001/chrome/browser/downloa...
chrome/browser/download/download_shelf.h:62: kSmallProgressIconOffset =
(kSmallProgressIconSize - kSmallIconSize) / 2
On 2015/07/16 20:02:23, Peter Kasting wrote:
> Nit: If we want to keep this, I'd name it kFiletypeIconSize.  However, it may
> make more sense to just compute it where needed.

Done.

[Evan Stade, 2015-07-22 16:39:52]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-07-22 16:39:52]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
asanka@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1236463002/#ps220001
(title: "names")

[commit-bot: I haz the power, 2015-07-22 16:40:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1236463002/220001

[Evan Stade, 2015-07-22 16:40:47]

The CQ bit was unchecked by estade@chromium.org

[Evan Stade, 2015-07-22 17:34:43]

oops, thought I already committed this. Mac unit tests have an issue. asanka@,
can you review the last little change?

https://codereview.chromium.org/1236463002/diff/240001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/download/download_item_cell.mm (right):

https://codereview.chromium.org/1236463002/diff/240001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/download/download_item_cell.mm:563: themeProvider =
&defaultTheme;
theme provider can be null during tests

[asanka, 2015-07-22 21:22:17]

https://codereview.chromium.org/1236463002/diff/240001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/download/download_item_cell.mm (right):

https://codereview.chromium.org/1236463002/diff/240001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/download/download_item_cell.mm:563: themeProvider =
&defaultTheme;
On 2015/07/22 at 17:34:43, Evan Stade wrote:
> theme provider can be null during tests

Can we instead mock the themeProvider selector on the test window in the
affected tests?

[Evan Stade, 2015-07-22 21:32:20]

https://codereview.chromium.org/1236463002/diff/240001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/download/download_item_cell.mm (right):

https://codereview.chromium.org/1236463002/diff/240001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/download/download_item_cell.mm:563: themeProvider =
&defaultTheme;
On 2015/07/22 21:22:17, asanka wrote:
> On 2015/07/22 at 17:34:43, Evan Stade wrote:
> > theme provider can be null during tests
> 
> Can we instead mock the themeProvider selector on the test window in the
> affected tests?

That would be nice, except I don't have a mac build handy so tinkering with that
would be difficult.

[Evan Stade, 2015-07-23 17:25:46]

On 2015/07/22 21:32:20, Evan Stade wrote:
>
https://codereview.chromium.org/1236463002/diff/240001/chrome/browser/ui/coco...
> File chrome/browser/ui/cocoa/download/download_item_cell.mm (right):
> 
>
https://codereview.chromium.org/1236463002/diff/240001/chrome/browser/ui/coco...
> chrome/browser/ui/cocoa/download/download_item_cell.mm:563: themeProvider =
> &defaultTheme;
> On 2015/07/22 21:22:17, asanka wrote:
> > On 2015/07/22 at 17:34:43, Evan Stade wrote:
> > > theme provider can be null during tests
> > 
> > Can we instead mock the themeProvider selector on the test window in the
> > affected tests?
> 
> That would be nice, except I don't have a mac build handy so tinkering with
that
> would be difficult.

ping

[asanka, 2015-07-23 23:29:34]

On 2015/07/23 at 17:25:46, estade wrote:
> On 2015/07/22 21:32:20, Evan Stade wrote:
> >
https://codereview.chromium.org/1236463002/diff/240001/chrome/browser/ui/coco...
> > File chrome/browser/ui/cocoa/download/download_item_cell.mm (right):
> > 
> >
https://codereview.chromium.org/1236463002/diff/240001/chrome/browser/ui/coco...
> > chrome/browser/ui/cocoa/download/download_item_cell.mm:563: themeProvider =
> > &defaultTheme;
> > On 2015/07/22 21:22:17, asanka wrote:
> > > On 2015/07/22 at 17:34:43, Evan Stade wrote:
> > > > theme provider can be null during tests
> > > 
> > > Can we instead mock the themeProvider selector on the test window in the
> > > affected tests?
> > 
> > That would be nice, except I don't have a mac build handy so tinkering with
that
> > would be difficult.
> 
> ping

lgtm

[Evan Stade, 2015-07-24 17:49:39]

On 2015/07/23 23:29:34, asanka wrote:
> On 2015/07/23 at 17:25:46, estade wrote:
> > On 2015/07/22 21:32:20, Evan Stade wrote:
> > >
>
https://codereview.chromium.org/1236463002/diff/240001/chrome/browser/ui/coco...
> > > File chrome/browser/ui/cocoa/download/download_item_cell.mm (right):
> > > 
> > >
>
https://codereview.chromium.org/1236463002/diff/240001/chrome/browser/ui/coco...
> > > chrome/browser/ui/cocoa/download/download_item_cell.mm:563: themeProvider
=
> > > &defaultTheme;
> > > On 2015/07/22 21:22:17, asanka wrote:
> > > > On 2015/07/22 at 17:34:43, Evan Stade wrote:
> > > > > theme provider can be null during tests
> > > > 
> > > > Can we instead mock the themeProvider selector on the test window in the
> > > > affected tests?
> > > 
> > > That would be nice, except I don't have a mac build handy so tinkering
with
> that
> > > would be difficult.
> > 
> > ping
> 
> lgtm

thanks!

[Evan Stade, 2015-07-24 17:49:43]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-07-24 17:49:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/1236463002/#ps240001
(title: "fix mac?")

[commit-bot: I haz the power, 2015-07-24 17:49:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1236463002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1236463002/240001

[commit-bot: I haz the power, 2015-07-24 18:00:24]

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

[commit-bot: I haz the power, 2015-07-24 18:00:25]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Evan Stade, 2015-07-24 20:44:35]

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

[Evan Stade, 2015-07-24 20:44:36]

+oshima tbr for chrome/app/theme

[Evan Stade, 2015-07-24 20:44:54]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2015-07-24 20:45:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1236463002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1236463002/240001

[commit-bot: I haz the power, 2015-07-24 20:52:17]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-07-24 20:53:13]

Patchset 13 (id:??) landed as
https://crrev.com/6ef02ec8fdd093a5c993c93b6938287eced4772b
Cr-Commit-Position: refs/heads/master@{#340328}

[oshima, 2015-07-24 20:54:36]

c/a/t lgtm