Remove redundant condition in ShouldHaveBookmarksTitle
The "Bookmarks" title is only built under certain conditions. One of
these codified conditions was that the parent menu is not empty, which
in practice is only ever true. Even if this assumption is wrong, then
including a condition for it here alone is not enough because we would
need to observe for changes to the parent menu.
Therefore, this CL removes the condition altogether, replacing it with a
DCHECK and "DumpWithoutCrashing", since this is a non-critical
assumption.
Bug: 40868588
Change-Id: I6e64bb82b6a9d1f6f366adc3897c4ac529af9456
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6226901
Reviewed-by: Darryl James <dljames@chromium.org>
Commit-Queue: Kaan Alsan <alsan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1416750}
diff --git a/chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc b/chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc
index 4fc9d6f..7d5ac80 100644
--- a/chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc
+++ b/chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc
@@ -8,6 +8,7 @@
#include <optional>
#include "base/containers/to_vector.h"
+#include "base/debug/dump_without_crashing.h"
#include "base/feature_list.h"
#include "base/functional/bind.h"
#include "base/memory/raw_ptr.h"
@@ -1107,6 +1108,7 @@
MenuItemView* BookmarkMenuDelegate::UpdateBookmarksTitle() {
CHECK(parent_menu_item_);
+ CHECK(parent_menu_item_->HasSubmenu());
// Check if we need to add/remove the bookmarks title. If not, then return
// null since the parent menu doesn't need to be updated.
const bool should_have_title = ShouldHaveBookmarksTitle();
@@ -1137,14 +1139,24 @@
bool BookmarkMenuDelegate::ShouldHaveBookmarksTitle() {
CHECK(parent_menu_item_);
+ // In practice, the parent menu item is never empty.
+ // If this assumption is wrong, there may be a redundant "separator" visual
+ // artifact, but the code will continue to function correctly (hence why we
+ // don't crash here).
+ // If this ever changes, then the delegate will need to observe and
+ // react to non-bookmark changes in its parent menu, which is currently not
+ // supported.
+ if (parent_menu_item_->GetSubmenu()->children().empty()) {
+ DCHECK(false) << "Expected parent menu item to be empty";
+ base::debug::DumpWithoutCrashing();
+ }
const BookmarkMergedSurfaceService* service =
GetBookmarkMergedSurfaceService();
const bool bookmark_bar_has_children =
service->GetChildrenCount(BookmarkParentFolder::BookmarkBarFolder());
- return (bookmark_bar_has_children ||
- ShouldBuildPermanentNode(service,
- BookmarkParentFolder::ManagedFolder())) &&
- !parent_menu_item_->GetSubmenu()->children().empty();
+ return (
+ bookmark_bar_has_children ||
+ ShouldBuildPermanentNode(service, BookmarkParentFolder::ManagedFolder()));
}
void BookmarkMenuDelegate::BuildBookmarksTitle(size_t index) {
diff --git a/chrome/browser/ui/views/bookmarks/bookmark_menu_delegate_unittest.cc b/chrome/browser/ui/views/bookmarks/bookmark_menu_delegate_unittest.cc
index 7a7c483..63d3c774 100644
--- a/chrome/browser/ui/views/bookmarks/bookmark_menu_delegate_unittest.cc
+++ b/chrome/browser/ui/views/bookmarks/bookmark_menu_delegate_unittest.cc
@@ -129,6 +129,9 @@
void NewAndBuildFullMenu() {
root_menu_ = std::make_unique<views::MenuItemView>();
+ // Add a placeholder here because in practice the full menu is never
+ // empty.
+ root_menu_->AppendTitle(std::u16string());
root_menu_->CreateSubmenu();
NewDelegate();
bookmark_menu_delegate_->BuildFullMenu(root_menu_.get());
@@ -243,9 +246,9 @@
NewAndBuildFullMenu();
views::MenuItemView* root_item = menu();
ASSERT_TRUE(root_item->HasSubmenu());
- EXPECT_EQ(6u, root_item->GetSubmenu()->GetMenuItems().size());
- EXPECT_EQ(7u, root_item->GetSubmenu()->children().size()); // + separator
- views::MenuItemView* f1_item = root_item->GetSubmenu()->GetMenuItemAt(2);
+ EXPECT_EQ(8u, root_item->GetSubmenu()->GetMenuItems().size());
+ EXPECT_EQ(10u, root_item->GetSubmenu()->children().size()); // + separators
+ views::MenuItemView* f1_item = root_item->GetSubmenu()->GetMenuItemAt(4);
ASSERT_TRUE(f1_item->HasSubmenu());
// f1 hasn't been loaded yet.
EXPECT_EQ(0u, f1_item->GetSubmenu()->GetMenuItems().size());
@@ -550,7 +553,7 @@
// Drop before managed node.
- auto* managed_folder_menu = root_item->GetSubmenu()->GetMenuItemAt(0);
+ auto* managed_folder_menu = root_item->GetSubmenu()->GetMenuItemAt(2);
ASSERT_EQ(managed_folder_menu->title(), managed_node()->GetTitle());
// Calling `CanDrop()` is required as it sets `drop_data_`.
ASSERT_TRUE(bookmark_menu_delegate_->CanDrop(managed_folder_menu, drop_data));
@@ -564,7 +567,7 @@
// Drop before mobile node.
size_t mobile_folder_menu_index =
- 2u + // managed + other node.
+ 4u + // placeholder + bookmarks title + managed + other node.
model()->bookmark_bar_node()->children().size();
auto* mobile_folder_menu =
root_item->GetSubmenu()->GetMenuItemAt(mobile_folder_menu_index);
@@ -599,7 +602,7 @@
// Drop on url.
- auto* url_item = root_item->GetSubmenu()->GetMenuItemAt(1);
+ auto* url_item = root_item->GetSubmenu()->GetMenuItemAt(3);
ASSERT_EQ(url_item->title(),
model()->bookmark_bar_node()->children()[0]->GetTitle());
ASSERT_TRUE(bookmark_menu_delegate_->CanDrop(url_item, drop_data));
@@ -615,7 +618,7 @@
views::MenuItemView* root_item = menu();
LoadAllMenus(root_item);
- auto* managed_folder_menu = root_item->GetSubmenu()->GetMenuItemAt(0);
+ auto* managed_folder_menu = root_item->GetSubmenu()->GetMenuItemAt(2);
ASSERT_EQ(managed_folder_menu->title(), managed_node()->GetTitle());
ui::OSExchangeData drop_data;
@@ -656,7 +659,7 @@
size_t bookmark_bar_nodes_size =
model()->bookmark_bar_node()->children().size();
auto* other_folder_menu = root_item->GetSubmenu()->GetMenuItemAt(
- bookmark_bar_nodes_size + 1u); // add managed folder.
+ bookmark_bar_nodes_size + 3u); // add managed folder + bookmarks title.
ASSERT_EQ(other_folder_menu->title(), model()->other_node()->GetTitle());
ui::OSExchangeData drop_data;
