Downloads bar for MD: work on sizing, text appearance.

Downloads bar for MD: work on sizing, text appearance.

BUG=505804
Committed: https://crrev.com/cf31d9128175cfbaed7ef624dad6a5b22c728d04
Cr-Commit-Position: refs/heads/master@{#350623}

[Evan Stade, 2015-09-23 22:29:16]

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

[sky, 2015-09-23 23:42:27]

https://codereview.chromium.org/1365763002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view_md.cc (right):

https://codereview.chromium.org/1365763002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.cc:70: // All values in
dp.
do we really use dp? I thought it was dips.

https://codereview.chromium.org/1365763002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.cc:323: 2 *
kMinimumVerticalPadding + font_list_.GetBaseline() +
Why are you using baseline instead of height?

https://codereview.chromium.org/1365763002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view_md.h (right):

https://codereview.chromium.org/1365763002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.h:140: // the file name
text relative to local bounds.
nit: fit moar text on previous line.

[Evan Stade, 2015-09-24 00:34:26]

https://codereview.chromium.org/1365763002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view_md.cc (right):

https://codereview.chromium.org/1365763002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.cc:70: // All values in
dp.
On 2015/09/23 23:42:27, sky wrote:
> do we really use dp? I thought it was dips.

I thought "dp" and "dip" were synonyms. Sources online say

Density-independent Pixels (“dip” or “dp” or “dps”)

https://codereview.chromium.org/1365763002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.cc:323: 2 *
kMinimumVerticalPadding + font_list_.GetBaseline() +
On 2015/09/23 23:42:27, sky wrote:
> Why are you using baseline instead of height?

The specs use the baseline for alignment, e.g. aligning the baseline to the
bottom of the progress indicator or measuring the distance between the lines of
text.

https://codereview.chromium.org/1365763002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view_md.h (right):

https://codereview.chromium.org/1365763002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.h:140: // the file name
text relative to local bounds.
On 2015/09/23 23:42:27, sky wrote:
> nit: fit moar text on previous line.

Done.

[Evan Stade, 2015-09-24 00:34:31]

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

[commit-bot: I haz the power, 2015-09-24 01:02:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1365763002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1365763002/40001

[commit-bot: I haz the power, 2015-09-24 02:30:34]

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

[commit-bot: I haz the power, 2015-09-24 02:30:34]

Dry run: This issue passed the CQ dry run.

[sky, 2015-09-24 15:05:53]

https://codereview.chromium.org/1365763002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view_md.cc (right):

https://codereview.chromium.org/1365763002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.cc:323: 2 *
kMinimumVerticalPadding + font_list_.GetBaseline() +
On 2015/09/24 00:34:26, Evan Stade wrote:
> On 2015/09/23 23:42:27, sky wrote:
> > Why are you using baseline instead of height?
> 
> The specs use the baseline for alignment, e.g. aligning the baseline to the
> bottom of the progress indicator or measuring the distance between the lines
of
> text.

For alignment, sure, but in calculating the preferred height if you don't
consider descenders then they might get chopped off, right?

[Evan Stade, 2015-09-24 17:28:03]

https://codereview.chromium.org/1365763002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view_md.cc (right):

https://codereview.chromium.org/1365763002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.cc:323: 2 *
kMinimumVerticalPadding + font_list_.GetBaseline() +
On 2015/09/24 15:05:53, sky wrote:
> On 2015/09/24 00:34:26, Evan Stade wrote:
> > On 2015/09/23 23:42:27, sky wrote:
> > > Why are you using baseline instead of height?
> > 
> > The specs use the baseline for alignment, e.g. aligning the baseline to the
> > bottom of the progress indicator or measuring the distance between the lines
> of
> > text.
> 
> For alignment, sure, but in calculating the preferred height if you don't
> consider descenders then they might get chopped off, right?

Hmm, I guess that's fair. I left the filename one on Baseline() because that's
how things are laid out. The worst case scenario is overlap between the lines of
text but I tested at 18pt fonts (which is basically the largest font size that
leaves Chrome usable) and there was still no overlap. The second one is
GetHeight() to make sure the descenders don't run out of space. All of this is
only relevant for unusual font sizes; kDefaultHeight is used normally.

[Evan Stade, 2015-09-24 17:29:10]

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

[commit-bot: I haz the power, 2015-09-24 17:29:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1365763002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1365763002/60001

[sky, 2015-09-24 19:14:49]

LGTM - while you might have noticed descender clipping with english I suspect
different languages may have bigger descenders that would be clipped.

[Evan Stade, 2015-09-24 19:21:49]

On 2015/09/24 19:14:49, sky wrote:
> LGTM - while you might have noticed descender clipping with english I suspect
> different languages may have bigger descenders that would be clipped.

good point, that will probably be troublesome

[Evan Stade, 2015-09-24 19:21:54]

The CQ bit was unchecked by estade@chromium.org

[Evan Stade, 2015-09-24 19:21:54]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2015-09-24 19:22:54]

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

[commit-bot: I haz the power, 2015-09-24 19:47:20]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-09-24 19:48:09]

Patchset 4 (id:??) landed as
https://crrev.com/cf31d9128175cfbaed7ef624dad6a5b22c728d04
Cr-Commit-Position: refs/heads/master@{#350623}