[win] Add a progress bar in the app launcher for app installs.

[win] Add a progress bar in the app launcher for app installs.


BUG=152854
TBR=thakis@chromium.org for chrome/browser/ui/cocoa for adding a stub function

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

[xiyuan, 2013-02-06 07:30:50]

LGTM with nits

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

https://codereview.chromium.org/12208040/diff/1/ui/app_list/views/app_list_it...
ui/app_list/views/app_list_item_view.cc:178:
apps_grid_view_->EnsureViewVisible(this);
Can we just do it when model_->is_installing() is true? Switching the the item
when install starts makes sense. But forcing user to see it when install
finishes feels a bit awkward. If user is interested, he probably would stay
there. If he moves away, it is probably better not taking him back.

https://codereview.chromium.org/12208040/diff/1/ui/app_list/views/app_list_it...
ui/app_list/views/app_list_item_view.cc:183:
apps_grid_view_->EnsureViewVisible(this);
Calling it here would prevent user from switching to other pages. This feel a
bit annoying to me.

[koz (OOO until 15th September), 2013-02-06 07:40:55]

Thanks for the quick review, Xiyuan!

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

https://codereview.chromium.org/12208040/diff/1/ui/app_list/views/app_list_it...
ui/app_list/views/app_list_item_view.cc:178:
apps_grid_view_->EnsureViewVisible(this);
On 2013/02/06 07:30:50, xiyuan wrote:
> Can we just do it when model_->is_installing() is true? Switching the the item
> when install starts makes sense. But forcing user to see it when install
> finishes feels a bit awkward. If user is interested, he probably would stay
> there. If he moves away, it is probably better not taking him back.

Good point, done.

https://codereview.chromium.org/12208040/diff/1/ui/app_list/views/app_list_it...
ui/app_list/views/app_list_item_view.cc:183:
apps_grid_view_->EnsureViewVisible(this);
On 2013/02/06 07:30:50, xiyuan wrote:
> Calling it here would prevent user from switching to other pages. This feel a
> bit annoying to me.

Done.

[tfarina, 2013-02-06 15:37:33]

https://codereview.chromium.org/12208040/diff/1016/ui/app_list/app_list_item_...
File ui/app_list/app_list_item_model.cc (right):

https://codereview.chromium.org/12208040/diff/1016/ui/app_list/app_list_item_...
ui/app_list/app_list_item_model.cc:47: is_installing_ = is_installing;
add a blank line above as it's done in SetHighlighted and SetTitle

https://codereview.chromium.org/12208040/diff/1016/ui/app_list/app_list_item_...
File ui/app_list/app_list_item_model.h (right):

https://codereview.chromium.org/12208040/diff/1016/ui/app_list/app_list_item_...
ui/app_list/app_list_item_model.h:40: virtual bool is_installing() const {
return is_installing_; }
why this needs to be virtual? Are you overriding it somewhere?

if it's virtual then IsInstalling() and GetPercentDownloaded()

[xiyuan, 2013-02-06 19:10:37]

https://codereview.chromium.org/12208040/diff/1016/ui/app_list/app_list_item_...
File ui/app_list/app_list_item_model.h (right):

https://codereview.chromium.org/12208040/diff/1016/ui/app_list/app_list_item_...
ui/app_list/app_list_item_model.h:40: virtual bool is_installing() const {
return is_installing_; }
On 2013/02/06 15:37:34, tfarina wrote:
> why this needs to be virtual? Are you overriding it somewhere?
> 
> if it's virtual then IsInstalling() and GetPercentDownloaded()

Right. This and percent_downloaded() below should not be virtual.

[benwells, 2013-02-06 21:54:44]

Lgtm with nit

https://codereview.chromium.org/12208040/diff/1016/chrome/browser/extensions/...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/12208040/diff/1016/chrome/browser/extensions/...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:473:
NOTREACHED();
Not: indenting of line 472

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

https://codereview.chromium.org/12208040/diff/1016/chrome/browser/extensions/...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/12208040/diff/1016/chrome/browser/extensions/...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:473:
NOTREACHED();
On 2013/02/06 21:54:44, benwells wrote:
> Not: indenting of line 472

Done.

https://codereview.chromium.org/12208040/diff/1016/ui/app_list/app_list_item_...
File ui/app_list/app_list_item_model.cc (right):

https://codereview.chromium.org/12208040/diff/1016/ui/app_list/app_list_item_...
ui/app_list/app_list_item_model.cc:47: is_installing_ = is_installing;
On 2013/02/06 15:37:34, tfarina wrote:
> add a blank line above as it's done in SetHighlighted and SetTitle

Done here and below.

https://codereview.chromium.org/12208040/diff/1016/ui/app_list/app_list_item_...
File ui/app_list/app_list_item_model.h (right):

https://codereview.chromium.org/12208040/diff/1016/ui/app_list/app_list_item_...
ui/app_list/app_list_item_model.h:40: virtual bool is_installing() const {
return is_installing_; }
On 2013/02/06 19:10:37, xiyuan wrote:
> On 2013/02/06 15:37:34, tfarina wrote:
> > why this needs to be virtual? Are you overriding it somewhere?
> > 
> > if it's virtual then IsInstalling() and GetPercentDownloaded()
> 
> Right. This and percent_downloaded() below should not be virtual.

Done.

[commit-bot: I haz the power, 2013-02-08 06:49:27]

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

[commit-bot: I haz the power, 2013-02-08 06:49:38]

Presubmit check for 12208040-10001 failed and returned exit status 1.

INFO:root:Found 17 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/ui/views/PRESUBMIT.py

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

Presubmit checks took 3.3s to calculate.

[commit-bot: I haz the power, 2013-02-08 06:55:18]

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

[commit-bot: I haz the power, 2013-02-08 07:20:54]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-02-08 08:54:40]

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

[commit-bot: I haz the power, 2013-02-08 10:56:10]

Change committed as 181481