app_list: Add search box and search result view for v2.

app_list: Add search box and search result view for v2. 

- Add a SearchBoxModel that represents search box's icon, placeholder text,
  user typed text and selection model (current selection and cursor position);
- Add a SearchBoxView that displays SearchBoxModel and observes SearchBoxModel
  changes via SearchBoxModelObserver;
  When user typed/changed text in search box, the view also fires query changed
  notification to its SearchBoxViewDelegate;
- Add a SearchResult that carries an icon, two tagged texts;
- Add a SearchResultView that displays a SearchResult;
- Change AppListModel to be a master model that has three sub models:
  apps list, search box and search results;
- Add a SearchResultListView that display search results sub model of
  AppListModel using a list of SeachResultView;
  The view supports up/down key navigation and when user selects a result
  via mouse click or enter key, it asks its SearchResultListViewDelegate to
  OpenResult;
- Update AppListViewDelegate:
  - Add new methods: StartSearch, StopSearch and OpenResult;
  - Deleted no-longer needed UpdateModel method;
  - Rename OnAppListItemActivated -> ActivateAppListItem;
- Update ash_shell's app_list to support a simple substr match search;
- Add a SearchBuilder that implements the search via AutoCompleteController;
- Other changes:
  - Rename AppListModelBuilder -> AppsModelBuilder;
  - Rename AppListModelView -> AppsGridView;

BUG=125964
TEST=Manual. In ash_shell, it searches the 5 example apps. In ash/chrome, it should show similar results as omnibox.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=138938

[xiyuan, 2012-05-22 17:46:40]

Sorry for the big CL. But it is not a complex one and it is big because a lot of
tiny things added up.

Here's a little briefing besides the CL description:
- For search box, we have SearchBoxModel whose UI is SearchBoxView
  and a QueryChanged notification is delivered to SearchBoxViewDelegate
  when user changes text in the box;
- AppListView is a SearchBoxViewDelegate. When it gets this notification,
  it calls StartSearch (or StopSearch when query is empty) on
AppListViewDelegate;
- AppListViewDelegate does the actual search and populate
AppListModel::SearchResults;
- AppListModel::SearchResults is displayed by SeachResultListView and each
  SearchResult is displayed by a SearchResultView;
- When user clicks or hit enter on a result, SearchResultListView calls
  OpenResult on SearchResultListViewDelegate;
- This OpenResult call goes to AppListView since it is also a
  SearchResultListViewDelegate. And AppListView would forward it to
  its AppListViewDelegate;

Please take a look. Thanks.

[sky, 2012-05-22 22:16:06]

http://codereview.chromium.org/10386224/diff/8046/ash/shell/app_list.cc
File ash/shell/app_list.cc (right):

http://codereview.chromium.org/10386224/diff/8046/ash/shell/app_list.cc#newco...
ash/shell/app_list.cc:81: static std::string GetDetails(Type type) {
Add a comment as to why this doesn't need to be localized.

http://codereview.chromium.org/10386224/diff/8046/ash/shell/app_list.cc#newco...
ash/shell/app_list.cc:137: class ExampleSearchResult : public
app_list::SearchResult {
Add a description

http://codereview.chromium.org/10386224/diff/8046/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10386224/diff/8046/chrome/app/generated_resour...
chrome/app/generated_resources.grd:16185: +        Google Search
Is this always a google search? Does it depend upon your search provider?

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_box_model.h
File ui/app_list/search_box_model.h (right):

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_box_mode...
ui/app_list/search_box_model.h:21: // model. The icon is rendered at side of the
query edit control. The hint text
at -> to the

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_box_mode...
ui/app_list/search_box_model.h:62: SkBitmap icon_;
Use ImageSkia

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_box_view.cc
File ui/app_list/search_box_view.cc (right):

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_box_view...
ui/app_list/search_box_view.cc:30: model_(NULL) {
I think it's worth initializing icon_view_ and search_box_ too.

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_box_view...
ui/app_list/search_box_view.cc:77: UpdateModel();
Shouldn't this only update if the contents actually changed?

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_box_view...
ui/app_list/search_box_view.cc:108: query_timer_.Stop();
If the delegate wants to throttle queries, shouldn't it do it and not here?

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_box_view...
ui/app_list/search_box_view.cc:128: views::View* results_view =
parent()->GetViewByID(VIEW_ID_SEARCH_RESULTS);
This strikes me as very brittle. Could you explicitly set these views?

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_box_view.h
File ui/app_list/search_box_view.h (right):

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_box_view...
ui/app_list/search_box_view.h:26: class SearchBoxView : public views::View,
Add a description.

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result.h
File ui/app_list/search_result.h (right):

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result.h...
ui/app_list/search_result.h:20: // SearchResult consists of an icon, a title
text and a details text. Title and
'SearchResult consists of an icon, a title text and a details text' ->
SearchResult consists of an icon, title text and details text.

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result.h...
ui/app_list/search_result.h:21: // details text could have tagged ranges that
are displayed differently from
could -> can

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result.h...
ui/app_list/search_result.h:47: virtual ~SearchResult();
Why did you make it so that folks subclass this? Since they can't really
override any behavior wouldn't a method to configure a SearchResult for a
particular use be better?

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result.h...
ui/app_list/search_result.h:65: SkBitmap icon_;
ImageSkia

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result_l...
File ui/app_list/search_result_list_view.cc (right):

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result_l...
ui/app_list/search_result_list_view.cc:37: AddChildView(new
SearchResultView(this));
What happens if you there isn't enough space to fit these vertically?

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result_l...
ui/app_list/search_result_list_view.cc:95: if (!update_factory_.HasWeakPtrs()) {
Document why this is throttled.

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result_l...
ui/app_list/search_result_list_view.cc:107: // Result list occupies the same
place as grid view + page switcher.
I don't understand this. The preferred size shouldn't vary depending upon
current size. Also, I don't like looking up views by id. Too brittle.

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result_l...
File ui/app_list/search_result_list_view_delegate.h (right):

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result_l...
ui/app_list/search_result_list_view_delegate.h:15: // Invoked to open given
|result|.
Document what |event_flags| is . Also, if result can never be NULL can it be a
const& ?

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result_v...
File ui/app_list/search_result_view.cc (right):

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result_v...
ui/app_list/search_result_view.cc:41: virtual bool HitTest(const gfx::Point& l)
const OVERRIDE {
Why do you need this?

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result_v...
ui/app_list/search_result_view.cc:195: gfx::Size title_size(text_bounds.width(),
Don't you want the title to always get the same bounds regardless of whether
there is details?

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result_v...
File ui/app_list/search_result_view.h (right):

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result_v...
ui/app_list/search_result_view.h:40: static const char kViewClassName[];
constants like this should be first in a section.

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result_v...
ui/app_list/search_result_view.h:53: bool selected_;
Could you instead get the selection from the ListView? Less to keep in sync that
way.

[xiyuan, 2012-05-23 21:25:22]

CL updated. Addressed all comments and explained why I am currently allowing
subclass SearchResult.

Please take another look.
Thanks.

http://codereview.chromium.org/10386224/diff/8046/ash/shell/app_list.cc
File ash/shell/app_list.cc (right):

http://codereview.chromium.org/10386224/diff/8046/ash/shell/app_list.cc#newco...
ash/shell/app_list.cc:81: static std::string GetDetails(Type type) {
On 2012/05/22 22:16:06, sky wrote:
> Add a comment as to why this doesn't need to be localized.

Done.

http://codereview.chromium.org/10386224/diff/8046/ash/shell/app_list.cc#newco...
ash/shell/app_list.cc:137: class ExampleSearchResult : public
app_list::SearchResult {
On 2012/05/22 22:16:06, sky wrote:
> Add a description

Done.

http://codereview.chromium.org/10386224/diff/8046/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10386224/diff/8046/chrome/app/generated_resour...
chrome/app/generated_resources.grd:16185: +        Google Search
On 2012/05/22 22:16:06, sky wrote:
> Is this always a google search? Does it depend upon your search provider?

This is what's used in the mock. In chrome, we use the same search providers as
Omnibox, which seems default to Google Search.

The hint text is customizable via SearchBoxModel and I used "Type to search" in
ash_shell.

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_box_model.h
File ui/app_list/search_box_model.h (right):

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_box_mode...
ui/app_list/search_box_model.h:21: // model. The icon is rendered at side of the
query edit control. The hint text
On 2012/05/22 22:16:06, sky wrote:
> at -> to the

Done.

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_box_mode...
ui/app_list/search_box_model.h:62: SkBitmap icon_;
On 2012/05/22 22:16:06, sky wrote:
> Use ImageSkia

Done.

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_box_view.cc
File ui/app_list/search_box_view.cc (right):

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_box_view...
ui/app_list/search_box_view.cc:30: model_(NULL) {
On 2012/05/22 22:16:06, sky wrote:
> I think it's worth initializing icon_view_ and search_box_ too.

Done. I agree and I don't know why I forgot this. :p

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_box_view...
ui/app_list/search_box_view.cc:77: UpdateModel();
On 2012/05/22 22:16:06, sky wrote:
> Shouldn't this only update if the contents actually changed?

Done. Moved this call to ContentsChanged handler.

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_box_view...
ui/app_list/search_box_view.cc:108: query_timer_.Stop();
On 2012/05/22 22:16:06, sky wrote:
> If the delegate wants to throttle queries, shouldn't it do it and not here?

Makes sense. Removed.

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_box_view...
ui/app_list/search_box_view.cc:128: views::View* results_view =
parent()->GetViewByID(VIEW_ID_SEARCH_RESULTS);
On 2012/05/22 22:16:06, sky wrote:
> This strikes me as very brittle. Could you explicitly set these views?

Done.

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_box_view.h
File ui/app_list/search_box_view.h (right):

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_box_view...
ui/app_list/search_box_view.h:26: class SearchBoxView : public views::View,
On 2012/05/22 22:16:06, sky wrote:
> Add a description.

Done.

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result.h
File ui/app_list/search_result.h (right):

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result.h...
ui/app_list/search_result.h:20: // SearchResult consists of an icon, a title
text and a details text. Title and
On 2012/05/22 22:16:06, sky wrote:
> 'SearchResult consists of an icon, a title text and a details text' ->
> SearchResult consists of an icon, title text and details text.

Done.

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result.h...
ui/app_list/search_result.h:21: // details text could have tagged ranges that
are displayed differently from
On 2012/05/22 22:16:06, sky wrote:
> could -> can

Done.

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result.h...
ui/app_list/search_result.h:47: virtual ~SearchResult();
On 2012/05/22 22:16:06, sky wrote:
> Why did you make it so that folks subclass this? Since they can't really
> override any behavior wouldn't a method to configure a SearchResult for a
> particular use be better?

Subclass is allowed here because SeachResult is really a simple model that only
carries data needed for UI. AppListDelegate would have problem to OpenResult
with just this amount of data. If we don't allow subclass, then we need to
provide an ID or something so that AppListDelegate could know how to OpenResult.
Using subclass, we could carry extra information with a SearchResult and make it
easy for the delegate to implement open.

In the current CL, both chrome search and ash_shell search is doing it this way.
Chrome search embeds an AutocompleteMatch and ash_shell search embeds a window
launch type. And the delegate could figure out how to do "open" directly from
this extra data in the SearchResult passed back.

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result.h...
ui/app_list/search_result.h:65: SkBitmap icon_;
On 2012/05/22 22:16:06, sky wrote:
> ImageSkia

Done.

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result_l...
File ui/app_list/search_result_list_view.cc (right):

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result_l...
ui/app_list/search_result_list_view.cc:37: AddChildView(new
SearchResultView(this));
On 2012/05/22 22:16:06, sky wrote:
> What happens if you there isn't enough space to fit these vertically?

Hosting AppListView is updated to leave enough vertical space for this. In its
Layout, we keep the max height of grid + page switcher and results now.

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result_l...
ui/app_list/search_result_list_view.cc:95: if (!update_factory_.HasWeakPtrs()) {
On 2012/05/22 22:16:06, sky wrote:
> Document why this is throttled.

Done.

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result_l...
ui/app_list/search_result_list_view.cc:107: // Result list occupies the same
place as grid view + page switcher.
On 2012/05/22 22:16:06, sky wrote:
> I don't understand this. The preferred size shouldn't vary depending upon
> current size. Also, I don't like looking up views by id. Too brittle.

Removed and explicitly layout those views in parent AppListView.

Also replaced view id related code with explicitly set view in SearchBoxView and
AppListBubbleBorder.

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result_l...
File ui/app_list/search_result_list_view_delegate.h (right):

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result_l...
ui/app_list/search_result_list_view_delegate.h:15: // Invoked to open given
|result|.
On 2012/05/22 22:16:06, sky wrote:
> Document what |event_flags| is . Also, if result can never be NULL can it be a
> const& ?

Done.

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result_v...
File ui/app_list/search_result_view.cc (right):

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result_v...
ui/app_list/search_result_view.cc:41: virtual bool HitTest(const gfx::Point& l)
const OVERRIDE {
On 2012/05/22 22:16:06, sky wrote:
> Why do you need this?

If we don't do this, the parent SearchResultView loses its hover state when
mouse is hovering on the icon.

Should we extend ImageView to make this configurable? I used this in
AppListItemView and saw this in LauncherButton::IconView too.

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result_v...
ui/app_list/search_result_view.cc:195: gfx::Size title_size(text_bounds.width(),
On 2012/05/22 22:16:06, sky wrote:
> Don't you want the title to always get the same bounds regardless of whether
> there is details?

Per mock, we should vertically center title text. So need to shift down a bit
when there is no details.

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result_v...
File ui/app_list/search_result_view.h (right):

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result_v...
ui/app_list/search_result_view.h:40: static const char kViewClassName[];
On 2012/05/22 22:16:06, sky wrote:
> constants like this should be first in a section.

Done.

http://codereview.chromium.org/10386224/diff/8046/ui/app_list/search_result_v...
ui/app_list/search_result_view.h:53: bool selected_;
On 2012/05/22 22:16:06, sky wrote:
> Could you instead get the selection from the ListView? Less to keep in sync
that
> way.

Done.

[xiyuan, 2012-05-24 17:18:41]

Sorry if you are looking at patch set 2. :p

CL updated with the following change:
- Hint text update "Google Search" -> "Search";
- Search box preferred height changed to 48px;
- Tile size changed from 80x88 -> 82x90;
- Removed the footer separator and color in page switcher area;
- Page switcher be 36px tall;
- Changed page switcher button to be full rounded rect and draw them in center;

[sky, 2012-05-24 17:34:53]

LGTM

[commit-bot: I haz the power, 2012-05-24 21:11:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiyuan@chromium.org/10386224/44001

[commit-bot: I haz the power, 2012-05-25 00:16:20]

Change committed as 138938

[tfarina, 2012-05-25 19:17:23]

http://codereview.chromium.org/10386224/diff/44001/ui/app_list/apps_grid_view.h
File ui/app_list/apps_grid_view.h (right):

http://codereview.chromium.org/10386224/diff/44001/ui/app_list/apps_grid_view...
ui/app_list/apps_grid_view.h:30: PaginationModel* pagination_model);
nit: fix indent here.