Revert of Add Search Service in Enhanced Bookmark Bridge (patchset #6...

Revert of Add Search Service in Enhanced Bookmark Bridge (patchset #6 id:100001 of https://codereview.chromium.org/637323005/)

Reason for revert:
Broke compilation in http://build.chromium.org/p/chromium.gpu/builders/Android%20Debug%20%28Nexus%207%29/builds/14002

Original issue's description:
> Add Search Service in Enhanced Bookmark Bridge
>
> A Search client is created during initialization of
> EnhancedBookmarkBridge. And when the bridge destructs, search service is
> also destroyed.
>
> BUG=415774
>
> Committed: https://crrev.com/e52346f194e9f2d097b4f6d3fbfa359eca7c92df
> Cr-Commit-Position: refs/heads/master@{#302107}

TBR=tedchoc@chromium.org,danduong@chromium.org,kkimlabs@chromium.org,noyau@chromium.org,lpromero@chromium.org,ianwen@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=415774

Review URL: https://codereview.chromium.org/688883002

Cr-Commit-Position: refs/heads/master@{#302130}

chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java

@@ -20,32 +20,20 @@ import java.util.List;
 @JNINamespace("enhanced_bookmarks::android")
 public final class EnhancedBookmarksBridge {
     private long mNativeEnhancedBookmarksBridge;
-    private final ObserverList<FiltersObserver> mFilterObservers =
+    private final ObserverList<FiltersObserver> mObservers =
             new ObserverList<FiltersObserver>();
-    private final ObserverList<SearchServiceObserver> mSearchObservers =
-            new ObserverList<SearchServiceObserver>();
 
     /**
      * Interface to provide consumers notifications to changes in clusters
      */
     public interface FiltersObserver {
         /**
-         * Invoked when client detects that filters have been added/removed from the server.
+         * Invoked when client detects that filters have been
+         * added / removed from the server.
          */
         void onFiltersChanged();
     }
 
-    /**
-     * Interface to provide consumers notifications to changes in search service results.
-     */
-    public interface SearchServiceObserver {
-        /**
-         * Invoked when client detects that search results have been updated. This callback is
-         * guaranteed to be called only once and only for the most recent query.
-         */
-        void onSearchResultsReturned();
-    }
-
     public EnhancedBookmarksBridge(Profile profile) {
         mNativeEnhancedBookmarksBridge = nativeInit(profile);
     }
@@ -104,7 +92,7 @@ public final class EnhancedBookmarksBridge {
      * @param observer Observer to add
      */
     public void addFiltersObserver(FiltersObserver observer) {
-        mFilterObservers.addObserver(observer);
+        mObservers.addObserver(observer);
     }
 
     /**
@@ -112,7 +100,7 @@ public final class EnhancedBookmarksBridge {
      * @param observer Observer to remove
      */
     public void removeFiltersObserver(FiltersObserver observer) {
-        mFilterObservers.removeObserver(observer);
+        mObservers.removeObserver(observer);
     }
 
     /**
@@ -126,42 +114,6 @@ public final class EnhancedBookmarksBridge {
         return list;
     }
 
-    /**
-     * Sends request to search server for querying related bookmarks.
-     * @param query Keyword used to find related bookmarks.
-     */
-    public void sendSearchRequest(String query) {
-        nativeSendSearchRequest(mNativeEnhancedBookmarksBridge, query);
-    }
-
-    /**
-     * Get list of bookmarks as result of a search request that was sent before in
-     * {@link EnhancedBookmarksBridge#sendSearchRequest(String)}. Normally this function should be
-     * called after {@link SearchServiceObserver#onSearchResultsReturned()}
-     * @param query Keyword used to find related bookmarks.
-     * @return List of BookmarkIds that are related to query. It will be null if the request is
-     *         still on the fly, or empty list if there are no results for the query.
-     */
-    public List<BookmarkId> getSearchResultsForQuery(String query) {
-        return nativeGetSearchResults(mNativeEnhancedBookmarksBridge, query);
-    }
-
-    /**
-     * Registers a SearchObserver that listens to search request updates.
-     * @param observer Observer to add
-     */
-    public void addSearchObserver(SearchServiceObserver observer) {
-        mSearchObservers.addObserver(observer);
-    }
-
-    /**
-     * Unregisters a SearchObserver that listens to search request updates.
-     * @param observer Observer to remove
-     */
-    public void removeSearchObserver(SearchServiceObserver observer) {
-        mSearchObservers.removeObserver(observer);
-    }
-
     /**
      * @return Current set of known auto-filters for bookmarks.
      */
@@ -173,23 +125,11 @@ public final class EnhancedBookmarksBridge {
 
     @CalledByNative
     private void onFiltersChanged() {
-        for (FiltersObserver observer : mFilterObservers) {
+        for (FiltersObserver observer : mObservers) {
             observer.onFiltersChanged();
         }
     }
 
-    @CalledByNative
-    private void onSearchResultReturned() {
-        for (SearchServiceObserver observer : mSearchObservers) {
-            observer.onSearchResultsReturned();
-        }
-    }
-
-    @CalledByNative
-    private static List<BookmarkId> createBookmarkIdList() {
-        return new ArrayList<BookmarkId>();
-    }
-
     @CalledByNative
     private static void addToBookmarkIdList(List<BookmarkId> bookmarkIdList, long id, int type) {
         bookmarkIdList.add(new BookmarkId(id, type));
@@ -203,8 +143,6 @@ public final class EnhancedBookmarksBridge {
             int type, String description);
     private native void nativeGetBookmarksForFilter(long nativeEnhancedBookmarksBridge,
             String filter, List<BookmarkId> list);
-    private native List<BookmarkId> nativeGetSearchResults(long nativeEnhancedBookmarksBridge,
-            String query);
     private native String[] nativeGetFilters(long nativeEnhancedBookmarksBridge);
     private native BookmarkId nativeAddFolder(long nativeEnhancedBookmarksBridge, BookmarkId parent,
             int index, String title);
@@ -212,5 +150,5 @@ public final class EnhancedBookmarksBridge {
             BookmarkId bookmarkId, BookmarkId newParentId, int index);
     private native BookmarkId nativeAddBookmark(long nativeEnhancedBookmarksBridge,
             BookmarkId parent, int index, String title, String url);
-    private native void nativeSendSearchRequest(long nativeEnhancedBookmarksBridge, String query);
+
 }

chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc

@@ -12,8 +12,6 @@
 #include "chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service_factory.h"
 #include "chrome/browser/enhanced_bookmarks/enhanced_bookmark_model_factory.h"
 #include "chrome/browser/profiles/profile_android.h"
-#include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
-#include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/common/pref_names.h"
 #include "components/bookmarks/browser/bookmark_model.h"
@@ -21,7 +19,6 @@
 #include "components/bookmarks/common/android/bookmark_id.h"
 #include "components/bookmarks/common/android/bookmark_type.h"
 #include "components/enhanced_bookmarks/enhanced_bookmark_model.h"
-#include "components/signin/core/browser/signin_manager.h"
 #include "content/public/browser/browser_thread.h"
 #include "jni/EnhancedBookmarksBridge_jni.h"
 
@@ -45,17 +42,10 @@ EnhancedBookmarksBridge::EnhancedBookmarksBridge(JNIEnv* env,
   cluster_service_ =
       ChromeBookmarkServerClusterServiceFactory::GetForBrowserContext(profile_);
   cluster_service_->AddObserver(this);
-  search_service_.reset(new BookmarkServerSearchService(
-      profile_->GetRequestContext(),
-      ProfileOAuth2TokenServiceFactory::GetForProfile(profile_),
-      SigninManagerFactory::GetForProfile(profile_),
-      EnhancedBookmarkModelFactory::GetForBrowserContext(profile_)));
-  search_service_->AddObserver(this);
 }
 
 EnhancedBookmarksBridge::~EnhancedBookmarksBridge() {
   cluster_service_->RemoveObserver(this);
-  search_service_->RemoveObserver(this);
 }
 
 void EnhancedBookmarksBridge::Destroy(JNIEnv*, jobject) {
@@ -109,7 +99,8 @@ ScopedJavaLocalRef<jobjectArray> EnhancedBookmarksBridge::GetFilters(
     JNIEnv* env,
     jobject obj) {
   DCHECK(enhanced_bookmark_model_->loaded());
-  const std::vector<std::string> filters = cluster_service_->GetClusters();
+  const std::vector<std::string> filters =
+      cluster_service_->GetClusters();
   return base::android::ToJavaArrayOfStrings(env, filters);
 }
 
@@ -185,35 +176,6 @@ ScopedJavaLocalRef<jobject> EnhancedBookmarksBridge::AddBookmark(
   return new_java_obj;
 }
 
-void EnhancedBookmarksBridge::SendSearchRequest(JNIEnv* env,
-                                                jobject obj,
-                                                jstring j_query) {
-  search_service_->Search(base::android::ConvertJavaStringToUTF8(env, j_query));
-}
-
-ScopedJavaLocalRef<jobject> EnhancedBookmarksBridge::GetSearchResults(
-    JNIEnv* env,
-    jobject obj,
-    jstring j_query) {
-  DCHECK(enhanced_bookmark_model_->loaded());
-
-  ScopedJavaLocalRef<jobject> j_list =
-          Java_EnhancedBookmarksBridge_createBookmarkIdList(env);
-  scoped_ptr<std::vector<const BookmarkNode*>> results =
-      search_service_->ResultForQuery(
-          base::android::ConvertJavaStringToUTF8(env, j_query));
-
-  // If result is null, return a null java reference.
-  if (!results.get())
-    return ScopedJavaLocalRef<jobject>();
-
-  for (const BookmarkNode* node : *results) {
-    Java_EnhancedBookmarksBridge_addToBookmarkIdList(env, j_list.obj(),
-                                                     node->id(), node->type());
-  }
-  return j_list;
-}
-
 void EnhancedBookmarksBridge::OnChange(BookmarkServerService* service) {
   DCHECK(enhanced_bookmark_model_->loaded());
   JNIEnv* env = AttachCurrentThread();
@@ -222,11 +184,7 @@ void EnhancedBookmarksBridge::OnChange(BookmarkServerService* service) {
   if (obj.is_null())
     return;
 
-  if (service == cluster_service_) {
-    Java_EnhancedBookmarksBridge_onFiltersChanged(env, obj.obj());
-  } else if (service == search_service_.get()){
-    Java_EnhancedBookmarksBridge_onSearchResultReturned(env, obj.obj());
-  }
+  Java_EnhancedBookmarksBridge_onFiltersChanged(env, obj.obj());
 }
 
 bool EnhancedBookmarksBridge::IsEditable(const BookmarkNode* node) const {

chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.h

@@ -9,7 +9,6 @@
 #include "base/android/jni_weak_ref.h"
 #include "chrome/browser/profiles/profile.h"
 #include "components/bookmarks/browser/bookmark_model.h"
-#include "components/enhanced_bookmarks/bookmark_server_search_service.h"
 #include "components/enhanced_bookmarks/bookmark_server_service.h"
 
 namespace enhanced_bookmarks {
@@ -29,12 +28,12 @@ class EnhancedBookmarksBridge : public BookmarkServerServiceObserver {
       jobject obj,
       jlong id,
       jint type);
+
   void SetBookmarkDescription(JNIEnv* env,
                               jobject obj,
                               jlong id,
                               jint type,
                               jstring description);
-
   void GetBookmarksForFilter(JNIEnv* env,
                              jobject obj,
                              jstring filter,
@@ -61,23 +60,16 @@ class EnhancedBookmarksBridge : public BookmarkServerServiceObserver {
       jint index,
       jstring j_title,
       jstring j_url);
-  void SendSearchRequest(JNIEnv* env, jobject obj, jstring j_query);
-
-  base::android::ScopedJavaLocalRef<jobject> GetSearchResults(JNIEnv* env,
-                                                              jobject obj,
-                                                              jstring j_query);
-
   // BookmarkServerServiceObserver
-  // Called on changes to cluster data or search results are returned.
+  // Called on changes to cluster data
   virtual void OnChange(BookmarkServerService* service) override;
 
  private:
   bool IsEditable(const BookmarkNode* node) const;
 
   JavaObjectWeakGlobalRef weak_java_ref_;
-  EnhancedBookmarkModel* enhanced_bookmark_model_; // weak
+  EnhancedBookmarkModel* enhanced_bookmark_model_;
   BookmarkServerClusterService* cluster_service_;  // weak
-  scoped_ptr<BookmarkServerSearchService> search_service_;
   Profile* profile_;                       // weak
   DISALLOW_COPY_AND_ASSIGN(EnhancedBookmarksBridge);
 };

components/enhanced_bookmarks/bookmark_server_search_service.cc

@@ -12,7 +12,6 @@
 
 namespace {
 const char kSearchUrl[] = "https://www.google.com/stars/search";
-const int kSearchCacheMaxSize = 50;
 }  // namespace
 
 namespace enhanced_bookmarks {
@@ -25,8 +24,7 @@ BookmarkServerSearchService::BookmarkServerSearchService(
     : BookmarkServerService(request_context_getter,
                             token_service,
                             signin_manager,
-                            enhanced_bookmark_model),
-      cache_(kSearchCacheMaxSize) {
+                            enhanced_bookmark_model) {
 }
 
 BookmarkServerSearchService::~BookmarkServerSearchService() {
@@ -34,34 +32,26 @@ 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;
-  }
   current_query_ = query;
   TriggerTokenRequest(true);
 }
 
-scoped_ptr<std::vector<const BookmarkNode*>>
-BookmarkServerSearchService::ResultForQuery(const std::string& query) {
+std::vector<const BookmarkNode*> BookmarkServerSearchService::ResultForQuery(
+    const std::string& query) {
   DCHECK(query.length());
-  scoped_ptr<std::vector<const BookmarkNode*>> result;
+  std::vector<const BookmarkNode*> result;
 
-  const auto& it = cache_.Get(query);
-  if (it == cache_.end())
+  std::map<std::string, std::vector<std::string> >::iterator it =
+      searches_.find(query);
+  if (it == searches_.end())
     return result;
 
-  result.reset(new std::vector<const BookmarkNode*>());
-
-  for (const std::string& clip_id : it->second) {
-    const BookmarkNode* node = BookmarkForRemoteId(clip_id);
+  for (std::vector<std::string>::iterator clip_it = it->second.begin();
+       clip_it != it->second.end();
+       ++clip_it) {
+    const BookmarkNode* node = BookmarkForRemoteId(*clip_it);
     if (node)
-      result->push_back(node);
+      result.push_back(node);
   }
   return result;
 }
@@ -90,36 +80,38 @@ bool BookmarkServerSearchService::ProcessResponse(const std::string& response,
     return false;  // Not formatted properly.
 
   std::vector<std::string> clip_ids;
-  for (const image::collections::CorpusSearchResult_ClipResult& clip_result :
-       response_proto.results()) {
-    const std::string& clip_id = clip_result.clip_id();
+  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();
     if (!clip_id.length())
       continue;
     clip_ids.push_back(clip_id);
   }
-  cache_.Put(current_query_, clip_ids);
+  searches_[current_query_] = clip_ids;
   current_query_.clear();
   return true;
 }
 
 void BookmarkServerSearchService::CleanAfterFailure() {
-  cache_.Clear();
-  current_query_.clear();
+  searches_.clear();
 }
 
 void BookmarkServerSearchService::EnhancedBookmarkAdded(
     const BookmarkNode* node) {
-  cache_.Clear();
+  searches_.clear();
 }
 
 void BookmarkServerSearchService::EnhancedBookmarkAllUserNodesRemoved() {
-  cache_.Clear();
+  searches_.clear();
 }
 
 void BookmarkServerSearchService::EnhancedBookmarkRemoteIdChanged(
     const BookmarkNode* node,
     const std::string& old_remote_id,
     const std::string& remote_id) {
-  cache_.Clear();
+  searches_.clear();
 }
 }  // namespace enhanced_bookmarks

components/enhanced_bookmarks/bookmark_server_search_service.h

@@ -8,7 +8,6 @@
 #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,20 +27,14 @@ class BookmarkServerSearchService : public BookmarkServerService {
   ~BookmarkServerSearchService() override;
 
   // Triggers a search. The query must not be empty. A call to this method
-  // cancels any previous searches. If there have been multiple queries in
-  // between, onChange will only be called for the last query.
-  // Note this method will be synchronous if query hits the cache.
+  // cancels any previous searches. OnChange() is garanteed to be called once
+  // per query.
   void Search(const std::string& query);
 
-  // Returns search results for a query. Results for a query are only available
-  // after Search() is called and after then OnChange() observer methods has
-  // been sent.This method might return an empty vector, meaning there are no
-  // bookmarks matching the given query. Returning null means we are still
-  // loading and no results have come to the client. Previously cancelled
-  // queries will not trigger onChange(), and this method will also return null
-  // for queries that have never been passed to Search() before.
-  scoped_ptr<std::vector<const BookmarkNode*>> ResultForQuery(
-      const std::string& query);
+  // Returns the search results. The results are only available after the
+  // OnChange() observer methods has been sent. This method will return an empty
+  // result otherwise. query should be a string passed to Search() previously.
+  std::vector<const BookmarkNode*> ResultForQuery(const std::string& query);
 
  protected:
   scoped_ptr<net::URLFetcher> CreateFetcher() override;
@@ -61,11 +54,8 @@ class BookmarkServerSearchService : public BookmarkServerService {
                                        const std::string& remote_id) override;
 
  private:
-  // Cache for previous search result, a map from a query string to vector of
-  // star_ids.
-  base::MRUCache<std::string, std::vector<std::string>> cache_;
-  // The query currently on the fly, and is cleared as soon as the result is
-  // available.
+  // The search data, a map from query to a vector of stars.id.
+  std::map<std::string, std::vector<std::string> > searches_;
   std::string current_query_;
   DISALLOW_COPY_AND_ASSIGN(BookmarkServerSearchService);
 };

components/enhanced_bookmarks/bookmark_server_service.cc

@@ -56,11 +56,6 @@ const std::string BookmarkServerService::RemoteIDForBookmark(
   return model_->GetRemoteId(bookmark);
 }
 
-void BookmarkServerService::Cancel() {
-  url_fetcher_.reset();
-  token_request_.reset();
-}
-
 void BookmarkServerService::Notify() {
   FOR_EACH_OBSERVER(BookmarkServerServiceObserver, observers_, OnChange(this));
 }

components/enhanced_bookmarks/bookmark_server_service.h

@@ -57,9 +57,6 @@ class BookmarkServerService : protected net::URLFetcherDelegate,
       const std::string& remote_id) const;
   const std::string RemoteIDForBookmark(const BookmarkNode* bookmark) const;
 
-  // Cancels the ongoing request, if any.
-  void Cancel();
-
   // Notifies the observers that something changed.
   void Notify();