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 e6a145e08e25aaf7536e498b8a2d5881ee91f695..71210bf05ff6e85bc8081f50884aa92c04cae2a1 100644
--- a/chrome/browser/bookmarks/chrome_bookmark_client.cc
+++ b/chrome/browser/bookmarks/chrome_bookmark_client.cc
@@ -43,13 +43,39 @@ void RunCallbackWithImage(
   callback.Run(result);
 }
 
+bool IsDescendantOf(const BookmarkNode* node, const BookmarkNode* root) {
+  return node && node->HasAncestor(root);
+}
+
+// Returns true if any node in |list| is a descendant of |root|.
+bool HasDescendantsOf(const std::vector<const BookmarkNode*>& list,
+                      const BookmarkNode* root) {
+  for (size_t i = 0; i < list.size(); ++i) {

[sky, 2015/02/04 18:36:58]

nit: use new for loop

[Marc Treib, 2015/02/05 12:57:06]

Done.

+    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.
+  int64 id = *next_node_id;
+  node->set_id(id);
+  *next_node_id = policy::ManagedBookmarksTracker::LoadInitial(
+      node, initial_bookmarks, id + 1);
+  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() {
@@ -64,8 +90,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() {
@@ -78,16 +110,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() {
@@ -150,8 +199,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;
@@ -165,33 +215,39 @@ 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));

[sky, 2015/02/04 18:36:58]

This conflicts with the id of the bookmark bar.

[Marc Treib, 2015/02/05 12:57:06]

Where is that defined, and what ID can I use instead?
Also, how does the conflict manifest? Everything seems to be working fine, as
far as I can tell. Sorry if I'm being dense here, I'm not very familiar with the
bookmarks code.

[sky, 2015/02/05 17:57:11]

From BookmarkNode:

  // Returns an unique id for this node.
  // For bookmark nodes that are managed by the bookmark model, the IDs are
  // persisted across sessions.
  int64 id() const { return id_; }

Your patch breaks this as the supervised node and bookmark bar nodes now have
the same id and are no longer unique. Functions like
bookmarks::GetBookmarkNodeByID() now are undeterministic.

The ids are used throughout the code in random places. Biggest case may be
copy/paste that writes the id (BookmarkNodeData).

As far as how to fix it, there needs to be some way for the client to have the
model create the nodes so the nodes have the right ids. Likely the client needs
to be handed a special interface for this.

[Marc Treib, 2015/02/06 13:40:37]

I've taken another look, and I don't think that's necessary: Here we create the
nodes with a temporary/"empty" ID, but we don't do anything with them, so it
should be okay if the ids aren't unique. Later, in LoadExtraNodes (specifically,
in LoadInitialContents), we do assign proper unique IDs before returning them.
I've changed the hardcoded ID to 0 and added a comment to make this clear.

+  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
-  // out.
-  return !IsDescendantOfManagedNode(permanent_node) ||
+  // out, since the name of the managed domain can appear in it. The
+  // |supervised_node_| has a fixed title which can never be updated.
+  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) {
@@ -223,30 +279,34 @@ void ChromeBookmarkClient::BookmarkAllUserNodesRemoved(
 
 void ChromeBookmarkClient::BookmarkModelLoaded(BookmarkModel* model,
                                                bool ids_reassigned) {
-  // Start tracking the managed bookmarks. This will detect any changes that
-  // may have occurred while the initial managed bookmarks were being loaded
-  // on the background.
+  // Start tracking the managed and supervised bookmarks. This will detect any
+  // changes that may have occurred while the initial managed and supervised
+  // 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();
 }