Implement ui for re-parenting an item from an app list folder to another position or folder in the …

Implement ui for re-parenting an item from an app list folder to another position or folder in the top level data model item list.

This cl implement the following features:

1. When an item is dragged inside a folder, an ink bubble will show to indicate whether the item is dragged within the folder(reorder within the folder), or dragged out of the folder.

2. If the item is dragged out of the folder, the folder view UI will transit back to top level apps grid view, the dragged item will be removed from its parent folder and reparented to:
(1). Another position in the top level item list, if user drops it to another position in the top level grid view.
(2). Another folder in the top level item list, if user drops it to another item or folder in the top level grid view.
(3). Go back its original parent folder, if user release the mouse without dropping it to a valid target.

3. Enable drag/drop a item inside a folder to the shelf.

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

[xiyuan, 2014-02-06 22:45:12]

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

https://codereview.chromium.org/136303008/diff/1/ui/app_list/views/app_list_f...
ui/app_list/views/app_list_folder_view.h:9: #include
"ui/app_list/views/apps_grid_view.h"
nit: This can go in cc file.

https://codereview.chromium.org/136303008/diff/1/ui/app_list/views/app_list_f...
ui/app_list/views/app_list_folder_view.h:76: const ui::LocatedEvent& event);
nit: 4-space indent

https://codereview.chromium.org/136303008/diff/1/ui/app_list/views/app_list_f...
ui/app_list/views/app_list_folder_view.h:84: bool hide_for_reparent() const {
return hide_for_reparent_; }
nit: accessor at the last section of methods

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

https://codereview.chromium.org/136303008/diff/1/ui/app_list/views/app_list_i...
ui/app_list/views/app_list_item_view.cc:99: if (item_)
When will this happen?

https://codereview.chromium.org/136303008/diff/1/ui/app_list/views/app_list_i...
ui/app_list/views/app_list_item_view.cc:468: void
AppListItemView::SetDragUIState() {
nit: Move the block under GetIconBounds to be consistent with header.

https://codereview.chromium.org/136303008/diff/1/ui/app_list/views/top_icon_a...
File ui/app_list/views/top_icon_animation_view.cc (right):

https://codereview.chromium.org/136303008/diff/1/ui/app_list/views/top_icon_a...
ui/app_list/views/top_icon_animation_view.cc:5: 
nit: nuke one empty line

https://codereview.chromium.org/136303008/diff/1/ui/app_list/views/top_icon_a...
ui/app_list/views/top_icon_animation_view.cc:31: #if defined(USE_AURA)
nit: Not sure if we still need this any more.

https://codereview.chromium.org/136303008/diff/1/ui/app_list/views/top_icon_a...
ui/app_list/views/top_icon_animation_view.cc:83: delete this;
This would cause use-after-free for the FOR_EACH_OBSERVER below.

https://codereview.chromium.org/136303008/diff/1/ui/app_list/views/top_icon_a...
File ui/app_list/views/top_icon_animation_view.h (right):

https://codereview.chromium.org/136303008/diff/1/ui/app_list/views/top_icon_a...
ui/app_list/views/top_icon_animation_view.h:72:
ObserverList<TopIconAnimationObserver> observers_;
nit: #include "base/observer_list.h"

[jennyz, 2014-02-07 00:03:10]

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

https://codereview.chromium.org/136303008/diff/1/ui/app_list/views/app_list_f...
ui/app_list/views/app_list_folder_view.h:9: #include
"ui/app_list/views/apps_grid_view.h"
On 2014/02/06 22:45:12, xiyuan wrote:
> nit: This can go in cc file.
Since AppsGridView::Pointer is used in DispatchDragEventForReparent(), and I
could not forward declare this enum defined in apps_grid_view.h, therefore, I
include it here.

https://codereview.chromium.org/136303008/diff/1/ui/app_list/views/app_list_f...
ui/app_list/views/app_list_folder_view.h:76: const ui::LocatedEvent& event);
On 2014/02/06 22:45:12, xiyuan wrote:
> nit: 4-space indent

Done.

https://codereview.chromium.org/136303008/diff/1/ui/app_list/views/app_list_f...
ui/app_list/views/app_list_folder_view.h:84: bool hide_for_reparent() const {
return hide_for_reparent_; }
On 2014/02/06 22:45:12, xiyuan wrote:
> nit: accessor at the last section of methods

Done.

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

https://codereview.chromium.org/136303008/diff/1/ui/app_list/views/app_list_i...
ui/app_list/views/app_list_item_view.cc:99: if (item_)
On 2014/02/06 22:45:12, xiyuan wrote:
> When will this happen?
You're right, I don't need to do so as long as making sure the view is always
removed before the item_. Removed this line.

https://codereview.chromium.org/136303008/diff/1/ui/app_list/views/app_list_i...
ui/app_list/views/app_list_item_view.cc:468: void
AppListItemView::SetDragUIState() {
On 2014/02/06 22:45:12, xiyuan wrote:
> nit: Move the block under GetIconBounds to be consistent with header.

Isn't this just under GetIconsBound?

https://codereview.chromium.org/136303008/diff/1/ui/app_list/views/top_icon_a...
File ui/app_list/views/top_icon_animation_view.cc (right):

https://codereview.chromium.org/136303008/diff/1/ui/app_list/views/top_icon_a...
ui/app_list/views/top_icon_animation_view.cc:5: 
On 2014/02/06 22:45:12, xiyuan wrote:
> nit: nuke one empty line

Done.

https://codereview.chromium.org/136303008/diff/1/ui/app_list/views/top_icon_a...
ui/app_list/views/top_icon_animation_view.cc:31: #if defined(USE_AURA)
On 2014/02/06 22:45:12, xiyuan wrote:
> nit: Not sure if we still need this any more.

Remove them, will watch if it breaks any compiler.

https://codereview.chromium.org/136303008/diff/1/ui/app_list/views/top_icon_a...
ui/app_list/views/top_icon_animation_view.cc:83: delete this;
On 2014/02/06 22:45:12, xiyuan wrote:
> This would cause use-after-free for the FOR_EACH_OBSERVER below.

Yes, this's wrong. Now make TopIconAnimation always self-cleaned, and won't pass
it to the observer.

https://codereview.chromium.org/136303008/diff/1/ui/app_list/views/top_icon_a...
File ui/app_list/views/top_icon_animation_view.h (right):

https://codereview.chromium.org/136303008/diff/1/ui/app_list/views/top_icon_a...
ui/app_list/views/top_icon_animation_view.h:72:
ObserverList<TopIconAnimationObserver> observers_;
On 2014/02/06 22:45:12, xiyuan wrote:
> nit: #include "base/observer_list.h"

Done.

[xiyuan, 2014-02-07 00:32:19]

lgtm

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

https://codereview.chromium.org/136303008/diff/1/ui/app_list/views/app_list_f...
ui/app_list/views/app_list_folder_view.h:9: #include
"ui/app_list/views/apps_grid_view.h"
On 2014/02/07 00:03:10, jennyz wrote:
> On 2014/02/06 22:45:12, xiyuan wrote:
> > nit: This can go in cc file.
> Since AppsGridView::Pointer is used in DispatchDragEventForReparent(), and I
> could not forward declare this enum defined in apps_grid_view.h, therefore, I
> include it here.

I see. Yep, enum could not be forward declared.

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

https://codereview.chromium.org/136303008/diff/1/ui/app_list/views/app_list_i...
ui/app_list/views/app_list_item_view.cc:468: void
AppListItemView::SetDragUIState() {
On 2014/02/07 00:03:10, jennyz wrote:
> On 2014/02/06 22:45:12, xiyuan wrote:
> > nit: Move the block under GetIconBounds to be consistent with header.
> 
> Isn't this just under GetIconsBound?

Oops, I am blind. :p

[jennyz, 2014-02-07 05:26:37]

The CQ bit was checked by jennyz@chromium.org

[commit-bot: I haz the power, 2014-02-07 05:27:14]

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

[commit-bot: I haz the power, 2014-02-07 06:54:56]

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

[commit-bot: I haz the power, 2014-02-07 06:54:57]

Retried try job too often on win_rel for step(s) ash_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[jennyz, 2014-02-07 07:09:23]

The CQ bit was checked by jennyz@chromium.org

[commit-bot: I haz the power, 2014-02-07 07:09:31]

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

[commit-bot: I haz the power, 2014-02-07 09:46:51]

Change committed as 249665