Move app list from ash to ui.

Move app list from ash to ui.

The goal is to make app list an independent component so that it could be reused in elsewhere.

BUG=none.
TEST=none.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=135981

[xiyuan, 2012-05-08 21:55:40]

Most of the files contain only renaming caused by the move. 

The change other than renaming is removing ash dependencies
from app_list_view.cc:
- The screen bounds, work area and bubble anchor view logic is
  moved into  ash/wm/app_list_controller.cc
  (ash/app_list/app_list.cc before the move) and passed into
  AppListView via its InitAsFullscreenWidget, InitAsBubble and
  UpdateBounds methods;
- The Close function is now implemented via added Close method
  in AppListViewDelegate;

[tfarina, 2012-05-08 21:59:16]

On 2012/05/08 21:55:40, xiyuan wrote:
> Most of the files contain only renaming caused by the move. 
> 
> The change other than renaming is removing ash dependencies
> from app_list_view.cc:
> - The screen bounds, work area and bubble anchor view logic is
>   moved into  ash/wm/app_list_controller.cc
>   (ash/app_list/app_list.cc before the move) and passed into
>   AppListView via its InitAsFullscreenWidget, InitAsBubble and
>   UpdateBounds methods;
> - The Close function is now implemented via added Close method
>   in AppListViewDelegate;

Could you include the reason for moving app_list from ash/ to ui/? Is not clear
to me why you are doing this, so it might not be to others too.

[tfarina, 2012-05-08 22:00:30]

Also, don't you want to add an OWNERS file to ui/app_list?

[xiyuan, 2012-05-08 22:02:31]

On 2012/05/08 21:59:16, tfarina wrote:
> On 2012/05/08 21:55:40, xiyuan wrote:
> > Most of the files contain only renaming caused by the move. 
> > 
> > The change other than renaming is removing ash dependencies
> > from app_list_view.cc:
> > - The screen bounds, work area and bubble anchor view logic is
> >   moved into  ash/wm/app_list_controller.cc
> >   (ash/app_list/app_list.cc before the move) and passed into
> >   AppListView via its InitAsFullscreenWidget, InitAsBubble and
> >   UpdateBounds methods;
> > - The Close function is now implemented via added Close method
> >   in AppListViewDelegate;
> 
> Could you include the reason for moving app_list from ash/ to ui/? Is not
clear
> to me why you are doing this, so it might not be to others too.

Updated CL description. The goal it so make app list an independent component so
that it could be reused in elsewhere, e.g. in desktop Chrome.

[xiyuan, 2012-05-08 22:05:58]

On 2012/05/08 22:00:30, tfarina wrote:
> Also, don't you want to add an OWNERS file to ui/app_list?

Strange, OWNERS file is not included in the patch. I did the move via "svn mv
dir1 dir2" and it's not showing up because I did not change it?

[Ben Goodger (Google), 2012-05-08 22:13:30]

You'll need to add a DEPS file in ui/app_list too to set up include rules
otherwise you'll get checkdeps failures.

LGTM.

http://codereview.chromium.org/10388032/diff/1123/ash/app_list/OWNERS
File ash/app_list/OWNERS (left):

http://codereview.chromium.org/10388032/diff/1123/ash/app_list/OWNERS#oldcode1
ash/app_list/OWNERS:1: xiyuan@chromium.org
don't forget to move this to your new location

[tfarina, 2012-05-08 22:59:54]

On Tue, May 8, 2012 at 7:02 PM,  <xiyuan@chromium.org> wrote:
> Updated CL description. The goal it so make app list an independent
> component so
> that it could be reused in elsewhere, e.g. in desktop Chrome.
>
Thanks for adding the goal to CL description, it lgtm.

-- 
Thiago

[tfarina, 2012-05-08 23:02:10]

On Tue, May 8, 2012 at 7:05 PM,  <xiyuan@chromium.org> wrote:
> On 2012/05/08 22:00:30, tfarina wrote:
>>
>> Also, don't you want to add an OWNERS file to ui/app_list?
>
>
> Strange, OWNERS file is not included in the patch. I did the move via "svn
> mv
> dir1 dir2" and it's not showing up because I did not change it?
>
I don't think you did anything wrong, but I don't use svn myself.
Usually this works: git mv ash/app_list/OWNERS ui/app_list/, or moving
everything git mv ash/app_list/* ui/app_list/

-- 
Thiago

[xiyuan, 2012-05-08 23:21:01]

On 2012/05/08 23:02:10, tfarina wrote:
> On Tue, May 8, 2012 at 7:05 PM,  <mailto:xiyuan@chromium.org> wrote:
> > On 2012/05/08 22:00:30, tfarina wrote:
> >>
> >> Also, don't you want to add an OWNERS file to ui/app_list?
> >
> >
> > Strange, OWNERS file is not included in the patch. I did the move via "svn
> > mv
> > dir1 dir2" and it's not showing up because I did not change it?
> >
> I don't think you did anything wrong, but I don't use svn myself.
> Usually this works: git mv ash/app_list/OWNERS ui/app_list/, or moving
> everything git mv ash/app_list/* ui/app_list/
> 
> -- 
> Thiago

Fixed the missing OWNERS by "svn mv" individual files instead of the dir.

So, your CL https://chromiumcodereview.appspot.com/10365007 is made from git?
Interesting, I thought "git mv" would lose file history because "git svn"
ignores file's ancestry.

[tfarina, 2012-05-08 23:22:54]

On Tue, May 8, 2012 at 8:21 PM,  <xiyuan@chromium.org> wrote:
> Fixed the missing OWNERS by "svn mv" individual files instead of the dir.
>
> So, your CL https://chromiumcodereview.appspot.com/10365007 is made from
> git?
Yep. ;)

> Interesting, I thought "git mv" would lose file history because "git svn"
> ignores file's ancestry.
>
This was fixed by Marc-Antonie (maruel).




-- 
Thiago

[tfarina, 2012-05-08 23:30:02]

http://codereview.chromium.org/10388032/diff/1127/ui/app_list/app_list_unitte...
File ui/app_list/app_list_unittests.cc (right):

http://codereview.chromium.org/10388032/diff/1127/ui/app_list/app_list_unitte...
ui/app_list/app_list_unittests.cc:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
and then rename this to run_all_unittests.cc?

http://codereview.chromium.org/10388032/diff/1127/ui/app_list/model_view_unit...
File ui/app_list/model_view_unittest.cc (right):

http://codereview.chromium.org/10388032/diff/1127/ui/app_list/model_view_unit...
ui/app_list/model_view_unittest.cc:21: TEST(AppListTest,
ModelViewCalculateLayout) {
nit: could you keep this file named app_list_unittest.cc?

[xiyuan, 2012-05-08 23:46:34]

http://codereview.chromium.org/10388032/diff/1127/ui/app_list/app_list_unitte...
File ui/app_list/app_list_unittests.cc (right):

http://codereview.chromium.org/10388032/diff/1127/ui/app_list/app_list_unitte...
ui/app_list/app_list_unittests.cc:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
On 2012/05/08 23:30:02, tfarina wrote:
> and then rename this to run_all_unittests.cc?

Done.

http://codereview.chromium.org/10388032/diff/1127/ui/app_list/model_view_unit...
File ui/app_list/model_view_unittest.cc (right):

http://codereview.chromium.org/10388032/diff/1127/ui/app_list/model_view_unit...
ui/app_list/model_view_unittest.cc:21: TEST(AppListTest,
ModelViewCalculateLayout) {
On 2012/05/08 23:30:02, tfarina wrote:
> nit: could you keep this file named app_list_unittest.cc?

I'll change the test suite name to ModelViewTest since this only tests the model
view.

[tfarina, 2012-05-09 00:20:56]

http://codereview.chromium.org/10388032/diff/10133/ui/app_list/model_view_uni...
File ui/app_list/model_view_unittest.cc (right):

http://codereview.chromium.org/10388032/diff/10133/ui/app_list/model_view_uni...
ui/app_list/model_view_unittest.cc:7: #include
"ui/app_list/app_list_model_view.h"
I think a more accurate name for this test file would be
app_list_model_view_unittest.cc and thus you could include this first. Because
what we are testing here is the CalculateLayout() function, no?

http://codereview.chromium.org/10388032/diff/10133/ui/app_list/model_view_uni...
ui/app_list/model_view_unittest.cc:26:
app_list::AppListItemView::SetMinTitleWidth(kMinTitleWidth);
nit: you can wrap this unit test in app_list namespace.

[tfarina, 2012-05-09 00:34:10]

http://codereview.chromium.org/10388032/diff/10133/ash/wm/app_list_controller.h
File ash/wm/app_list_controller.h (right):

http://codereview.chromium.org/10388032/diff/10133/ash/wm/app_list_controller...
ash/wm/app_list_controller.h:43: bool IsVisible();
nit: we could we make this function const in a follow up CL.

[tfarina, 2012-05-09 00:35:18]

sorry for the noise :-(

http://codereview.chromium.org/10388032/diff/10133/ash/wm/app_list_controller.cc
File ash/wm/app_list_controller.cc (right):

http://codereview.chromium.org/10388032/diff/10133/ash/wm/app_list_controller...
ash/wm/app_list_controller.cc:246: aura::KeyEvent* event) {
nit: fix indentation here and below.

[xiyuan, 2012-05-09 01:28:46]

http://codereview.chromium.org/10388032/diff/10133/ash/wm/app_list_controller.cc
File ash/wm/app_list_controller.cc (right):

http://codereview.chromium.org/10388032/diff/10133/ash/wm/app_list_controller...
ash/wm/app_list_controller.cc:246: aura::KeyEvent* event) {
On 2012/05/09 00:35:18, tfarina wrote:
> nit: fix indentation here and below.

Done.

http://codereview.chromium.org/10388032/diff/10133/ash/wm/app_list_controller.h
File ash/wm/app_list_controller.h (right):

http://codereview.chromium.org/10388032/diff/10133/ash/wm/app_list_controller...
ash/wm/app_list_controller.h:43: bool IsVisible();
On 2012/05/09 00:34:10, tfarina wrote:
> nit: we could we make this function const in a follow up CL.

Done.

http://codereview.chromium.org/10388032/diff/10133/ui/app_list/model_view_uni...
File ui/app_list/model_view_unittest.cc (right):

http://codereview.chromium.org/10388032/diff/10133/ui/app_list/model_view_uni...
ui/app_list/model_view_unittest.cc:7: #include
"ui/app_list/app_list_model_view.h"
On 2012/05/09 00:20:57, tfarina wrote:
> I think a more accurate name for this test file would be
> app_list_model_view_unittest.cc and thus you could include this first. Because
> what we are testing here is the CalculateLayout() function, no?

Done.

http://codereview.chromium.org/10388032/diff/10133/ui/app_list/model_view_uni...
ui/app_list/model_view_unittest.cc:26:
app_list::AppListItemView::SetMinTitleWidth(kMinTitleWidth);
On 2012/05/09 00:20:57, tfarina wrote:
> nit: you can wrap this unit test in app_list namespace. 

Done.