Makes all permanent nodes share the same node class so that visibility

chrome/browser/bookmarks/bookmark_model.cc

Index: chrome/browser/bookmarks/bookmark_model.cc
diff --git a/chrome/browser/bookmarks/bookmark_model.cc b/chrome/browser/bookmarks/bookmark_model.cc
index 2d735bc88cc3e03cb2951e7f5226934c9a9315e8..6cfa173648d57f09a713307a339ca4b886f8e546 100644
--- a/chrome/browser/bookmarks/bookmark_model.cc
+++ b/chrome/browser/bookmarks/bookmark_model.cc
@@ -99,40 +99,22 @@ class SortComparator : public std::binary_function<const BookmarkNode*,
   icu::Collator* collator_;
 };
 
-// MobileNode ------------------------------------------------------------------
-
-// The visibility of the mobile node changes based on sync state, requiring a
-// special subclass.
-class MobileNode : public BookmarkNode {
- public:
-  explicit MobileNode(int64 id);
-  virtual ~MobileNode();
-
-  void set_visible(bool value) { visible_ = value; }
-
-  // BookmarkNode overrides:
-  virtual bool IsVisible() const OVERRIDE;
-
- private:
-  bool visible_;
+}  // namespace
 
-  DISALLOW_COPY_AND_ASSIGN(MobileNode);
-};
+// BookmarkPermanentNode -------------------------------------------------------
 
-MobileNode::MobileNode(int64 id)
+BookmarkPermanentNode::BookmarkPermanentNode(int64 id)
     : BookmarkNode(id, GURL()),
-      visible_(false) {
+      visible_(true) {

[noyau (Ping after 24h), 2011/12/07 10:27:56]

Maybe I do not understand the code enough, but it seems to me that the Mobile
folder now defaults to visible if sync is not enabled. Your observer only
changes the state when a synchronized node is associated to the mobile folder,
and this code doesn't run when synchronization is not turned on.

In other word, I think there is a need to specify the default visibility of
permanent nodes in the constructor.

[sky, 2011/12/07 16:36:53]

See line 733. The mobile node is special cased.

[noyau (Ping after 24h), 2011/12/07 16:49:33]

Ah, I missed that line!

 }
 
-MobileNode::~MobileNode() {
+BookmarkPermanentNode::~BookmarkPermanentNode() {
 }
 
-bool MobileNode::IsVisible() const {
+bool BookmarkPermanentNode::IsVisible() const {
   return visible_ || !empty();
 }
 
-}  // namespace
-
 // BookmarkModel --------------------------------------------------------------
 
 BookmarkModel::BookmarkModel(Profile* profile)
@@ -550,7 +532,7 @@ void BookmarkModel::ClearStore() {
 
 void BookmarkModel::SetMobileFolderVisible(bool value) {

[noyau (Ping after 24h), 2011/12/07 09:45:22]

Maybe revisiting this method to be SetFolderVisible(BookmarkNode::Type type,
bool value)? Otherwise I'd have to create one method per type.

[sky, 2011/12/07 16:36:53]

Will do.

   DCHECK(loaded_);
-  static_cast<MobileNode*>(mobile_node_)->set_visible(value);
+  mobile_node_->set_visible(value);
 }
 
 bool BookmarkModel::IsBookmarkedNoLock(const GURL& url) {
@@ -740,26 +722,33 @@ bool BookmarkModel::IsValidIndex(const BookmarkNode* parent,
                           (allow_end && index == parent->child_count()))));
 }
 
-BookmarkNode* BookmarkModel::CreatePermanentNode(BookmarkNode::Type type) {
+BookmarkPermanentNode* BookmarkModel::CreatePermanentNode(
+    BookmarkNode::Type type) {
   DCHECK(type == BookmarkNode::BOOKMARK_BAR ||
          type == BookmarkNode::OTHER_NODE ||
          type == BookmarkNode::MOBILE);
-  BookmarkNode* node;
-  if (type == BookmarkNode::MOBILE) {
-    node = new MobileNode(generate_next_node_id());
-    node->set_title(
-        l10n_util::GetStringUTF16(IDS_BOOKMARK_BAR_MOBILE_FOLDER_NAME));
-  } else {
-    node = new BookmarkNode(generate_next_node_id(), GURL());
-    if (type == BookmarkNode::BOOKMARK_BAR) {
-      node->set_title(l10n_util::GetStringUTF16(IDS_BOOKMARK_BAR_FOLDER_NAME));
-    } else if (type == BookmarkNode::OTHER_NODE) {
-      node->set_title(
-          l10n_util::GetStringUTF16(IDS_BOOKMARK_BAR_OTHER_FOLDER_NAME));
-    } else {
+  BookmarkPermanentNode* node =
+      new BookmarkPermanentNode(generate_next_node_id());
+  if (type == BookmarkNode::MOBILE)
+    node->set_visible(false);  // Mobile node is initially hidden.
+
+  int title_id;
+  switch (type) {
+    case BookmarkNode::BOOKMARK_BAR:
+      title_id = IDS_BOOKMARK_BAR_FOLDER_NAME;
+      break;
+    case BookmarkNode::OTHER_NODE:
+      title_id = IDS_BOOKMARK_BAR_OTHER_FOLDER_NAME;
+      break;
+    case BookmarkNode::MOBILE:
+      title_id = IDS_BOOKMARK_BAR_MOBILE_FOLDER_NAME;
+      break;
+    default:
       NOTREACHED();
-    }
+      title_id = IDS_BOOKMARK_BAR_FOLDER_NAME;
+      break;
   }
+  node->set_title(l10n_util::GetStringUTF16(title_id));
   node->set_type(type);
   return node;
 }
@@ -856,9 +845,12 @@ int64 BookmarkModel::generate_next_node_id() {
 }
 
 BookmarkLoadDetails* BookmarkModel::CreateLoadDetails() {
-  BookmarkNode* bb_node = CreatePermanentNode(BookmarkNode::BOOKMARK_BAR);
-  BookmarkNode* other_node = CreatePermanentNode(BookmarkNode::OTHER_NODE);
-  BookmarkNode* mobile_node = CreatePermanentNode(BookmarkNode::MOBILE);
+  BookmarkPermanentNode* bb_node =
+      CreatePermanentNode(BookmarkNode::BOOKMARK_BAR);
+  BookmarkPermanentNode* other_node =
+      CreatePermanentNode(BookmarkNode::OTHER_NODE);
+  BookmarkPermanentNode* mobile_node =
+      CreatePermanentNode(BookmarkNode::MOBILE);
   return new BookmarkLoadDetails(bb_node, other_node, mobile_node,
                                  new BookmarkIndex(profile_), next_node_id_);
 }