Revert "[CR2023] Position the bubble close button at the end of the title row"

This reverts commit 284f1e3dcf.

Reason for revert: DCEHCK fails in BubbleFrameView::Layout().
https://chromium-swarm.appspot.com/task?id=61dd3dee66996f10
https://ci.chromium.org/ui/p/chromium/builders/ci/Mac12%20Tests%20%28dbg%29/5903/overview

Original change's description:
> [CR2023] Position the bubble close button at the end of the title row
>
> If there is a title row and no header row in a bubble, position the
> close button and the minimize button at the end of the title row.
> Otherwise, keep using the existing positioning, where the buttons are
> positioned closer to the frame edge.
>
> screenshot: https://screenshot.googleplex.com/3Weohy5uASGAUeE
>
> Bug: 1422128
> Change-Id: I6270ce648a94fc0a04cc6e08972d79621ee86d20
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4475931
> Reviewed-by: Allen Bauer <kylixrd@chromium.org>
> Commit-Queue: Keren Zhu <kerenzhu@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1136603}

Bug: 1422128
Change-Id: I36199b957ddf553cfb706248116435b2ac818c9b
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4484680
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Keren Zhu <kerenzhu@chromium.org>
Commit-Queue: Keren Zhu <kerenzhu@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1136837}

ui/views/bubble/bubble_dialog_delegate_view_unittest.cc

@@ -611,7 +611,9 @@ TEST_F(BubbleDialogDelegateViewTest, CustomTitle) {
 
   Button* close_button = bubble_frame->GetCloseButtonForTesting();
   // Title moves over for the close button.
-  EXPECT_GT(close_button->x(), title_container->bounds().right());
+  EXPECT_EQ(close_button->x() - LayoutProvider::Get()->GetDistanceMetric(
+                                    DISTANCE_CLOSE_BUTTON_MARGIN),
+            title_container->bounds().right());
 
   LayoutProvider* provider = LayoutProvider::Get();
   const gfx::Insets content_margins = provider->GetDialogInsetsForContentType(

ui/views/bubble/bubble_frame_view.cc

@@ -15,7 +15,6 @@
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/metadata/metadata_impl_macros.h"
 #include "ui/base/resource/resource_bundle.h"
-#include "ui/base/ui_base_features.h"
 #include "ui/color/color_id.h"
 #include "ui/color/color_provider.h"
 #include "ui/compositor/layer.h"
@@ -458,7 +457,6 @@ void BubbleFrameView::Layout() {
 
   const gfx::Rect contents_bounds = GetContentsBounds();
 
-  // Lay out the progress bar.
   progress_indicator_->SetBounds(contents_bounds.x(), contents_bounds.y(),
                                  contents_bounds.width(),
                                  kProgressIndicatorHeight);
@@ -466,64 +464,56 @@ void BubbleFrameView::Layout() {
   gfx::Rect bounds = contents_bounds;
   bounds.Inset(title_margins_);
 
-  // Lay out the close and the minimize button.
-  gfx::Point buttons_top_right;
-  if (GetButtonsPositioning() == ButtonsPositioning::kOnFrameEdge) {
-    // The buttons are positioned on the upper trailing corner of the bubble.
-    const int buttons_on_frame_edge_margin =
-        LayoutProvider::Get()->GetDistanceMetric(DISTANCE_CLOSE_BUTTON_MARGIN);
-    buttons_top_right =
-        gfx::Point(contents_bounds.right() - buttons_on_frame_edge_margin,
-                   contents_bounds.y() + buttons_on_frame_edge_margin);
-  } else {
-    CHECK_EQ(GetButtonsPositioning(), ButtonsPositioning::kInTitleRow);
-    // The buttons are positioned at the end of the title row.
-    buttons_top_right = gfx::Point(bounds.right(), bounds.y());
+  int header_bottom = 0;
+  int header_height = GetHeaderHeightForFrameWidth(contents_bounds.width());
+  if (header_height > 0) {
+    header_view_->SetBounds(contents_bounds.x(), contents_bounds.y(),
+                            contents_bounds.width(), header_height);
+    bounds.Inset(gfx::Insets::TLBR(header_height, 0, 0, 0));
+    header_bottom = header_view_->bounds().bottom();
   }
-  gfx::Rect buttons_rect(buttons_top_right, gfx::Size());
+
+  // Only account for footnote_container_'s height if it's visible, because
+  // content_margins_ adds extra padding even if all child views are invisible.
+  if (footnote_container_ && footnote_container_->GetVisible()) {
+    const int width = contents_bounds.width();
+    const int height = footnote_container_->GetHeightForWidth(width);
+    footnote_container_->SetBounds(
+        contents_bounds.x(), contents_bounds.bottom() - height, width, height);
+  }
+
+  NonClientFrameView::Layout();
+
+  if (bounds.IsEmpty()) {
+    return;
+  }
+
+  // The buttons are positioned somewhat closer to the edge of the bubble.
+  const int close_margin =
+      LayoutProvider::Get()->GetDistanceMetric(DISTANCE_CLOSE_BUTTON_MARGIN);
+  const int button_y = contents_bounds.y() + close_margin;
+  int button_right = contents_bounds.right() - close_margin;
+  int title_label_right = bounds.right();
   for (Button* button : {close_, minimize_}) {
-    if (!button->GetVisible()) {
+    if (!button->GetVisible())
       continue;
+    button->SetPosition(gfx::Point(button_right - button->width(), button_y));
+    button_right -= button->width();
+    button_right -= LayoutProvider::Get()->GetDistanceMetric(
+        DISTANCE_RELATED_BUTTON_HORIZONTAL);
+    // Only reserve space if the button extends over the header.
+    if (button->bounds().bottom() > header_bottom) {
+      title_label_right =
+          std::min(title_label_right, button->x() - close_margin);
     }
-    // Add spacing between buttons.
-    if (button == minimize_) {
-      buttons_rect.Outset(
-          gfx::Outsets::TLBR(0,
-                             LayoutProvider::Get()->GetDistanceMetric(
-                                 DISTANCE_RELATED_BUTTON_HORIZONTAL),
-                             0, 0));
-    }
-    button->SetPosition(
-        gfx::Point(buttons_rect.right() - button->width(), buttons_rect.y()));
-    buttons_rect.Union(button->bounds());
-  }
-  // Add a left margin that equals the right margin.
-  buttons_rect.Outset(gfx::Outsets::TLBR(
-      0, contents_bounds.right() - buttons_rect.right(), 0, 0));
-
-  // Lay out the header.
-  gfx::Rect header_rect = contents_bounds;
-  header_rect.set_height(GetHeaderHeightForFrameWidth(contents_bounds.width()));
-  if (header_rect.height() > 0) {
-    header_view_->SetBoundsRect(header_rect);
-    bounds.Inset(gfx::Insets::TLBR(header_rect.height(), 0, 0, 0));
   }
 
-  // Lay out the title.
   gfx::Size title_icon_pref_size(title_icon_->GetPreferredSize());
   const int title_icon_padding =
       title_icon_pref_size.width() > 0 ? title_margins_.left() : 0;
   const int title_label_x = bounds.x() + title_icon_pref_size.width() +
                             title_icon_padding + GetMainImageLeftInsets();
-  int title_label_right = bounds.right();
-  if (!buttons_rect.IsEmpty() && buttons_rect.bottom() > header_rect.bottom()) {
-    title_label_right = std::min(title_label_right, buttons_rect.x());
-  }
 
-  // On Fuchsia, width() may be zero due to async bounds setting during bubble
-  // creation, causing checkfail. Non-Fuchsia platforms set bounds synchronously
-  // in Widget::OnNativeWidgetSizeChanged().
-#if !BUILDFLAG(IS_FUCHSIA)
   // TODO(tapted): Layout() should skip more surrounding code when !HasTitle().
   // Currently DCHECKs fail since title_insets is 0 when there is no title.
   if (DCHECK_IS_ON() && HasTitle()) {
@@ -532,7 +522,6 @@ void BubbleFrameView::Layout() {
     DCHECK_EQ(title_insets.left(), title_label_x);
     DCHECK_EQ(title_insets.right(), width() - title_label_right);
   }
-#endif
 
   const int title_available_width =
       std::max(1, title_label_right - title_label_x);
@@ -550,19 +539,6 @@ void BubbleFrameView::Layout() {
 
   main_image_->SetBounds(0, 0, main_image_->GetPreferredSize().width(),
                          main_image_->GetPreferredSize().height());
-
-  // Lay out the footnote.
-  // Only account for footnote_container_'s height if it's visible, because
-  // content_margins_ adds extra padding even if all child views are invisible.
-  if (footnote_container_ && footnote_container_->GetVisible()) {
-    const int width = contents_bounds.width();
-    const int height = footnote_container_->GetHeightForWidth(width);
-    footnote_container_->SetBounds(
-        contents_bounds.x(), contents_bounds.bottom() - height, width, height);
-  }
-
-  // Lay out the client view.
-  NonClientFrameView::Layout();
 }
 
 void BubbleFrameView::OnThemeChanged() {
@@ -987,57 +963,17 @@ bool BubbleFrameView::HasTitle() const {
          title_icon_->GetPreferredSize().height() > 0;
 }
 
-BubbleFrameView::ButtonsPositioning BubbleFrameView::GetButtonsPositioning()
-    const {
-  if (!features::IsChromeRefresh2023()) {
-    return ButtonsPositioning::kOnFrameEdge;
-  }
-
-  // Positions the buttons in the title row when there's no header row.
-  return HasTitle() && !(header_view_ && header_view_->GetVisible())
-             ? ButtonsPositioning::kInTitleRow
-             : ButtonsPositioning::kOnFrameEdge;
-}
-
-bool BubbleFrameView::TitleRowHasButtons() const {
-  return GetButtonsPositioning() == ButtonsPositioning::kInTitleRow &&
-         (GetWidget()->widget_delegate()->ShouldShowCloseButton() ||
-          GetWidget()->widget_delegate()->CanMinimize());
-}
-
 gfx::Insets BubbleFrameView::GetTitleLabelInsetsFromFrame() const {
   int header_height = GetHeaderHeightForFrameWidth(GetContentsBounds().width());
   int insets_right = 0;
-  if (GetButtonsPositioning() == ButtonsPositioning::kOnFrameEdge) {
-    if (GetWidget()->widget_delegate()->ShouldShowCloseButton()) {
-      const int close_margin = LayoutProvider::Get()->GetDistanceMetric(
-          DISTANCE_CLOSE_BUTTON_MARGIN);
-      // Note: |close_margin| is not applied on the bottom of the icon.
-      int close_height = close_margin + close_->height();
-      // Only reserve space if the close button extends over the header.
-      if (close_height > header_height) {
-        insets_right = 2 * close_margin + close_->width();
-      }
-    }
-  } else {
-    CHECK_EQ(GetButtonsPositioning(), ButtonsPositioning::kInTitleRow);
-    // Reserve space for buttons only when they exist in the title row.
-    if (TitleRowHasButtons()) {
-      int buttons_count =
-          GetWidget()->widget_delegate()->ShouldShowCloseButton() +
-          GetWidget()->widget_delegate()->CanMinimize();
-      // Add button width.
-      insets_right = close_->width() * buttons_count;
-      // Add spacing around buttons.
-      if (buttons_count > 0) {
-        insets_right += title_margins_.right() * 2;
-      }
-      // Add spacing between buttons.
-      if (buttons_count == 2) {
-        insets_right += LayoutProvider::Get()->GetDistanceMetric(
-            DISTANCE_RELATED_BUTTON_HORIZONTAL);
-      }
-    }
+  if (GetWidget()->widget_delegate()->ShouldShowCloseButton()) {
+    const int close_margin =
+        LayoutProvider::Get()->GetDistanceMetric(DISTANCE_CLOSE_BUTTON_MARGIN);
+    // Note: |close_margin| is not applied on the bottom of the icon.
+    int close_height = close_margin + close_->height();
+    // Only reserve space if the close button extends over the header.
+    if (close_height > header_height)
+      insets_right = 2 * close_margin + close_->width();
   }
 
   if (!HasTitle())
@@ -1058,7 +994,6 @@ gfx::Insets BubbleFrameView::GetClientInsetsForFrameWidth(
   int header_height = GetHeaderHeightForFrameWidth(frame_width);
   int close_height = 0;
   if (!ExtendClientIntoTitle() &&
-      GetButtonsPositioning() == ButtonsPositioning::kOnFrameEdge &&
       GetWidget()->widget_delegate()->ShouldShowCloseButton()) {
     const int close_margin =
         LayoutProvider::Get()->GetDistanceMetric(DISTANCE_CLOSE_BUTTON_MARGIN);

ui/views/bubble/bubble_frame_view.h

@@ -31,30 +31,6 @@ class FootnoteContainerView;
 class ImageView;
 
 // The non-client frame view of bubble-styled widgets.
-//  +- BubbleFrameView ------------------+
-//  | +- ProgressBar ------------------+ |
-//  | +-----------------------(-)-(x)-+  |
-//  | | HeaderView                    |  |
-//  | +-------------------------------+  |
-//  | +-------------------------------+  |
-//  | | TitleView                     |  |
-//  | +-------------------------------+  |
-//  | +-- DialogClientView------------+  |
-//  | | <<Dialog Contents View>>      |  |
-//  | | <<OK and Cancel Buttons>>     |  |
-//  | | <<...>>                       |  |
-//  | +-------------------------------+  |
-//  | +-------------------------------+  |
-//  | | FootnoteView                  |  |
-//  | +-------------------------------+  |
-//  +------------------------------------+
-// All views are optional except for DialogClientView. An ImageView
-// `main_image` might optionally occupy the top left corner (not
-// illustrated above).
-// If TitleView exists and HeaderView does not exists, the close
-// and the minimize buttons will be positioned at the end of the
-// title row. Otherwise, they will be positioned closer to the frame
-// edge.
 class VIEWS_EXPORT BubbleFrameView : public NonClientFrameView {
  public:
   METADATA_HEADER(BubbleFrameView);
@@ -250,16 +226,6 @@ class VIEWS_EXPORT BubbleFrameView : public NonClientFrameView {
   FRIEND_TEST_ALL_PREFIXES(BubbleDialogDelegateViewTest, CloseMethods);
   FRIEND_TEST_ALL_PREFIXES(BubbleDialogDelegateViewTest, CreateDelegate);
 
-  // The positioning options for the close button and the minimize button.
-  enum class ButtonsPositioning {
-    // The buttons are positioned at the end of the title row.
-    kInTitleRow,
-    // The buttons are positioned on the upper trailing corner of the
-    // bubble. The distance between buttons and the frame edge will be shorter
-    // than `kInTitleRow`.
-    kOnFrameEdge,
-  };
-
   // Mirrors the bubble's arrow location on the |vertical| or horizontal axis,
   // if the generated window bounds don't fit in the given available bounds.
   void MirrorArrowIfOutOfBounds(bool vertical,
@@ -286,12 +252,6 @@ class VIEWS_EXPORT BubbleFrameView : public NonClientFrameView {
   // button.
   bool HasTitle() const;
 
-  // Returns the positioning options for the buttons.
-  ButtonsPositioning GetButtonsPositioning() const;
-
-  // Returns true if there're buttons in the title row.
-  bool TitleRowHasButtons() const;
-
   // The insets of the text portion of the title, based on |title_margins_| and
   // whether there is an icon and/or close button. Note there may be no title,
   // in which case only insets required for the close button are returned.