Bring up of the enhanced bookmarks cluster service.

components/enhanced_bookmarks/bookmark_server_cluster_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_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_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,
+    BookmarkModel* bookmark_model,
+    PrefService* pref_service)
+    : BookmarkServerService(request_context_getter,
+                            token_service,
+                            signin_manager,
+                            bookmark_model),
+      application_language_code_(application_language_code),
+      pref_service_(pref_service) {
+  LoadModel();
+
+  if (bookmark_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) {
+    std::string stars_id = *it;

[Yaron, 2014/09/23 17:08:30]

star_id

[battre, 2014/09/26 14:33:04]

const std::string& star_id = *it; ?

[noyau (Ping after 24h), 2014/10/01 16:04:42]

Done.

+    const BookmarkNode* bookmark = BookmarkForRemoteId(stars_id);
+    if (bookmark)
+      results.push_back(bookmark);
+  }
+  return results;
+}
+
+const std::vector<std::string>
+BookmarkServerClusterService::ClustersForBookmark(
+    const BookmarkNode* bookmark) const {
+  std::string starid = RemoteIDForBookmark(bookmark);

[Yaron, 2014/09/23 17:08:30]

star_id

[noyau (Ping after 24h), 2014/10/01 16:04:43]

Done.

+
+  // 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;
+    if (std::find(v.begin(), v.end(), starid) != 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) {
+  // Register a string, unsyncable preference.
+  registry->RegisterStringPref(
+      prefs::kBookmarkClusters,
+      std::string(),
+      user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
+}
+
+net::URLFetcher* BookmarkServerClusterService::CreateFetcher() {
+  // 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_);
+
+  // 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.mutable_options()->set_min_cluster_size(1);
+  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 =
+        response_proto.clusters(i);
+    const std::string& title = cluster.title();
+    if (title.empty())
+      continue;
+    std::vector<std::string> starids;
+    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);
+  }
+  return true;
+}
+
+void BookmarkServerClusterService::CleanAfterFailure() {
+  if (cluster_data_.empty())
+    return;
+
+  ClusterMap empty;
+  UpdateModel(&empty);
+}
+
+void BookmarkServerClusterService::BookmarkModelLoaded(BookmarkModel* model,
+                                                       bool ids_reassigned) {
+  BookmarkServerService::BookmarkModelLoaded(model, ids_reassigned);
+  TriggerTokenRequest(false);
+}
+
+void BookmarkServerClusterService::BookmarkAllUserNodesRemoved(
+    BookmarkModel* model,
+    const std::set<GURL>& removed_urls) {
+  if (!cluster_data_.empty()) {
+    ClusterMap empty;
+    UpdateModel(&empty);
+  }
+  BookmarkServerService::BookmarkAllUserNodesRemoved(model, removed_urls);
+}
+
+void BookmarkServerClusterService::GoogleSignedOut(
+    const std::string& account_id,
+    const std::string& username) {
+  if (!cluster_data_.empty()) {
+    ClusterMap empty;
+    UpdateModel(&empty);
+  }
+}
+
+void BookmarkServerClusterService::UpdateModel(ClusterMap* cluster_map) {
+  cluster_data_.swap(*cluster_map);
+  std::string serialized;
+  std::string auth_id = GetSigninManager()->GetAuthenticatedAccountId();
+  BookmarkServerClusterService::Serialize(cluster_data_, &serialized, auth_id);

[battre, 2014/09/26 14:33:05]

Nit: Do you need the BookmarkServerClusterService:: prefix?

[noyau (Ping after 24h), 2014/10/01 16:04:42]

Done.

+  pref_service_->SetString(prefs::kBookmarkClusters, serialized);

[battre, 2014/09/26 14:33:04]

Why do you store this as a string instead of a dictionary?

[noyau (Ping after 24h), 2014/10/01 16:04:42]

I didn't do this code originally, so I must admit I didn't think too much about
it when I moved it here. I changed it to store the dictionary directly.

+}
+
+void BookmarkServerClusterService::LoadModel() {
+  std::string serialized(pref_service_->GetString(prefs::kBookmarkClusters));
+  std::string auth_id = GetSigninManager()->GetAuthenticatedAccountId();
+
+  ClusterMap loaded_data;
+  BookmarkServerClusterService::Deserialize(serialized, &loaded_data, auth_id);
+  cluster_data_.swap(loaded_data);
+}
+
+//
+// Serialization.
+//
+void BookmarkServerClusterService::Serialize(const ClusterMap& cluster_map,
+                                             std::string* out_result,
+                                             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.
+  // This non reference counted pointer is going to be consumed by |data|.
+  base::ListValue* all_clusters = new base::ListValue;

[battre, 2014/09/26 14:33:05]

scoped_ptr?

[noyau (Ping after 24h), 2014/10/01 16:04:43]

Used scoped_ptr and release() to make the ownership transfer explicit.

+  for (ClusterMap::const_iterator it = cluster_map.begin();
+       it != cluster_map.end();
+       ++it) {
+    // This non reference counted pointer is going to be consumed by
+    // |all_clusters|.
+    base::ListValue* cluster = new base::ListValue;

[battre, 2014/09/26 14:33:05]

scoped_ptr?

[noyau (Ping after 24h), 2014/10/01 16:04:43]

Same, removed the comment and used scoped_ptr and release() to make the transfer
of ownership explicit.

+    cluster->AppendString(it->first);
+    cluster->AppendStrings(it->second);
+    all_clusters->Append(cluster);
+  }
+
+  // The dictionary that will be serialized has two fields: a version field and
+  // a data field.
+  base::DictionaryValue data;
+  data.SetInteger(kPrefServiceVersionKey, kPrefServiceVersion);
+  data.Set(kPrefServiceDataKey, all_clusters);
+  data.SetString(kAuthIdKey, auth_id);

[battre, 2014/09/26 14:33:05]

Why don't you just return data?

[noyau (Ping after 24h), 2014/10/01 16:04:43]

base::DictionaryValue is not copiable (DISALLOW_COPY_AND_ASSIGN). I made it
return a scoped pointer instead.

+
+  std::string output;
+  bool result = base::JSONWriter::Write(&data, &output);
+  DCHECK(result);
+  out_result->swap(output);
+}
+
+bool BookmarkServerClusterService::Deserialize(const std::string& input,
+                                               ClusterMap* out_map,
+                                               const std::string& auth_id) {
+  if (input.empty())
+    return false;
+
+  base::JSONReader reader;

[Mark, 2014/09/18 18:10:43]

Any reason why you would serialize this down to JSON instead of just serializing
the protobufs directly?

[Yaron, 2014/09/23 17:08:30]

+1

[battre, 2014/09/26 14:33:04]

I think this is used only for the pref service. There is no need to serialize it
at all. Just store it as a dictionary.

[noyau (Ping after 24h), 2014/10/01 16:04:42]

The is no precedent in storing binary data in prefs, so it is now stored as a
dictionary.

+  scoped_ptr<base::Value> value(reader.ReadToValue(input));
+  if (value == NULL)
+    return false;
+  if (!value->IsType(base::Value::TYPE_DICTIONARY))
+    return false;
+
+  base::DictionaryValue* dictionary_value;
+  bool result = value->GetAsDictionary(&dictionary_value);
+  if (!result)

[battre, 2014/09/26 14:33:05]

nit: I would inline all of these
if (!value->GetAsDictionary(&dictionary_value))
  return false;

[noyau (Ping after 24h), 2014/10/01 16:04:43]

Done.

+    return false;
+
+  // Check version.
+  int version;
+  result = dictionary_value->GetInteger(kPrefServiceVersionKey, &version);
+  if (!result)
+    return false;
+  if (version != kPrefServiceVersion)
+    return false;
+
+  // Check auth id.
+  std::string id;
+  result = dictionary_value->GetString(kAuthIdKey, &id);
+  if (!result)
+    return false;
+  if (id != auth_id)
+    return false;
+
+  base::ListValue* all_clusters;
+  result = dictionary_value->GetList(kPrefServiceDataKey, &all_clusters);
+  if (!result)
+    return false;
+
+  ClusterMap output;
+  for (size_t index = 0; index < all_clusters->GetSize(); ++index) {
+    base::ListValue* cluster;

[battre, 2014/09/26 14:33:05]

base::ListValue* cluster = NULL;

[noyau (Ping after 24h), 2014/10/01 16:04:42]

Done.

+    result = all_clusters->GetList(index, &cluster);
+    if (!result)
+      return false;
+    if (cluster->GetSize() < 1)
+      return false;
+    std::string key;
+    result = cluster->GetString(0, &key);
+    if (!result)
+      return false;
+    std::vector<std::string> stars_ids;
+    for (size_t index = 1; index < cluster->GetSize(); ++index) {
+      std::string stars_id;
+      result = cluster->GetString(index, &stars_id);
+      if (!result)
+        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