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..0e023d4ff73fd9fbbca951eaf75e5d01036b1d26 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 kMRUCacheMaxSize = 50;

[Kibeom Kim (inactive), 2014/10/29 07:51:47]

I think it's better to indicate what this variable is for, in the name. Maybe
kSearchCacheMaxSize.

[Ian Wen, 2014/10/29 21:54:36]

Done.

 }  // namespace
 
 namespace enhanced_bookmarks {
@@ -25,14 +26,24 @@ BookmarkServerSearchService::BookmarkServerSearchService(
                             token_service,
                             signin_manager,
                             enhanced_bookmark_model) {
+  searches_ = new base::MRUCache<std::string, std::vector<std::string> >(

[Kibeom Kim (inactive), 2014/10/29 07:51:47]

nit: I think now we can use >> instead of > >

[noyau (Ping after 24h), 2014/10/29 12:49:48]

Better, don't use new and delete at all, make it a scoped_ptr. Bestest(!), just
don't make it a pointer at all. 

[Ian Wen, 2014/10/29 21:54:36]

LOL "Bestest". Made it not a pointer. Done.

+      kMRUCacheMaxSize);
 }
 
 BookmarkServerSearchService::~BookmarkServerSearchService() {
+  delete searches_;
 }
 
 void BookmarkServerSearchService::Search(const std::string& query) {
   DCHECK(query.length());

[noyau (Ping after 24h), 2014/10/29 12:49:48]

Add

if (query == current_query_)
  return;

to protect against the edge case of cancelling a query that's actually needed.

[Ian Wen, 2014/10/29 21:54:36]

Done.

   current_query_ = query;

[noyau (Ping after 24h), 2014/10/29 12:49:48]

Move after the if, only set it if a request is to be made.

[Ian Wen, 2014/10/29 21:54:36]

Done.

+
+  // If result is already stored in cache, immediately notify observers.
+  if (searches_->Get(current_query_) != searches_->end()) {
+    Cancel();
+    Notify();
+    return;
+  }
   TriggerTokenRequest(true);
 }
 
@@ -41,9 +52,9 @@ std::vector<const BookmarkNode*> BookmarkServerSearchService::ResultForQuery(
   DCHECK(query.length());
   std::vector<const BookmarkNode*> result;
 
-  std::map<std::string, std::vector<std::string> >::iterator it =
-      searches_.find(query);
-  if (it == searches_.end())
+  base::MRUCache<std::string, std::vector<std::string> >::iterator it =

[noyau (Ping after 24h), 2014/10/29 12:49:48]

use auto? At least remove the space between the two '>'.

[Ian Wen, 2014/10/29 21:54:36]

Done.

+      searches_->Get(query);
+  if (it == searches_->end())
     return result;
 
   for (std::vector<std::string>::iterator clip_it = it->second.begin();
@@ -56,6 +67,14 @@ std::vector<const BookmarkNode*> BookmarkServerSearchService::ResultForQuery(
   return result;
 }
 
+std::vector<const BookmarkNode*> BookmarkServerSearchService::ResultForQuery() {
+  return ResultForQuery(current_query_);
+}
+
+std::string BookmarkServerSearchService::GetCurrentQuery() {
+  return current_query_;
+}
+
 scoped_ptr<net::URLFetcher> BookmarkServerSearchService::CreateFetcher() {
   // Add the necessary arguments to the URI.
   GURL url(kSearchUrl);
@@ -90,28 +109,27 @@ bool BookmarkServerSearchService::ProcessResponse(const std::string& response,
       continue;
     clip_ids.push_back(clip_id);
   }
-  searches_[current_query_] = clip_ids;
-  current_query_.clear();

[noyau (Ping after 24h), 2014/10/29 12:49:48]

Again, I'm not convinced the lifecycle of current_query needs to change.

[Ian Wen, 2014/10/29 21:54:36]

Done. Put it back.

+  searches_->Put(current_query_, clip_ids);
   return true;
 }
 
 void BookmarkServerSearchService::CleanAfterFailure() {
-  searches_.clear();
+  searches_->Clear();

[noyau (Ping after 24h), 2014/10/29 12:49:48]

Bug: This should clear the current_query as well.

[Ian Wen, 2014/10/29 21:54:36]

Done.

 }
 
 void BookmarkServerSearchService::EnhancedBookmarkAdded(
     const BookmarkNode* node) {
-  searches_.clear();
+  searches_->Clear();

[noyau (Ping after 24h), 2014/10/29 12:49:48]

What if there is a query in progress at that point? The code as it is will just
store it. Maybe it should cancel the fetching part of the query and restart it?

Maybe not in this CL.

 }
 
 void BookmarkServerSearchService::EnhancedBookmarkAllUserNodesRemoved() {
-  searches_.clear();
+  searches_->Clear();
 }
 
 void BookmarkServerSearchService::EnhancedBookmarkRemoteIdChanged(
     const BookmarkNode* node,
     const std::string& old_remote_id,
     const std::string& remote_id) {
-  searches_.clear();
+  searches_->Clear();
 }
 }  // namespace enhanced_bookmarks