cros: Make SearchResultTileItemView layout per DisplayType customized

cros: Make SearchResultTileItemView layout per DisplayType customized

changes:
(1) TileItemView used to do layout work for both start page tile items
and search box searched app results tile items. For new launcher, their
layout requirements are different. We shall consider put the layout
code in SearchResultTileItemView, as there we can know the
SearchResult::DISPLAY_TYPE and then customize the layout.
(2) Some UI adjustment of recommendation DISPLAY_TYPE:
based on specification of https://screenshot.googleplex.com/wb5PjqfLSCf
tile size, icon top padding, title top spacing, title color/size.

R=xiyuan@chromium.org, weidongg@chromium.org
CC=newcomer@chromium.org
BUG=734164
TEST=tested with and without --enable-features=EnableFullscreenAppList
flag

Review-Url: https://codereview.chromium.org/2952823002
Cr-Commit-Position: refs/heads/master@{#481307}
Committed: https://chromium.googlesource.com/chromium/src/+/261cfcff3d89a69ca461e9db74e75acd907b057b

[Qiang(Joe) Xu, 2017-06-21 17:55:41]

Description was changed from

==========
cros: Make SearchResultTitleItemView layout per display_type customized

R=xiyuan@chromium.org, weidongg@chromium.org
CC=newcomer@chromium.org
BUG=734164
TEST=
==========

to

==========
cros: Make SearchResultTitleItemView layout per DISPLAY_TYPE customized

changes:
(1) TileItemView used to do layout work for both start page tile items
and search box searched app results tile items. For new launcher, their
layout requirements are different. We shall consider put the layout
code in SearchResultTileItemView, as there we can know the
SearchResult::DISPLAY_TYPE and then customize the layout.
(2) Some UI adjustment of recommendation DISPLAY_TYPE:
based on specification of https://screenshot.googleplex.com/wb5PjqfLSCf
tile size, icon top padding, title top spacing, title color/size.

R=xiyuan@chromium.org, weidongg@chromium.org
CC=newcomer@chromium.org
BUG=734164
TEST=
==========

[Qiang(Joe) Xu, 2017-06-21 17:58:59]

Description was changed from

==========
cros: Make SearchResultTitleItemView layout per DISPLAY_TYPE customized

changes:
(1) TileItemView used to do layout work for both start page tile items
and search box searched app results tile items. For new launcher, their
layout requirements are different. We shall consider put the layout
code in SearchResultTileItemView, as there we can know the
SearchResult::DISPLAY_TYPE and then customize the layout.
(2) Some UI adjustment of recommendation DISPLAY_TYPE:
based on specification of https://screenshot.googleplex.com/wb5PjqfLSCf
tile size, icon top padding, title top spacing, title color/size.

R=xiyuan@chromium.org, weidongg@chromium.org
CC=newcomer@chromium.org
BUG=734164
TEST=
==========

to

==========
cros: Make SearchResultTitleItemView layout per DisplayType customized

changes:
(1) TileItemView used to do layout work for both start page tile items
and search box searched app results tile items. For new launcher, their
layout requirements are different. We shall consider put the layout
code in SearchResultTileItemView, as there we can know the
SearchResult::DISPLAY_TYPE and then customize the layout.
(2) Some UI adjustment of recommendation DISPLAY_TYPE:
based on specification of https://screenshot.googleplex.com/wb5PjqfLSCf
tile size, icon top padding, title top spacing, title color/size.

R=xiyuan@chromium.org, weidongg@chromium.org
CC=newcomer@chromium.org
BUG=734164
TEST=
==========

[Qiang(Joe) Xu, 2017-06-21 17:59:12]

Description was changed from

==========
cros: Make SearchResultTitleItemView layout per DisplayType customized

changes:
(1) TileItemView used to do layout work for both start page tile items
and search box searched app results tile items. For new launcher, their
layout requirements are different. We shall consider put the layout
code in SearchResultTileItemView, as there we can know the
SearchResult::DISPLAY_TYPE and then customize the layout.
(2) Some UI adjustment of recommendation DISPLAY_TYPE:
based on specification of https://screenshot.googleplex.com/wb5PjqfLSCf
tile size, icon top padding, title top spacing, title color/size.

R=xiyuan@chromium.org, weidongg@chromium.org
CC=newcomer@chromium.org
BUG=734164
TEST=
==========

to

==========
cros: Make SearchResultTitleItemView layout per DisplayType customized

changes:
(1) TileItemView used to do layout work for both start page tile items
and search box searched app results tile items. For new launcher, their
layout requirements are different. We shall consider put the layout
code in SearchResultTileItemView, as there we can know the
SearchResult::DISPLAY_TYPE and then customize the layout.
(2) Some UI adjustment of recommendation DISPLAY_TYPE:
based on specification of https://screenshot.googleplex.com/wb5PjqfLSCf
tile size, icon top padding, title top spacing, title color/size.

R=xiyuan@chromium.org, weidongg@chromium.org
CC=newcomer@chromium.org
BUG=734164
TEST=
==========

[Qiang(Joe) Xu, 2017-06-21 17:59:45]

Hi xiyuan, what do you think of this?

weidongg, sorry I firstly put the badging layout code in
TileItemView. We would better not put there, otherwise the
layout would be a mess. Your work should be under
DisplayType::DISPLAY_TILE I think.

[xiyuan, 2017-06-21 18:24:00]

The approach looks fine to me.

https://codereview.chromium.org/2952823002/diff/1/ui/app_list/views/search_re...
File ui/app_list/views/search_result_tile_item_view.cc (right):

https://codereview.chromium.org/2952823002/diff/1/ui/app_list/views/search_re...
ui/app_list/views/search_result_tile_item_view.cc:146: if
(is_fullscreen_app_list_enabled_ && item_) {
nit: revise to bail early to have less indent.

i.e.

  if (!is_fullscreen_app_list_enabled_ || !item_) {
    TileItemView::Layout();
    return;
  }

  ...

https://codereview.chromium.org/2952823002/diff/1/ui/app_list/views/tile_item...
File ui/app_list/views/tile_item_view.h (right):

https://codereview.chromium.org/2952823002/diff/1/ui/app_list/views/tile_item...
ui/app_list/views/tile_item_view.h:63: gfx::Size CalculatePreferredSize() const
override;
nit: Move GetTooltipText() with this.

https://codereview.chromium.org/2952823002/diff/1/ui/app_list/views/tile_item...
ui/app_list/views/tile_item_view.h:67: views::Label* title() const { return
title_; }
nit: Move accessors after other methods in the same section. Or group them with
cooresponding setters.

[weidongg, 2017-06-21 18:45:18]

lgtm, I totally agree with the approach.

[Qiang(Joe) Xu, 2017-06-21 18:50:13]

feedback addressed, PTAL thanks

https://codereview.chromium.org/2952823002/diff/1/ui/app_list/views/search_re...
File ui/app_list/views/search_result_tile_item_view.cc (right):

https://codereview.chromium.org/2952823002/diff/1/ui/app_list/views/search_re...
ui/app_list/views/search_result_tile_item_view.cc:146: if
(is_fullscreen_app_list_enabled_ && item_) {
On 2017/06/21 18:24:00, xiyuan wrote:
> nit: revise to bail early to have less indent.
> 
> i.e.
> 
>   if (!is_fullscreen_app_list_enabled_ || !item_) {
>     TileItemView::Layout();
>     return;
>   }
> 
>   ...

Done.

https://codereview.chromium.org/2952823002/diff/1/ui/app_list/views/tile_item...
File ui/app_list/views/tile_item_view.h (right):

https://codereview.chromium.org/2952823002/diff/1/ui/app_list/views/tile_item...
ui/app_list/views/tile_item_view.h:63: gfx::Size CalculatePreferredSize() const
override;
On 2017/06/21 18:24:00, xiyuan wrote:
> nit: Move GetTooltipText() with this.

Done.

[weidongg, 2017-06-21 18:56:43]

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

https://codereview.chromium.org/2952823002/diff/20001/ui/app_list/views/tile_...
ui/app_list/views/tile_item_view.h:70: views::ImageView* GetBadge() const {
return badge_; }
If the badge is only used in SearchResultTitleItemView, is it better to move
this part as well as other badge related code to SearchResultTitleItemView?

[Qiang(Joe) Xu, 2017-06-21 19:07:13]

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

https://codereview.chromium.org/2952823002/diff/20001/ui/app_list/views/tile_...
ui/app_list/views/tile_item_view.h:70: views::ImageView* GetBadge() const {
return badge_; }
On 2017/06/21 18:56:42, weidongg wrote:
> If the badge is only used in SearchResultTitleItemView, is it better to move
> this part as well as other badge related code to SearchResultTitleItemView?

If we don't have badging requirement for suggested apps (it seems will not), we
should move it I think. Let us keep it first (as it is not a blocking need right
now).

[xiyuan, 2017-06-21 19:21:55]

lgtm

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

https://codereview.chromium.org/2952823002/diff/20001/ui/app_list/views/tile_...
ui/app_list/views/tile_item_view.h:67: views::ImageView* GetIcon() const {
return icon_; }
nit: GetIcon -> icon, here and other accessors.

Sorry for not being clear in my previous comment. The naming in previous CL is
good. I only wanted to adjust the declaration order.

[weidongg, 2017-06-21 19:45:34]

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

https://codereview.chromium.org/2952823002/diff/20001/ui/app_list/views/tile_...
ui/app_list/views/tile_item_view.h:70: views::ImageView* GetBadge() const {
return badge_; }
On 2017/06/21 19:07:13, Qiang(Joe) Xu wrote:
> On 2017/06/21 18:56:42, weidongg wrote:
> > If the badge is only used in SearchResultTitleItemView, is it better to move
> > this part as well as other badge related code to SearchResultTitleItemView?
> 
> If we don't have badging requirement for suggested apps (it seems will not),
we
> should move it I think. Let us keep it first (as it is not a blocking need
right
> now).

That makes sense.

[Qiang(Joe) Xu, 2017-06-21 20:29:36]

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

https://codereview.chromium.org/2952823002/diff/20001/ui/app_list/views/tile_...
ui/app_list/views/tile_item_view.h:67: views::ImageView* GetIcon() const {
return icon_; }
On 2017/06/21 19:21:55, xiyuan wrote:
> nit: GetIcon -> icon, here and other accessors.
> 
> Sorry for not being clear in my previous comment. The naming in previous CL is
> good. I only wanted to adjust the declaration order.

Done.

[Qiang(Joe) Xu, 2017-06-21 20:29:57]

The CQ bit was checked by warx@chromium.org

[Qiang(Joe) Xu, 2017-06-21 20:29:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
weidongg@chromium.org, xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/2952823002/#ps40001
(title: "nits")

[commit-bot: I haz the power, 2017-06-21 20:30:23]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Qiang(Joe) Xu, 2017-06-21 20:57:32]

Description was changed from

==========
cros: Make SearchResultTitleItemView layout per DisplayType customized

changes:
(1) TileItemView used to do layout work for both start page tile items
and search box searched app results tile items. For new launcher, their
layout requirements are different. We shall consider put the layout
code in SearchResultTileItemView, as there we can know the
SearchResult::DISPLAY_TYPE and then customize the layout.
(2) Some UI adjustment of recommendation DISPLAY_TYPE:
based on specification of https://screenshot.googleplex.com/wb5PjqfLSCf
tile size, icon top padding, title top spacing, title color/size.

R=xiyuan@chromium.org, weidongg@chromium.org
CC=newcomer@chromium.org
BUG=734164
TEST=
==========

to

==========
cros: Make SearchResultTileItemView layout per DisplayType customized

changes:
(1) TileItemView used to do layout work for both start page tile items
and search box searched app results tile items. For new launcher, their
layout requirements are different. We shall consider put the layout
code in SearchResultTileItemView, as there we can know the
SearchResult::DISPLAY_TYPE and then customize the layout.
(2) Some UI adjustment of recommendation DISPLAY_TYPE:
based on specification of https://screenshot.googleplex.com/wb5PjqfLSCf
tile size, icon top padding, title top spacing, title color/size.

R=xiyuan@chromium.org, weidongg@chromium.org
CC=newcomer@chromium.org
BUG=734164
TEST=tested with and without --enable-features=EnableFullscreenAppList
flag
==========

[commit-bot: I haz the power, 2017-06-21 21:21:16]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1498076997052130,
"parent_rev": "31abd407f04f171a95b204f3df20212c5616f57b", "commit_rev":
"261cfcff3d89a69ca461e9db74e75acd907b057b"}

[commit-bot: I haz the power, 2017-06-21 21:21:32]

Description was changed from

==========
cros: Make SearchResultTileItemView layout per DisplayType customized

changes:
(1) TileItemView used to do layout work for both start page tile items
and search box searched app results tile items. For new launcher, their
layout requirements are different. We shall consider put the layout
code in SearchResultTileItemView, as there we can know the
SearchResult::DISPLAY_TYPE and then customize the layout.
(2) Some UI adjustment of recommendation DISPLAY_TYPE:
based on specification of https://screenshot.googleplex.com/wb5PjqfLSCf
tile size, icon top padding, title top spacing, title color/size.

R=xiyuan@chromium.org, weidongg@chromium.org
CC=newcomer@chromium.org
BUG=734164
TEST=tested with and without --enable-features=EnableFullscreenAppList
flag
==========

to

==========
cros: Make SearchResultTileItemView layout per DisplayType customized

changes:
(1) TileItemView used to do layout work for both start page tile items
and search box searched app results tile items. For new launcher, their
layout requirements are different. We shall consider put the layout
code in SearchResultTileItemView, as there we can know the
SearchResult::DISPLAY_TYPE and then customize the layout.
(2) Some UI adjustment of recommendation DISPLAY_TYPE:
based on specification of https://screenshot.googleplex.com/wb5PjqfLSCf
tile size, icon top padding, title top spacing, title color/size.

R=xiyuan@chromium.org, weidongg@chromium.org
CC=newcomer@chromium.org
BUG=734164
TEST=tested with and without --enable-features=EnableFullscreenAppList
flag

Review-Url: https://codereview.chromium.org/2952823002
Cr-Commit-Position: refs/heads/master@{#481307}
Committed:
https://chromium.googlesource.com/chromium/src/+/261cfcff3d89a69ca461e9db74e7...
==========

[commit-bot: I haz the power, 2017-06-21 21:21:33]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/261cfcff3d89a69ca461e9db74e7...