Bring up of the enhanced bookmarks cluster service.

components/enhanced_bookmarks/bookmark_server_cluster_service.cc

Index: components/enhanced_bookmarks/bookmark_server_cluster_service.cc
diff --git a/components/enhanced_bookmarks/bookmark_server_cluster_service.cc b/components/enhanced_bookmarks/bookmark_server_cluster_service.cc
new file mode 100644
index 0000000000000000000000000000000000000000..5f8e45a6e60903d850062ad17bc289defc95f227
--- /dev/null
+++ b/components/enhanced_bookmarks/bookmark_server_cluster_service.cc
@@ -0,0 +1,326 @@
+// 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_cluster_service.h"
+
+#include "base/json/json_reader.h"
+#include "base/json/json_writer.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/prefs/pref_service.h"
+#include "base/values.h"
+#include "components/bookmarks/browser/bookmark_model.h"
+#include "components/enhanced_bookmarks/enhanced_bookmark_model.h"
+#include "components/enhanced_bookmarks/enhanced_bookmark_utils.h"
+#include "components/enhanced_bookmarks/pref_names.h"
+#include "components/enhanced_bookmarks/proto/cluster.pb.h"
+#include "components/pref_registry/pref_registry_syncable.h"
+#include "components/signin/core/browser/signin_manager.h"
+#include "net/base/url_util.h"
+#include "net/url_request/url_fetcher.h"
+#include "net/url_request/url_request_context_getter.h"
+
+namespace {
+const std::string kClusterUrl("https://www.google.com/stars/cluster");
+const int kPrefServiceVersion = 1;
+const char* kPrefServiceVersionKey = "version";
+const char* kPrefServiceDataKey = "data";
+const char* kAuthIdKey = "auth_id";
+}  // namespace
+
+namespace enhanced_bookmarks {
+
+BookmarkServerClusterService::BookmarkServerClusterService(
+    const std::string& application_language_code,
+    scoped_refptr<net::URLRequestContextGetter> request_context_getter,
+    ProfileOAuth2TokenService* token_service,
+    SigninManagerBase* signin_manager,
+    enhanced_bookmarks::EnhancedBookmarkModel* enhanced_bookmark_model,
+    PrefService* pref_service)
+    : BookmarkServerService(request_context_getter,
+                            token_service,
+                            signin_manager,
+                            enhanced_bookmark_model),
+      application_language_code_(application_language_code),
+      pref_service_(pref_service) {
+  LoadModel();
+
+  if (model_->loaded())
+    TriggerTokenRequest(false);
+
+  GetSigninManager()->AddObserver(this);
+}
+
+BookmarkServerClusterService::~BookmarkServerClusterService() {
+  GetSigninManager()->RemoveObserver(this);
+}
+
+const std::vector<const BookmarkNode*>
+BookmarkServerClusterService::BookmarksForClusterNamed(
+    const std::string& cluster_name) const {
+  std::vector<const BookmarkNode*> results;
+
+  ClusterMap::const_iterator cluster_it = cluster_data_.find(cluster_name);
+  if (cluster_it == cluster_data_.end())
+    return results;
+
+  for (std::vector<std::string>::const_iterator it = cluster_it->second.begin();
+       it != cluster_it->second.end();
+       ++it) {
+    const std::string& star_id = *it;

[battre, 2014/10/07 13:39:29]

nit/opt:
you can use C++11 syntax now http://chromium-cpp.appspot.com/

for (const std::string& star_id : cluster_it->second) {

That might simplify some loops below as well.

[noyau (Ping after 24h), 2014/10/07 15:19:32]

Arrrgh! DOne another pass reverting the changes to go back to C++11 loops.

+    const BookmarkNode* bookmark = BookmarkForRemoteId(star_id);
+    if (bookmark)
+      results.push_back(bookmark);
+  }
+  return results;
+}
+
+const std::vector<std::string>
+BookmarkServerClusterService::ClustersForBookmark(
+    const BookmarkNode* bookmark) const {
+  const std::string& star_id = RemoteIDForBookmark(bookmark);
+
+  // TODO(noyau): if this turns out to be a perf bottleneck this may be improved
+  // by storing a reverse map from id to cluster.
+  std::vector<std::string> clusters;
+  for (ClusterMap::const_iterator it = cluster_data_.begin();
+       it != cluster_data_.end();
+       ++it) {
+    const std::vector<std::string>& v = it->second;

[battre, 2014/10/07 13:39:28]

nit/opt: rename v to stars_ids?

[noyau (Ping after 24h), 2014/10/07 15:19:32]

Done.

+    if (std::find(v.begin(), v.end(), star_id) != v.end())
+      clusters.push_back(it->first);
+  }
+  return clusters;
+}
+
+const std::vector<std::string> BookmarkServerClusterService::GetClusters()
+    const {
+  std::vector<std::string> cluster_names;
+
+  for (ClusterMap::const_iterator it = cluster_data_.begin();
+       it != cluster_data_.end();
+       ++it) {
+    cluster_names.push_back(it->first);
+  }
+
+  return cluster_names;
+}
+
+void BookmarkServerClusterService::RegisterPrefs(
+    user_prefs::PrefRegistrySyncable* registry) {
+  registry->RegisterDictionaryPref(
+      prefs::kBookmarkClusters,
+      user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
+}
+
+net::URLFetcher* BookmarkServerClusterService::CreateFetcher() {

[battre, 2014/10/07 13:39:28]

scoped_ptr<net::URLFetcher>?

[noyau (Ping after 24h), 2014/10/07 15:19:32]

Done.

+  // Add the necessary arguments to the URI.
+  GURL url(kClusterUrl);
+  url = net::AppendQueryParameter(url, "output", "proto");
+
+  // Append language.
+  if (!application_language_code_.empty())
+    url = net::AppendQueryParameter(url, "hl", application_language_code_);
+
+  url = net::AppendQueryParameter(url, "v", model_->GetVersionString());
+
+  // Build the URLFetcher to perform the request.
+  net::URLFetcher* url_fetcher =
+      net::URLFetcher::Create(url, net::URLFetcher::POST, this);
+
+  // Binary encode a basic request proto.
+  image_collections::ClusterRequest request_proto;
+  request_proto.set_cluster_all(true);
+
+  std::string proto_output;
+  bool result = request_proto.SerializePartialToString(&proto_output);
+  DCHECK(result);
+
+  url_fetcher->SetUploadData("application/octet-stream", proto_output);
+  return url_fetcher;
+}
+
+bool BookmarkServerClusterService::ProcessResponse(const std::string& response,
+                                                   bool* should_notify) {
+  DCHECK(*should_notify);
+  image_collections::ClusterResponse response_proto;
+  bool result = response_proto.ParseFromString(response);
+  if (!result)
+    return false;  // Not formatted properly.
+
+  ClusterMap new_cluster_data;
+  for (int i = 0; i < response_proto.clusters_size(); ++i) {
+    const image_collections::ClusterResponse_Cluster cluster =

[battre, 2014/10/07 13:39:28]

nit/opt: You could store the result as a const reference to a temporayr object
like you do in BookmarkServerClusterService::ClustersForBookmark

[noyau (Ping after 24h), 2014/10/07 15:19:31]

Used auto&

+        response_proto.clusters(i);
+    const std::string& title = cluster.title();
+    if (title.empty())
+      continue;
+    std::vector<std::string> starids;

[battre, 2014/10/07 13:39:28]

stars_ids? (plural and underscore)

[noyau (Ping after 24h), 2014/10/07 15:19:32]

Done.

+    for (int j = 0; j < cluster.docs_size(); ++j) {
+      const std::string& doc = cluster.docs(j);
+      if (!doc.empty())
+        starids.push_back(doc);
+    }
+    if (starids.size())
+      new_cluster_data[title] = starids;
+  }
+
+  if (new_cluster_data.size() == cluster_data_.size() &&
+      std::equal(new_cluster_data.begin(),
+                 new_cluster_data.end(),
+                 cluster_data_.begin())) {
+    *should_notify = false;
+  } else {
+    UpdateModel(&new_cluster_data);

[battre, 2014/10/07 13:39:29]

Do you want to call this SwapModel?

[noyau (Ping after 24h), 2014/10/07 15:19:32]

Done.

+  }
+  return true;
+}
+
+void BookmarkServerClusterService::CleanAfterFailure() {
+  if (cluster_data_.empty())
+    return;
+
+  ClusterMap empty;
+  UpdateModel(&empty);
+}
+
+void BookmarkServerClusterService::EnhancedBookmarkModelLoaded() {
+  TriggerTokenRequest(false);
+}
+
+void BookmarkServerClusterService::EnhancedBookmarkAdded(
+    const BookmarkNode* node) {
+  // Nothing to do.
+}
+
+void BookmarkServerClusterService::EnhancedBookmarkRemoved(
+    const BookmarkNode* node) {
+  // It is possible to remove the entries from the map here, but as those are
+  // filtered in ClustersForBookmark() this is not strictly necessary.
+}
+
+void BookmarkServerClusterService::EnhancedBookmarkAllUserNodesRemoved() {
+  if (!cluster_data_.empty()) {
+    ClusterMap empty;
+    UpdateModel(&empty);
+  }
+}
+
+void BookmarkServerClusterService::EnhancedBookmarkRemoteIdChanged(
+    const BookmarkNode* node,
+    const std::string& old_remote_id,
+    const std::string& remote_id) {
+  std::vector<std::string> clusters;
+  for (ClusterMap::iterator it = cluster_data_.begin();
+       it != cluster_data_.end();
+       ++it) {
+    std::vector<std::string>& v = it->second;
+    std::replace(v.begin(), v.end(), old_remote_id, remote_id);
+  }
+}
+
+void BookmarkServerClusterService::GoogleSignedOut(
+    const std::string& account_id,
+    const std::string& username) {
+  if (!cluster_data_.empty()) {
+    ClusterMap empty;
+    UpdateModel(&empty);

[battre, 2014/10/07 13:39:29]

You don't have unit tests. Did you check whether the
GetSigninManager()->GetAuthenticatedAccountId() call in UpdateModel still
returns a valid account_id when you get a GoogleSignedOut() call?

[noyau (Ping after 24h), 2014/10/07 15:19:32]

This returns the empty string when signed out. But you're right, I didn't have
unit tests covering this particular case, I was only checking swapping from a
signed user to another signed user. I've added the test in the iOS tree.

+  }
+}
+
+void BookmarkServerClusterService::UpdateModel(ClusterMap* cluster_map) {
+  cluster_data_.swap(*cluster_map);
+  const std::string& auth_id = GetSigninManager()->GetAuthenticatedAccountId();
+  scoped_ptr<base::DictionaryValue> dictionary(
+      Serialize(cluster_data_, auth_id));
+  pref_service_->Set(prefs::kBookmarkClusters, *dictionary);
+}
+
+void BookmarkServerClusterService::LoadModel() {
+  const base::DictionaryValue* dictionary =
+      pref_service_->GetDictionary(prefs::kBookmarkClusters);
+  const std::string& auth_id = GetSigninManager()->GetAuthenticatedAccountId();
+
+  ClusterMap loaded_data;
+  bool result = BookmarkServerClusterService::Deserialize(
+      *dictionary, auth_id, &loaded_data);
+  if (result)
+    cluster_data_.swap(loaded_data);
+}
+
+//
+// Serialization.
+//
+scoped_ptr<base::DictionaryValue> BookmarkServerClusterService::Serialize(
+    const ClusterMap& cluster_map,
+

[battre, 2014/10/07 13:39:29]

nit: remove empty line

[noyau (Ping after 24h), 2014/10/07 15:19:31]

Done.

+    const std::string& auth_id) {
+  // Create a list of all clusters. For each cluster, make another list. The
+  // first element in the list is the key (cluster name). All subsequent
+  // elements are stars ids.
+  scoped_ptr<base::ListValue> all_clusters(new base::ListValue);
+  for (auto it : cluster_map) {

[battre, 2014/10/07 13:39:29]

auto&

[noyau (Ping after 24h), 2014/10/07 15:19:32]

Done.

+    scoped_ptr<base::ListValue> cluster(new base::ListValue);
+    cluster->AppendString(it.first);
+    cluster->AppendStrings(it.second);
+    all_clusters->Append(cluster.release());
+  }
+
+  // The dictionary that will be serialized has two fields: a version field and
+  // a data field.
+  scoped_ptr<base::DictionaryValue> data(new base::DictionaryValue());

[battre, 2014/10/07 13:39:28]

I would be consistent with () in calls of the constructor in the function.

[noyau (Ping after 24h), 2014/10/07 15:19:31]

Done.

+  data->SetInteger(kPrefServiceVersionKey, kPrefServiceVersion);
+  data->Set(kPrefServiceDataKey, all_clusters.release());
+  data->SetString(kAuthIdKey, auth_id);
+
+  return data.Pass();
+}
+
+bool BookmarkServerClusterService::Deserialize(
+    const base::DictionaryValue& value,
+

[battre, 2014/10/07 13:39:28]

-newline

[noyau (Ping after 24h), 2014/10/07 15:19:32]

Done.

+    const std::string& auth_id,
+    ClusterMap* out_map) {
+  ClusterMap output;
+
+  // Check version.
+  int version;
+  if (!value.GetInteger(kPrefServiceVersionKey, &version))
+    return false;
+  if (version != kPrefServiceVersion)
+    return false;
+
+  // Check auth id.
+  std::string id;
+  if (!value.GetString(kAuthIdKey, &id))
+    return false;
+  if (id != auth_id)
+    return false;
+
+  base::ListValue const* all_clusters = NULL;

[battre, 2014/10/07 13:39:28]

I think that the common syntax is "const base::ListValue*". Same below.

[noyau (Ping after 24h), 2014/10/07 15:19:32]

Done.

+  if (!value.GetList(kPrefServiceDataKey, &all_clusters))
+    return false;
+
+  for (size_t index = 0; index < all_clusters->GetSize(); ++index) {
+    base::ListValue const* cluster = NULL;
+    if (!all_clusters->GetList(index, &cluster))
+      return false;
+    if (cluster->GetSize() < 1)
+      return false;
+    std::string key;
+    if (!cluster->GetString(0, &key))
+      return false;
+    std::vector<std::string> stars_ids;
+    for (size_t index = 1; index < cluster->GetSize(); ++index) {
+      std::string stars_id;
+      if (!cluster->GetString(index, &stars_id))
+        return false;
+      stars_ids.push_back(stars_id);
+    }
+    output.insert(std::make_pair(key, stars_ids));
+  }
+  out_map->swap(output);
+  return true;
+}
+
+}  // namespace enhanced_bookmarks