Managed bookmarks for supervised users

chrome/browser/bookmarks/chrome_bookmark_client.cc

Index: chrome/browser/bookmarks/chrome_bookmark_client.cc
diff --git a/chrome/browser/bookmarks/chrome_bookmark_client.cc b/chrome/browser/bookmarks/chrome_bookmark_client.cc
index 1823aafadcd1f51824aee5404f06d4ae84ac894e..41fd8f63586f467e1087d176b09514f492d414da 100644
--- a/chrome/browser/bookmarks/chrome_bookmark_client.cc
+++ b/chrome/browser/bookmarks/chrome_bookmark_client.cc
@@ -41,13 +41,38 @@ void RunCallbackWithImage(
   callback.Run(result);
 }
 
+bool IsDescendantOf(const BookmarkNode* node, const BookmarkNode* root) {
+  return node && node->HasAncestor(root);
+}
+
+bool HasDescendantsOf(const std::vector<const BookmarkNode*>& list,

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

This name is a little misleading, so a quick comment would be good.

// Returns true if any node in |list| is a descendant of |root|.

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

Done.

+                      const BookmarkNode* root) {
+  for (size_t i = 0; i < list.size(); ++i) {
+    if (IsDescendantOf(list[i], root))
+      return true;
+  }
+  return false;
+}
+
+void LoadInitialContents(BookmarkPermanentNode* node,
+                         base::ListValue* initial_bookmarks,
+                         int64* next_node_id) {
+  // Load the initial contents of the |node| now, and assign it an unused ID.

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

What are "initial" contents, as opposed to any other kind?

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

AIUI, the "initial contents" are just "whatever's already in the pref when this
thing starts up".

+  int64 id = *next_node_id;
+  node->set_id(id);
+  *next_node_id = policy::ManagedBookmarksTracker::LoadInitial(
+      node, initial_bookmarks, id + 1);

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

I suppose this object is profile-keyed and not multi-threaded, so no chance of a
problem here, right?

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

"this object" meaning what, exactly? LoadInitial is a static method.
In any case, this isn't actually new code, so if there were any problem, it
should have been there already.

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

OK, then, I suppose there's nothing to do here... apart from scanning the bug
tracker to see if there are any suspicious-looking bookmarks bugs. :) (I did a
quick search, but not a really thorough reading.)

+  node->set_visible(!node->empty());
+}
+
 }  // namespace
 
 ChromeBookmarkClient::ChromeBookmarkClient(Profile* profile)
     : profile_(profile),
       history_service_(NULL),
       model_(NULL),
-      managed_node_(NULL) {
+      managed_node_(NULL),
+      supervised_node_(NULL) {
 }
 
 ChromeBookmarkClient::~ChromeBookmarkClient() {
@@ -62,8 +87,14 @@ void ChromeBookmarkClient::Init(BookmarkModel* model) {
   managed_bookmarks_tracker_.reset(new policy::ManagedBookmarksTracker(
       model_,
       profile_->GetPrefs(),
+      false,
       base::Bind(&ChromeBookmarkClient::GetManagedBookmarksDomain,
                  base::Unretained(this))));
+  supervised_bookmarks_tracker_.reset(new policy::ManagedBookmarksTracker(
+      model_,
+      profile_->GetPrefs(),
+      true,
+      base::Callback<std::string()>()));
 }
 
 void ChromeBookmarkClient::Shutdown() {
@@ -76,16 +107,33 @@ void ChromeBookmarkClient::Shutdown() {
 }
 
 bool ChromeBookmarkClient::IsDescendantOfManagedNode(const BookmarkNode* node) {
-  return node && node->HasAncestor(managed_node_);
+  return IsDescendantOf(node, managed_node_);
 }
 
 bool ChromeBookmarkClient::HasDescendantsOfManagedNode(
     const std::vector<const BookmarkNode*>& list) {
-  for (size_t i = 0; i < list.size(); ++i) {
-    if (IsDescendantOfManagedNode(list[i]))
-      return true;
-  }
-  return false;
+  return HasDescendantsOf(list, managed_node_);
+}
+
+bool ChromeBookmarkClient::IsDescendantOfSupervisedNode(
+    const BookmarkNode* node) {
+  return IsDescendantOf(node, supervised_node_);
+}
+
+bool ChromeBookmarkClient::HasDescendantsOfSupervisedNode(
+    const std::vector<const BookmarkNode*>& list) {
+  return HasDescendantsOf(list, supervised_node_);
+}
+
+bool ChromeBookmarkClient::IsDescendantOfManagedOrSupervisedNode(
+    const BookmarkNode* node) {
+  return IsDescendantOfManagedNode(node) || IsDescendantOfSupervisedNode(node);
+}
+
+bool ChromeBookmarkClient::HasDescendantsOfManagedOrSupervisedNode(
+    const std::vector<const BookmarkNode*>& list) {
+  return HasDescendantsOfManagedNode(list) ||
+         HasDescendantsOfSupervisedNode(list);
 }
 
 bool ChromeBookmarkClient::PreferTouchIcon() {
@@ -148,8 +196,9 @@ bool ChromeBookmarkClient::IsPermanentNodeVisible(
   DCHECK(node->type() == BookmarkNode::BOOKMARK_BAR ||
          node->type() == BookmarkNode::OTHER_NODE ||
          node->type() == BookmarkNode::MOBILE ||
-         node == managed_node_);
-  if (node == managed_node_)
+         node == managed_node_ ||
+         node == supervised_node_);
+  if (node == managed_node_ || node == supervised_node_)
     return false;
 #if !defined(OS_IOS)
   return node->type() != BookmarkNode::MOBILE;
@@ -163,33 +212,38 @@ void ChromeBookmarkClient::RecordAction(const base::UserMetricsAction& action) {
 }
 
 bookmarks::LoadExtraCallback ChromeBookmarkClient::GetLoadExtraNodesCallback() {
-  // Create the managed_node now; it will be populated in the LoadExtraNodes
-  // callback.
-  // The ownership of managed_node_ is in limbo until LoadExtraNodes runs,
-  // so we leave it in the care of the closure meanwhile.
+  // Create the managed_node_ and supervised_node_ now; they will be populated
+  // in the LoadExtraNodes callback.
+  // The ownership of managed_node_ and supervised_node_ is in limbo until
+  // LoadExtraNodes runs, so we leave them in the care of the closure meanwhile.
   scoped_ptr<BookmarkPermanentNode> managed(new BookmarkPermanentNode(0));
   managed_node_ = managed.get();
+  scoped_ptr<BookmarkPermanentNode> supervised(new BookmarkPermanentNode(1));
+  supervised_node_ = supervised.get();
 
   return base::Bind(
       &ChromeBookmarkClient::LoadExtraNodes,
       base::Passed(&managed),
-      base::Passed(managed_bookmarks_tracker_->GetInitialManagedBookmarks()));
+      base::Passed(managed_bookmarks_tracker_->GetInitialManagedBookmarks()),
+      base::Passed(&supervised),
+      base::Passed(
+          supervised_bookmarks_tracker_->GetInitialManagedBookmarks()));
 }
 
 bool ChromeBookmarkClient::CanSetPermanentNodeTitle(
     const BookmarkNode* permanent_node) {
   // The |managed_node_| can have its title updated if the user signs in or

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

Please explain why |managed_node_| (that is, the top-level folder) can have its
title updated, but |supervised_node_| can't. (The Managed Bookmarks folder can
have the name of the managed domain in its title.)

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

Done.

   // out.
-  return !IsDescendantOfManagedNode(permanent_node) ||
+  return !IsDescendantOfManagedOrSupervisedNode(permanent_node) ||
          permanent_node == managed_node_;
 }
 
 bool ChromeBookmarkClient::CanSyncNode(const BookmarkNode* node) {
-  return !IsDescendantOfManagedNode(node);
+  return !IsDescendantOfManagedOrSupervisedNode(node);
 }
 
 bool ChromeBookmarkClient::CanBeEditedByUser(const BookmarkNode* node) {
-  return !IsDescendantOfManagedNode(node);
+  return !IsDescendantOfManagedOrSupervisedNode(node);
 }
 
 void ChromeBookmarkClient::SetHistoryService(HistoryService* history_service) {
@@ -225,26 +279,30 @@ void ChromeBookmarkClient::BookmarkModelLoaded(BookmarkModel* model,
   // may have occurred while the initial managed bookmarks were being loaded
   // on the background.
   managed_bookmarks_tracker_->Init(managed_node_);
+  supervised_bookmarks_tracker_->Init(supervised_node_);
 }
 
 // static
 bookmarks::BookmarkPermanentNodeList ChromeBookmarkClient::LoadExtraNodes(
     scoped_ptr<BookmarkPermanentNode> managed_node,
     scoped_ptr<base::ListValue> initial_managed_bookmarks,
+    scoped_ptr<BookmarkPermanentNode> supervised_node,
+    scoped_ptr<base::ListValue> initial_supervised_bookmarks,
     int64* next_node_id) {
-  // Load the initial contents of the |managed_node| now, and assign it an
-  // unused ID.
-  int64 managed_id = *next_node_id;
-  managed_node->set_id(managed_id);
-  *next_node_id = policy::ManagedBookmarksTracker::LoadInitial(
-      managed_node.get(), initial_managed_bookmarks.get(), managed_id + 1);
-  managed_node->set_visible(!managed_node->empty());
-  managed_node->SetTitle(
-      l10n_util::GetStringUTF16(IDS_BOOKMARK_BAR_MANAGED_FOLDER_DEFAULT_NAME));
+  LoadInitialContents(
+      managed_node.get(), initial_managed_bookmarks.get(), next_node_id);
+  managed_node->SetTitle(l10n_util::GetStringUTF16(
+      IDS_BOOKMARK_BAR_MANAGED_FOLDER_DEFAULT_NAME));
+
+  LoadInitialContents(
+      supervised_node.get(), initial_supervised_bookmarks.get(), next_node_id);
+  supervised_node->SetTitle(l10n_util::GetStringUTF16(
+      IDS_BOOKMARK_BAR_SUPERVISED_FOLDER_DEFAULT_NAME));
 
   bookmarks::BookmarkPermanentNodeList extra_nodes;
-  // Ownership of the managed node passed to the caller.
+  // Ownership of the managed and supervised nodes passed to the caller.
   extra_nodes.push_back(managed_node.release());
+  extra_nodes.push_back(supervised_node.release());
 
   return extra_nodes.Pass();
 }