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..48a0e052704b86a225c4d4040cd52488f5ba8add
--- /dev/null
+++ b/components/enhanced_bookmarks/bookmark_server_cluster_service.cc
@@ -0,0 +1,340 @@
+// 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/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 char* kPrefServiceKey =
+    "pref_service_key_for_stored_bookmark_clusters";

[battre, 2014/09/12 16:49:02]

Please use a different key. E.g. "stored_bookmark_clusters"

[noyau (Ping after 24h), 2014/09/18 12:41:47]

Done. And moved to pref_names.

+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;
+    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);
+
+  // 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 (auto pair : cluster_data_)

[Kibeom Kim (inactive), 2014/09/12 21:31:25]

I'm not sure if this is changed, but people on chromium-dev once told not to use
c++11 things yet few weeks ago.
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/A5lr2jKAIaI/I2ZDw...

[noyau (Ping after 24h), 2014/09/18 12:41:47]

Yes you're right. Porting code from iOS I'm always caught by that one (not to
mention that the bots all compiled this code with no problem!). Fixed, I removed
all the auto and C++11 for loops from the file.

+    cluster_names.push_back(pair.first);
+
+  return cluster_names;
+}
+
+void BookmarkServerClusterService::RegisterPrefs(
+    user_prefs::PrefRegistrySyncable* registry) {
+  // Register a string, unsyncable preference.
+  registry->RegisterStringPref(
+      kPrefServiceKey,

[battre, 2014/09/12 16:49:01]

Please follow the style of e.g. components/component_updater/pref_names.h and
put the preference into a file pair pref_names.*, use a nameservice, add
documentation, etc.

[noyau (Ping after 24h), 2014/09/18 12:41:48]

Done.

+      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_);

[Mark, 2014/09/18 18:08:51]

You are also going to need to specify the version parameter (v=[version tag])

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

Done. Added the version for the search service as well.

+
+  // 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.
+
+  std::map<std::string, std::vector<std::string> > new_cluster_data;
+  for (auto cluster : response_proto.clusters()) {
+    const std::string& title = cluster.title();
+    if (!title.length())

[Kibeom Kim (inactive), 2014/09/12 21:31:24]

.empty()? (also #157 and #160)

[noyau (Ping after 24h), 2014/09/18 12:41:47]

Done.

+      continue;
+    std::vector<std::string> starids;
+    for (std::string doc : cluster.docs()) {
+      if (doc.length())
+        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())) {

[Kibeom Kim (inactive), 2014/09/12 21:31:24]

can we use std:map's operator == here? (I think the result should be same,
though unsure if there is some performance difference)

[noyau (Ping after 24h), 2014/09/18 12:41:47]

Excellent question. According to the spec std::map does not implement operator
==, but it compiles on iOS, so obviously something implements it. The semantics
of std::equals are clear, and I can't find documentation on the semantics of ==
for std::map, so in doubt, I'd rather keep that code as is: I know it works,
we've been using it for months on iOS.

Performance there is quite irrelevant anyway, as this only happens following a
user action.

[Kibeom Kim (inactive), 2014/09/29 21:15:55]

I agree that we don't need to change already working code. :)

Just for reference: http://en.cppreference.com/w/cpp/container/map/operator_cmp

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

Acknowledged.

+    *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::BookmarkNodeAdded(BookmarkModel* model,

[Kibeom Kim (inactive), 2014/09/12 21:31:25]

Q: do we need BookmarkNodeAdded and BookmarkMetaInfoChanged functions here? It
seems we can omit them and let them fall back to BookmarkServerService functions
directly.

[noyau (Ping after 24h), 2014/09/18 12:41:47]

Done.

+                                                     const BookmarkNode* parent,
+                                                     int index) {
+  BookmarkServerService::BookmarkNodeAdded(model, parent, index);
+}
+
+void BookmarkServerClusterService::BookmarkMetaInfoChanged(
+    BookmarkModel* model,
+    const BookmarkNode* node) {
+  BookmarkServerService::BookmarkMetaInfoChanged(model, node);
+}
+
+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();
+  bool result = BookmarkServerClusterService::Serialize(
+      cluster_data_, &serialized, auth_id);
+  if (result)
+    pref_service_->SetString(kPrefServiceKey, serialized);

[Kibeom Kim (inactive), 2014/09/12 21:31:25]

Q: looks like this line won't be called on ::GoogleSignedOut, is it ok to leave
kPrefServiceKey perf key ?

[noyau (Ping after 24h), 2014/09/18 12:41:48]

The key will be there, with the proper version number, but with empty content.
So it is fine.  The only case where this would fail is if Serialize returns
false, and this actually can't happen! Fixing the function signature.

+}
+
+void BookmarkServerClusterService::LoadModel() {
+  std::string serialized(pref_service_->GetString(kPrefServiceKey));
+  std::string auth_id = GetSigninManager()->GetAuthenticatedAccountId();
+
+  ClusterMap loaded_data;
+  bool result = BookmarkServerClusterService::Deserialize(
+      serialized, &loaded_data, auth_id);
+  if (result)
+    cluster_data_.swap(loaded_data);
+}
+
+//
+// Serialization.
+//
+bool 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;
+  for (auto it : cluster_map) {
+    // This non reference counted pointer is going to be consumed by
+    // |all_clusters|.
+    base::ListValue* cluster = new base::ListValue;
+    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);
+
+  std::string output;
+  bool result = base::JSONWriter::Write(&data, &output);
+  DCHECK(result);
+  out_result->swap(output);
+  return true;
+}
+
+bool BookmarkServerClusterService::Deserialize(const std::string& input,
+                                               ClusterMap* out_map,
+                                               const std::string& auth_id) {
+  if (input.empty())
+    return false;
+
+  base::JSONReader reader;
+  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)
+    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;
+    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