Show separator in SearchResultTileItemListView

Show separator in SearchResultTileItemListView

This CL makes following changes:
1. Add ResultType enum to SearchResult.
2. Add separator between SearchResults of different ResultType in
SearchResultTileItemListView.

The assumption is that the SearchResults are already grouped based on
the ResultType, so we don't need to sort them in this CL.

specs: https://screenshot.googleplex.com/5FKt8ZRembk
screenshot: https://screenshot.googleplex.com/L8URwj8bWQo

BUG=736157
Review-Url: https://codereview.chromium.org/2949413002
Cr-Commit-Position: refs/heads/master@{#482045}
Committed: https://chromium.googlesource.com/chromium/src/+/198b9e6b983a55bcce7bc299a4fde8c8611ef6f8

[weidongg, 2017-06-23 01:10:56]

weidongg@chromium.org changed reviewers:
+ hejq@chromium.org, warx@chromium.org, xiyuan@chromium.org

[weidongg, 2017-06-23 01:31:15]

Description was changed from

==========
Show separator in SearchResultTileItemListView

This CL makes following changes:
1. Add ResultType enum to SearchResult.
2. Add separator between SearchResults of different ResultType in
SearchResultTileItemListView.

The assumption is that the SearchResults are already grouped based on
the ResultType, so we don't need to sort them in this CL.

BUG=736157
==========

to

==========
Show separator in SearchResultTileItemListView

This CL makes following changes:
1. Add ResultType enum to SearchResult.
2. Add separator between SearchResults of different ResultType in
SearchResultTileItemListView.

The assumption is that the SearchResults are already grouped based on
the ResultType, so we don't need to sort them in this CL.

specs: https://screenshot.googleplex.com/5FKt8ZRembk
screenshot: https://screenshot.googleplex.com/L8URwj8bWQo

BUG=736157
==========

[xiyuan, 2017-06-23 15:43:09]

https://codereview.chromium.org/2949413002/diff/1/ui/app_list/search_result.h
File ui/app_list/search_result.h (right):

https://codereview.chromium.org/2949413002/diff/1/ui/app_list/search_result.h...
ui/app_list/search_result.h:59: RESULT_CONTACT,        // Contacts.
People search is removed. There is no RESULT_CONTACT anymore.

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

https://codereview.chromium.org/2949413002/diff/1/ui/app_list/views/search_re...
ui/app_list/views/search_result_separator.cc:20:
SearchResultSeparator::SearchResultSeparator() {
This class seems unnecessary. Can we achieve the same effect by using the
existing Separator class?

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

https://codereview.chromium.org/2949413002/diff/1/ui/app_list/views/search_re...
ui/app_list/views/search_result_tile_item_list_view.cc:104:
RemoveAllChildViews(true);
Removing then adding views is heavy and could result in UI flickering. That is
why current code pre-creates the tile views and update them in-place.

How about keeping this, adding separators between all tiles initially and
updating separators visibility here (and Layout())?

[weidongg, 2017-06-23 18:32:20]

https://codereview.chromium.org/2949413002/diff/1/ui/app_list/search_result.h
File ui/app_list/search_result.h (right):

https://codereview.chromium.org/2949413002/diff/1/ui/app_list/search_result.h...
ui/app_list/search_result.h:59: RESULT_CONTACT,        // Contacts.
On 2017/06/23 15:43:08, xiyuan wrote:
> People search is removed. There is no RESULT_CONTACT anymore.

Done.

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

https://codereview.chromium.org/2949413002/diff/1/ui/app_list/views/search_re...
ui/app_list/views/search_result_separator.cc:20:
SearchResultSeparator::SearchResultSeparator() {
On 2017/06/23 15:43:09, xiyuan wrote:
> This class seems unnecessary. Can we achieve the same effect by using the
> existing Separator class?

Using Separator::SetPreferredHeight to set height does not seem to work. The
separator's height always matches the parent's height. Should I make change in
the Separator class to force the height to be preferred height?

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

https://codereview.chromium.org/2949413002/diff/1/ui/app_list/views/search_re...
ui/app_list/views/search_result_tile_item_list_view.cc:104:
RemoveAllChildViews(true);
On 2017/06/23 15:43:09, xiyuan wrote:
> Removing then adding views is heavy and could result in UI flickering. That is
> why current code pre-creates the tile views and update them in-place.
> 
> How about keeping this, adding separators between all tiles initially and
> updating separators visibility here (and Layout())? 

That's a better idea. Thanks. Done.

[xiyuan, 2017-06-23 20:09:01]

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

https://codereview.chromium.org/2949413002/diff/1/ui/app_list/views/search_re...
ui/app_list/views/search_result_separator.cc:20:
SearchResultSeparator::SearchResultSeparator() {
On 2017/06/23 18:32:20, weidongg wrote:
> On 2017/06/23 15:43:09, xiyuan wrote:
> > This class seems unnecessary. Can we achieve the same effect by using the
> > existing Separator class?
> 
> Using Separator::SetPreferredHeight to set height does not seem to work. The
> separator's height always matches the parent's height. Should I make change in
> the Separator class to force the height to be preferred height?

Preferred height is just preferred and parent can choose to ignore it. For our
case here, we need need top/bottom padding. We can add that with the
CreateEmptyBorder call.

[weidongg, 2017-06-23 21:05:23]

Thanks for the review. Uploaded a new patch to replace SearchResultSeparator
with Separator.

[xiyuan, 2017-06-23 21:30:38]

lgtm

[weidongg, 2017-06-23 21:38:41]

The CQ bit was checked by weidongg@chromium.org

[commit-bot: I haz the power, 2017-06-23 21:39:12]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-06-23 22:14:29]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1498253921697550,
"parent_rev": "c685d499f1a3b958c039c79c8456792746ce85bd", "commit_rev":
"198b9e6b983a55bcce7bc299a4fde8c8611ef6f8"}

[commit-bot: I haz the power, 2017-06-23 22:14:42]

Description was changed from

==========
Show separator in SearchResultTileItemListView

This CL makes following changes:
1. Add ResultType enum to SearchResult.
2. Add separator between SearchResults of different ResultType in
SearchResultTileItemListView.

The assumption is that the SearchResults are already grouped based on
the ResultType, so we don't need to sort them in this CL.

specs: https://screenshot.googleplex.com/5FKt8ZRembk
screenshot: https://screenshot.googleplex.com/L8URwj8bWQo

BUG=736157
==========

to

==========
Show separator in SearchResultTileItemListView

This CL makes following changes:
1. Add ResultType enum to SearchResult.
2. Add separator between SearchResults of different ResultType in
SearchResultTileItemListView.

The assumption is that the SearchResults are already grouped based on
the ResultType, so we don't need to sort them in this CL.

specs: https://screenshot.googleplex.com/5FKt8ZRembk
screenshot: https://screenshot.googleplex.com/L8URwj8bWQo

BUG=736157

Review-Url: https://codereview.chromium.org/2949413002
Cr-Commit-Position: refs/heads/master@{#482045}
Committed:
https://chromium.googlesource.com/chromium/src/+/198b9e6b983a55bcce7bc299a4fd...
==========

[commit-bot: I haz the power, 2017-06-23 22:14:43]

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