Download bar MD overhaul, part 3

Download bar MD overhaul, part 3

- download item click target takes up the entire vertical space
- move progress indicator padding from implicit (in DownloadShelf) to explicit (in relevant view)
- MD for shelf controls (show all, close buttons)

Unfortunately I did have to touch the existing shelf code, and therefore risk breaking it, but my visual inspection shows no changes to the appearance in LTR or RTL, warning state or normal state.

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

[Evan Stade, 2015-09-25 00:46:47]

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

[Evan Stade, 2015-09-25 00:48:09]

+sky for review
+groby for double checking that OSX doesn't need any updates (since I touched
some shared code). By just reading the code I got the impression OSX didn't need
any updates (pat on the back to whoever wrote that code), but we should check to
be sure. Can you build this patch and let me have a look?

https://codereview.chromium.org/1367083002/diff/1/chrome/browser/ui/views/dow...
File chrome/browser/ui/views/download/download_shelf_view.cc (right):

https://codereview.chromium.org/1367083002/diff/1/chrome/browser/ui/views/dow...
chrome/browser/ui/views/download/download_shelf_view.cc:143: arrow_image_ = new
views::ImageView();
I moved this code because of pkasting's comment here[1]

[1]
https://codereview.chromium.org/1354823002/diff/40001/chrome/browser/ui/views...

https://codereview.chromium.org/1367083002/diff/1/chrome/browser/ui/views/dow...
File chrome/browser/ui/views/download/download_shelf_view.h (left):

https://codereview.chromium.org/1367083002/diff/1/chrome/browser/ui/views/dow...
chrome/browser/ui/views/download/download_shelf_view.h:137:
scoped_ptr<gfx::SlideAnimation> shelf_animation_;
no idea why these were heap allocated previously (to avoid using |this| in
initializer list?)

[Evan Stade, 2015-09-25 00:48:42]

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

[commit-bot: I haz the power, 2015-09-25 00:49:19]

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

[commit-bot: I haz the power, 2015-09-25 01:09:12]

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

[commit-bot: I haz the power, 2015-09-25 01:09:12]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2015-09-25 15:05:01]

LGTM

[Evan Stade, 2015-09-25 17:11:12]

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

[commit-bot: I haz the power, 2015-09-25 17:11:19]

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

[commit-bot: I haz the power, 2015-09-25 18:48:30]

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

[commit-bot: I haz the power, 2015-09-25 18:48:31]

Dry run: This issue passed the CQ dry run.

[groby-ooo-7-16, 2015-09-28 18:17:47]

AFAICT, Cocoa is fine. LGTM.

https://codereview.chromium.org/1367083002/diff/1/chrome/browser/ui/views/dow...
File chrome/browser/ui/views/download/download_shelf_view.h (left):

https://codereview.chromium.org/1367083002/diff/1/chrome/browser/ui/views/dow...
chrome/browser/ui/views/download/download_shelf_view.h:137:
scoped_ptr<gfx::SlideAnimation> shelf_animation_;
On 2015/09/25 00:48:08, Evan Stade wrote:
> no idea why these were heap allocated previously (to avoid using |this| in
> initializer list?)

Not that it particularly matters, but it keeps the header file for animations
from bleeding out to the rest of the world.

[Evan Stade, 2015-09-28 18:27:05]

On 2015/09/28 18:17:47, groby wrote:
> AFAICT, Cocoa is fine. LGTM.
> 
>
https://codereview.chromium.org/1367083002/diff/1/chrome/browser/ui/views/dow...
> File chrome/browser/ui/views/download/download_shelf_view.h (left):
> 
>
https://codereview.chromium.org/1367083002/diff/1/chrome/browser/ui/views/dow...
> chrome/browser/ui/views/download/download_shelf_view.h:137:
> scoped_ptr<gfx::SlideAnimation> shelf_animation_;
> On 2015/09/25 00:48:08, Evan Stade wrote:
> > no idea why these were heap allocated previously (to avoid using |this| in
> > initializer list?)
> 
> Not that it particularly matters, but it keeps the header file for animations
> from bleeding out to the rest of the world.

the style guide expressly forbids applying this logic: "Do not replace data
members with pointers just to avoid an #include."

[Evan Stade, 2015-09-28 18:27:24]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-09-28 18:27:25]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1367083002/#ps20001
(title: "incl")

[Evan Stade, 2015-09-28 18:27:32]

On 2015/09/28 18:27:05, Evan Stade wrote:
> On 2015/09/28 18:17:47, groby wrote:
> > AFAICT, Cocoa is fine. LGTM.
> > 
> >
>
https://codereview.chromium.org/1367083002/diff/1/chrome/browser/ui/views/dow...
> > File chrome/browser/ui/views/download/download_shelf_view.h (left):
> > 
> >
>
https://codereview.chromium.org/1367083002/diff/1/chrome/browser/ui/views/dow...
> > chrome/browser/ui/views/download/download_shelf_view.h:137:
> > scoped_ptr<gfx::SlideAnimation> shelf_animation_;
> > On 2015/09/25 00:48:08, Evan Stade wrote:
> > > no idea why these were heap allocated previously (to avoid using |this| in
> > > initializer list?)
> > 
> > Not that it particularly matters, but it keeps the header file for
animations
> > from bleeding out to the rest of the world.
> 
> the style guide expressly forbids applying this logic: "Do not replace data
> members with pointers just to avoid an #include."

also thanks for checking OSX!

[commit-bot: I haz the power, 2015-09-28 18:29:18]

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

[commit-bot: I haz the power, 2015-09-28 21:16:30]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-09-28 21:17:40]

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