Add search results to experimental app list start page.

Add search results to experimental app list start page.

This CL moves the SearchResultListView into the StartPageView for
the experimental app list.

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

[tapted, 2014-06-04 06:12:43]

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

https://codereview.chromium.org/307333002/diff/30001/ui/app_list/views/conten...
ui/app_list/views/contents_view.cc:124: else if (show_state_ ==
SHOW_SEARCH_RESULTS)
Do you need to trigger a start_page-view->results_view->UpdateAutoLaunchState()
when show_state_ != SHOW_SEARCH_RESULTS

it looks like StartPageView only does it when it becomes visible. Maybe here you
need

GetStartPageView(view_model_.get())->SetShowState(show_state_)

with appropriate handling, which StartPageView::Reset() also calls

https://codereview.chromium.org/307333002/diff/30001/ui/app_list/views/conten...
ui/app_list/views/contents_view.cc:151: case SHOW_SEARCH_RESULTS:  // Falls
through.
nit: might be slightly less weird to leave this the way it was

https://codereview.chromium.org/307333002/diff/30001/ui/app_list/views/conten...
ui/app_list/views/contents_view.cc:218: 
nit: move this blank line up after the return

https://codereview.chromium.org/307333002/diff/30001/ui/app_list/views/conten...
ui/app_list/views/contents_view.cc:236: else
nit: no else after a return statement

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

https://codereview.chromium.org/307333002/diff/30001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.cc:164: return
results_view_->OnKeyPressed(event);
this seems a bit odd here and might have accessibility implications - can you
give the results view keyboard focus when it becomes visible?

[calamity, 2014-06-12 05:16:56]

Rebased on top of the named pages refactor and added a test for showing search
results.

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

https://codereview.chromium.org/307333002/diff/30001/ui/app_list/views/conten...
ui/app_list/views/contents_view.cc:124: else if (show_state_ ==
SHOW_SEARCH_RESULTS)
On 2014/06/04 06:12:43, tapted wrote:
> Do you need to trigger a
start_page-view->results_view->UpdateAutoLaunchState()
> when show_state_ != SHOW_SEARCH_RESULTS
> 
> it looks like StartPageView only does it when it becomes visible. Maybe here
you
> need
> 
> GetStartPageView(view_model_.get())->SetShowState(show_state_)
> 
> with appropriate handling, which StartPageView::Reset() also calls

Done.

https://codereview.chromium.org/307333002/diff/30001/ui/app_list/views/conten...
ui/app_list/views/contents_view.cc:151: case SHOW_SEARCH_RESULTS:  // Falls
through.
On 2014/06/04 06:12:43, tapted wrote:
> nit: might be slightly less weird to leave this the way it was

This is gone.

https://codereview.chromium.org/307333002/diff/30001/ui/app_list/views/conten...
ui/app_list/views/contents_view.cc:218: 
On 2014/06/04 06:12:43, tapted wrote:
> nit: move this blank line up after the return

Also gone.

https://codereview.chromium.org/307333002/diff/30001/ui/app_list/views/conten...
ui/app_list/views/contents_view.cc:236: else
On 2014/06/04 06:12:43, tapted wrote:
> nit: no else after a return statement

Gone.

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

https://codereview.chromium.org/307333002/diff/30001/ui/app_list/views/start_...
ui/app_list/views/start_page_view.cc:164: return
results_view_->OnKeyPressed(event);
On 2014/06/04 06:12:43, tapted wrote:
> this seems a bit odd here and might have accessibility implications - can you
> give the results view keyboard focus when it becomes visible?

The SearchResultListView sends accessibility events appropriately. Since this is
how the keypresses are plumbed to the results view in the normal app list's
ContentsView, I don't think there would be a problem here.

[tapted, 2014-06-12 05:43:28]

lgtm

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

https://codereview.chromium.org/307333002/diff/170001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view_unittest.cc:377:
contents_view->ShowSearchResults(true);
perhaps before this, something like

if (test_type_ == EXPERIMENTAL) {
  EXPECT_FALSE(main_view->search_box_view()->visible())
} else {
  EXPECT_TRUE(main_view->search_box_view()->visible())
}

https://codereview.chromium.org/307333002/diff/170001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view_unittest.cc:405: }
EXPECT_TRUE(main_view->search_box_view()->visible()); ?

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

https://codereview.chromium.org/307333002/diff/170001/ui/app_list/views/start...
ui/app_list/views/start_page_view.cc:155: if (show_state_ == show_state)
Note I think this will early exit when Reset() is called from the constructor
(via SetModel), but at a glance that looks OK to do.. exccept maybe for a
missing results_view_->SetVisible(false)?

[calamity, 2014-06-12 07:09:38]

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

https://codereview.chromium.org/307333002/diff/170001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view_unittest.cc:377:
contents_view->ShowSearchResults(true);
On 2014/06/12 05:43:28, tapted wrote:
> perhaps before this, something like
> 
> if (test_type_ == EXPERIMENTAL) {
>   EXPECT_FALSE(main_view->search_box_view()->visible())
> } else {
>   EXPECT_TRUE(main_view->search_box_view()->visible())
> }
> 

I set the page to the apps page to have a consistent base page for this test.
The case of the start page's search box not being visible is handled in the
start page test.

Added a check that the apps page always starts with a visible search box.

https://codereview.chromium.org/307333002/diff/170001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view_unittest.cc:405: }
On 2014/06/12 05:43:28, tapted wrote:
> EXPECT_TRUE(main_view->search_box_view()->visible()); ?

Done.

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

https://codereview.chromium.org/307333002/diff/170001/ui/app_list/views/start...
ui/app_list/views/start_page_view.cc:155: if (show_state_ == show_state)
On 2014/06/12 05:43:28, tapted wrote:
> Note I think this will early exit when Reset() is called from the constructor
> (via SetModel), but at a glance that looks OK to do.. exccept maybe for a
> missing results_view_->SetVisible(false)?

Yep. That is completely necessary. Moved that up.

[calamity, 2014-06-12 07:09:45]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-06-12 07:17:04]

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

[commit-bot: I haz the power, 2014-06-12 13:12:37]

Change committed as 276655