bookmarks: Simplify visibility of permanent nodes

This removes BookmarkPermanentNode::set_visible(), which was misnamed
in the sense that it didn't directly determine what
BookmarkNode::IsVisible() returns, but instead the returned value for
the case where the node has no children.

As it turns out, none of the callers need to change this attribute
(i.e. the visibility when empty) dynamically. For the managed node, it
needs to always be false, and the remaining permanent nodes take this
value from BookmarkClient upon construction.

BookmarkClient gets a minor refactoring to adopt more accurate names
and to allow evaluating the visibility of permanent nodes prior to their
construction.

Change-Id: I4435140e80216800ce1dcee5604e8370bcc2cedc
Bug: 1060311
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2095125
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#748891}

chrome/browser/bookmarks/chrome_bookmark_client.cc

@@ -89,16 +89,29 @@ void ChromeBookmarkClient::GetTypedCountForUrls(
   }
 }
 
-bool ChromeBookmarkClient::IsPermanentNodeVisible(
-    const bookmarks::BookmarkPermanentNode* node) {
-  DCHECK(bookmarks::IsPermanentNode(node, managed_bookmark_service_));
-  if (bookmarks::IsManagedNode(node, managed_bookmark_service_))
-    return false;
+bool ChromeBookmarkClient::IsPermanentNodeVisibleWhenEmpty(
+    bookmarks::BookmarkNode::Type type) {
 #if defined(OS_ANDROID)
-  return node->type() == bookmarks::BookmarkNode::MOBILE;
+  const bool is_mobile = true;
 #else
-  return node->type() != bookmarks::BookmarkNode::MOBILE;
+  const bool is_mobile = false;
 #endif
+
+  switch (type) {
+    case bookmarks::BookmarkNode::URL:
+      NOTREACHED();
+      return false;
+    case bookmarks::BookmarkNode::FOLDER:
+      // Managed node.
+      return false;
+    case bookmarks::BookmarkNode::BOOKMARK_BAR:
+    case bookmarks::BookmarkNode::OTHER_NODE:
+      return !is_mobile;
+    case bookmarks::BookmarkNode::MOBILE:
+      return is_mobile;
+  }
+
+  return false;
 }
 
 void ChromeBookmarkClient::RecordAction(const base::UserMetricsAction& action) {

chrome/browser/bookmarks/chrome_bookmark_client.h

@@ -19,7 +19,6 @@ class Profile;
 namespace bookmarks {
 class BookmarkModel;
 class BookmarkNode;
-class BookmarkPermanentNode;
 class ManagedBookmarkService;
 }
 
@@ -51,8 +50,8 @@ class ChromeBookmarkClient : public bookmarks::BookmarkClient {
       base::CancelableTaskTracker* tracker) override;
   bool SupportsTypedCountForUrls() override;
   void GetTypedCountForUrls(UrlTypedCountMap* url_typed_count_map) override;
-  bool IsPermanentNodeVisible(
-      const bookmarks::BookmarkPermanentNode* node) override;
+  bool IsPermanentNodeVisibleWhenEmpty(
+      bookmarks::BookmarkNode::Type type) override;
   void RecordAction(const base::UserMetricsAction& action) override;
   bookmarks::LoadManagedNodeCallback GetLoadManagedNodeCallback() override;
   bool CanSetPermanentNodeTitle(

components/bookmarks/browser/bookmark_client.h

@@ -10,6 +10,7 @@
 
 #include "base/callback_forward.h"
 #include "base/task/cancelable_task_tracker.h"
+#include "components/bookmarks/browser/bookmark_node.h"
 #include "components/bookmarks/browser/bookmark_storage.h"
 #include "components/favicon_base/favicon_callback.h"
 #include "components/favicon_base/favicon_types.h"
@@ -24,8 +25,6 @@ struct UserMetricsAction;
 namespace bookmarks {
 
 class BookmarkModel;
-class BookmarkNode;
-class BookmarkPermanentNode;
 
 // This class abstracts operations that depends on the embedder's environment,
 // e.g. Chrome.
@@ -66,9 +65,9 @@ class BookmarkClient {
   // |url_typed_count_map| must not be null.
   virtual void GetTypedCountForUrls(UrlTypedCountMap* url_typed_count_map);
 
-  // Returns whether the embedder wants permanent node |node|
+  // Returns whether the embedder wants permanent node of type |type|
   // to always be visible or to only show them when not empty.
-  virtual bool IsPermanentNodeVisible(const BookmarkPermanentNode* node) = 0;
+  virtual bool IsPermanentNodeVisibleWhenEmpty(BookmarkNode::Type type) = 0;
 
   // Wrapper around RecordAction defined in base/metrics/user_metrics.h
   // that ensure that the action is posted from the correct thread.

components/bookmarks/browser/bookmark_model.cc

@@ -58,10 +58,8 @@ class VisibilityComparator {
                   const std::unique_ptr<BookmarkNode>& n2) {
     DCHECK(n1->is_permanent_node());
     DCHECK(n2->is_permanent_node());
-    bool n1_visible = client_->IsPermanentNodeVisible(
-        static_cast<BookmarkPermanentNode*>(n1.get()));
-    bool n2_visible = client_->IsPermanentNodeVisible(
-        static_cast<BookmarkPermanentNode*>(n2.get()));
+    bool n1_visible = client_->IsPermanentNodeVisibleWhenEmpty(n1->type());
+    bool n2_visible = client_->IsPermanentNodeVisibleWhenEmpty(n2->type());
     return n1_visible != n2_visible && n1_visible;
   }
 

components/bookmarks/browser/bookmark_node.cc

@@ -162,15 +162,22 @@ void BookmarkNode::InvalidateFavicon() {
 
 // BookmarkPermanentNode -------------------------------------------------------
 
-BookmarkPermanentNode::BookmarkPermanentNode(int64_t id, Type type)
-    : BookmarkNode(id, PermanentNodeTypeToGuid(type), GURL(), type, true) {
+BookmarkPermanentNode::BookmarkPermanentNode(int64_t id,
+                                             Type type,
+                                             bool visible_when_empty)
+    : BookmarkNode(id,
+                   PermanentNodeTypeToGuid(type),
+                   GURL(),
+                   type,
+                   /*is_permanent_node=*/true),
+      visible_when_empty_(visible_when_empty) {
   DCHECK(type != URL);
 }
 
 BookmarkPermanentNode::~BookmarkPermanentNode() = default;
 
 bool BookmarkPermanentNode::IsVisible() const {
-  return visible_ || !children().empty();
+  return visible_when_empty_ || !children().empty();
 }
 
 }  // namespace bookmarks

components/bookmarks/browser/bookmark_node.h

@@ -222,17 +222,15 @@ class BookmarkNode : public ui::TreeNode<BookmarkNode>, public TitledUrlNode {
 // Node used for the permanent folders (excluding the root).
 class BookmarkPermanentNode : public BookmarkNode {
  public:
-  BookmarkPermanentNode(int64_t id, Type type);
+  // TODO(mastiz): Remove default value for |visible_when_empty|.
+  BookmarkPermanentNode(int64_t id, Type type, bool visible_when_empty = false);
   ~BookmarkPermanentNode() override;
 
-  // WARNING: this code is used for other projects. Contact noyau@ for details.
-  void set_visible(bool value) { visible_ = value; }
-
   // BookmarkNode overrides:
   bool IsVisible() const override;
 
  private:
-  bool visible_ = false;
+  const bool visible_when_empty_;
 
   DISALLOW_COPY_AND_ASSIGN(BookmarkPermanentNode);
 };

components/bookmarks/browser/bookmark_storage.cc

@@ -241,8 +241,9 @@ BookmarkPermanentNode* BookmarkLoadDetails::CreatePermanentNode(
   DCHECK(type == BookmarkNode::BOOKMARK_BAR ||
          type == BookmarkNode::OTHER_NODE || type == BookmarkNode::MOBILE);
   std::unique_ptr<BookmarkPermanentNode> node =
-      std::make_unique<BookmarkPermanentNode>(max_id_++, type);
-  node->set_visible(client->IsPermanentNodeVisible(node.get()));
+      std::make_unique<BookmarkPermanentNode>(
+          max_id_++, type,
+          /*visible_when_empty=*/client->IsPermanentNodeVisibleWhenEmpty(type));
 
   int title_id;
   switch (type) {

components/bookmarks/managed/managed_bookmark_service.cc

@@ -49,7 +49,6 @@ class BookmarkPermanentNodeLoader {
     node_->set_id(*next_node_id);
     *next_node_id = ManagedBookmarksTracker::LoadInitial(
         node_.get(), initial_bookmarks_.get(), node_->id() + 1);
-    node_->set_visible(!node_->children().empty());
     node_->SetTitle(l10n_util::GetStringUTF16(title_id_));
     return std::move(node_);
   }
@@ -100,8 +99,8 @@ LoadManagedNodeCallback ManagedBookmarkService::GetLoadManagedNodeCallback() {
   // Create a BookmarkPermanentNode with a temporary id of 0. It will be
   // populated and assigned a proper id in the LoadManagedNode callback. Until
   // then, it is owned by the returned closure.
-  std::unique_ptr<BookmarkPermanentNode> managed(
-      new BookmarkPermanentNode(0, BookmarkNode::FOLDER));
+  auto managed = std::make_unique<BookmarkPermanentNode>(
+      0, BookmarkNode::FOLDER, /*visible_when_empty=*/false);
 
   managed_node_ = managed.get();
 

components/bookmarks/managed/managed_bookmarks_tracker.cc

@@ -109,9 +109,6 @@ void ManagedBookmarksTracker::ReloadManagedBookmarks() {
   // Recursively update all the managed bookmarks and folders.
   const base::ListValue* list = prefs_->GetList(prefs::kManagedBookmarks);
   UpdateBookmarks(managed_node_, list);
-
-  // The managed bookmarks folder isn't visible when that pref isn't present.
-  managed_node_->set_visible(!managed_node_->children().empty());
 }
 
 void ManagedBookmarksTracker::UpdateBookmarks(const BookmarkNode* folder,

components/bookmarks/managed/managed_bookmarks_tracker_unittest.cc

@@ -59,7 +59,6 @@ class ManagedBookmarksTrackerTest : public testing::Test {
     BookmarkPermanentNode* managed_node = owned_managed_node.get();
     ManagedBookmarksTracker::LoadInitial(
         managed_node, prefs_.GetList(prefs::kManagedBookmarks), 101);
-    managed_node->set_visible(!managed_node->children().empty());
     managed_node->SetTitle(l10n_util::GetStringUTF16(
         IDS_BOOKMARK_BAR_MANAGED_FOLDER_DEFAULT_NAME));
 

components/bookmarks/test/test_bookmark_client.cc

@@ -58,12 +58,22 @@ bool TestBookmarkClient::IsAManagedNode(const BookmarkNode* node) {
   return node && node->HasAncestor(unowned_managed_node_);
 }
 
-bool TestBookmarkClient::IsPermanentNodeVisible(
-    const BookmarkPermanentNode* node) {
-  DCHECK(node->type() == BookmarkNode::BOOKMARK_BAR ||
-         node->type() == BookmarkNode::OTHER_NODE ||
-         node->type() == BookmarkNode::MOBILE || IsManagedNodeRoot(node));
-  return node->type() != BookmarkNode::MOBILE && !IsManagedNodeRoot(node);
+bool TestBookmarkClient::IsPermanentNodeVisibleWhenEmpty(
+    BookmarkNode::Type type) {
+  switch (type) {
+    case bookmarks::BookmarkNode::URL:
+      NOTREACHED();
+      return false;
+    case bookmarks::BookmarkNode::BOOKMARK_BAR:
+    case bookmarks::BookmarkNode::OTHER_NODE:
+      return true;
+    case bookmarks::BookmarkNode::FOLDER:
+    case bookmarks::BookmarkNode::MOBILE:
+      return false;
+  }
+
+  NOTREACHED();
+  return false;
 }
 
 void TestBookmarkClient::RecordAction(const base::UserMetricsAction& action) {

components/bookmarks/test/test_bookmark_client.h

@@ -44,7 +44,7 @@ class TestBookmarkClient : public BookmarkClient {
 
  private:
   // BookmarkClient:
-  bool IsPermanentNodeVisible(const BookmarkPermanentNode* node) override;
+  bool IsPermanentNodeVisibleWhenEmpty(BookmarkNode::Type type) override;
   void RecordAction(const base::UserMetricsAction& action) override;
   LoadManagedNodeCallback GetLoadManagedNodeCallback() override;
   bool CanSetPermanentNodeTitle(const BookmarkNode* permanent_node) override;

ios/chrome/browser/bookmarks/bookmark_client_impl.cc

@@ -71,9 +71,9 @@ void BookmarkClientImpl::GetTypedCountForUrls(
   }
 }
 
-bool BookmarkClientImpl::IsPermanentNodeVisible(
-    const bookmarks::BookmarkPermanentNode* node) {
-  return node->type() == bookmarks::BookmarkNode::MOBILE;
+bool BookmarkClientImpl::IsPermanentNodeVisibleWhenEmpty(
+    bookmarks::BookmarkNode::Type type) {
+  return type == bookmarks::BookmarkNode::MOBILE;
 }
 
 void BookmarkClientImpl::RecordAction(const base::UserMetricsAction& action) {

ios/chrome/browser/bookmarks/bookmark_client_impl.h

@@ -17,8 +17,6 @@ class GURL;
 
 namespace bookmarks {
 class BookmarkModel;
-class BookmarkNode;
-class BookmarkPermanentNode;
 }
 
 namespace sync_bookmarks {
@@ -42,8 +40,8 @@ class BookmarkClientImpl : public bookmarks::BookmarkClient {
       base::CancelableTaskTracker* tracker) override;
   bool SupportsTypedCountForUrls() override;
   void GetTypedCountForUrls(UrlTypedCountMap* url_typed_count_map) override;
-  bool IsPermanentNodeVisible(
-      const bookmarks::BookmarkPermanentNode* node) override;
+  bool IsPermanentNodeVisibleWhenEmpty(
+      bookmarks::BookmarkNode::Type type) override;
   void RecordAction(const base::UserMetricsAction& action) override;
   bookmarks::LoadManagedNodeCallback GetLoadManagedNodeCallback() override;
   bool CanSetPermanentNodeTitle(