1. bottom_view_ is removed.

2. dialog content view height calculation is fixed.

The button height calculation in GetPreferredSize(), LayoutContentsView(), and LayoutDialogButtons() do not match.

kButtonVEdgeMargin is missed out in LayoutContentsView() which results in content_view's height is greater than that computed in GetPreferredSize() by kButtonVEdgeMargin. 

Subsequently, if there are multiple children Views in the dialog content view (such as JS message box), the spacing between children views (in rows) are  not evenly distributed in views::GridLayout::SizeRowsAndColumns().

BUG=127698
TEST=manual test a JS alert with multiple lines, the lines should be evenly spaced. Tested other UI that uses DialogClientView, such as about chromium and bookmark edit, no regression.

Review URL: https://chromiumcodereview.appspot.com/10384064

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@137011 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 24 insertions, 53 deletions

diff --git a/ui/views/window/dialog_client_view.cc b/ui/views/window/dialog_client_view.cc
index 8c9d9f8..5a43622 100644
--- a/ui/views/window/dialog_client_view.cc
+++ b/ui/views/window/dialog_client_view.cc
@@ -108,8 +108,7 @@ DialogClientView::DialogClientView(Widget* owner, View* contents_view)
       size_extra_view_height_to_buttons_(false),
       notified_delegate_(false),
       listening_to_focus_(false),
-      saved_focus_manager_(NULL),
-      bottom_view_(NULL) {
+      saved_focus_manager_(NULL) {
 }
 
 DialogClientView::~DialogClientView() {
@@ -239,16 +238,6 @@ void DialogClientView::CancelWindow() {
   Close();
 }
 
-void DialogClientView::SetBottomView(View* bottom_view) {
-  if (bottom_view_) {
-    RemoveChildView(bottom_view_);
-    delete bottom_view_;
-  }
-  bottom_view_ = bottom_view;
-  if (bottom_view_)
-    AddChildView(bottom_view_);
-}
-
 ///////////////////////////////////////////////////////////////////////////////
 // DialogClientView, View overrides:
 
@@ -319,13 +308,6 @@ void DialogClientView::PaintChildren(gfx::Canvas* canvas) {
 void DialogClientView::Layout() {
   if (has_dialog_buttons())
     LayoutDialogButtons();
-  if (bottom_view_) {
-    gfx::Rect bounds = GetContentsBounds();
-    gfx::Size pref = bottom_view_->GetPreferredSize();
-    bottom_view_->SetBounds(bounds.x(),
-        bounds.bottom() - pref.height() - kButtonVEdgeMargin,
-        bounds.width(), pref.height());
-  }
   LayoutContentsView();
 }
 
@@ -350,15 +332,8 @@ void DialogClientView::ViewHierarchyChanged(bool is_add, View* parent,
 
 gfx::Size DialogClientView::GetPreferredSize() {
   gfx::Size prefsize = contents_view()->GetPreferredSize();
-  int button_height = 0;
+  int button_height = GetDialogButtonsAreaHeight();
   if (has_dialog_buttons()) {
-    if (cancel_button_)
-      button_height = cancel_button_->height();
-    else
-      button_height = ok_button_->height();
-    // Account for padding above and below the button.
-    button_height += kDialogButtonContentSpacing + kButtonVEdgeMargin;
-
     // Make sure the view is sized to the buttons's width if they are wider than
     // the contents.
     int width = 0;
@@ -379,10 +354,6 @@ gfx::Size DialogClientView::GetPreferredSize() {
       prefsize.set_width(std::max(prefsize.width(), width));
     }
   }
-  if (bottom_view_) {
-    gfx::Size bottom_pref = bottom_view_->GetPreferredSize();
-    prefsize.Enlarge(0, bottom_pref.height() + kButtonVEdgeMargin);
-  }
   prefsize.Enlarge(0, button_height);
   return prefsize;
 }
@@ -455,23 +426,26 @@ int DialogClientView::GetButtonWidth(int button) const {
 }
 
 int DialogClientView::GetButtonsHeight() const {
-  if (has_dialog_buttons()) {
-    if (cancel_button_)
-      return cancel_button_->height() + kDialogButtonContentSpacing;
-    return ok_button_->height() + kDialogButtonContentSpacing;
-  }
-  return 0;
+  int button_height = 0;
+  if (cancel_button_)
+    button_height = std::max(button_height,
+                             cancel_button_->GetPreferredSize().height());
+  if (ok_button_)
+    button_height = std::max(button_height,
+                             ok_button_->GetPreferredSize().height());
+  return button_height;
+}
+
+int DialogClientView::GetDialogButtonsAreaHeight() const {
+  return !has_dialog_buttons() ? 0 :
+      GetButtonsHeight() + kDialogButtonContentSpacing + kButtonVEdgeMargin;
 }
 
 void DialogClientView::LayoutDialogButtons() {
   gfx::Rect lb = GetContentsBounds();
   gfx::Rect extra_bounds;
   int bottom_y = lb.bottom() - kButtonVEdgeMargin;
-  int button_height = 0;
-  if (bottom_view_) {
-    gfx::Size bottom_pref = bottom_view_->GetPreferredSize();
-    bottom_y -= bottom_pref.height() + kButtonVEdgeMargin + kButtonVEdgeMargin;
-  }
+  int button_height = GetButtonsHeight();
   if (cancel_button_) {
     gfx::Size ps = cancel_button_->GetPreferredSize();
     int button_width = std::max(
@@ -482,7 +456,6 @@ void DialogClientView::LayoutDialogButtons() {
     // The extra view bounds are dependent on this button.
     extra_bounds.set_width(std::max(0, cancel_button_->x()));
     extra_bounds.set_y(cancel_button_->y());
-    button_height = std::max(button_height, ps.height());
   }
   if (ok_button_) {
     gfx::Size ps = ok_button_->GetPreferredSize();
@@ -498,7 +471,6 @@ void DialogClientView::LayoutDialogButtons() {
     // The extra view bounds are dependent on this button.
     extra_bounds.set_width(std::max(0, ok_button_->x()));
     extra_bounds.set_y(ok_button_->y());
-    button_height = std::max(button_height, ps.height());
   }
   if (extra_view_) {
     gfx::Size ps = extra_view_->GetPreferredSize();
@@ -512,7 +484,7 @@ void DialogClientView::LayoutDialogButtons() {
 
 void DialogClientView::LayoutContentsView() {
   gfx::Rect lb = GetContentsBounds();
-  lb.set_height(std::max(0, lb.height() - GetButtonsHeight()));
+  lb.set_height(std::max(0, lb.height() - GetDialogButtonsAreaHeight()));
   contents_view()->SetBoundsRect(lb);
   contents_view()->Layout();
 }
diff --git a/ui/views/window/dialog_client_view.h b/ui/views/window/dialog_client_view.h
index bb3dfc3..c32522e 100644
--- a/ui/views/window/dialog_client_view.h
+++ b/ui/views/window/dialog_client_view.h
@@ -57,11 +57,6 @@ class VIEWS_EXPORT DialogClientView : public ClientView,
   NativeTextButton* ok_button() const { return ok_button_; }
   NativeTextButton* cancel_button() const { return cancel_button_; }
 
-  // Sets the view that is positioned along the bottom of the buttons. The
-  // bottom view is positioned beneath the buttons at the full width of the
-  // dialog. If there is an existing bottom view it is removed and deleted.
-  void SetBottomView(View* bottom_view);
-
   // Overridden from View:
   virtual void NativeViewHierarchyChanged(
       bool attached,
@@ -102,8 +97,15 @@ class VIEWS_EXPORT DialogClientView : public ClientView,
 
   // Returns the width of the specified dialog button using the correct font.
   int GetButtonWidth(int button) const;
+
+  // Returns the greater of ok and cancel button's preferred height.
   int GetButtonsHeight() const;
 
+  // Returns the height of the dialog buttons area, including the spacing
+  // between bottom of contents view and top of buttons, the buttons height,
+  // and the spacing between bottom of buttons to end of the dialog.
+  int GetDialogButtonsAreaHeight() const;
+
   // Position and size various sub-views.
   void LayoutDialogButtons();
   void LayoutContentsView();
@@ -154,9 +156,6 @@ class VIEWS_EXPORT DialogClientView : public ClientView,
   // When ancestor gets changed focus manager gets changed as well.
   FocusManager* saved_focus_manager_;
 
-  // View positioned along the bottom, beneath the buttons.
-  View* bottom_view_;
-
   DISALLOW_COPY_AND_ASSIGN(DialogClientView);
 };