Add search suggestions fetched from the server

athena/home/app_list_view_delegate.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "athena/home/app_list_view_delegate.h"
 
 #include <string>
 
 #include "athena/home/public/app_model_builder.h"
 #include "base/basictypes.h"
@@ skipping 33 matching lines @@
   search_provider->set_result_changed_callback(base::Bind(
       &AppListViewDelegate::SearchResultChanged, base::Unretained(this)));
   search_providers_.push_back(search_provider);
 }
 
 void AppListViewDelegate::SearchResultChanged() {
   // TODO(mukai): port app-list's Mixer to reorder the results properly.
   app_list::SearchProvider* search_provider = search_providers_[0];
   std::vector<app_list::SearchResult*> results;
   search_provider->ReleaseResult(&results);
-  model_->results()->DeleteAll();
-  for (size_t i = 0; i < results.size(); ++i)
-    model_->results()->Add(results[i]);
+  if (results.empty()) {

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

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]

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]

Isn't calling model_->results()->DeleteAll() when the result is empty (i.e.
ClearResults() is called by the provider) enough?
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:22]

You made the point.
yeah, I'm in the same boat.

+    model_->results()->DeleteAll();
+  } else {
+    for (size_t i = 0; i < results.size(); ++i)
+      model_->results()->Add(results[i]);
+  }
 }
 
 bool AppListViewDelegate::ForceNativeDesktop() const {
   return false;
 }
 
 void AppListViewDelegate::SetProfileByPath(const base::FilePath& profile_path) {
   // Nothing needs to be done.
 }
 
@@ skipping 100 matching lines @@
 const app_list::AppListViewDelegate::Users&
 AppListViewDelegate::GetUsers() const {
   return users_;
 }
 
 bool AppListViewDelegate::ShouldCenterWindow() const {
   return true;
 }
 
 }  // namespace athena