chromeos: Sync animation.

chromeos: Sync animation.

- Pulse app list button while LauncherModel has STATUS_LOADING;
- Remove LauncherItemStatus::STATUS_IS_PENDING etc since it is no longer needed;
- ChromeLauncherController sets LauncherModel::STATUS_LOADING on starting and
  watch for sync finish and pending extension install. It sets
  LauncherModel::STATUS_NORMAL when sync is finished and there is no pending
  extension install from sync, or when a maximum 60 seconds timeout since
  turning on loading status;

BUG=129236
TEST=Verify sync animation after OOBE.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152221

[xiyuan, 2012-08-14 17:57:44]

sky please drive the overall review.
tim please review sync related change in chrome_launcher_controller.cc/h
kalman please help with changes in pending_extension_manager.cc/h and
chrome_launcher_controller.cc/h

Thanks.

[sky, 2012-08-14 20:02:33]

http://codereview.chromium.org/10829268/diff/5002/ash/launcher/app_placeholde...
File ash/launcher/app_placeholder_view.cc (right):

http://codereview.chromium.org/10829268/diff/5002/ash/launcher/app_placeholde...
ash/launcher/app_placeholder_view.cc:31: int g_animation_start_delay = 0;
This isn't really a global, I think it should be animation_start_delay.

http://codereview.chromium.org/10829268/diff/5002/ash/launcher/app_placeholde...
ash/launcher/app_placeholder_view.cc:40: g_animation_start_delay +=
kAnimationDurationInMs / 2;
This seems arbitrary. Shouldn't it be based on when parented?

http://codereview.chromium.org/10829268/diff/5002/ash/launcher/app_placeholde...
ash/launcher/app_placeholder_view.cc:48: DCHECK(arraysize(kAnimationOpacity) ==
arraysize(kAnimationScale));
DCHECK_EQ

http://codereview.chromium.org/10829268/diff/5002/ash/launcher/app_placeholde...
File ash/launcher/app_placeholder_view.h (right):

http://codereview.chromium.org/10829268/diff/5002/ash/launcher/app_placeholde...
ash/launcher/app_placeholder_view.h:14: class AppPlaceholderView : public
views::View {
Add a description.

http://codereview.chromium.org/10829268/diff/5002/ash/launcher/launcher_model.cc
File ash/launcher/launcher_model.cc (right):

http://codereview.chromium.org/10829268/diff/5002/ash/launcher/launcher_model...
ash/launcher/launcher_model.cc:181: if (items_[index].type ==
TYPE_APP_PLACEHOLDER) {
Having to toggle the type like this feels hacky. What didn't you like about
STATUS_IS_PENDING? Or renaming it to STATUS_LOADING?

[xiyuan, 2012-08-14 23:11:32]

http://codereview.chromium.org/10829268/diff/5002/ash/launcher/launcher_model.cc
File ash/launcher/launcher_model.cc (right):

http://codereview.chromium.org/10829268/diff/5002/ash/launcher/launcher_model...
ash/launcher/launcher_model.cc:181: if (items_[index].type ==
TYPE_APP_PLACEHOLDER) {
On 2012/08/14 20:02:34, sky wrote:
> Having to toggle the type like this feels hacky. What didn't you like about
> STATUS_IS_PENDING? Or renaming it to STATUS_LOADING?

The problem with STATUS_IS_PENDING approach is that it is just a status and
needs to be bound with a TYPE_APP_SHORTCUT (or other types). But the pulse
animation could be started before we could create such items and needs to have
at least 4 of them running in loading state. And we need to create fake
TYPE_APP_SHORTCUT with STAUTS_IS_PENDING in this case. I feel using a new type
is simpler and cleaner than doing this.

[sky, 2012-08-14 23:46:36]

On Tue, Aug 14, 2012 at 4:11 PM,  <xiyuan@chromium.org> wrote:
>
>
http://codereview.chromium.org/10829268/diff/5002/ash/launcher/launcher_model.cc
> File ash/launcher/launcher_model.cc (right):
>
>
http://codereview.chromium.org/10829268/diff/5002/ash/launcher/launcher_model...
> ash/launcher/launcher_model.cc:181: if (items_[index].type ==
> TYPE_APP_PLACEHOLDER) {
> On 2012/08/14 20:02:34, sky wrote:
>>
>> Having to toggle the type like this feels hacky. What didn't you like
>
> about
>>
>> STATUS_IS_PENDING? Or renaming it to STATUS_LOADING?
>
>
> The problem with STATUS_IS_PENDING approach is that it is just a status
> and needs to be bound with a TYPE_APP_SHORTCUT (or other types). But the
> pulse animation could be started before we could create such items and
> needs to have at least 4 of them running in loading state. And we need
> to create fake TYPE_APP_SHORTCUT with STAUTS_IS_PENDING in this case. I
> feel using a new type is simpler and cleaner than doing this.

What I've proposed doesn't sound any harder, and it feels cleaner in
so far as the LauncherModel doesn't have a policy about what PENDING
means. Additionally it doesn't seem like more code in so far as you
already have to have code to track state, it's just a matter of
keeping it in Chrome rather than ash.

  -Scott

[not at google - send to devlin, 2012-08-15 05:25:09]

extensions lgtm

[xiyuan, 2012-08-15 18:23:27]

CL updated. It still uses TYPE_APP_PLACEHOLDER instead of STATE_IS_PENDING but
adding/removing placholders logic is moved into chrome.

Please take another look. Thanks.

http://codereview.chromium.org/10829268/diff/5002/ash/launcher/app_placeholde...
File ash/launcher/app_placeholder_view.cc (right):

http://codereview.chromium.org/10829268/diff/5002/ash/launcher/app_placeholde...
ash/launcher/app_placeholder_view.cc:31: int g_animation_start_delay = 0;
On 2012/08/14 20:02:34, sky wrote:
> This isn't really a global, I think it should be animation_start_delay.

Done.

http://codereview.chromium.org/10829268/diff/5002/ash/launcher/app_placeholde...
ash/launcher/app_placeholder_view.cc:40: g_animation_start_delay +=
kAnimationDurationInMs / 2;
On 2012/08/14 20:02:34, sky wrote:
> This seems arbitrary. Shouldn't it be based on when parented?

Done.

http://codereview.chromium.org/10829268/diff/5002/ash/launcher/app_placeholde...
ash/launcher/app_placeholder_view.cc:48: DCHECK(arraysize(kAnimationOpacity) ==
arraysize(kAnimationScale));
On 2012/08/14 20:02:34, sky wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/10829268/diff/5002/ash/launcher/app_placeholde...
File ash/launcher/app_placeholder_view.h (right):

http://codereview.chromium.org/10829268/diff/5002/ash/launcher/app_placeholde...
ash/launcher/app_placeholder_view.h:14: class AppPlaceholderView : public
views::View {
On 2012/08/14 20:02:34, sky wrote:
> Add a description.

Done.

[sky, 2012-08-15 20:52:36]

http://codereview.chromium.org/10829268/diff/10001/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

http://codereview.chromium.org/10829268/diff/10001/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1027: for (int i =
0; i < kMaxLoadingPlaceholders; ++i) {
Do we unconditionally add kMaxLoadingPlaceholders apps?

http://codereview.chromium.org/10829268/diff/10001/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right):

http://codereview.chromium.org/10829268/diff/10001/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:368:
std::deque<ash::LauncherID> app_placeholders_;
Add a description of this. Or perhaps a general description of app placeholders
some where and a see description here.

[xiyuan, 2012-08-15 21:36:03]

http://codereview.chromium.org/10829268/diff/10001/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

http://codereview.chromium.org/10829268/diff/10001/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1027: for (int i =
0; i < kMaxLoadingPlaceholders; ++i) {
On 2012/08/15 20:52:36, sky wrote:
> Do we unconditionally add kMaxLoadingPlaceholders apps?

I am doing this because StartLoadingUI is called before
UpdateAppLaunchersFromPref. We always create kMaxLoadingPlaceholders and those
placeholders will be trimmed when app shortcuts are added (from
UpdateAppLaunchersFromPref).

http://codereview.chromium.org/10829268/diff/10001/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right):

http://codereview.chromium.org/10829268/diff/10001/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:368:
std::deque<ash::LauncherID> app_placeholders_;
On 2012/08/15 20:52:36, sky wrote:
> Add a description of this. Or perhaps a general description of app
placeholders
> some where and a see description here.

Added general description above StartLoadingUI.

[xiyuan, 2012-08-16 21:02:34]

CL updated to show pulsing app list button instead of adding 4 pulsing squares
per discussion with sky, alex and minli.

Please take another look. Thanks.

[sky, 2012-08-16 22:32:45]

LGTM

[commit-bot: I haz the power, 2012-08-17 22:21:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiyuan@chromium.org/10829268/8036

[commit-bot: I haz the power, 2012-08-18 02:22:18]

Change committed as 152221