Make the app list use images for the progress bar.

Make the app list use images for the progress bar.

BUG=152854
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182195

[koz (OOO until 15th September), 2013-02-12 07:43:24]

This replaces the 'fill-rect' progress bar with proper images, but I'm not sure
about the changes to the DEPS file to allow grit #includes. Xiyuan, could you
provide guidance here? The other approach I thought of was to plumb the four
images through the app list delegate interfaces (similar to how the window icon
is done), but a third way that doesn't involve lots of boilerplate would be
cool.

[koz (OOO until 15th September), 2013-02-12 07:44:17]

[+chrome-apps-syd-reviews@]

[benwells, 2013-02-12 07:55:17]

https://codereview.chromium.org/12217129/diff/1/ui/app_list/app_list.gyp
File ui/app_list/app_list.gyp (right):

https://codereview.chromium.org/12217129/diff/1/ui/app_list/app_list.gyp#newc...
ui/app_list/app_list.gyp:19:
'../../chrome/chrome_resources.gyp:chrome_resources',
Do we need both of these? I like restricting strings to come via delegates.

https://codereview.chromium.org/12217129/diff/1/ui/app_list/views/app_list_it...
File ui/app_list/views/app_list_item_view.cc (right):

https://codereview.chromium.org/12217129/diff/1/ui/app_list/views/app_list_it...
ui/app_list/views/app_list_item_view.cc:11: #include "grit/theme_resources.h"
Again, do we need generated resources here? I thought theme_resources would be
enough.

https://codereview.chromium.org/12217129/diff/1/ui/app_list/views/app_list_it...
ui/app_list/views/app_list_item_view.cc:246: icon_->bounds().bottom() +
kIconTitleSpacing;
This can fit on one line.

https://codereview.chromium.org/12217129/diff/1/ui/app_list/views/app_list_it...
ui/app_list/views/app_list_item_view.cc:247: // rect.y() + icon_size_.height() +
kIconTitleSpacing;
Did you mean these comments to be here?

https://codereview.chromium.org/12217129/diff/1/ui/app_list/views/app_list_it...
ui/app_list/views/app_list_item_view.cc:251: if (model_->percent_downloaded() !=
-1) {
Should this be > 0?

[koz (OOO until 15th September), 2013-02-12 08:20:11]

https://codereview.chromium.org/12217129/diff/1/ui/app_list/app_list.gyp
File ui/app_list/app_list.gyp (right):

https://codereview.chromium.org/12217129/diff/1/ui/app_list/app_list.gyp#newc...
ui/app_list/app_list.gyp:19:
'../../chrome/chrome_resources.gyp:chrome_resources',
On 2013/02/12 07:55:17, benwells wrote:
> Do we need both of these? I like restricting strings to come via delegates.

If I delete chrome_resources I get a "cannot open include file
'grit/theme_resources.h'" error :-(

https://codereview.chromium.org/12217129/diff/1/ui/app_list/views/app_list_it...
File ui/app_list/views/app_list_item_view.cc (right):

https://codereview.chromium.org/12217129/diff/1/ui/app_list/views/app_list_it...
ui/app_list/views/app_list_item_view.cc:11: #include "grit/theme_resources.h"
On 2013/02/12 07:55:17, benwells wrote:
> Again, do we need generated resources here? I thought theme_resources would be
> enough.

Right you are. Done.

https://codereview.chromium.org/12217129/diff/1/ui/app_list/views/app_list_it...
ui/app_list/views/app_list_item_view.cc:246: icon_->bounds().bottom() +
kIconTitleSpacing;
On 2013/02/12 07:55:17, benwells wrote:
> This can fit on one line.

Done.

https://codereview.chromium.org/12217129/diff/1/ui/app_list/views/app_list_it...
ui/app_list/views/app_list_item_view.cc:247: // rect.y() + icon_size_.height() +
kIconTitleSpacing;
On 2013/02/12 07:55:17, benwells wrote:
> Did you mean these comments to be here?

Done.

https://codereview.chromium.org/12217129/diff/1/ui/app_list/views/app_list_it...
ui/app_list/views/app_list_item_view.cc:251: if (model_->percent_downloaded() !=
-1) {
On 2013/02/12 07:55:17, benwells wrote:
> Should this be > 0?

No, on a 0 percent downloaded item we still want to show the edges of the
progress bar. Well, it could go either way (show nothing or just show the
smallest possible bar). The way the code is structured here the percent done
applies to the width of the bar between the left and right images, not including
them.

[benwells, 2013-02-12 09:57:20]

lgtm

[xiyuan, 2013-02-12 17:38:40]

mostly good except where to put the image resources. Since this is a UI
component, I would think the images should go into ui/resources rather than
chrome/app/theme.

https://codereview.chromium.org/12217129/diff/9001/ui/app_list/views/app_list...
File ui/app_list/views/app_list_item_view.cc (right):

https://codereview.chromium.org/12217129/diff/9001/ui/app_list/views/app_list...
ui/app_list/views/app_list_item_view.cc:212:
title_->SetVisible(!model_->is_installing());
We had the logic in ItemIsInstallingChanged already. Do we still need this?

[koz (OOO until 15th September), 2013-02-13 00:09:03]

Cool, sounds good. I've moved the files to the ui_resources directory.

https://codereview.chromium.org/12217129/diff/9001/ui/app_list/views/app_list...
File ui/app_list/views/app_list_item_view.cc (right):

https://codereview.chromium.org/12217129/diff/9001/ui/app_list/views/app_list...
ui/app_list/views/app_list_item_view.cc:212:
title_->SetVisible(!model_->is_installing());
On 2013/02/12 17:38:40, xiyuan wrote:
> We had the logic in ItemIsInstallingChanged already. Do we still need this?

Experimentation told me I needed this here, but your comment prompted me to
investigate further. We don't actually need this here - it just needs to move to
the constructor. Layout() occurs shortly after (during?) the constructor, so
putting it here made things work, but the constructor is the right place for it.

[xiyuan, 2013-02-13 00:20:44]

LGTM

[commit-bot: I haz the power, 2013-02-13 06:12:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/12217129/14001

[commit-bot: I haz the power, 2013-02-13 06:12:52]

Presubmit check for 12217129-14001 failed and returned exit status 1.

INFO:root:Found 4 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/ui/resources/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    ui/resources

Presubmit checks took 1.3s to calculate.

[koz (OOO until 15th September), 2013-02-13 06:20:44]

+oshima@ for ui/resources

[oshima, 2013-02-13 06:54:57]

On 2013/02/13 06:20:44, koz wrote:
> +oshima@ for ui/resources

ui/resources lgtm

[benwells, 2013-02-13 07:32:16]

On 2013/02/13 06:54:57, oshima wrote:
> On 2013/02/13 06:20:44, koz wrote:
> > +oshima@ for ui/resources
> 
> ui/resources lgtm

(checking the box for koz)

[commit-bot: I haz the power, 2013-02-13 07:33:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/12217129/14001

[commit-bot: I haz the power, 2013-02-13 11:53:14]

Change committed as 182195