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

chrome/browser/bookmarks/bookmark_model.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/bookmarks/bookmark_model.h"
 
 #include <algorithm>
 #include <functional>
 
 #include "base/bind.h"
@@ skipping 81 matching lines @@
           collator_, n1->GetTitle(), n2->GetTitle()) == UCOL_LESS;
     }
     // Types differ, sort such that folders come first.
     return n1->is_folder();
   }
 
  private:
   icu::Collator* collator_;
 };
 
-// MobileNode ------------------------------------------------------------------
+}  // namespace
 
-// 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();
+// BookmarkPermanentNode -------------------------------------------------------
 
-  void set_visible(bool value) { visible_ = value; }
-
-  // BookmarkNode overrides:
-  virtual bool IsVisible() const OVERRIDE;
-
- private:
-  bool visible_;
-
-  DISALLOW_COPY_AND_ASSIGN(MobileNode);
-};
-
-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)
     : profile_(profile),
       loaded_(false),
       file_changed_(false),
       root_(GURL()),
       bookmark_bar_node_(NULL),
       other_node_(NULL),
       mobile_node_(NULL),
@@ skipping 395 matching lines @@
     return;
 
   index_->GetBookmarksWithTitlesMatching(text, max_count, matches);
 }
 
 void BookmarkModel::ClearStore() {
   registrar_.RemoveAll();
   store_ = NULL;
 }
 
 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) {
   BookmarkNode tmp_node(url);
   return (nodes_ordered_by_url_set_.find(&tmp_node) !=
           nodes_ordered_by_url_set_.end());
 }
 
 void BookmarkModel::RemoveNode(BookmarkNode* node,
                                std::set<GURL>* removed_urls) {
@@ skipping 169 matching lines @@
 }
 
 bool BookmarkModel::IsValidIndex(const BookmarkNode* parent,
                                  int index,
                                  bool allow_end) {
   return (parent && parent->is_folder() &&
           (index >= 0 && (index < parent->child_count() ||
                           (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;
 }
 
 void BookmarkModel::OnFaviconDataAvailable(
     FaviconService::Handle handle,
     history::FaviconData favicon) {
   SkBitmap favicon_bitmap;
   BookmarkNode* node =
       load_consumer_.GetClientData(
@@ skipping 76 matching lines @@
     nodes_ordered_by_url_set_.insert(node);
   for (int i = 0; i < node->child_count(); ++i)
     PopulateNodesByURL(node->GetChild(i));
 }
 
 int64 BookmarkModel::generate_next_node_id() {
   return 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_);
 }