Fix componentization of chrome/browser/bookmarks

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 a3ca17a783545e887cc048085eb02816f16de113..9daebd4ec72c4a665ad1b42a2462c9be186ea639 100644
--- a/chrome/browser/bookmarks/chrome_bookmark_client.cc
+++ b/chrome/browser/bookmarks/chrome_bookmark_client.cc
@@ -10,13 +10,11 @@
 #include "base/values.h"
 #include "chrome/browser/favicon/favicon_service_factory.h"
 #include "chrome/browser/history/history_service_factory.h"
-#include "chrome/browser/policy/profile_policy_connector.h"
-#include "chrome/browser/policy/profile_policy_connector_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/bookmarks/browser/bookmark_node.h"
 #include "components/bookmarks/browser/bookmark_utils.h"
-#include "components/bookmarks/managed/managed_bookmarks_tracker.h"
+#include "components/bookmarks/managed/managed_bookmark_service.h"
 #include "components/favicon/core/favicon_service.h"
 #include "components/history/core/browser/history_service.h"
 #include "components/history/core/browser/url_database.h"
@@ -28,11 +26,6 @@
 #include "policy/policy_constants.h"
 #include "ui/base/l10n/l10n_util.h"
 
-using bookmarks::BookmarkModel;
-using bookmarks::BookmarkNode;
-using bookmarks::BookmarkPermanentNode;
-using bookmarks::ManagedBookmarksTracker;
-
 namespace {
 
 void RunCallbackWithImage(
@@ -49,53 +42,41 @@ void RunCallbackWithImage(
   callback.Run(result);
 }
 
-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 = ManagedBookmarksTracker::LoadInitial(
-      node, initial_bookmarks, id + 1);
-  node->set_visible(!node->empty());
+bool IsPermanentNodeValid(

[sky, 2015/07/20 15:53:19]

I was a bit confused by Valid in the name. How about IsPermanentNode?

[sdefresne, 2015/07/20 17:43:40]

Done.

+    const bookmarks::BookmarkPermanentNode* node,
+    bookmarks::ManagedBookmarkService* managed_bookmark_service) {
+  bookmarks::BookmarkNode::Type type = node->type();
+  if (type == bookmarks::BookmarkNode::BOOKMARK_BAR ||
+      type == bookmarks::BookmarkNode::OTHER_NODE ||
+      type == bookmarks::BookmarkNode::MOBILE) {
+    return true;
+  }
+
+  if (!managed_bookmark_service)
+    return false;
+
+  return node == managed_bookmark_service->managed_node() ||
+         node == managed_bookmark_service->supervised_node();
 }
 
 }  // namespace
 
-ChromeBookmarkClient::ChromeBookmarkClient(Profile* profile)
-    : profile_(profile),
-      model_(nullptr),
-      managed_node_(nullptr),
-      supervised_node_(nullptr) {
+ChromeBookmarkClient::ChromeBookmarkClient(
+    Profile* profile,
+    bookmarks::ManagedBookmarkService* managed_bookmark_service)
+    : profile_(profile), managed_bookmark_service_(managed_bookmark_service) {
 }
 
 ChromeBookmarkClient::~ChromeBookmarkClient() {
 }
 
-void ChromeBookmarkClient::Init(BookmarkModel* model) {
-  DCHECK(model);
-  DCHECK(!model_);
-  model_ = model;
-  model_->AddObserver(this);
-
-  managed_bookmarks_tracker_.reset(new ManagedBookmarksTracker(
-      model_,
-      profile_->GetPrefs(),
-      false,
-      base::Bind(&ChromeBookmarkClient::GetManagedBookmarksDomain,
-                 base::Unretained(this))));
-  supervised_bookmarks_tracker_.reset(new ManagedBookmarksTracker(
-      model_,
-      profile_->GetPrefs(),
-      true,
-      base::Callback<std::string()>()));
+void ChromeBookmarkClient::Init(bookmarks::BookmarkModel* model) {
+  if (managed_bookmark_service_)
+    managed_bookmark_service_->BookmarkModelCreated(model);

[sky, 2015/07/20 15:53:19]

As managed_bookmark_service_ is a BookmarkModelObserver can it override
BookmarkModelLoaded rather than explicitly calling BookmarkModelCreated?

[sdefresne, 2015/07/20 17:43:40]

ManagedBookmarkService needs to use the BookmarkModel before it has started
loading (as it returns a callback that will be invoked during BookmarkModel
loading). It is thus not possible to listen to BookmarkModelLoaded and remove
this call as the event will happen too late.

Another option is to change the interface of GetLoadExtraNodesCallback() to
receive the BookmarkModel and then we can remove this Init() method and
ManagedBookmarkService::BookmarkModelCreated().

[sky, 2015/07/20 20:10:56]

Ok, leave what you have.

 }
 
 void ChromeBookmarkClient::Shutdown() {
-  if (model_) {
-    model_->RemoveObserver(this);
-    model_ = nullptr;
-  }
+  managed_bookmark_service_ = nullptr;
   BookmarkClient::Shutdown();
 }
 
@@ -159,18 +140,17 @@ void ChromeBookmarkClient::GetTypedCountForNodes(
 }
 
 bool ChromeBookmarkClient::IsPermanentNodeVisible(
-    const BookmarkPermanentNode* node) {
-  DCHECK(node->type() == BookmarkNode::BOOKMARK_BAR ||
-         node->type() == BookmarkNode::OTHER_NODE ||
-         node->type() == BookmarkNode::MOBILE ||
-         node == managed_node_ ||
-         node == supervised_node_);
-  if (node == managed_node_ || node == supervised_node_)
+    const bookmarks::BookmarkPermanentNode* node) {
+  DCHECK(IsPermanentNodeValid(node, managed_bookmark_service_));
+  if (managed_bookmark_service_ &&
+      (node == managed_bookmark_service_->managed_node() ||
+       node == managed_bookmark_service_->supervised_node())) {
     return false;
+  }
 #if !defined(OS_IOS)
-  return node->type() != BookmarkNode::MOBILE;
+  return node->type() != bookmarks::BookmarkNode::MOBILE;
 #else
-  return node->type() == BookmarkNode::MOBILE;
+  return node->type() == bookmarks::BookmarkNode::MOBILE;
 #endif
 }
 
@@ -179,89 +159,29 @@ void ChromeBookmarkClient::RecordAction(const base::UserMetricsAction& action) {
 }
 
 bookmarks::LoadExtraCallback ChromeBookmarkClient::GetLoadExtraNodesCallback() {
-  // Create the managed_node_ and supervised_node_ with a temporary ID of 0 now.
-  // They will be populated (and assigned proper IDs) 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(0));
-  supervised_node_ = supervised.get();
+  if (!managed_bookmark_service_)
+    return bookmarks::LoadExtraCallback();
 
-  return base::Bind(
-      &ChromeBookmarkClient::LoadExtraNodes,
-      base::Passed(&managed),
-      base::Passed(managed_bookmarks_tracker_->GetInitialManagedBookmarks()),
-      base::Passed(&supervised),
-      base::Passed(
-          supervised_bookmarks_tracker_->GetInitialManagedBookmarks()));
+  return managed_bookmark_service_->GetLoadExtraNodesCallback();
 }
 
 bool ChromeBookmarkClient::CanSetPermanentNodeTitle(
-    const BookmarkNode* permanent_node) {
-  // The |managed_node_| can have its title updated if the user signs in or
-  // out, since the name of the managed domain can appear in it.
-  // Also, both |managed_node_| and |supervised_node_| can have their title
-  // updated on locale changes (crbug.com/459448).
-  return (!bookmarks::IsDescendantOf(permanent_node, managed_node_) &&
-          !bookmarks::IsDescendantOf(permanent_node, supervised_node_)) ||
-         permanent_node == managed_node_ ||
-         permanent_node == supervised_node_;
-}
-
-bool ChromeBookmarkClient::CanSyncNode(const BookmarkNode* node) {
-  return !bookmarks::IsDescendantOf(node, managed_node_) &&
-         !bookmarks::IsDescendantOf(node, supervised_node_);
+    const bookmarks::BookmarkNode* permanent_node) {
+  return !managed_bookmark_service_
+             ? true
+             : managed_bookmark_service_->CanSetPermanentNodeTitle(
+                   permanent_node);
 }
 
-bool ChromeBookmarkClient::CanBeEditedByUser(const BookmarkNode* node) {
-  return !bookmarks::IsDescendantOf(node, managed_node_) &&
-         !bookmarks::IsDescendantOf(node, supervised_node_);
-}
-
-void ChromeBookmarkClient::BookmarkModelChanged() {
-}
-
-void ChromeBookmarkClient::BookmarkModelLoaded(BookmarkModel* model,
-                                               bool ids_reassigned) {
-  BaseBookmarkModelObserver::BookmarkModelLoaded(model, ids_reassigned);
-  // 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) {
-  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 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();
+bool ChromeBookmarkClient::CanSyncNode(const bookmarks::BookmarkNode* node) {
+  return !managed_bookmark_service_
+             ? true
+             : managed_bookmark_service_->CanSyncNode(node);
 }
 
-std::string ChromeBookmarkClient::GetManagedBookmarksDomain() {
-  policy::ProfilePolicyConnector* connector =
-      policy::ProfilePolicyConnectorFactory::GetForBrowserContext(profile_);
-  if (connector->IsPolicyFromCloudPolicy(policy::key::kManagedBookmarks))
-    return connector->GetManagementDomain();
-  return std::string();
+bool ChromeBookmarkClient::CanBeEditedByUser(
+    const bookmarks::BookmarkNode* node) {
+  return !managed_bookmark_service_
+             ? true
+             : managed_bookmark_service_->CanBeEditedByUser(node);
 }