Add Search Service in Enhanced Bookmark Bridge

components/enhanced_bookmarks/bookmark_server_search_service.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 "components/enhanced_bookmarks/bookmark_server_search_service.h"
 
 #include "components/enhanced_bookmarks/enhanced_bookmark_model.h"
 #include "components/enhanced_bookmarks/enhanced_bookmark_utils.h"
 #include "components/enhanced_bookmarks/proto/search.pb.h"
 #include "net/base/url_util.h"
 #include "net/url_request/url_fetcher.h"
 
 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 {
 
 BookmarkServerSearchService::BookmarkServerSearchService(
     scoped_refptr<net::URLRequestContextGetter> request_context_getter,
     ProfileOAuth2TokenService* token_service,
     SigninManagerBase* signin_manager,
     EnhancedBookmarkModel* enhanced_bookmark_model)
     : BookmarkServerService(request_context_getter,
                             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);
 }
 
 std::vector<const BookmarkNode*> BookmarkServerSearchService::ResultForQuery(
     const std::string& query) {
   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();
        clip_it != it->second.end();
        ++clip_it) {

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

for (const std::string& clip_id : it->second)

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

Done.

     const BookmarkNode* node = BookmarkForRemoteId(*clip_it);
     if (node)
       result.push_back(node);
   }
   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);
   url = net::AppendQueryParameter(url, "output", "proto");
   url = net::AppendQueryParameter(url, "q", current_query_);
   url = net::AppendQueryParameter(url, "v", model_->GetVersionString());
 
   // Build the URLFetcher to perform the request.
   scoped_ptr<net::URLFetcher> url_fetcher(
       net::URLFetcher::Create(url, net::URLFetcher::GET, this));
 
   return url_fetcher;
 }
 
 bool BookmarkServerSearchService::ProcessResponse(const std::string& response,
                                                   bool* should_notify) {
   DCHECK(*should_notify);
   DCHECK(current_query_.length());
   image::collections::CorpusSearchResult response_proto;
   bool result = response_proto.ParseFromString(response);
   if (!result)
     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();

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

Replace this whole mess by

for (const std::string& clip_id : response_proto.results()) {

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

Done.

     if (!clip_id.length())
       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