Implement app list folder management page UI.

Implement app list folder management page UI, including the following feature:
1. Clicking on a folder item in the app list grid will open the app list folder management page to show the folder name and items in the folder.
2. User can click on the back button on folder management page to navigate back to the app list page.
3. User can change the app list folder name on folder management page.

BUG=303224
TBR=sky

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

[xiyuan, 2013-10-17 23:50:29]

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

https://codereview.chromium.org/27777002/diff/1/ui/app_list/views/app_list_fo...
ui/app_list/views/app_list_folder_view.cc:41: app_list_main_view,
pagination_model, start_page_contents);
AppsGridView needs a AppsGridViewDelegate to pass back which item needs to be
activated. Here, we should make |this| such a delegate instead of using the root
one.

Similar to pagination_model and start_page_contents, create dummy ones here
instead of using the outer ones.

https://codereview.chromium.org/27777002/diff/1/ui/app_list/views/app_list_fo...
ui/app_list/views/app_list_folder_view.cc:53: void
AppListFolderView::SetAppListFolderItem(AppListFolderItem* folder) {
Why not pass |folder| in constructor?

https://codereview.chromium.org/27777002/diff/1/ui/app_list/views/app_list_fo...
ui/app_list/views/app_list_folder_view.cc:87: void
AppListFolderView::ButtonPressed(views::Button* sender,
Move this into FolderHeaderView and use a delegate to call back if needed. Make
this a directly listener for a button inside a child view is easy to break.

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

https://codereview.chromium.org/27777002/diff/1/ui/app_list/views/app_list_fo...
ui/app_list/views/app_list_folder_view.h:54: AppsGridView* items_grid_view_;  //
Owned by the view.
// Owned by views hierarchy.

https://codereview.chromium.org/27777002/diff/1/ui/app_list/views/app_list_fo...
ui/app_list/views/app_list_folder_view.h:61: 
nit: nuke the empty line

https://codereview.chromium.org/27777002/diff/1/ui/app_list/views/app_list_ma...
File ui/app_list/views/app_list_main_view.cc (right):

https://codereview.chromium.org/27777002/diff/1/ui/app_list/views/app_list_ma...
ui/app_list/views/app_list_main_view.cc:195:
contents_view_->ShowFolderContent(static_cast<AppListFolderItem*>(item));
Per discussion, put to TODO here to achieve this via model (or delegate).

https://codereview.chromium.org/27777002/diff/1/ui/app_list/views/apps_grid_v...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/27777002/diff/1/ui/app_list/views/apps_grid_v...
ui/app_list/views/apps_grid_view.cc:647: if (model_ && apps_->item_count())
model_ -> app_ ?

https://codereview.chromium.org/27777002/diff/1/ui/app_list/views/contents_vi...
File ui/app_list/views/contents_view.cc (right):

https://codereview.chromium.org/27777002/diff/1/ui/app_list/views/contents_vi...
ui/app_list/views/contents_view.cc:92: view_model_->Add(folder_view,
kIndexFolderContent);
Suggest to put folder view and root grid view into another container view. Add
the container view to ContentsView and we do the root grid and folder switching
inside the container.

https://codereview.chromium.org/27777002/diff/1/ui/app_list/views/folder_head...
File ui/app_list/views/folder_header_view.cc (right):

https://codereview.chromium.org/27777002/diff/1/ui/app_list/views/folder_head...
ui/app_list/views/folder_header_view.cc:50: virtual void
OnPaintFocusBorder(gfx::Canvas* canvas) OVERRIDE {
nit: move it with other views::View methods

https://codereview.chromium.org/27777002/diff/1/ui/app_list/views/folder_head...
ui/app_list/views/folder_header_view.cc:109: return;
nit: alignment

https://codereview.chromium.org/27777002/diff/1/ui/app_list/views/folder_head...
ui/app_list/views/folder_header_view.cc:136: views::View::OnPaint(canvas);
nit: move this to the beginning. It probably does not affect final result here.
But usually we should let the base class to draw first then we draw new content
on top.

[jennyz, 2013-10-18 22:09:01]

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

https://codereview.chromium.org/27777002/diff/1/ui/app_list/views/app_list_fo...
ui/app_list/views/app_list_folder_view.cc:41: app_list_main_view,
pagination_model, start_page_contents);
On 2013/10/17 23:50:30, xiyuan wrote:
> AppsGridView needs a AppsGridViewDelegate to pass back which item needs to be
> activated. Here, we should make |this| such a delegate instead of using the
root
> one.
> 
> Similar to pagination_model and start_page_contents, create dummy ones here
> instead of using the outer ones.
Per our discussion, since the folder item activation is no different than apps
page item activation, we can still use AppsGridViewDelegate from the root. Get
rid of the pagination_model and start_page_contents from the root.

https://codereview.chromium.org/27777002/diff/1/ui/app_list/views/app_list_fo...
ui/app_list/views/app_list_folder_view.cc:53: void
AppListFolderView::SetAppListFolderItem(AppListFolderItem* folder) {
On 2013/10/17 23:50:30, xiyuan wrote:
> Why not pass |folder| in constructor?

Since AppsListFolderView is constructed before it knows which folder it will
belong to, so we can't pass it in constructor, instead, need to set it when the
folder item is activated.

https://codereview.chromium.org/27777002/diff/1/ui/app_list/views/app_list_fo...
ui/app_list/views/app_list_folder_view.cc:87: void
AppListFolderView::ButtonPressed(views::Button* sender,
On 2013/10/17 23:50:30, xiyuan wrote:
> Move this into FolderHeaderView and use a delegate to call back if needed.
Make
> this a directly listener for a button inside a child view is easy to break.
Moved this into FolderHeaderView and make AppListFolderView a delegate to
FolderHeaderView to server the call back for back button pressed event.

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

https://codereview.chromium.org/27777002/diff/1/ui/app_list/views/app_list_fo...
ui/app_list/views/app_list_folder_view.h:54: AppsGridView* items_grid_view_;  //
Owned by the view.
On 2013/10/17 23:50:30, xiyuan wrote:
> // Owned by views hierarchy.

Done.

https://codereview.chromium.org/27777002/diff/1/ui/app_list/views/app_list_fo...
ui/app_list/views/app_list_folder_view.h:61: 
On 2013/10/17 23:50:30, xiyuan wrote:
> nit: nuke the empty line

Done.

https://codereview.chromium.org/27777002/diff/1/ui/app_list/views/app_list_ma...
File ui/app_list/views/app_list_main_view.cc (right):

https://codereview.chromium.org/27777002/diff/1/ui/app_list/views/app_list_ma...
ui/app_list/views/app_list_main_view.cc:195:
contents_view_->ShowFolderContent(static_cast<AppListFolderItem*>(item));
On 2013/10/17 23:50:30, xiyuan wrote:
> Per discussion, put to TODO here to achieve this via model (or delegate).

Done.

https://codereview.chromium.org/27777002/diff/1/ui/app_list/views/apps_grid_v...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/27777002/diff/1/ui/app_list/views/apps_grid_v...
ui/app_list/views/apps_grid_view.cc:647: if (model_ && apps_->item_count())
On 2013/10/17 23:50:30, xiyuan wrote:
> model_ -> app_ ?

Done.

https://codereview.chromium.org/27777002/diff/1/ui/app_list/views/contents_vi...
File ui/app_list/views/contents_view.cc (right):

https://codereview.chromium.org/27777002/diff/1/ui/app_list/views/contents_vi...
ui/app_list/views/contents_view.cc:92: view_model_->Add(folder_view,
kIndexFolderContent);
On 2013/10/17 23:50:30, xiyuan wrote:
> Suggest to put folder view and root grid view into another container view. Add
> the container view to ContentsView and we do the root grid and folder
switching
> inside the container.

Added a new AppsContainerView to achieve the above.

https://codereview.chromium.org/27777002/diff/1/ui/app_list/views/folder_head...
File ui/app_list/views/folder_header_view.cc (right):

https://codereview.chromium.org/27777002/diff/1/ui/app_list/views/folder_head...
ui/app_list/views/folder_header_view.cc:50: virtual void
OnPaintFocusBorder(gfx::Canvas* canvas) OVERRIDE {
On 2013/10/17 23:50:30, xiyuan wrote:
> nit: move it with other views::View methods

Done.

https://codereview.chromium.org/27777002/diff/1/ui/app_list/views/folder_head...
ui/app_list/views/folder_header_view.cc:109: return;
On 2013/10/17 23:50:30, xiyuan wrote:
> nit: alignment

Done.

https://codereview.chromium.org/27777002/diff/1/ui/app_list/views/folder_head...
ui/app_list/views/folder_header_view.cc:136: views::View::OnPaint(canvas);
On 2013/10/17 23:50:30, xiyuan wrote:
> nit: move this to the beginning. It probably does not affect final result
here.
> But usually we should let the base class to draw first then we draw new
content
> on top.

Done.

[xiyuan, 2013-10-18 22:36:00]

Mostly good.

https://codereview.chromium.org/27777002/diff/31001/ui/app_list/views/apps_co...
File ui/app_list/views/apps_container_view.h (right):

https://codereview.chromium.org/27777002/diff/31001/ui/app_list/views/apps_co...
ui/app_list/views/apps_container_view.h:33: AppsGridView* apps_grid_view() {
return apps_grid_view_; }
nit: move accessors to the end of methods

https://codereview.chromium.org/27777002/diff/31001/ui/app_list/views/content...
File ui/app_list/views/contents_view.cc (right):

https://codereview.chromium.org/27777002/diff/31001/ui/app_list/views/content...
ui/app_list/views/contents_view.cc:78:
apps_container_view_->apps_grid_view()->has_dragged_view())
NULL test of apps_container_view_->apps_grid_view() is not needed since they are
created on ctor and never change during life time.

If we want to keep it, we need wrap the branch below with {} since condition is
multiple-line.

https://codereview.chromium.org/27777002/diff/31001/ui/app_list/views/content...
ui/app_list/views/contents_view.cc:152: void ContentsView::ShowApps() {
Do we still need this?

https://codereview.chromium.org/27777002/diff/31001/ui/app_list/views/folder_...
File ui/app_list/views/folder_header_view.cc (right):

https://codereview.chromium.org/27777002/diff/31001/ui/app_list/views/folder_...
ui/app_list/views/folder_header_view.cc:94: folder_item_->AddObserver(this);
Could a new |folder_item_| be NULL?

https://codereview.chromium.org/27777002/diff/31001/ui/app_list/views/folder_...
ui/app_list/views/folder_header_view.cc:100:
folder_name_view_->SetText(UTF8ToUTF16(folder_item_->title()));
Could |folder_item_| be NULL?

https://codereview.chromium.org/27777002/diff/31001/ui/app_list/views/folder_...
ui/app_list/views/folder_header_view.cc:143: folder_item_->RemoveObserver(this);
Handle NULL folder_item_?

[jennyz, 2013-10-18 22:53:19]

https://codereview.chromium.org/27777002/diff/31001/ui/app_list/views/apps_co...
File ui/app_list/views/apps_container_view.h (right):

https://codereview.chromium.org/27777002/diff/31001/ui/app_list/views/apps_co...
ui/app_list/views/apps_container_view.h:33: AppsGridView* apps_grid_view() {
return apps_grid_view_; }
On 2013/10/18 22:36:00, xiyuan wrote:
> nit: move accessors to the end of methods

Done.

https://codereview.chromium.org/27777002/diff/31001/ui/app_list/views/content...
File ui/app_list/views/contents_view.cc (right):

https://codereview.chromium.org/27777002/diff/31001/ui/app_list/views/content...
ui/app_list/views/contents_view.cc:78:
apps_container_view_->apps_grid_view()->has_dragged_view())
On 2013/10/18 22:36:00, xiyuan wrote:
> NULL test of apps_container_view_->apps_grid_view() is not needed since they
are
> created on ctor and never change during life time.
> 
> If we want to keep it, we need wrap the branch below with {} since condition
is
> multiple-line.

Done.

https://codereview.chromium.org/27777002/diff/31001/ui/app_list/views/content...
ui/app_list/views/contents_view.cc:152: void ContentsView::ShowApps() {
On 2013/10/18 22:36:00, xiyuan wrote:
> Do we still need this?
Removed.

https://codereview.chromium.org/27777002/diff/31001/ui/app_list/views/folder_...
File ui/app_list/views/folder_header_view.cc (right):

https://codereview.chromium.org/27777002/diff/31001/ui/app_list/views/folder_...
ui/app_list/views/folder_header_view.cc:94: folder_item_->AddObserver(this);
On 2013/10/18 22:36:00, xiyuan wrote:
> Could a new |folder_item_| be NULL?
Check null before proceeding.

https://codereview.chromium.org/27777002/diff/31001/ui/app_list/views/folder_...
ui/app_list/views/folder_header_view.cc:100:
folder_name_view_->SetText(UTF8ToUTF16(folder_item_->title()));
On 2013/10/18 22:36:00, xiyuan wrote:
> Could |folder_item_| be NULL?
ditto.

https://codereview.chromium.org/27777002/diff/31001/ui/app_list/views/folder_...
ui/app_list/views/folder_header_view.cc:143: folder_item_->RemoveObserver(this);
On 2013/10/18 22:36:00, xiyuan wrote:
> Handle NULL folder_item_?
ditto.

[xiyuan, 2013-10-18 23:31:19]

lgtm

[jennyz, 2013-10-18 23:36:03]

I haven't gone thru your cl:
https://codereview.chromium.org/27438002/

It might or might not have conflicts since we both apps_grid_view.cc/h.

[jennyz, 2013-10-18 23:36:58]

On 2013/10/18 23:36:03, jennyz wrote:
> I haven't gone thru your cl:
> https://codereview.chromium.org/27438002/
> 
> It might or might not have conflicts since we both apps_grid_view.cc/h.

I mean the previous message is for stevenjb.

[commit-bot: I haz the power, 2013-10-21 19:59:40]

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

[commit-bot: I haz the power, 2013-10-21 22:51:06]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[jennyz, 2013-10-22 18:12:14]

PTAL.
I fixed the LauncherAppBrowserTest.ClickItem and
LauncherAppBrowserTest.DragAndDrop.

[xiyuan, 2013-10-22 18:15:45]

SLGTM

https://codereview.chromium.org/27777002/diff/781001/ui/app_list/views/apps_c...
File ui/app_list/views/apps_container_view.h (right):

https://codereview.chromium.org/27777002/diff/781001/ui/app_list/views/apps_c...
ui/app_list/views/apps_container_view.h:39: AppsGridView* apps_grid_view() {
return apps_grid_view_; }
nit: move it after all "virtual"s

https://codereview.chromium.org/27777002/diff/781001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/27777002/diff/781001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:296: LOG(ERROR) << "AppsGridView::SetModel";
nit: remove this

https://codereview.chromium.org/27777002/diff/781001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:308: LOG(ERROR) << "AppsGridView::SetApps";
nit: remove this

[jennyz, 2013-10-22 18:21:31]

https://codereview.chromium.org/27777002/diff/781001/ui/app_list/views/apps_c...
File ui/app_list/views/apps_container_view.h (right):

https://codereview.chromium.org/27777002/diff/781001/ui/app_list/views/apps_c...
ui/app_list/views/apps_container_view.h:39: AppsGridView* apps_grid_view() {
return apps_grid_view_; }
On 2013/10/22 18:15:46, xiyuan wrote:
> nit: move it after all "virtual"s

Done.

https://codereview.chromium.org/27777002/diff/781001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/27777002/diff/781001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:296: LOG(ERROR) << "AppsGridView::SetModel";
On 2013/10/22 18:15:46, xiyuan wrote:
> nit: remove this

Done.

https://codereview.chromium.org/27777002/diff/781001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:308: LOG(ERROR) << "AppsGridView::SetApps";
On 2013/10/22 18:15:46, xiyuan wrote:
> nit: remove this

Done.

[commit-bot: I haz the power, 2013-10-22 18:23:10]

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

[stevenjb, 2013-10-22 18:34:26]

https://codereview.chromium.org/27777002/diff/781001/chrome/browser/ui/ash/la...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc
(right):

https://codereview.chromium.org/27777002/diff/781001/chrome/browser/ui/ash/la...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1788:
LOG(ERROR) << "vm_grid->view_size=" << vm_grid->view_size();
VLOG (or remove)

https://codereview.chromium.org/27777002/diff/951001/ui/app_list/app_list_fol...
File ui/app_list/app_list_folder_item.cc (right):

https://codereview.chromium.org/27777002/diff/951001/ui/app_list/app_list_fol...
ui/app_list/app_list_folder_item.cc:147: // TODO(stevenjb/jennyz): Implement.
Update this comment to something like "Folder handling is implemented by the
View, so do nothing."
Remove the VLOG.

https://codereview.chromium.org/27777002/diff/951001/ui/app_list/views/apps_c...
File ui/app_list/views/apps_container_view.h (right):

https://codereview.chromium.org/27777002/diff/951001/ui/app_list/views/apps_c...
ui/app_list/views/apps_container_view.h:24: 
Only 1 blank line + add a comment to describe this class.

https://codereview.chromium.org/27777002/diff/951001/ui/app_list/views/folder...
File ui/app_list/views/folder_header_view.cc (right):

https://codereview.chromium.org/27777002/diff/951001/ui/app_list/views/folder...
ui/app_list/views/folder_header_view.cc:135: return;
indentation

https://codereview.chromium.org/27777002/diff/951001/ui/app_list/views/folder...
File ui/app_list/views/folder_header_view.h (right):

https://codereview.chromium.org/27777002/diff/951001/ui/app_list/views/folder...
ui/app_list/views/folder_header_view.h:31:
FolderHeaderView(FolderHeaderViewDelegate* delegate);
explicit

[jennyz, 2013-10-22 18:55:35]

https://codereview.chromium.org/27777002/diff/781001/chrome/browser/ui/ash/la...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc
(right):

https://codereview.chromium.org/27777002/diff/781001/chrome/browser/ui/ash/la...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:1788:
LOG(ERROR) << "vm_grid->view_size=" << vm_grid->view_size();
On 2013/10/22 18:34:27, stevenjb wrote:
> VLOG (or remove)

Done.

https://codereview.chromium.org/27777002/diff/951001/ui/app_list/app_list_fol...
File ui/app_list/app_list_folder_item.cc (right):

https://codereview.chromium.org/27777002/diff/951001/ui/app_list/app_list_fol...
ui/app_list/app_list_folder_item.cc:147: // TODO(stevenjb/jennyz): Implement.
On 2013/10/22 18:34:27, stevenjb wrote:
> Update this comment to something like "Folder handling is implemented by the
> View, so do nothing."
> Remove the VLOG.

Done.

https://codereview.chromium.org/27777002/diff/951001/ui/app_list/views/apps_c...
File ui/app_list/views/apps_container_view.h (right):

https://codereview.chromium.org/27777002/diff/951001/ui/app_list/views/apps_c...
ui/app_list/views/apps_container_view.h:24: 
On 2013/10/22 18:34:27, stevenjb wrote:
> Only 1 blank line + add a comment to describe this class.

Done.

https://codereview.chromium.org/27777002/diff/951001/ui/app_list/views/folder...
File ui/app_list/views/folder_header_view.cc (right):

https://codereview.chromium.org/27777002/diff/951001/ui/app_list/views/folder...
ui/app_list/views/folder_header_view.cc:135: return;
On 2013/10/22 18:34:27, stevenjb wrote:
> indentation

Done.

https://codereview.chromium.org/27777002/diff/951001/ui/app_list/views/folder...
File ui/app_list/views/folder_header_view.h (right):

https://codereview.chromium.org/27777002/diff/951001/ui/app_list/views/folder...
ui/app_list/views/folder_header_view.h:31:
FolderHeaderView(FolderHeaderViewDelegate* delegate);
On 2013/10/22 18:34:27, stevenjb wrote:
> explicit

Done.

[commit-bot: I haz the power, 2013-10-22 18:58:34]

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

[commit-bot: I haz the power, 2013-10-22 22:17:50]

Change committed as 230227

[Jeffrey Yasskin, 2013-10-23 00:02:35]

FYI, this broke
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20OS%20%28v...,
so it's reverted in r230256.

[xiyuan, 2013-10-23 00:43:24]

https://codereview.chromium.org/27777002/diff/1171001/ui/app_list/views/app_l...
File ui/app_list/views/app_list_folder_view.cc (right):

https://codereview.chromium.org/27777002/diff/1171001/ui/app_list/views/app_l...
ui/app_list/views/app_list_folder_view.cc:51:
AppListFolderView::~AppListFolderView() {
Add "RemoveAllChildViews(true);" here to explicitly delete |items_grid_view_|.
This should fix the use-after-free

[jennyz, 2013-10-23 05:25:51]

https://codereview.chromium.org/27777002/diff/1171001/ui/app_list/views/app_l...
File ui/app_list/views/app_list_folder_view.cc (right):

https://codereview.chromium.org/27777002/diff/1171001/ui/app_list/views/app_l...
ui/app_list/views/app_list_folder_view.cc:51:
AppListFolderView::~AppListFolderView() {
On 2013/10/23 00:43:25, xiyuan wrote:
> Add "RemoveAllChildViews(true);" here to explicitly delete |items_grid_view_|.
> This should fix the use-after-free

Thanks, fixed in a new patch since it is reverted and running on bots to try.