Add Search Service in Enhanced Bookmark Bridge

components/enhanced_bookmarks/bookmark_server_search_service.cc

Index: components/enhanced_bookmarks/bookmark_server_search_service.cc
diff --git a/components/enhanced_bookmarks/bookmark_server_search_service.cc b/components/enhanced_bookmarks/bookmark_server_search_service.cc
index 0cf4690164817ce3fd88c415ef936c118cce9496..e1ab78eeef443f3b134520d18854662bb8e1e4b7 100644
--- a/components/enhanced_bookmarks/bookmark_server_search_service.cc
+++ b/components/enhanced_bookmarks/bookmark_server_search_service.cc
@@ -12,6 +12,7 @@
 
 namespace {
 const char kSearchUrl[] = "https://www.google.com/stars/search";
+const int kSearchCacheMaxSize = 50;
 }  // namespace
 
 namespace enhanced_bookmarks {
@@ -24,7 +25,8 @@ BookmarkServerSearchService::BookmarkServerSearchService(
     : BookmarkServerService(request_context_getter,
                             token_service,
                             signin_manager,
-                            enhanced_bookmark_model) {
+                            enhanced_bookmark_model),
+      cache_(kSearchCacheMaxSize) {
 }
 
 BookmarkServerSearchService::~BookmarkServerSearchService() {
@@ -32,26 +34,34 @@ BookmarkServerSearchService::~BookmarkServerSearchService() {
 
 void BookmarkServerSearchService::Search(const std::string& query) {
   DCHECK(query.length());
+  if (current_query_ == query)
+    return;
+
+  // If result is already stored in cache, immediately notify observers.
+  if (cache_.Get(current_query_) != cache_.end()) {
+    Cancel();
+    Notify();
+    return;
+  }

[Kibeom Kim (inactive), 2014/10/30 03:13:11]

Just a note: In practice, this seems redundant since UI code will call
|ResultForQuery| first to see if we have the result already. Though, I think
it's good to keep this code.

[Ian Wen, 2014/10/30 04:31:00]

I am afraid this is not how we are using this class. UI will call
search(Banana), and onChange() will be called synchronously (if it hits the
cache) and UI will then call ResultForQuery(Banana) to get a list of results.
Calling resultForQuery() directly does not make much sense, isn't it? If there
has been no query, why should there be any result? :)

What I am saying, in conclusion, is that whenever the UI wants to search for
something, it should always call Search(SomeFruit). And it is the bridge/model's
responsibility to call ResultForQuery() later after onChange callback is
triggered.

Details of how we use this class on Android can be found at:
https://chrome-internal-review.googlesource.com/#/c/180705/

[Kibeom Kim (inactive), 2014/10/30 04:39:05]

Yes, so we should change the downstream CL(I didn't send comments yet there). If
the result is already in the cache, there is no point at waiting 200ms so we
should not wait. But the reason you put the 200ms delay is to avoid issuing too
many search queries. So we need a way to check if the cache has the result, but
not issuing search function, and that is your current |ResultForQuery|
implementation, although maybe you didn't intend explicitly.

   current_query_ = query;
   TriggerTokenRequest(true);
 }
 
-std::vector<const BookmarkNode*> BookmarkServerSearchService::ResultForQuery(
-    const std::string& query) {
+scoped_ptr<std::vector<const BookmarkNode*>>
+BookmarkServerSearchService::ResultForQuery(const std::string& query) {
   DCHECK(query.length());
-  std::vector<const BookmarkNode*> result;
+  scoped_ptr<std::vector<const BookmarkNode*>> result;
 
-  std::map<std::string, std::vector<std::string> >::iterator it =
-      searches_.find(query);
-  if (it == searches_.end())
+  const auto& it = cache_.Get(query);
+  if (it == cache_.end())
     return result;
 
-  for (std::vector<std::string>::iterator clip_it = it->second.begin();
-       clip_it != it->second.end();
-       ++clip_it) {
-    const BookmarkNode* node = BookmarkForRemoteId(*clip_it);
+  result.reset(new std::vector<const BookmarkNode*>());
+
+  for (const std::string& clip_id : it->second) {
+    const BookmarkNode* node = BookmarkForRemoteId(clip_id);
     if (node)
-      result.push_back(node);
+      result->push_back(node);
   }
   return result;
 }
@@ -80,38 +90,36 @@ bool BookmarkServerSearchService::ProcessResponse(const std::string& response,
     return false;  // Not formatted properly.
 
   std::vector<std::string> clip_ids;
-  for (google::protobuf::RepeatedPtrField<
-           image::collections::CorpusSearchResult_ClipResult>::const_iterator
-           it = response_proto.results().begin();
-       it != response_proto.results().end();
-       ++it) {
-    const std::string& clip_id = it->clip_id();
+  for (const image::collections::CorpusSearchResult_ClipResult& clip_result :
+       response_proto.results()) {
+    const std::string& clip_id = clip_result.clip_id();
     if (!clip_id.length())
       continue;
     clip_ids.push_back(clip_id);
   }
-  searches_[current_query_] = clip_ids;
+  cache_.Put(current_query_, clip_ids);
   current_query_.clear();
   return true;
 }
 
 void BookmarkServerSearchService::CleanAfterFailure() {
-  searches_.clear();
+  cache_.Clear();
+  current_query_.clear();
 }
 
 void BookmarkServerSearchService::EnhancedBookmarkAdded(
     const BookmarkNode* node) {
-  searches_.clear();
+  cache_.Clear();
 }
 
 void BookmarkServerSearchService::EnhancedBookmarkAllUserNodesRemoved() {
-  searches_.clear();
+  cache_.Clear();
 }
 
 void BookmarkServerSearchService::EnhancedBookmarkRemoteIdChanged(
     const BookmarkNode* node,
     const std::string& old_remote_id,
     const std::string& remote_id) {
-  searches_.clear();
+  cache_.Clear();
 }
 }  // namespace enhanced_bookmarks