Managed bookmarks for supervised users

chrome/browser/supervised_user/supervised_user_bookmarks_handler.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 "chrome/browser/supervised_user/supervised_user_bookmarks_handler.h"
+
+#include "base/logging.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/values.h"
+#include "components/url_fixer/url_fixer.h"
+
+namespace {
+
+const char kKeyLink[] = "SupervisedBookmarkLink";

[Pam (message me for reviews), 2015/01/14 14:03:37]

A little more description of where these keys apply or come from would be good.

[Marc Treib, 2015/01/14 16:40:50]

Done.

+const char kKeyFolder[] = "SupervisedBookmarkFolder";
+
+const char kId[] = "id";
+const char kName[] = "name";
+const char kUrl[] = "url";
+const char kChildren[] = "children";
+
+bool ExtractId(const std::string& key, int* id) {
+  // |key| can be either "<ID>-<Value>" or just "<ID>".

[Pam (message me for reviews), 2015/01/14 14:03:37]

Under what circumstances does each of these |key| formats come up? And should
the - be a : ?

[Marc Treib, 2015/01/14 16:40:50]

Updated the comment. And yes, that should be ":", thanks.

+  std::string id_str = key.substr(0, key.find_first_of(':'));
+  if (!base::StringToInt(id_str, id)) {
+    LOG(WARNING) << "Failed to parse id from " << key;
+    return false;
+  }
+  LOG_IF(WARNING, *id < 0) << "IDs should be >= 0, but got "
+                           << *id << " from " << key;
+  return true;
+}
+
+bool ExtractValue(const std::string& key, std::string* value) {
+  // |key| must be "<ID>-<Value>".

[Pam (message me for reviews), 2015/01/14 14:03:37]

- --> :

[Marc Treib, 2015/01/14 16:40:50]

Done.

+  size_t pos = key.find_first_of(':');
+  if (pos == std::string::npos) {
+    LOG(WARNING) << "Failed to parse value from " << key;
+    return false;
+  }
+  *value = key.substr(pos + 1);
+  return true;
+}
+
+bool ExtractIdAndValue(const std::string& key, int* id, std::string* value) {
+  return ExtractId(key, id) && ExtractValue(key, value);
+}
+
+base::ListValue* FindNode(base::ListValue* root, int id) {

[Pam (message me for reviews), 2015/01/14 14:03:37]

FindFolder would be a clearer function name. Also, function-level comment
please, since this is not completely trivial.

[Marc Treib, 2015/01/14 16:40:50]

Done.

+  if (id == 0)  // We're looking for the root folder. Assume this is it.
+    return root;
+
+  for (size_t i = 0; i < root->GetSize(); ++i) {
+    base::DictionaryValue* item = nullptr;
+    root->GetDictionary(i, &item);
+    DCHECK_NE(item, (base::DictionaryValue*)nullptr);
+
+    base::ListValue* children;
+    if (!item->GetList(kChildren, &children))
+      continue;  // Skip bookmarks. Only interested in folders.
+
+    // Is this it?
+    int node_id;
+    if (item->GetInteger(kId, &node_id) && node_id == id)
+      return children;
+
+    // Recurse.
+    base::ListValue* result = FindNode(children, id);
+    if (result)
+      return result;
+  }
+  return nullptr;
+}
+
+}  // namespace
+
+SupervisedUserBookmarksHandler::Folder::Folder(
+    int id, const std::string& name, int parent_id)
+    : id(id), name(name), parent_id(parent_id) {
+}
+
+SupervisedUserBookmarksHandler::Link::Link(
+    const std::string& url, const std::string& name, int parent_id)
+    : url(url), name(name), parent_id(parent_id) {
+}
+
+SupervisedUserBookmarksHandler::SupervisedUserBookmarksHandler() {
+}
+
+SupervisedUserBookmarksHandler::~SupervisedUserBookmarksHandler() {
+}
+
+scoped_ptr<base::ListValue> SupervisedUserBookmarksHandler::BuildBookmarksTree(
+    const base::DictionaryValue& settings) {
+  SupervisedUserBookmarksHandler handler;
+  handler.ParseSettings(settings);
+  return handler.BuildTree();
+}
+
+void SupervisedUserBookmarksHandler::ParseSettings(
+    const base::DictionaryValue& settings) {
+  const base::DictionaryValue* folders;
+  if (settings.GetDictionary(kKeyFolder, &folders))
+    ParseFolders(*folders);
+
+  const base::DictionaryValue* links;
+  if (settings.GetDictionary(kKeyLink, &links))
+    ParseLinks(*links);
+}
+
+void SupervisedUserBookmarksHandler::ParseFolders(
+    const base::DictionaryValue& folders) {
+  for (base::DictionaryValue::Iterator it(folders); !it.IsAtEnd();
+       it.Advance()) {
+    int id;
+    if (!ExtractId(it.key(), &id))
+      continue;
+    std::string value;
+    it.value().GetAsString(&value);
+    std::string name;
+    int parent_id;
+    if (!ExtractIdAndValue(value, &parent_id, &name))
+      continue;
+    folders_.push_back(Folder(id, name, parent_id));
+  }
+}
+
+void SupervisedUserBookmarksHandler::ParseLinks(
+    const base::DictionaryValue& links) {
+  for (base::DictionaryValue::Iterator it(links); !it.IsAtEnd(); it.Advance()) {
+    std::string url;
+    if (!ExtractValue(it.key(), &url))
+      continue;
+    std::string value;
+    it.value().GetAsString(&value);
+    std::string name;
+    int parent_id;
+    if (!ExtractIdAndValue(value, &parent_id, &name))
+      continue;
+    links_.push_back(Link(url, name, parent_id));

[Pam (message me for reviews), 2015/01/14 14:03:37]

I'm almost tempted to make a union for id|url, and merge ParseFolders and
ParseLinks into a single method. But maybe that's too clever for its own good.

[Marc Treib, 2015/01/14 16:40:50]

Hrm. I agree that the mostly-duplicated code isn't too pretty, but a union..
ugh. I prefer to keep it as is.

[Pam (message me for reviews), 2015/01/15 11:37:04]

Could also be a struct or class with separate url and id fields. But it's also
OK as-is.

+  }
+}
+
+scoped_ptr<base::ListValue> SupervisedUserBookmarksHandler::BuildTree() {
+  root_.reset(new base::ListValue);
+  AddFoldersToTree();
+  AddLinksToTree();
+  return root_.Pass();
+}
+
+void SupervisedUserBookmarksHandler::AddFoldersToTree() {
+  std::vector<Folder> folders = folders_;
+  std::vector<Folder> folders_failed;
+  while (!folders.empty() && folders.size() != folders_failed.size()) {

[Pam (message me for reviews), 2015/01/14 14:03:37]

This snippet is a bit hard to understand. A comment or two describing the
overall process would help.

Also, consider at least logging an error if there are folders that can't be
added.

[Marc Treib, 2015/01/14 16:40:50]

Done.

+    folders_failed.clear();
+    for (const auto& folder : folders) {
+      scoped_ptr<base::DictionaryValue> node(new base::DictionaryValue);
+      node->SetIntegerWithoutPathExpansion(kId, folder.id);
+      node->SetStringWithoutPathExpansion(kName, folder.name);
+      node->SetWithoutPathExpansion(kChildren, new base::ListValue);
+      if (!AddNodeToTree(folder.parent_id, node.Pass()))
+        folders_failed.push_back(folder);
+    }
+    folders.swap(folders_failed);
+  }
+}
+
+void SupervisedUserBookmarksHandler::AddLinksToTree() {
+  for (const auto& link : links_) {
+    scoped_ptr<base::DictionaryValue> node(new base::DictionaryValue);
+    GURL url = url_fixer::FixupURL(link.url, std::string());
+    if (!url.is_valid()) {
+      DLOG(WARNING) << "Got invalid URL: " << link.url;
+      continue;
+    }
+    node->SetStringWithoutPathExpansion(kUrl, url.spec());
+    node->SetStringWithoutPathExpansion(kName, link.name);
+    AddNodeToTree(link.parent_id, node.Pass());

[Pam (message me for reviews), 2015/01/14 14:03:37]

Also consider logging a warning if the bookmark can't be added.

[Marc Treib, 2015/01/14 16:40:50]

Done.

+  }
+}
+
+bool SupervisedUserBookmarksHandler::AddNodeToTree(
+    int parent_id,
+    scoped_ptr<base::DictionaryValue> node) {
+  base::ListValue* parent = FindNode(root_.get(), parent_id);
+  if (!parent)
+    return false;
+  parent->Append(node.release());
+  return true;
+}