Add search suggestions fetched from the server

Add search suggestions fetched from the server

Fix athena::AppListViewDelegate to show multiple results.
Use SearchSuggestionParser in UrlSearchProvider

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

[hashimoto, 2014-08-04 11:28:54]

https://codereview.chromium.org/438023002/diff/20001/athena/home/app_list_vie...
File athena/home/app_list_view_delegate.cc (right):

https://codereview.chromium.org/438023002/diff/20001/athena/home/app_list_vie...
athena/home/app_list_view_delegate.cc:54: if (results.empty()) {
This code assumes that |results| is empty iff SearchResultChanged() is called by
app_list::SearchProvider::ClearResults().
Do you think it's OK to rely on this assumption?

BTW, Chrome's app list and Athena are handling results returned by
SearchProvider in totally different ways.
Chrome does not mutate the state of the providers and duplicates the results to
display them while Athena mutates the providers by calling ReleaseResult().
Which way is the preferred way? Is one of these two going to change to be
consistent with another?

[Jun Mukai, 2014-08-04 16:43:10]

https://codereview.chromium.org/438023002/diff/20001/athena/home/app_list_vie...
File athena/home/app_list_view_delegate.cc (right):

https://codereview.chromium.org/438023002/diff/20001/athena/home/app_list_vie...
athena/home/app_list_view_delegate.cc:54: if (results.empty()) {
On 2014/08/04 11:28:54, hashimoto wrote:
> This code assumes that |results| is empty iff SearchResultChanged() is called
by
> app_list::SearchProvider::ClearResults().
> Do you think it's OK to rely on this assumption?
> 
> BTW, Chrome's app list and Athena are handling results returned by
> SearchProvider in totally different ways.
> Chrome does not mutate the state of the providers and duplicates the results
to
> display them while Athena mutates the providers by calling ReleaseResult().
> Which way is the preferred way? Is one of these two going to change to be
> consistent with another?

This is not enough. You should clear model_->results() when the query has been
changed, otherwise old results and new results are mixed.

The handling of results are totally different because of me; I personally don't
really like to copy the results, so I have introduced this mechanism here (also
I didn't want to migrate copying method to SearchResult).

That is my personal preference, and I don't know which is the preferred way.  My
original idea was to fix chrome side and avoid copying at all, but considering
this issue, it might be good for now to follow chrome's way.
sorry for all.

# I still think that a better way is to abandon the results list from each
SearchProvider and instead SearchController or Mixer in Chrome should keep the
consistent list of the results.

[hashimoto, 2014-08-05 01:03:35]

https://codereview.chromium.org/438023002/diff/20001/athena/home/app_list_vie...
File athena/home/app_list_view_delegate.cc (right):

https://codereview.chromium.org/438023002/diff/20001/athena/home/app_list_vie...
athena/home/app_list_view_delegate.cc:54: if (results.empty()) {
On 2014/08/04 16:43:10, Jun Mukai wrote:
> On 2014/08/04 11:28:54, hashimoto wrote:
> > This code assumes that |results| is empty iff SearchResultChanged() is
called
> by
> > app_list::SearchProvider::ClearResults().
> > Do you think it's OK to rely on this assumption?
> > 
> > BTW, Chrome's app list and Athena are handling results returned by
> > SearchProvider in totally different ways.
> > Chrome does not mutate the state of the providers and duplicates the results
> to
> > display them while Athena mutates the providers by calling ReleaseResult().
> > Which way is the preferred way? Is one of these two going to change to be
> > consistent with another?
> 
> This is not enough. You should clear model_->results() when the query has been
> changed, otherwise old results and new results are mixed.
Isn't calling model_->results()->DeleteAll() when the result is empty (i.e.
ClearResults() is called by the provider) enough?
> 
> The handling of results are totally different because of me; I personally
don't
> really like to copy the results, so I have introduced this mechanism here
(also
> I didn't want to migrate copying method to SearchResult).
> 
> That is my personal preference, and I don't know which is the preferred way. 
My
> original idea was to fix chrome side and avoid copying at all, but considering
> this issue, it might be good for now to follow chrome's way.
> sorry for all.
> 
> # I still think that a better way is to abandon the results list from each
> SearchProvider and instead SearchController or Mixer in Chrome should keep the
> consistent list of the results.
I think it's OK in either way, but in future we should chose on of them because
it's nice to support only 1 usage rather than 2 completely different ways (I
also slightly prefer the non-copying solution.)
If we are to chose the non-copying solution, we should remove results() from the
public interface of app_list::SearchProvider to make ReleaseResult() the only
way to access the results.
If we are to chose the copying solution, we should remove ReleaseResult() and
add Duplicate() to SearchResult (it's currently available in
ChromeSearchResult).

[Jun Mukai, 2014-08-05 07:33:23]

lgtm

https://codereview.chromium.org/438023002/diff/20001/athena/home/app_list_vie...
File athena/home/app_list_view_delegate.cc (right):

https://codereview.chromium.org/438023002/diff/20001/athena/home/app_list_vie...
athena/home/app_list_view_delegate.cc:54: if (results.empty()) {
On 2014/08/05 01:03:35, hashimoto wrote:
> On 2014/08/04 16:43:10, Jun Mukai wrote:
> > On 2014/08/04 11:28:54, hashimoto wrote:
> > > This code assumes that |results| is empty iff SearchResultChanged() is
> called
> > by
> > > app_list::SearchProvider::ClearResults().
> > > Do you think it's OK to rely on this assumption?
> > > 
> > > BTW, Chrome's app list and Athena are handling results returned by
> > > SearchProvider in totally different ways.
> > > Chrome does not mutate the state of the providers and duplicates the
results
> > to
> > > display them while Athena mutates the providers by calling
ReleaseResult().
> > > Which way is the preferred way? Is one of these two going to change to be
> > > consistent with another?
> > 
> > This is not enough. You should clear model_->results() when the query has
been
> > changed, otherwise old results and new results are mixed.
> Isn't calling model_->results()->DeleteAll() when the result is empty (i.e.
> ClearResults() is called by the provider) enough?

You made the point.

> > 
> > The handling of results are totally different because of me; I personally
> don't
> > really like to copy the results, so I have introduced this mechanism here
> (also
> > I didn't want to migrate copying method to SearchResult).
> > 
> > That is my personal preference, and I don't know which is the preferred way.

> My
> > original idea was to fix chrome side and avoid copying at all, but
considering
> > this issue, it might be good for now to follow chrome's way.
> > sorry for all.
> > 
> > # I still think that a better way is to abandon the results list from each
> > SearchProvider and instead SearchController or Mixer in Chrome should keep
the
> > consistent list of the results.
> I think it's OK in either way, but in future we should chose on of them
because
> it's nice to support only 1 usage rather than 2 completely different ways (I
> also slightly prefer the non-copying solution.)
> If we are to chose the non-copying solution, we should remove results() from
the
> public interface of app_list::SearchProvider to make ReleaseResult() the only
> way to access the results.
> If we are to chose the copying solution, we should remove ReleaseResult() and
> add Duplicate() to SearchResult (it's currently available in
> ChromeSearchResult).

yeah, I'm in the same boat.

[hashimoto, 2014-08-05 08:04:21]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-08-05 08:04:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hashimoto@chromium.org/438023002/20001

[commit-bot: I haz the power, 2014-08-05 10:08:38]

Change committed as 287504