Implement animation UI for opening/closing an app list folder.

Implement animation UI for opening/closing an app list folder.

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

[xiyuan, 2014-01-16 01:46:48]

Mostly nits.

https://codereview.chromium.org/140203003/diff/1/ui/app_list/app_list_folder_...
File ui/app_list/app_list_folder_item.cc (right):

https://codereview.chromium.org/140203003/diff/1/ui/app_list/app_list_folder_...
ui/app_list/app_list_folder_item.cc:62:
AppListFolderItem::GetTopIconsBounds(gfx::Rect(size()));
nit: gfx::Rect(size()) -> GetLocalBounds()

https://codereview.chromium.org/140203003/diff/1/ui/app_list/app_list_folder_...
File ui/app_list/app_list_folder_item.h (right):

https://codereview.chromium.org/140203003/diff/1/ui/app_list/app_list_folder_...
ui/app_list/app_list_folder_item.h:21: typedef std::vector<gfx::Rect>
TopIconsBounds;
nit: TopIconsBounds -> Rects

https://codereview.chromium.org/140203003/diff/1/ui/app_list/views/app_list_f...
File ui/app_list/views/app_list_folder_view.cc (right):

https://codereview.chromium.org/140203003/diff/1/ui/app_list/views/app_list_f...
ui/app_list/views/app_list_folder_view.cc:116: if (animate_to_show_) {
Can we check layer()->opacity to find it out? If so, we can probably get rid of
|animate_to_show_|.

https://codereview.chromium.org/140203003/diff/1/ui/app_list/views/app_list_f...
File ui/app_list/views/app_list_folder_view.h (right):

https://codereview.chromium.org/140203003/diff/1/ui/app_list/views/app_list_f...
ui/app_list/views/app_list_folder_view.h:44: // Schedules animation to show or
hide the view.
nit: Schedules _an_ animation

https://codereview.chromium.org/140203003/diff/1/ui/app_list/views/app_list_f...
ui/app_list/views/app_list_folder_view.h:45: void ScheduleAnimationToShow(bool
show);
nit: ScheduleAnimationToShow makes reader think it is runs animation for showing
the content but it is actually both ways and controlled by |show|. Can we get a
better name? Maybe ScheduleShowHideAnimation?

https://codereview.chromium.org/140203003/diff/1/ui/app_list/views/app_list_f...
ui/app_list/views/app_list_folder_view.h:51: 
nit: nuke one empty line

https://codereview.chromium.org/140203003/diff/1/ui/app_list/views/apps_conta...
File ui/app_list/views/apps_container_view.cc (right):

https://codereview.chromium.org/140203003/diff/1/ui/app_list/views/apps_conta...
ui/app_list/views/apps_container_view.cc:30: 
nit: nuke empty line.

https://codereview.chromium.org/140203003/diff/1/ui/app_list/views/apps_conta...
ui/app_list/views/apps_container_view.cc:31: explicit TopIconAnimationView(const
gfx::ImageSkia& icon,
nit: "explicit" is not needed when ctor takes more than 1 args.

https://codereview.chromium.org/140203003/diff/1/ui/app_list/views/apps_conta...
ui/app_list/views/apps_container_view.cc:47: SetFillsBoundsOpaquely(false);
nit: 2 more space indent

https://codereview.chromium.org/140203003/diff/1/ui/app_list/views/apps_conta...
ui/app_list/views/apps_container_view.cc:68: TransformView();
TransforView() should not be called from Layout(). It should probably be invoked
direct in AppsContainerView::CreateViewsForFolderTopItemsAnimation after the
SetBoundsRect call.

https://codereview.chromium.org/140203003/diff/1/ui/app_list/views/apps_conta...
File ui/app_list/views/apps_container_view.h (right):

https://codereview.chromium.org/140203003/diff/1/ui/app_list/views/apps_conta...
ui/app_list/views/apps_container_view.h:43: {
nit: This goes to the end of previous line.

https://codereview.chromium.org/140203003/diff/1/ui/app_list/views/apps_grid_...
File ui/app_list/views/apps_grid_view.h (right):

https://codereview.chromium.org/140203003/diff/1/ui/app_list/views/apps_grid_...
ui/app_list/views/apps_grid_view.h:147: void ShowTopItemViews(bool show);
nit: ShowTopItemViews -> SetTopItemsVisible

[jennyz, 2014-01-16 17:49:52]

https://codereview.chromium.org/140203003/diff/1/ui/app_list/app_list_folder_...
File ui/app_list/app_list_folder_item.cc (right):

https://codereview.chromium.org/140203003/diff/1/ui/app_list/app_list_folder_...
ui/app_list/app_list_folder_item.cc:62:
AppListFolderItem::GetTopIconsBounds(gfx::Rect(size()));
On 2014/01/16 01:46:48, xiyuan wrote:
> nit: gfx::Rect(size()) -> GetLocalBounds()

GetLocalBounds() is a not available for CanvasImageSource.

https://codereview.chromium.org/140203003/diff/1/ui/app_list/app_list_folder_...
File ui/app_list/app_list_folder_item.h (right):

https://codereview.chromium.org/140203003/diff/1/ui/app_list/app_list_folder_...
ui/app_list/app_list_folder_item.h:21: typedef std::vector<gfx::Rect>
TopIconsBounds;
On 2014/01/16 01:46:48, xiyuan wrote:
> nit: TopIconsBounds -> Rects

Done.

https://codereview.chromium.org/140203003/diff/1/ui/app_list/views/app_list_f...
File ui/app_list/views/app_list_folder_view.cc (right):

https://codereview.chromium.org/140203003/diff/1/ui/app_list/views/app_list_f...
ui/app_list/views/app_list_folder_view.cc:116: if (animate_to_show_) {
On 2014/01/16 01:46:48, xiyuan wrote:
> Can we check layer()->opacity to find it out? If so, we can probably get rid
of
> |animate_to_show_|.

Done.

https://codereview.chromium.org/140203003/diff/1/ui/app_list/views/app_list_f...
File ui/app_list/views/app_list_folder_view.h (right):

https://codereview.chromium.org/140203003/diff/1/ui/app_list/views/app_list_f...
ui/app_list/views/app_list_folder_view.h:44: // Schedules animation to show or
hide the view.
On 2014/01/16 01:46:48, xiyuan wrote:
> nit: Schedules _an_ animation

Done.

https://codereview.chromium.org/140203003/diff/1/ui/app_list/views/app_list_f...
ui/app_list/views/app_list_folder_view.h:45: void ScheduleAnimationToShow(bool
show);
On 2014/01/16 01:46:48, xiyuan wrote:
> nit: ScheduleAnimationToShow makes reader think it is runs animation for
showing
> the content but it is actually both ways and controlled by |show|. Can we get
a
> better name? Maybe ScheduleShowHideAnimation?

Done.

https://codereview.chromium.org/140203003/diff/1/ui/app_list/views/app_list_f...
ui/app_list/views/app_list_folder_view.h:51: 
On 2014/01/16 01:46:48, xiyuan wrote:
> nit: nuke one empty line

Done.

https://codereview.chromium.org/140203003/diff/1/ui/app_list/views/apps_conta...
File ui/app_list/views/apps_container_view.cc (right):

https://codereview.chromium.org/140203003/diff/1/ui/app_list/views/apps_conta...
ui/app_list/views/apps_container_view.cc:30: 
On 2014/01/16 01:46:48, xiyuan wrote:
> nit: nuke empty line.

Done.

https://codereview.chromium.org/140203003/diff/1/ui/app_list/views/apps_conta...
ui/app_list/views/apps_container_view.cc:31: explicit TopIconAnimationView(const
gfx::ImageSkia& icon,
On 2014/01/16 01:46:48, xiyuan wrote:
> nit: "explicit" is not needed when ctor takes more than 1 args.

Done.

https://codereview.chromium.org/140203003/diff/1/ui/app_list/views/apps_conta...
ui/app_list/views/apps_container_view.cc:47: SetFillsBoundsOpaquely(false);
On 2014/01/16 01:46:48, xiyuan wrote:
> nit: 2 more space indent

Done.

https://codereview.chromium.org/140203003/diff/1/ui/app_list/views/apps_conta...
ui/app_list/views/apps_container_view.cc:68: TransformView();
On 2014/01/16 01:46:48, xiyuan wrote:
> TransforView() should not be called from Layout(). It should probably be
invoked
> direct in AppsContainerView::CreateViewsForFolderTopItemsAnimation after the
> SetBoundsRect call.

Done.

https://codereview.chromium.org/140203003/diff/1/ui/app_list/views/apps_conta...
File ui/app_list/views/apps_container_view.h (right):

https://codereview.chromium.org/140203003/diff/1/ui/app_list/views/apps_conta...
ui/app_list/views/apps_container_view.h:43: {
On 2014/01/16 01:46:48, xiyuan wrote:
> nit: This goes to the end of previous line.

Done.

https://codereview.chromium.org/140203003/diff/1/ui/app_list/views/apps_grid_...
File ui/app_list/views/apps_grid_view.h (right):

https://codereview.chromium.org/140203003/diff/1/ui/app_list/views/apps_grid_...
ui/app_list/views/apps_grid_view.h:147: void ShowTopItemViews(bool show);
On 2014/01/16 01:46:48, xiyuan wrote:
> nit: ShowTopItemViews -> SetTopItemsVisible

Done.

[xiyuan, 2014-01-16 18:14:09]

lgtm

[commit-bot: I haz the power, 2014-01-16 18:46:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennyz@chromium.org/140203003/230001

[commit-bot: I haz the power, 2014-01-16 20:59:42]

Change committed as 245314