Allow app list tiles to show search results in the experimental app list.

Allow app list tiles to show search results in the experimental app list.

This CL makes app results show as tiles in the experimental app list by
making TileItemViews take SearchResults instead of AppListItems and by
introducing a DisplayType for SearchResults which determines which
surface the result will display on.

Only 4 tiles appear at any one time. This will be fixed in a future
patch.

This CL also has the side effect of fixing a focus bug which was
happening because StartPageView was listening to the AppListModel.

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

[Matt Giuca, 2014-08-06 08:54:53]

So it looks like the plan to split this up has at least 3 parts:
1. TileItemView, fixing the little update bug without switching over from
AppListItem to SearchResult.
2. Switching AppListItems over to SearchResults, but not having an Update()
method, so you don't actually get any new behaviour -- only recommendations, no
search results.
3. Implementing dynamically updating tiles from search results.

https://codereview.chromium.org/439703002/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/app_result.cc (right):

https://codereview.chromium.org/439703002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/app_result.cc:40:
set_display_type(DISPLAY_TILE);
I assume this will have no effect if !IsExperimentalAppListEnabled(). Still,
best to put it in an if statement so it's clear that it only affects
experimental.

(Otherwise, reading this code is confusing because it always sets DISPLAY_TILE
even though later it uses EXTENSION_ICON_SMALL.)

https://codereview.chromium.org/439703002/diff/40001/ui/app_list/app_list_mod...
File ui/app_list/app_list_model.h (right):

https://codereview.chromium.org/439703002/diff/40001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.h:122: static std::vector<SearchResult*>
GetSearchResultsForDisplayType(
Rename to FilterSearchResultsByDisplayType. ("Get" implies that it is fetching
them from somewhere else, whereas this just munges its own input.)

https://codereview.chromium.org/439703002/diff/40001/ui/app_list/search_resul...
File ui/app_list/search_result_observer.h (right):

https://codereview.chromium.org/439703002/diff/40001/ui/app_list/search_resul...
ui/app_list/search_result_observer.h:15: virtual void OnIconChanged() {}
Hooray! Death to = 0 on observers.

https://codereview.chromium.org/439703002/diff/40001/ui/app_list/search_resul...
ui/app_list/search_result_observer.h:32: virtual void OnResultDestroying() {}
// Invoked when ...

https://codereview.chromium.org/439703002/diff/40001/ui/app_list/views/conten...
File ui/app_list/views/contents_view.cc (right):

https://codereview.chromium.org/439703002/diff/40001/ui/app_list/views/conten...
ui/app_list/views/contents_view.cc:19: #include
"ui/app_list/views/search_box_view.h"
nit: Is this necessary?

https://codereview.chromium.org/439703002/diff/40001/ui/app_list/views/search...
File ui/app_list/views/search_result_list_view.cc (right):

https://codereview.chromium.org/439703002/diff/40001/ui/app_list/views/search...
ui/app_list/views/search_result_list_view.cc:178: if (i <
display_results.size())
I think I prefer the old formulation of:

if (i < display_results.size()) {
  result_view->SetResult(display_results[i]);
  result_view->SetVisible(true);
} else {
  result_view->SetResult(NULL);
  result_view->SetVisible(false);
}

It's clearer than inventing a new variable just to write it conditionally, and
then act on it conditionally.

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

https://codereview.chromium.org/439703002/diff/40001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.h:70: // Overriden from ui::ListModelObserver:
nit: Overridden (you specifically introduced a spelling mistake?)

https://codereview.chromium.org/439703002/diff/40001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.h:94: base::WeakPtrFactory<StartPageView>
update_factory_;
// ScheduleUpdate() generates a single weak pointer; if one exists then an
update is pending. Further calls to ScheduleUpdate() will have no effect. Once
the Update() completes, the weak pointer is invalidated.

Or something.

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

https://codereview.chromium.org/439703002/diff/40001/ui/app_list/views/tile_i...
ui/app_list/views/tile_item_view.cc:129:
!item->icon().BackedBySameObjectAs(old_item->icon())) {
Do this for AppListItem in a separate patch. (You address the above TODO in a
nice clean small patch.)

[calamity, 2014-08-12 05:03:37]

https://codereview.chromium.org/439703002/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/app_result.cc (right):

https://codereview.chromium.org/439703002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/app_result.cc:40:
set_display_type(DISPLAY_TILE);
On 2014/08/06 08:54:53, Matt Giuca wrote:
> I assume this will have no effect if !IsExperimentalAppListEnabled(). Still,
> best to put it in an if statement so it's clear that it only affects
> experimental.
> 
> (Otherwise, reading this code is confusing because it always sets DISPLAY_TILE
> even though later it uses EXTENSION_ICON_SMALL.)

Done.

https://codereview.chromium.org/439703002/diff/40001/ui/app_list/app_list_mod...
File ui/app_list/app_list_model.h (right):

https://codereview.chromium.org/439703002/diff/40001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.h:122: static std::vector<SearchResult*>
GetSearchResultsForDisplayType(
On 2014/08/06 08:54:53, Matt Giuca wrote:
> Rename to FilterSearchResultsByDisplayType. ("Get" implies that it is fetching
> them from somewhere else, whereas this just munges its own input.)

Done.

https://codereview.chromium.org/439703002/diff/40001/ui/app_list/search_resul...
File ui/app_list/search_result_observer.h (right):

https://codereview.chromium.org/439703002/diff/40001/ui/app_list/search_resul...
ui/app_list/search_result_observer.h:15: virtual void OnIconChanged() {}
On 2014/08/06 08:54:53, Matt Giuca wrote:
> Hooray! Death to = 0 on observers.

Acknowledged.

https://codereview.chromium.org/439703002/diff/40001/ui/app_list/search_resul...
ui/app_list/search_result_observer.h:32: virtual void OnResultDestroying() {}
On 2014/08/06 08:54:53, Matt Giuca wrote:
> // Invoked when ...

Done.

https://codereview.chromium.org/439703002/diff/40001/ui/app_list/views/conten...
File ui/app_list/views/contents_view.cc (right):

https://codereview.chromium.org/439703002/diff/40001/ui/app_list/views/conten...
ui/app_list/views/contents_view.cc:19: #include
"ui/app_list/views/search_box_view.h"
On 2014/08/06 08:54:53, Matt Giuca wrote:
> nit: Is this necessary?

Done.

https://codereview.chromium.org/439703002/diff/40001/ui/app_list/views/search...
File ui/app_list/views/search_result_list_view.cc (right):

https://codereview.chromium.org/439703002/diff/40001/ui/app_list/views/search...
ui/app_list/views/search_result_list_view.cc:178: if (i <
display_results.size())
On 2014/08/06 08:54:53, Matt Giuca wrote:
> I think I prefer the old formulation of:
> 
> if (i < display_results.size()) {
>   result_view->SetResult(display_results[i]);
>   result_view->SetVisible(true);
> } else {
>   result_view->SetResult(NULL);
>   result_view->SetVisible(false);
> }
> 
> It's clearer than inventing a new variable just to write it conditionally, and
> then act on it conditionally.

Done.

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

https://codereview.chromium.org/439703002/diff/40001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.h:70: // Overriden from ui::ListModelObserver:
On 2014/08/06 08:54:53, Matt Giuca wrote:
> nit: Overridden (you specifically introduced a spelling mistake?)

Done.

https://codereview.chromium.org/439703002/diff/40001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.h:94: base::WeakPtrFactory<StartPageView>
update_factory_;
On 2014/08/06 08:54:53, Matt Giuca wrote:
> // ScheduleUpdate() generates a single weak pointer; if one exists then an
> update is pending. Further calls to ScheduleUpdate() will have no effect. Once
> the Update() completes, the weak pointer is invalidated.
> 
> Or something.

Done.

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

https://codereview.chromium.org/439703002/diff/40001/ui/app_list/views/tile_i...
ui/app_list/views/tile_item_view.cc:129:
!item->icon().BackedBySameObjectAs(old_item->icon())) {
On 2014/08/06 08:54:53, Matt Giuca wrote:
> Do this for AppListItem in a separate patch. (You address the above TODO in a
> nice clean small patch.)

As discussed, doing this for AppListItems could have weird consequences. It's
safer to just do here.

[Matt Giuca, 2014-08-12 06:46:45]

lgtm

[calamity, 2014-08-13 08:50:46]

+mukai for athena/

Please note that this change breaks the start page view tiles for athena. To fix
this, a new search provider that provides DISPLAY_TILE results needs to be
registered. (This CL provides a good example of how to do that)

[Jun Mukai, 2014-08-13 15:47:23]

athena lgtm

I'm thinking to stop using tile_item_view because our latest design mock looks
completely different. More precisely, I will remove bottom_home_view files
themselves soon. I think it will happen in a few days (the CL is not ready yet),
then you don't have to do this.

But please go ahead if you don't want to wait for that.

[calamity, 2014-08-14 00:28:26]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-08-14 00:32:55]

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

[calamity, 2014-08-14 00:47:34]

The CQ bit was unchecked by calamity@chromium.org

[calamity, 2014-08-14 00:47:35]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-08-14 00:49:30]

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

[calamity, 2014-08-14 01:07:13]

The CQ bit was unchecked by calamity@chromium.org

[Matt Giuca, 2014-08-14 02:54:22]

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

https://codereview.chromium.org/439703002/diff/140001/ui/app_list/views/start...
ui/app_list/views/start_page_view.h:50: friend class
test::AppListViewTestContext;
As discussed, instead of friending, add a public method UpdateForTests() which
just calls Update().

Rationale: Official reason = Because as much stuff should be in the anon
namespace as possible; it isn't worth having so much stuff outside of the
namespace just so we can be friends.
Real (selfish) reason = I don't want a huge diff in this CL where you move
everything out of the anonymous namespace.

But I think the official reason is still good enough.

[calamity, 2014-08-14 04:28:05]

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

https://codereview.chromium.org/439703002/diff/140001/ui/app_list/views/start...
ui/app_list/views/start_page_view.h:50: friend class
test::AppListViewTestContext;
On 2014/08/14 02:54:22, Matt Giuca wrote:
> As discussed, instead of friending, add a public method UpdateForTests() which
> just calls Update().
> 
> Rationale: Official reason = Because as much stuff should be in the anon
> namespace as possible; it isn't worth having so much stuff outside of the
> namespace just so we can be friends.
> Real (selfish) reason = I don't want a huge diff in this CL where you move
> everything out of the anonymous namespace.
> 
> But I think the official reason is still good enough.

Done.

[calamity, 2014-08-14 04:28:10]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-08-14 04:30:26]

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

[Matt Giuca, 2014-08-14 04:51:56]

https://codereview.chromium.org/439703002/diff/200001/ui/app_list/views/app_l...
File ui/app_list/views/app_list_view_unittest.cc (right):

https://codereview.chromium.org/439703002/diff/200001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view_unittest.cc:324:
EXPECT_FALSE(main_view->search_box_view()->visible());
nit: Maybe put this back with expect(0, ...).

Maybe do it just before you call Add instead of here.

If you think it's not immediaely clear that it starts empty.

https://codereview.chromium.org/439703002/diff/200001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view_unittest.cc:437:
original_test_model->results()->Add(new TestTileSearchResult());
Why are there two of this?

[calamity, 2014-08-14 05:42:37]

https://codereview.chromium.org/439703002/diff/200001/ui/app_list/views/app_l...
File ui/app_list/views/app_list_view_unittest.cc (right):

https://codereview.chromium.org/439703002/diff/200001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view_unittest.cc:324:
EXPECT_FALSE(main_view->search_box_view()->visible());
On 2014/08/14 04:51:56, Matt Giuca wrote:
> nit: Maybe put this back with expect(0, ...).
> 
> Maybe do it just before you call Add instead of here.
> 
> If you think it's not immediaely clear that it starts empty.

I think a zero check here is unnecessary.

https://codereview.chromium.org/439703002/diff/200001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view_unittest.cc:437:
original_test_model->results()->Add(new TestTileSearchResult());
On 2014/08/14 04:51:56, Matt Giuca wrote:
> Why are there two of this?

To differentiate it from the single result model above (otherwise the
EXPECT_EQ(1u, ...) below would pass even if this was wrong. This used to have a
different number from the actual model which is why this wasn't necessary.

[Matt Giuca, 2014-08-14 05:45:51]

slgtm

https://codereview.chromium.org/439703002/diff/200001/ui/app_list/views/app_l...
File ui/app_list/views/app_list_view_unittest.cc (right):

https://codereview.chromium.org/439703002/diff/200001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view_unittest.cc:324:
EXPECT_FALSE(main_view->search_box_view()->visible());
On 2014/08/14 05:42:37, calamity wrote:
> On 2014/08/14 04:51:56, Matt Giuca wrote:
> > nit: Maybe put this back with expect(0, ...).
> > 
> > Maybe do it just before you call Add instead of here.
> > 
> > If you think it's not immediaely clear that it starts empty.
> 
> I think a zero check here is unnecessary.

Acknowledged.

https://codereview.chromium.org/439703002/diff/200001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view_unittest.cc:437:
original_test_model->results()->Add(new TestTileSearchResult());
On 2014/08/14 05:42:37, calamity wrote:
> On 2014/08/14 04:51:56, Matt Giuca wrote:
> > Why are there two of this?
> 
> To differentiate it from the single result model above (otherwise the
> EXPECT_EQ(1u, ...) below would pass even if this was wrong. This used to have
a
> different number from the actual model which is why this wasn't necessary.

Acknowledged.

[commit-bot: I haz the power, 2014-08-14 10:34:06]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-14 11:01:22]

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

[commit-bot: I haz the power, 2014-08-14 11:01:23]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[calamity, 2014-08-15 00:49:33]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-08-15 00:50:52]

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

[Jun Mukai, 2014-08-15 00:56:15]

On 2014/08/15 00:50:52, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-status.appspot.com/cq/calamity@chromium.org/439703002/200001

I've submitted my CL to remove bottom_home_view. You don't have to edit it
anymore.

[commit-bot: I haz the power, 2014-08-15 01:23:17]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[calamity, 2014-08-15 01:43:13]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-08-15 01:48:14]

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

[calamity, 2014-08-15 02:47:10]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-08-15 02:51:12]

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

[commit-bot: I haz the power, 2014-08-15 09:20:50]

Committed patchset #9 (240001) as 289826