Add Search Service in Enhanced Bookmark Bridge

components/enhanced_bookmarks/bookmark_server_search_service.h

Index: components/enhanced_bookmarks/bookmark_server_search_service.h
diff --git a/components/enhanced_bookmarks/bookmark_server_search_service.h b/components/enhanced_bookmarks/bookmark_server_search_service.h
index 97713185418368ce34a427eb54a50588a96c7eee..c2b61b607b116a28c7cb8e71f1753f9230f31c4c 100644
--- a/components/enhanced_bookmarks/bookmark_server_search_service.h
+++ b/components/enhanced_bookmarks/bookmark_server_search_service.h
@@ -8,6 +8,7 @@
 #include <string>
 #include <vector>
 
+#include "base/containers/mru_cache.h"
 #include "components/enhanced_bookmarks/bookmark_server_service.h"
 #include "net/url_request/url_fetcher.h"
 
@@ -28,7 +29,8 @@ class BookmarkServerSearchService : public BookmarkServerService {
 
   // Triggers a search. The query must not be empty. A call to this method
   // cancels any previous searches. OnChange() is garanteed to be called once
-  // per query.
+  // per query. If there have been multiple queries in between, onChange will
+  // only be called for the last query.

[lpromero, 2014/10/29 10:31:35]

This comment contradicts the previous sentence: 
"OnChange() is garanteed to be called once per query."
Now this is not true anymore.

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

Done.

   void Search(const std::string& query);
 
   // Returns the search results. The results are only available after the
@@ -36,6 +38,12 @@ class BookmarkServerSearchService : public BookmarkServerService {
   // result otherwise. query should be a string passed to Search() previously.
   std::vector<const BookmarkNode*> ResultForQuery(const std::string& query);

[lpromero, 2014/10/29 10:31:35]

With the new API you introduce, will we ever use this one? Might as well remove
it. Or private for the time being since you use it in this class.

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

Probably add HasResultForQuery() returning a boolean, and adding a DCHECK in
resultForQuery.

 
+  // Returns search retults for most recent query.

[lpromero, 2014/10/29 10:31:35]

s/retults/results

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

Done.

+  std::vector<const BookmarkNode*> ResultForQuery();

[lpromero, 2014/10/29 10:31:35]

s/ResultForQuery/ResultsForLastQuery or ResultsForMostRecentQuery or
ResultsForCurrentQuery

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

I'm curious why you think you need this API? I'm reluctant to approve it as it
is not very well defined what the last query is. It could be easy for the UI and
the last_query to get out of sync. The UI will have the query somewhere anyway
as the user sees it, so why duplicate this info?

As it is the current_query_ is short lived, only used to build the query and
then discarded. Your CL keeps it, and I'm not sure what its life cycle it now.

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

The reason why I made the server to remember the latest query is that I noticed
the fact that as long as UI and the service run on the same thread (which is the
case on Android, not sure about iOS). Then this service will only call
onChange() for the last query.

For example, if UI searches Apple first and before the result of Apple ever
arrives, UI seaches Banana. Then if onChange() is called in future, it will be
meant for Banana only (as we destroy the URL_fetcher for previous searches).

Pros for this approach: less function call stack between UI and server (one
round trip instead of three).
Cons: 1. Restriction of the usage of this class as other classes can only
retrieve results for the most recent query using this ResultForQuery(). 2. This
approach assumes frontend and backend runs on same thread and URL_Fetcher does
what it should do, which may fail?

After careful consideration I decided to hold back changes that I made about
keeping current_query_ in server, in order to help this CL land faster. We are
targeting M40, isn't it? :)

Still there are two things that we all agree:
1. Introduce cache to reduce time for repeated search queries.
2. Make the service live a bit longer so that we could fully utilize this cache.

+
+  // Returns most recent query string.
+  std::string GetCurrentQuery();

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

GetSomething is a javaism. Just use CurrentQuery() instead. But I'd actually
favor not adding this API at all.

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

Done.

+
  protected:
   scoped_ptr<net::URLFetcher> CreateFetcher() override;
 
@@ -55,7 +63,7 @@ class BookmarkServerSearchService : public BookmarkServerService {
 
  private:
   // The search data, a map from query to a vector of stars.id.

[lpromero, 2014/10/29 10:31:35]

s/map/cache?

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

Done.

-  std::map<std::string, std::vector<std::string> > searches_;
+  base::MRUCache<std::string, std::vector<std::string> >* searches_;

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

Since the lifetime of |searches_| is the scope of this class, it doesn't have to
be a pointer.

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

Done.

   std::string current_query_;

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

Add a comment to this variable: current_query_ holds the query currently in
flight, and is cleared as soon as the result is available.

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

Done.

   DISALLOW_COPY_AND_ASSIGN(BookmarkServerSearchService);
 };