Add a SearchResultPageView to the experimental app list.

Add a SearchResultPageView to the experimental app list.

This CL adds a SearchResultPageView which hosts the search results for
the experimental app list. Previously the search results were part of
the StartPageView but the new design necessitates a separate View.

This separation has also allowed some simplification in the
ContentsView which was tracking the search result state independently
of the other app list states.

The StartPageView will be cleaned up in a follow-up CL.

BUG=425444
Committed: https://crrev.com/8e9c4026e0ddad6915b29c252ee7f061b70c50a0
Cr-Commit-Position: refs/heads/master@{#303425}

[calamity, 2014-10-29 02:42:42]

calamity@chromium.org changed reviewers:
+ mgiuca@chromium.org

[Matt Giuca, 2014-10-29 03:40:53]

Initial comments.

https://codereview.chromium.org/660463003/diff/20001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_view_unittest.cc (right):

https://codereview.chromium.org/660463003/diff/20001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view_unittest.cc:131: bool
ShowContentsViewPageAndVerify(AppListModel::State state);
Why did you change to bools now? Is it just because you get the failure messages
on the correct line, or is there actually a reason you needed it here?

(Just interested.. I'm happy either way.)

You should rename these methods, though, since they are no longer verifying.
Also update their documentation.

ShowContentsViewPageAndVerify -> ShowContentsViewPage (document that it returns
true if the page is now active).
VerifyPageActive -> IsPageActive.

May as well also s/Page/State here.

Note that it might make more sense to just make ShowContentsViewPage void, and
force all callers to explicitly do:
ShowContentsViewPage(state)
EXPECT_TRUE(IsPageActive(state))

That would be a more conventional test pattern: do action; verify result.

https://codereview.chromium.org/660463003/diff/20001/ui/app_list/views/conten...
File ui/app_list/views/contents_view.h (right):

https://codereview.chromium.org/660463003/diff/20001/ui/app_list/views/conten...
ui/app_list/views/contents_view.h:165: SearchResultListView*
search_results_list_view_;
Is it now the case that search_results_list_view_ is only used by classic
launcher, and search_results_page_view_ is only used by experimental launcher?
Please document if so. (Otherwise, I'm not sure why you have both here.)

[calamity, 2014-10-29 06:27:53]

https://codereview.chromium.org/660463003/diff/20001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_view_unittest.cc (right):

https://codereview.chromium.org/660463003/diff/20001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view_unittest.cc:131: bool
ShowContentsViewPageAndVerify(AppListModel::State state);
On 2014/10/29 03:40:53, Matt Giuca wrote:
> Why did you change to bools now? Is it just because you get the failure
messages
> on the correct line, or is there actually a reason you needed it here?
> 
> (Just interested.. I'm happy either way.)
> 
Yah, got a failure, got angry trying to find it.
> You should rename these methods, though, since they are no longer verifying.
> Also update their documentation.
> 
> ShowContentsViewPageAndVerify -> ShowContentsViewPage (document that it
returns
> true if the page is now active).
> VerifyPageActive -> IsPageActive.
> 
> May as well also s/Page/State here.
> 
Done.
> Note that it might make more sense to just make ShowContentsViewPage void,
and
> force all callers to explicitly do:
> ShowContentsViewPage(state)
> EXPECT_TRUE(IsPageActive(state))
> 
> That would be a more conventional test pattern: do action; verify result.

I'd prefer to leave them bundled so that it's impossible to forget to verify.

https://codereview.chromium.org/660463003/diff/20001/ui/app_list/views/conten...
File ui/app_list/views/contents_view.h (right):

https://codereview.chromium.org/660463003/diff/20001/ui/app_list/views/conten...
ui/app_list/views/contents_view.h:165: SearchResultListView*
search_results_list_view_;
On 2014/10/29 03:40:53, Matt Giuca wrote:
> Is it now the case that search_results_list_view_ is only used by classic
> launcher, and search_results_page_view_ is only used by experimental launcher?
> Please document if so. (Otherwise, I'm not sure why you have both here.)
Done.

[Matt Giuca, 2014-11-04 07:44:37]

lgtm with nits

https://codereview.chromium.org/660463003/diff/40001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_view_unittest.cc (right):

https://codereview.chromium.org/660463003/diff/40001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view_unittest.cc:523:
EXPECT_EQ(AppListModel::STATE_SEARCH_RESULTS,
This line appears in both branches and can be moved out.

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

https://codereview.chromium.org/660463003/diff/40001/ui/app_list/views/conten...
ui/app_list/views/contents_view.cc:32: : apps_container_view_(NULL),
Good catch. This was actually quite bad: there is quite a lot of code in between
the call to ContentsView's constructor and Init() which finally initializes this
value.

Can you use nullptr for all of these instead of NULL.

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

https://codereview.chromium.org/660463003/diff/40001/ui/app_list/views/search...
ui/app_list/views/search_result_page_view.cc:56:
tiles_container_->SetLayoutManager(tiles_layout_manager);
nit: Feels cramped; make some blank lines.

https://codereview.chromium.org/660463003/diff/40001/ui/app_list/views/search...
ui/app_list/views/search_result_page_view.cc:72: results(),
SearchResult::DISPLAY_TILE, kNumSearchResultTiles);
nit: ditto

https://codereview.chromium.org/660463003/diff/40001/ui/app_list/views/search...
ui/app_list/views/search_result_page_view.cc:74: SearchResult* item = NULL;
nullptr

https://codereview.chromium.org/660463003/diff/40001/ui/app_list/views/search...
ui/app_list/views/search_result_page_view.cc:76: item = display_results[i];
I think this can read nicer as:
SearchResult* item = i < display_results.size() ? display_results[i] : nullptr;

https://codereview.chromium.org/660463003/diff/40001/ui/app_list/views/search...
File ui/app_list/views/search_result_page_view.h (right):

https://codereview.chromium.org/660463003/diff/40001/ui/app_list/views/search...
ui/app_list/views/search_result_page_view.h:30: virtual ~SearchResultPageView();
~SearchResultPageView() override;

[calamity, 2014-11-06 04:55:02]

https://codereview.chromium.org/660463003/diff/40001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_view_unittest.cc (right):

https://codereview.chromium.org/660463003/diff/40001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view_unittest.cc:523:
EXPECT_EQ(AppListModel::STATE_SEARCH_RESULTS,
On 2014/11/04 07:44:37, Matt Giuca wrote:
> This line appears in both branches and can be moved out.

Done.

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

https://codereview.chromium.org/660463003/diff/40001/ui/app_list/views/conten...
ui/app_list/views/contents_view.cc:32: : apps_container_view_(NULL),
On 2014/11/04 07:44:37, Matt Giuca wrote:
> Good catch. This was actually quite bad: there is quite a lot of code in
between
> the call to ContentsView's constructor and Init() which finally initializes
this
> value.
> 
> Can you use nullptr for all of these instead of NULL.

Done.

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

https://codereview.chromium.org/660463003/diff/40001/ui/app_list/views/search...
ui/app_list/views/search_result_page_view.cc:56:
tiles_container_->SetLayoutManager(tiles_layout_manager);
On 2014/11/04 07:44:37, Matt Giuca wrote:
> nit: Feels cramped; make some blank lines.

Done.

https://codereview.chromium.org/660463003/diff/40001/ui/app_list/views/search...
ui/app_list/views/search_result_page_view.cc:72: results(),
SearchResult::DISPLAY_TILE, kNumSearchResultTiles);
On 2014/11/04 07:44:37, Matt Giuca wrote:
> nit: ditto

Done.

https://codereview.chromium.org/660463003/diff/40001/ui/app_list/views/search...
ui/app_list/views/search_result_page_view.cc:74: SearchResult* item = NULL;
On 2014/11/04 07:44:37, Matt Giuca wrote:
> nullptr

Done.

https://codereview.chromium.org/660463003/diff/40001/ui/app_list/views/search...
ui/app_list/views/search_result_page_view.cc:76: item = display_results[i];
On 2014/11/04 07:44:37, Matt Giuca wrote:
> I think this can read nicer as:
> SearchResult* item = i < display_results.size() ? display_results[i] :
nullptr;

Done.

https://codereview.chromium.org/660463003/diff/40001/ui/app_list/views/search...
File ui/app_list/views/search_result_page_view.h (right):

https://codereview.chromium.org/660463003/diff/40001/ui/app_list/views/search...
ui/app_list/views/search_result_page_view.h:30: virtual ~SearchResultPageView();
On 2014/11/04 07:44:37, Matt Giuca wrote:
> ~SearchResultPageView() override;

Done.

[calamity, 2014-11-10 03:45:19]

Patchset #4 (id:60001) has been deleted

[calamity, 2014-11-10 04:05:52]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-11-10 04:06:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/660463003/80001

[commit-bot: I haz the power, 2014-11-10 05:14:46]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2014-11-10 05:15:18]

Patchset 4 (id:??) landed as
https://crrev.com/8e9c4026e0ddad6915b29c252ee7f061b70c50a0
Cr-Commit-Position: refs/heads/master@{#303425}