aura: Implement app list M19 mock.

aura: Implement app list M19 mock.

- Make AppListModel flat, i.e. remove AppListItemGroupModel and its view;
- Move BuildAppListModel from ShellDelegate into AppListViewDelegate;
- Make AppListModelBuilder ready to support simple query;
- Make AppListItemView based on CustomButton and get rid of its listener
  interface since we can use ButtonListener for that now;
- Update UI based on M19 mock:
  - AppListItemView show label on its right instead of bottom;
  - AppListItemView has a mouse hover effect now;
  - AppListModelView show AppListItemView tiles alphabetically from top to
    bottom instead of from left to right;
  - AppListView to occupy full screen include area behind the launcher;
  - AppListView to have a 0.4 black shade;

BUG=98308
TEST=Verify applist should look like M19 mock.

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

[xiyuan, 2012-02-29 22:45:14]

Got most stuff in M19 mock except for the keyboard related stuff in
http://code.google.com/p/chromium/issues/detail?id=98308#c44 and I would do that
in a separate CL since this one is quite big already. 

The simple query part is unrelated to M19 mock but I'd like to keep them in so
that I'll have less conflict when merging with
http://codereview.chromium.org/9479031/. :)

[tfarina, 2012-02-29 22:59:38]

http://codereview.chromium.org/9559005/diff/1/ash/app_list/app_list_item_view.cc
File ash/app_list/app_list_item_view.cc (right):

http://codereview.chromium.org/9559005/diff/1/ash/app_list/app_list_item_view...
ash/app_list/app_list_item_view.cc:98: int preferred_icon_size = rect.height() -
kPadding * 2;
nit: - 2 * kPadding; seems easier to read.

http://codereview.chromium.org/9559005/diff/1/ash/app_list/app_list_model_vie...
File ash/app_list/app_list_model_view.cc (right):

http://codereview.chromium.org/9559005/diff/1/ash/app_list/app_list_model_vie...
ash/app_list/app_list_model_view.cc:82: gfx::Rect current_tile(rect.x(),
rect.y(),
nit: Shorter:

gfx::Rect current_tile(rect.origin(), tile_size);

[xiyuan, 2012-02-29 23:05:45]

http://codereview.chromium.org/9559005/diff/1/ash/app_list/app_list_item_view.cc
File ash/app_list/app_list_item_view.cc (right):

http://codereview.chromium.org/9559005/diff/1/ash/app_list/app_list_item_view...
ash/app_list/app_list_item_view.cc:98: int preferred_icon_size = rect.height() -
kPadding * 2;
On 2012/02/29 22:59:43, tfarina wrote:
> nit: - 2 * kPadding; seems easier to read.

Done.

http://codereview.chromium.org/9559005/diff/1/ash/app_list/app_list_model_vie...
File ash/app_list/app_list_model_view.cc (right):

http://codereview.chromium.org/9559005/diff/1/ash/app_list/app_list_model_vie...
ash/app_list/app_list_model_view.cc:82: gfx::Rect current_tile(rect.x(),
rect.y(),
On 2012/02/29 22:59:43, tfarina wrote:
> nit: Shorter:
> 
> gfx::Rect current_tile(rect.origin(), tile_size);

Done.

[tfarina, 2012-02-29 23:09:58]

http://codereview.chromium.org/9559005/diff/4001/ash/app_list/app_list_item_v...
File ash/app_list/app_list_item_view.cc (right):

http://codereview.chromium.org/9559005/diff/4001/ash/app_list/app_list_item_v...
ash/app_list/app_list_item_view.cc:104: gfx::Size title_size =
title_->GetPreferredSize();
nit: This seems to be unused below, remove?

[xiyuan, 2012-02-29 23:15:56]

http://codereview.chromium.org/9559005/diff/4001/ash/app_list/app_list_item_v...
File ash/app_list/app_list_item_view.cc (right):

http://codereview.chromium.org/9559005/diff/4001/ash/app_list/app_list_item_v...
ash/app_list/app_list_item_view.cc:104: gfx::Size title_size =
title_->GetPreferredSize();
On 2012/02/29 23:09:58, tfarina wrote:
> nit: This seems to be unused below, remove?

Yep. I moved this code but did not realized it's no loner being used. :p

[sky, 2012-03-01 00:26:57]

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list.cc
File ash/app_list/app_list.cc (right):

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list.cc#new...
ash/app_list/app_list.cc:27: return
gfx::Screen::GetMonitorAreaNearestPoint(cursor);
You sure you don't want work area?

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_item_v...
File ash/app_list/app_list_item_view.cc (right):

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_item_v...
ash/app_list/app_list_item_view.cc:40: class StaticImageView : public
views::ImageView {
Why do you need this?

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_model.cc
File ash/app_list/app_list_model.cc (right):

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_model....
ash/app_list/app_list_model.cc:20: DCHECK(index >= 0 && index < item_count());
The same check is in ListModel::item_at, so nuke this.

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_model.h
File ash/app_list/app_list_model.h (right):

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_model....
ash/app_list/app_list_model.h:16: // Model for AppListView. It is consisted of a
list of AppListItemGroupModels,
Update comment.

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_model....
ash/app_list/app_list_model.h:20: typedef ui::ListModel<AppListItemModel> Items;
Can this be private?

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_model....
ash/app_list/app_list_model.h:25: void AddItem(AppListItemModel* item);
Document ownership.

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_model_...
File ash/app_list/app_list_model_view.cc (right):

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_model_...
ash/app_list/app_list_model_view.cc:88: current_tile.Offset(0,
tile_size.height());
Do we really want to tile vertically instead of horizontally?

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_view.cc
File ash/app_list/app_list_view.cc (right):

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_view.c...
ash/app_list/app_list_view.cc:67: delegate_->BuildAppListModel("",
new_model.get());
"" -> std::string()

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_view.c...
ash/app_list/app_list_view.cc:82: gfx::Rect workarea_rect =
gfx::Screen::GetMonitorWorkAreaNearestWindow(
These aren't necessarily in the same coordinates. Can't you make AppList use the
work area?

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_view.c...
ash/app_list/app_list_view.cc:110:
static_cast<AppListItemView*>(sender)->model(),
This seems a bit fragile.

http://codereview.chromium.org/9559005/diff/7003/chrome/browser/ui/views/aura...
File chrome/browser/ui/views/aura/app_list/app_list_model_builder_unittest.cc
(right):

http://codereview.chromium.org/9559005/diff/7003/chrome/browser/ui/views/aura...
chrome/browser/ui/views/aura/app_list/app_list_model_builder_unittest.cc:37:
builder.Build("");
std::string()

[tfarina, 2012-03-01 01:05:23]

http://codereview.chromium.org/9559005/diff/7003/chrome/browser/ui/views/aura...
File chrome/browser/ui/views/aura/app_list/app_list_model_builder.cc (right):

http://codereview.chromium.org/9559005/diff/7003/chrome/browser/ui/views/aura...
chrome/browser/ui/views/aura/app_list/app_list_model_builder.cc:28: typedef
struct {
nit: please no typedef struct, C++ doesn't need this.
Can't you do:
struct BrowserCommandData { ... };

instead?

http://codereview.chromium.org/9559005/diff/7003/chrome/browser/ui/views/aura...
chrome/browser/ui/views/aura/app_list/app_list_model_builder.cc:42: bool
MatchesQuery(const string16 query, const string16 str) {
nit: const string16& ?

[tfarina, 2012-03-01 01:10:00]

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_model_...
File ash/app_list/app_list_model_view.h (right):

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_model_...
ash/app_list/app_list_model_view.h:9: #include <string>
nit: remove both, unused.

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_model_...
ash/app_list/app_list_model_view.h:14: #include
"ui/views/controls/button/button.h"
nit: it seems we can forward declare ButtonListener?

namespace views {
class ButtonListener;
}

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_model_...
ash/app_list/app_list_model_view.h:18: class BoundsAnimator;
nit: remove, unused.

[xiyuan, 2012-03-01 19:58:47]

CL updated:
- Addressed all comments in #3;
- Made AppList fit when RootWindow size changes;
- Updated tile size calculation in AppListModelView to make tiles fit content
rect as much as possible;

Please take another look. Thanks.

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list.cc
File ash/app_list/app_list.cc (right):

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list.cc#new...
ash/app_list/app_list.cc:27: return
gfx::Screen::GetMonitorAreaNearestPoint(cursor);
On 2012/03/01 00:26:57, sky wrote:
> You sure you don't want work area?

Applist does not go behind the launcher in the mock but Glen and Alex mentioned
in issue 98308 @44 that the shades should go behind the launcher. That's why
using full monitor rect instead of work area.

Will add a comment.

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_item_v...
File ash/app_list/app_list_item_view.cc (right):

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_item_v...
ash/app_list/app_list_item_view.cc:40: class StaticImageView : public
views::ImageView {
On 2012/03/01 00:26:57, sky wrote:
> Why do you need this?

ImageView returns true for HitTest and causing the hovering on its parent to
disappear. StaticImageView class overrides HitTest to prevent that.

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_model.cc
File ash/app_list/app_list_model.cc (right):

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_model....
ash/app_list/app_list_model.cc:20: DCHECK(index >= 0 && index < item_count());
On 2012/03/01 00:26:57, sky wrote:
> The same check is in ListModel::item_at, so nuke this.

Done.

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_model.h
File ash/app_list/app_list_model.h (right):

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_model....
ash/app_list/app_list_model.h:16: // Model for AppListView. It is consisted of a
list of AppListItemGroupModels,
On 2012/03/01 00:26:57, sky wrote:
> Update comment.

Done.

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_model....
ash/app_list/app_list_model.h:20: typedef ui::ListModel<AppListItemModel> Items;
On 2012/03/01 00:26:57, sky wrote:
> Can this be private?

Done.

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_model....
ash/app_list/app_list_model.h:25: void AddItem(AppListItemModel* item);
On 2012/03/01 00:26:57, sky wrote:
> Document ownership.

Done.

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_model_...
File ash/app_list/app_list_model_view.cc (right):

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_model_...
ash/app_list/app_list_model_view.cc:88: current_tile.Offset(0,
tile_size.height());
On 2012/03/01 00:26:57, sky wrote:
> Do we really want to tile vertically instead of horizontally?

Yes. The mock lists all items vertically in columns.

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_model_...
File ash/app_list/app_list_model_view.h (right):

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_model_...
ash/app_list/app_list_model_view.h:9: #include <string>
On 2012/03/01 01:10:00, tfarina wrote:
> nit: remove both, unused.

Done.

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_model_...
ash/app_list/app_list_model_view.h:14: #include
"ui/views/controls/button/button.h"
On 2012/03/01 01:10:00, tfarina wrote:
> nit: it seems we can forward declare ButtonListener?
> 
> namespace views {
> class ButtonListener;
> }

Done.

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_model_...
ash/app_list/app_list_model_view.h:18: class BoundsAnimator;
On 2012/03/01 01:10:00, tfarina wrote:
> nit: remove, unused.

Done.

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_view.cc
File ash/app_list/app_list_view.cc (right):

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_view.c...
ash/app_list/app_list_view.cc:67: delegate_->BuildAppListModel("",
new_model.get());
On 2012/03/01 00:26:57, sky wrote:
> "" -> std::string()

Done.

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_view.c...
ash/app_list/app_list_view.cc:82: gfx::Rect workarea_rect =
gfx::Screen::GetMonitorWorkAreaNearestWindow(
On 2012/03/01 00:26:57, sky wrote:
> These aren't necessarily in the same coordinates. Can't you make AppList use
the
> work area?

Done.

http://codereview.chromium.org/9559005/diff/7003/ash/app_list/app_list_view.c...
ash/app_list/app_list_view.cc:110:
static_cast<AppListItemView*>(sender)->model(),
On 2012/03/01 00:26:57, sky wrote:
> This seems a bit fragile.

Added a class name to AppListItemView to make sure it's okay to cast here.

http://codereview.chromium.org/9559005/diff/7003/chrome/browser/ui/views/aura...
File chrome/browser/ui/views/aura/app_list/app_list_model_builder.cc (right):

http://codereview.chromium.org/9559005/diff/7003/chrome/browser/ui/views/aura...
chrome/browser/ui/views/aura/app_list/app_list_model_builder.cc:28: typedef
struct {
On 2012/03/01 01:05:23, tfarina wrote:
> nit: please no typedef struct, C++ doesn't need this.
> Can't you do:
> struct BrowserCommandData { ... };
> 
> instead?

Done.

http://codereview.chromium.org/9559005/diff/7003/chrome/browser/ui/views/aura...
chrome/browser/ui/views/aura/app_list/app_list_model_builder.cc:42: bool
MatchesQuery(const string16 query, const string16 str) {
On 2012/03/01 01:05:23, tfarina wrote:
> nit: const string16& ?

Done.

http://codereview.chromium.org/9559005/diff/7003/chrome/browser/ui/views/aura...
File chrome/browser/ui/views/aura/app_list/app_list_model_builder_unittest.cc
(right):

http://codereview.chromium.org/9559005/diff/7003/chrome/browser/ui/views/aura...
chrome/browser/ui/views/aura/app_list/app_list_model_builder_unittest.cc:37:
builder.Build("");
On 2012/03/01 00:26:57, sky wrote:
> std::string()

Done.

[sky, 2012-03-01 20:56:22]

LGTM

[sky, 2012-03-01 20:56:53]

PS I think tiling vertically is just plain *WRONG*, but Ben said that's what was
decided.

[commit-bot: I haz the power, 2012-03-02 00:40:36]

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

[commit-bot: I haz the power, 2012-03-02 01:46:12]

Try job failure for 9559005-18006 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...