Add tiles to the experimental app list start page.

Add tiles to the experimental app list start page.

This CL adds TileItemViews to the experimental app list which currently
show the first 5 apps in the app list. This CL also changes the width
of the placeholder bar on the start page to align with the tiles.

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

[tapted, 2014-05-20 05:22:31]

StartPage needs some tests (I asked for some in
https://codereview.chromium.org/186483004/ as well :/)

I think the "old" start page had some infrastructure to do this, but that all
got purged.

It at least needs something that would catch the out-of-bounds access (which I'm
pretty sure exists) in StartPageView::Reset(), and also something to simulate a
profile change.

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/start_...
File ui/app_list/views/start_page_view.cc (right):

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.cc:79: model_(view_delegate->GetModel()),
What if GetModel() returns NULL?

Also, the model in the AppListViewDelegate changes when
AppListViewDelegateObserver::OnProfilesChanged() fires -- where is that handled?

It might be nicer to hold on to the AppListViewDelegate, which we know won't
change, rather than the model. And then use view_delegate_->GetModel() inside
::Reset()

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.cc:85: // The view containing the start page
WebContants and the BarPlaceholder.
typo: WebContants -> WebContents

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.cc:112:
tile->SetAppListItem(model_->top_level_item_list()->item_at(i));
nit: might be good to skip this line, and just call Reset() at the end of the
constructor, to ensure initial state and "reset" are consistent

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.cc:124:
static_cast<TileItemView*>(tiles_container_->child_at(i));
I think this would be nicer in a private function, just to isolate the
static_cast:

TileItemView* StartPageView::GetTileViewAt(int index) {
  return static_cast<TileItemView*>(tiles_container_->child_at(i));
}

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.cc:125:
tile->SetAppListItem(model_->top_level_item_list()->item_at(i));
What if `i` >= model_->top_level_item_list().size() ?

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/tile_i...
File ui/app_list/views/tile_item_view.cc (right):

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/tile_i...
ui/app_list/views/tile_item_view.cc:25: const SkColor kTileBorderColor =
0X80D0D0D0;
use SkColorSetARGB(0x80, 0xD0, 0xD0, 0xD0);

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/tile_i...
ui/app_list/views/tile_item_view.cc:39: virtual void Paint(gfx::Canvas* canvas,
views::View* view) const OVERRIDE {
nit: // Overidden from views::Background::Paint:

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/tile_i...
ui/app_list/views/tile_item_view.cc:44:
canvas->DrawRoundRect(view->GetContentsBounds(), kTileCornerRadius, paint);
The tiles in the screenshot look a little glitchy - 2px might never be enough
for something nice on regular-DPI screens

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/tile_i...
ui/app_list/views/tile_item_view.cc:62: };
nit: DISALLOW_COPY_AND_ASSIGN(..)

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/tile_i...
ui/app_list/views/tile_item_view.cc:71: icon_(new views::ImageView),
Does this need a drop-shadow to match the grid? Probably fine for a follow-up

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/tile_i...
ui/app_list/views/tile_item_view.cc:86:
title_->SetHorizontalAlignment(gfx::ALIGN_CENTER);
You should set set the background color on title_ as well. That will give you
nicer text rendering. I don't think the text overlaps anything, so 

  title_->SetBackgroundColor(kTileBackgroundColor);
  title_->set_background(
      views::Background::CreateSolidBackground(kTileBackgroundColor));

will probably do what you want

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/tile_i...
File ui/app_list/views/tile_item_view.h (right):

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/tile_i...
ui/app_list/views/tile_item_view.h:34: virtual gfx::Size GetPreferredSize()
OVERRIDE;
heads-up: I think this is about to become a const function -- 
https://codereview.chromium.org/273223002/

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/tile_i...
ui/app_list/views/tile_item_view.h:40: AppListItem* item_;
these should have ownership comments

[tapted, 2014-05-20 05:24:40]

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/tile_i...
File ui/app_list/views/tile_item_view.cc (right):

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/tile_i...
ui/app_list/views/tile_item_view.cc:86:
title_->SetHorizontalAlignment(gfx::ALIGN_CENTER);
On 2014/05/20 05:22:31, tapted wrote:
> You should set set the background color on title_ as well. That will give you
> nicer text rendering. I don't think the text overlaps anything, so 
> 
>   title_->SetBackgroundColor(kTileBackgroundColor);
>   title_->set_background(
>       views::Background::CreateSolidBackground(kTileBackgroundColor));
> 
> will probably do what you want

Actually, it looks like views can find an opaque background, and is giving you
subpixel AA already (woohoo!)

[calamity, 2014-05-22 03:31:15]

Added a basic test in https://codereview.chromium.org/291653003 .

I think that testing framework wasn't ready at the time of that other review.

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/start_...
File ui/app_list/views/start_page_view.cc (right):

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.cc:79: model_(view_delegate->GetModel()),
On 2014/05/20 05:22:31, tapted wrote:
> What if GetModel() returns NULL?
> 
> Also, the model in the AppListViewDelegate changes when
> AppListViewDelegateObserver::OnProfilesChanged() fires -- where is that
handled?
> 
> It might be nicer to hold on to the AppListViewDelegate, which we know won't
> change, rather than the model. And then use view_delegate_->GetModel() inside
> ::Reset()

Ah yeah.. I forgot how much the app list had changed since I was last here.

Done and done.

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.cc:85: // The view containing the start page
WebContants and the BarPlaceholder.
On 2014/05/20 05:22:31, tapted wrote:
> typo: WebContants -> WebContents

Done.

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.cc:112:
tile->SetAppListItem(model_->top_level_item_list()->item_at(i));
On 2014/05/20 05:22:31, tapted wrote:
> nit: might be good to skip this line, and just call Reset() at the end of the
> constructor, to ensure initial state and "reset" are consistent

Done.

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.cc:124:
static_cast<TileItemView*>(tiles_container_->child_at(i));
On 2014/05/20 05:22:31, tapted wrote:
> I think this would be nicer in a private function, just to isolate the
> static_cast:
> 
> TileItemView* StartPageView::GetTileViewAt(int index) {
>   return static_cast<TileItemView*>(tiles_container_->child_at(i));
> }

Done.

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.cc:125:
tile->SetAppListItem(model_->top_level_item_list()->item_at(i));
On 2014/05/20 05:22:31, tapted wrote:
> What if `i` >= model_->top_level_item_list().size() ?

Done.

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/tile_i...
File ui/app_list/views/tile_item_view.cc (right):

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/tile_i...
ui/app_list/views/tile_item_view.cc:25: const SkColor kTileBorderColor =
0X80D0D0D0;
On 2014/05/20 05:22:31, tapted wrote:
> use SkColorSetARGB(0x80, 0xD0, 0xD0, 0xD0);

Done.

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/tile_i...
ui/app_list/views/tile_item_view.cc:39: virtual void Paint(gfx::Canvas* canvas,
views::View* view) const OVERRIDE {
On 2014/05/20 05:22:31, tapted wrote:
> nit: // Overidden from views::Background::Paint:

Done.

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/tile_i...
ui/app_list/views/tile_item_view.cc:44:
canvas->DrawRoundRect(view->GetContentsBounds(), kTileCornerRadius, paint);
On 2014/05/20 05:22:31, tapted wrote:
> The tiles in the screenshot look a little glitchy - 2px might never be enough
> for something nice on regular-DPI screens

Switched the border drawing to use fill rather than stroke.

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/tile_i...
ui/app_list/views/tile_item_view.cc:62: };
On 2014/05/20 05:22:31, tapted wrote:
> nit: DISALLOW_COPY_AND_ASSIGN(..)

Done.

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/tile_i...
ui/app_list/views/tile_item_view.cc:71: icon_(new views::ImageView),
On 2014/05/20 05:22:31, tapted wrote:
> Does this need a drop-shadow to match the grid? Probably fine for a follow-up

Unknown. Mocks don't have them.

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/tile_i...
File ui/app_list/views/tile_item_view.h (right):

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/tile_i...
ui/app_list/views/tile_item_view.h:34: virtual gfx::Size GetPreferredSize()
OVERRIDE;
On 2014/05/20 05:22:31, tapted wrote:
> heads-up: I think this is about to become a const function -- 
> https://codereview.chromium.org/273223002/

Cool. I"ll rebase when I need to I guess.

https://codereview.chromium.org/297643002/diff/40001/ui/app_list/views/tile_i...
ui/app_list/views/tile_item_view.h:40: AppListItem* item_;
On 2014/05/20 05:22:31, tapted wrote:
> these should have ownership comments

Done.

[tapted, 2014-05-22 04:56:11]

https://codereview.chromium.org/297643002/diff/60001/ui/app_list/views/start_...
File ui/app_list/views/start_page_view.cc (right):

https://codereview.chromium.org/297643002/diff/60001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.cc:139: Layout();
I'm guessing this will left-align tiles when there are < 5? Which is fine for
now - we can iterate and check with UX folks for what they prefer.

https://codereview.chromium.org/297643002/diff/60001/ui/app_list/views/tile_i...
File ui/app_list/views/tile_item_view.cc (right):

https://codereview.chromium.org/297643002/diff/60001/ui/app_list/views/tile_i...
ui/app_list/views/tile_item_view.cc:25: const SkColor kTileBorderColor =
SkColorSetARGB(0X30, 0xD0, 0xD0, 0xD0);
nit: the first 'x', in '0X30', should be lowercase

https://codereview.chromium.org/297643002/diff/60001/ui/app_list/views/tile_i...
ui/app_list/views/tile_item_view.cc:34: TileItemBackground() {}
initialize strip_color_?

https://codereview.chromium.org/297643002/diff/60001/ui/app_list/views/tile_i...
ui/app_list/views/tile_item_view.cc:81: new
views::BoxLayout(views::BoxLayout::kVertical, 10, 0, 0);
nit 10 -> some constant?
kSomethingGapSpacingSeparationMarginBorderPaddingPixels?

https://codereview.chromium.org/297643002/diff/60001/ui/app_list/views/tile_i...
ui/app_list/views/tile_item_view.cc:109: icon_->SetImage(item_->icon());
I think when an item is installing it's not guaranteed to be 48x48 -- see `void
AppListItemView::UpdateIcon()`. (maybe we should just fix that on the Chrome
side... but that's not how it's currently done).

https://codereview.chromium.org/297643002/diff/60001/ui/app_list/views/tile_i...
ui/app_list/views/tile_item_view.cc:114: }
This will need to observe icon/title updates. Also, users might expect newly
installed apps to appear here. Those are probably fine for a follow-up.

However, a more dangerous one is that if one of these items is uninstalled I
think it will cause |item_| to become a destroyed pointer. And ButtonPressed(..)
to crash. Mayybe that's OK for now... But there might be an easy way to just
observe all changes and mash StartPageView::Reset(..) for the first cut.

There should be tests for all these operations on the model too

[calamity, 2014-05-27 04:29:11]

https://codereview.chromium.org/297643002/diff/60001/ui/app_list/views/start_...
File ui/app_list/views/start_page_view.cc (right):

https://codereview.chromium.org/297643002/diff/60001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.cc:139: Layout();
On 2014/05/22 04:56:11, tapted wrote:
> I'm guessing this will left-align tiles when there are < 5? Which is fine for
> now - we can iterate and check with UX folks for what they prefer.

This center aligns due to BOX_ALIGN_CENTER. To change this behavior we'd set the
tiles to paint to layer and then set the opacity instead.

https://codereview.chromium.org/297643002/diff/60001/ui/app_list/views/tile_i...
File ui/app_list/views/tile_item_view.cc (right):

https://codereview.chromium.org/297643002/diff/60001/ui/app_list/views/tile_i...
ui/app_list/views/tile_item_view.cc:25: const SkColor kTileBorderColor =
SkColorSetARGB(0X30, 0xD0, 0xD0, 0xD0);
On 2014/05/22 04:56:11, tapted wrote:
> nit: the first 'x', in '0X30', should be lowercase

Done.

https://codereview.chromium.org/297643002/diff/60001/ui/app_list/views/tile_i...
ui/app_list/views/tile_item_view.cc:34: TileItemBackground() {}
On 2014/05/22 04:56:11, tapted wrote:
> initialize strip_color_?

Done.

https://codereview.chromium.org/297643002/diff/60001/ui/app_list/views/tile_i...
ui/app_list/views/tile_item_view.cc:81: new
views::BoxLayout(views::BoxLayout::kVertical, 10, 0, 0);
On 2014/05/22 04:56:11, tapted wrote:
> nit 10 -> some constant?
> kSomethingGapSpacingSeparationMarginBorderPaddingPixels?

Done.

https://codereview.chromium.org/297643002/diff/60001/ui/app_list/views/tile_i...
ui/app_list/views/tile_item_view.cc:109: icon_->SetImage(item_->icon());
On 2014/05/22 04:56:11, tapted wrote:
> I think when an item is installing it's not guaranteed to be 48x48 -- see
`void
> AppListItemView::UpdateIcon()`. (maybe we should just fix that on the Chrome
> side... but that's not how it's currently done).

Done.

https://codereview.chromium.org/297643002/diff/60001/ui/app_list/views/tile_i...
ui/app_list/views/tile_item_view.cc:114: }
On 2014/05/22 04:56:11, tapted wrote:
> This will need to observe icon/title updates. Also, users might expect newly
> installed apps to appear here. Those are probably fine for a follow-up.
> 
> However, a more dangerous one is that if one of these items is uninstalled I
> think it will cause |item_| to become a destroyed pointer. And
ButtonPressed(..)
> to crash. Mayybe that's OK for now... But there might be an easy way to just
> observe all changes and mash StartPageView::Reset(..) for the first cut.
> 
> There should be tests for all these operations on the model too

Done.

[tapted, 2014-05-27 05:10:10]

lgtm % nits

https://codereview.chromium.org/297643002/diff/120001/ui/app_list/views/start...
File ui/app_list/views/start_page_view.cc (right):

https://codereview.chromium.org/297643002/diff/120001/ui/app_list/views/start...
ui/app_list/views/start_page_view.cc:119: view_delegate->AddObserver(this);
nit view_delegate -> view_delegate_, just so it's symmetric with the line below

https://codereview.chromium.org/297643002/diff/120001/ui/app_list/views/start...
ui/app_list/views/start_page_view.cc:127: return
static_cast<TileItemView*>(tiles_container_->child_at(index));
This can probably use |tile_views_| now. Or perhaps remove the function
completely since the static_cast is no longer needed.

If you keep the function a DCHECK_LT(index, kNumStartPageTiles); might also be a
good touch

https://codereview.chromium.org/297643002/diff/120001/ui/app_list/views/start...
ui/app_list/views/start_page_view.cc:131: if (model_)
nit: maybe DCHECK(model); before this, since this line suggests model_ is also
null "sometimes"

https://codereview.chromium.org/297643002/diff/120001/ui/app_list/views/start...
ui/app_list/views/start_page_view.cc:140: AppListModel* model = model_;
nit: I don't think you need this temporary

https://codereview.chromium.org/297643002/diff/120001/ui/app_list/views/start...
File ui/app_list/views/start_page_view.h (right):

https://codereview.chromium.org/297643002/diff/120001/ui/app_list/views/start...
ui/app_list/views/start_page_view.h:37: const std::vector<TileItemView*>&
tile_views() { return tile_views_; }
nit: also declare function const?

https://codereview.chromium.org/297643002/diff/120001/ui/app_list/views/tile_...
File ui/app_list/views/tile_item_view.h (right):

https://codereview.chromium.org/297643002/diff/120001/ui/app_list/views/tile_...
ui/app_list/views/tile_item_view.h:47: };
nit: DISALLOW_COPY_AND_ASSIGN

[calamity, 2014-05-27 06:21:07]

https://codereview.chromium.org/297643002/diff/120001/ui/app_list/views/start...
File ui/app_list/views/start_page_view.cc (right):

https://codereview.chromium.org/297643002/diff/120001/ui/app_list/views/start...
ui/app_list/views/start_page_view.cc:119: view_delegate->AddObserver(this);
On 2014/05/27 05:10:10, tapted wrote:
> nit view_delegate -> view_delegate_, just so it's symmetric with the line
below

Done.

https://codereview.chromium.org/297643002/diff/120001/ui/app_list/views/start...
ui/app_list/views/start_page_view.cc:127: return
static_cast<TileItemView*>(tiles_container_->child_at(index));
On 2014/05/27 05:10:10, tapted wrote:
> This can probably use |tile_views_| now. Or perhaps remove the function
> completely since the static_cast is no longer needed.
> 
> If you keep the function a DCHECK_LT(index, kNumStartPageTiles); might also be
a
> good touch

Done.

https://codereview.chromium.org/297643002/diff/120001/ui/app_list/views/start...
ui/app_list/views/start_page_view.cc:131: if (model_)
On 2014/05/27 05:10:10, tapted wrote:
> nit: maybe DCHECK(model); before this, since this line suggests model_ is also
> null "sometimes"

Done.

https://codereview.chromium.org/297643002/diff/120001/ui/app_list/views/start...
ui/app_list/views/start_page_view.cc:140: AppListModel* model = model_;
On 2014/05/27 05:10:10, tapted wrote:
> nit: I don't think you need this temporary

Done.

https://codereview.chromium.org/297643002/diff/120001/ui/app_list/views/start...
File ui/app_list/views/start_page_view.h (right):

https://codereview.chromium.org/297643002/diff/120001/ui/app_list/views/start...
ui/app_list/views/start_page_view.h:37: const std::vector<TileItemView*>&
tile_views() { return tile_views_; }
On 2014/05/27 05:10:10, tapted wrote:
> nit: also declare function const?

Done.

https://codereview.chromium.org/297643002/diff/120001/ui/app_list/views/tile_...
File ui/app_list/views/tile_item_view.h (right):

https://codereview.chromium.org/297643002/diff/120001/ui/app_list/views/tile_...
ui/app_list/views/tile_item_view.h:47: };
On 2014/05/27 05:10:10, tapted wrote:
> nit: DISALLOW_COPY_AND_ASSIGN

Done.

[calamity, 2014-05-27 06:21:13]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-05-27 06:21:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/calamity@chromium.org/297643002/160001

[calamity, 2014-05-27 07:57:59]

The CQ bit was checked by calamity@chromium.org

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

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/calamity@chromium.org/297643002/200001

[commit-bot: I haz the power, 2014-05-27 13:59:29]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-27 14:06:41]

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

[commit-bot: I haz the power, 2014-05-27 14:06:42]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)

[calamity, 2014-05-27 23:25:53]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-05-27 23:26:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/calamity@chromium.org/297643002/220001

[commit-bot: I haz the power, 2014-05-28 03:08:44]

Change committed as 273134

[jacklovesdee1132, 2015-10-09 05:13:19]

jacklovesdee1132@gmail.com changed reviewers:
+ jacklovesdee1132@gmail.com