BookmarkClient can add extra nodes to BookmarkModel.

components/bookmarks/browser/bookmark_model.cc

Index: components/bookmarks/browser/bookmark_model.cc
diff --git a/components/bookmarks/browser/bookmark_model.cc b/components/bookmarks/browser/bookmark_model.cc
index 31bd316153276fd3cfda791afae4f1aa5a006219..c835f9b787aa577b3346cbe7e2ecf1465acd06d5 100644
--- a/components/bookmarks/browser/bookmark_model.cc
+++ b/components/bookmarks/browser/bookmark_model.cc
@@ -54,8 +54,8 @@ class VisibilityComparator
   // Returns true if |n1| preceeds |n2|.
   bool operator()(const BookmarkPermanentNode* n1,
                   const BookmarkPermanentNode* n2) {
-    bool n1_visible = client_->IsPermanentNodeVisible(n1->type());
-    bool n2_visible = client_->IsPermanentNodeVisible(n2->type());
+    bool n1_visible = client_->IsPermanentNodeVisible(n1);
+    bool n2_visible = client_->IsPermanentNodeVisible(n2);
     return n1_visible != n2_visible && n1_visible;
   }
 
@@ -212,6 +212,10 @@ void BookmarkModel::RemoveAll() {
     base::AutoLock url_lock(url_lock_);
     for (int i = 0; i < root_.child_count(); ++i) {
       BookmarkNode* permanent_node = root_.GetChild(i);
+
+      if (!client_->CanRemoveNode(permanent_node))
+        continue;
+
       for (int j = permanent_node->child_count() - 1; j >= 0; --j) {
         BookmarkNode* child_node = permanent_node->GetChild(j);
         removed_nodes.push_back(child_node);
@@ -319,7 +323,7 @@ void BookmarkModel::SetTitle(const BookmarkNode* node,
   if (node->GetTitle() == title)
     return;
 
-  if (is_permanent_node(node)) {
+  if (is_permanent_node(node) && !client_->CanSetTitle(node)) {
     NOTREACHED();
     return;
   }
@@ -431,6 +435,8 @@ void BookmarkModel::DeleteNodeMetaInfo(const BookmarkNode* node,
 void BookmarkModel::SetNodeSyncTransactionVersion(
     const BookmarkNode* node,
     int64 sync_transaction_version) {
+  DCHECK(client_->CanSyncNode(node));
+
   if (sync_transaction_version == node->sync_transaction_version())
     return;
 
@@ -614,6 +620,8 @@ const BookmarkNode* BookmarkModel::AddURLWithCreationTimeAndMetaInfo(
 }
 
 void BookmarkModel::SortChildren(const BookmarkNode* parent) {
+  DCHECK(client_->CanReorderChildren(parent));
+
   if (!parent || !parent->is_folder() || is_root_node(parent) ||
       parent->child_count() <= 1) {
     return;
@@ -641,6 +649,8 @@ void BookmarkModel::SortChildren(const BookmarkNode* parent) {
 void BookmarkModel::ReorderChildren(
     const BookmarkNode* parent,
     const std::vector<const BookmarkNode*>& ordered_nodes) {
+  DCHECK(client_->CanReorderChildren(parent));
+
   // Ensure that all children in |parent| are in |ordered_nodes|.
   DCHECK_EQ(static_cast<size_t>(parent->child_count()), ordered_nodes.size());
   for (size_t i = 0; i < ordered_nodes.size(); ++i)
@@ -688,8 +698,8 @@ void BookmarkModel::ClearStore() {
 
 void BookmarkModel::SetPermanentNodeVisible(BookmarkNode::Type type,
                                             bool value) {
-  AsMutable(PermanentNode(type))->set_visible(
-      value || client_->IsPermanentNodeVisible(type));
+  BookmarkPermanentNode* node = AsMutable(PermanentNode(type));
+  node->set_visible(value || client_->IsPermanentNodeVisible(node));
 }
 
 const BookmarkPermanentNode* BookmarkModel::PermanentNode(
@@ -758,18 +768,27 @@ void BookmarkModel::DoneLoading(scoped_ptr<BookmarkLoadDetails> details) {
   mobile_node_ = details->release_mobile_folder_node();
   index_.reset(details->release_index());
 
+  // Get any extra nodes and take ownership of them at the |root_|.
+  std::vector<BookmarkPermanentNode*> extra_nodes;
+  details->release_extra_nodes(&extra_nodes);
+
+  // Let the client get handles to the extra permanent nodes on the main thread.
+  client_->ExtraNodesLoaded(extra_nodes);

[sky, 2014/06/04 20:39:52]

Can this be nuked and instead use BookmarkModelLoaded and dig out the nodes?
Alternatively can't the callback track the created nodes if it wants to?

[Joao da Silva, 2014/06/04 22:35:58]

BookmarkModelLoaded() comes a tad too late, because the stable_sort below relies
on IsPermanentNodeVisible(), which requires the client to have handles to the
extra nodes. The nodes can also be out-of-order by then, again due to the sort.

But the callback can indeed track the extra nodes and this call is redundant, so
Done!

+
   // WARNING: order is important here, various places assume the order is
   // constant (but can vary between embedders with the initial visibility
   // of permanent nodes).
-  BookmarkPermanentNode* root_children[] = {
-      bookmark_bar_node_, other_node_, mobile_node_,
-  };
-  std::stable_sort(root_children,
-                   root_children + arraysize(root_children),
+  std::vector<BookmarkPermanentNode*> root_children;
+  root_children.push_back(bookmark_bar_node_);
+  root_children.push_back(other_node_);
+  root_children.push_back(mobile_node_);
+  for (size_t i = 0; i < extra_nodes.size(); ++i)
+    root_children.push_back(extra_nodes[i]);
+  std::stable_sort(root_children.begin(),
+                   root_children.end(),
                    VisibilityComparator(client_));
-  for (size_t i = 0; i < arraysize(root_children); ++i) {
+  for (size_t i = 0; i < root_children.size(); ++i)
     root_.Add(root_children[i], static_cast<int>(i));
-  }
 
   root_.SetMetaInfoMap(details->model_meta_info_map());
   root_.set_sync_transaction_version(details->model_sync_transaction_version());
@@ -883,7 +902,8 @@ BookmarkPermanentNode* BookmarkModel::CreatePermanentNode(
          type == BookmarkNode::MOBILE);
   BookmarkPermanentNode* node =
       new BookmarkPermanentNode(generate_next_node_id());
-  node->set_visible(client_->IsPermanentNodeVisible(type));
+  node->set_type(type);
+  node->set_visible(client_->IsPermanentNodeVisible(node));
 
   int title_id;
   switch (type) {
@@ -902,7 +922,6 @@ BookmarkPermanentNode* BookmarkModel::CreatePermanentNode(
       break;
   }
   node->SetTitle(l10n_util::GetStringUTF16(title_id));
-  node->set_type(type);
   return node;
 }
 
@@ -984,6 +1003,7 @@ scoped_ptr<BookmarkLoadDetails> BookmarkModel::CreateLoadDetails(
       bb_node,
       other_node,
       mobile_node,
+      client_->GetLoadExtraNodesCallback(),
       new BookmarkIndex(client_, index_urls_, accept_languages),
       next_node_id_));
 }