Correct padding and focus ring for frameless constrained window dialog

chrome/browser/ui/views/constrained_window_frame_simple.cc

Index: chrome/browser/ui/views/constrained_window_frame_simple.cc
diff --git a/chrome/browser/ui/views/constrained_window_frame_simple.cc b/chrome/browser/ui/views/constrained_window_frame_simple.cc
index 3292b063b0b6272fbd35844ffffe0ff647aa05d9..38a9f8bf47f59eab0d5bc21d3a40e2a2697c6111 100644
--- a/chrome/browser/ui/views/constrained_window_frame_simple.cc
+++ b/chrome/browser/ui/views/constrained_window_frame_simple.cc
@@ -34,7 +34,10 @@ typedef ConstrainedWindowFrameSimple::HeaderViews HeaderViews;
 // and sized to the widget's client view.
 class HeaderLayout : public views::LayoutManager {
  public:
-  explicit HeaderLayout(views::Widget* container) : container_(container) {}
+  explicit HeaderLayout(
+      views::Widget* container,
+      int client_left_inset,
+      int client_right_inset);
   virtual ~HeaderLayout() {}
 
   // Overridden from LayoutManager
@@ -43,30 +46,40 @@ class HeaderLayout : public views::LayoutManager {
 
  private:
   views::Widget* container_;
+  int client_left_inset_;
+  int client_right_inset_;
 
   DISALLOW_COPY_AND_ASSIGN(HeaderLayout);
 };
 
+HeaderLayout::HeaderLayout(
+    views::Widget* container,
+    int client_left_inset,
+    int client_right_inset)
+    : container_(container),
+      client_left_inset_(client_left_inset),
+      client_right_inset_(client_right_inset) {}
+
 void HeaderLayout::Layout(views::View* host) {
   if (!host->has_children())
     return;
 
-  int horizontal_padding = ConstrainedWindow::kHorizontalPadding;
-  int vertical_padding = ConstrainedWindow::kVerticalPadding;
-  int row_padding = ConstrainedWindow::kRowPadding;
+  int top_padding = ConstrainedWindow::kCloseButtonPadding;
+  int left_padding = ConstrainedWindow::kHorizontalPadding;
+  int right_padding = ConstrainedWindow::kCloseButtonPadding;
 
   views::View* header = host->child_at(0);
   gfx::Size preferred_size = GetPreferredSize(host);
-  int width = preferred_size.width() - 2 * horizontal_padding;
-  int height = preferred_size.height() - vertical_padding - row_padding;
+  int width = preferred_size.width() - left_padding - right_padding;
+  int height = preferred_size.height() - top_padding;
 
-  header->SetBounds(horizontal_padding, vertical_padding, width, height);
+  header->SetBounds(left_padding, top_padding, width, height);
 }
 
 gfx::Size HeaderLayout::GetPreferredSize(views::View* host) {
-  int horizontal_padding = ConstrainedWindow::kHorizontalPadding;
-  int vertical_padding = ConstrainedWindow::kVerticalPadding;
-  int row_padding = ConstrainedWindow::kRowPadding;
+  int left_padding = ConstrainedWindow::kHorizontalPadding;
+  int top_padding = ConstrainedWindow::kCloseButtonPadding;
+  int right_padding = ConstrainedWindow::kCloseButtonPadding;
 
   views::View* header = host->child_at(0);
   views::View* client_view = container_->client_view();
@@ -74,9 +87,10 @@ gfx::Size HeaderLayout::GetPreferredSize(views::View* host) {
   gfx::Size header_size = header ? header->GetPreferredSize() : gfx::Size();
   gfx::Size client_size =
       client_view ? client_view->GetPreferredSize() : gfx::Size();
-  int width = std::max(client_size.width(), header_size.width()) +
-      2 * horizontal_padding;
-  int height = vertical_padding + header_size.height() + row_padding;
+  int width = std::max(
+      client_left_inset_ + client_size.width() + client_right_inset_,
+      left_padding + header_size.width() + right_padding);
+  int height = header_size.height() + top_padding;
   return gfx::Size(width, height);
 }
 
@@ -94,16 +108,26 @@ ConstrainedWindowFrameSimple::HeaderViews::HeaderViews(
 
 ConstrainedWindowFrameSimple::ConstrainedWindowFrameSimple(
     ConstrainedWindowViews* container)
-    : container_(container) {
+    : container_(container),
+      client_insets_(ConstrainedWindow::kClientTopPadding,

[Peter Kasting, 2012/10/10 20:22:26]

I didn't totally grasp why you wanted this member.  You said "ConstrainedWindow
will need to override the insets".  How is that going to happen, since there
doesn't currently seem to be any access to this member?  It seems like the right
way would be for the ConstrainedWindow to delete the border, or specify that it
shouldn't be added in the first place, maybe?

[please use gerrit instead, 2012/10/11 00:34:57]

I have not decided how this is going to be done. For now, let's just get rid of
the member.

+                     ConstrainedWindow::kHorizontalPadding,
+                     ConstrainedWindow::kClientBottomPadding,
+                     ConstrainedWindow::kHorizontalPadding) {
   container_->set_frame_type(views::Widget::FRAME_TYPE_FORCE_CUSTOM);
 
-  layout_ = new HeaderLayout(container_);
+  layout_ = new HeaderLayout(container_, client_insets_.left(),
+                                         client_insets_.right());

[Peter Kasting, 2012/10/10 20:22:26]

It seems like the header shouldn't have to worry about these insets since the
border should already be taking care of them.

[please use gerrit instead, 2012/10/11 00:34:57]

Done.

   SetLayoutManager(layout_);
 
   SetHeaderView(CreateDefaultHeaderView());
 
   set_background(views::Background::CreateSolidBackground(
       ConstrainedWindow::GetBackgroundColor()));
+
+  set_border(views::Border::CreateEmptyBorder(client_insets_.top(),
+                                              client_insets_.left(),
+                                              client_insets_.bottom(),
+                                              client_insets_.right()));
 }
 
 ConstrainedWindowFrameSimple::~ConstrainedWindowFrameSimple() {
@@ -119,6 +143,12 @@ void ConstrainedWindowFrameSimple::SetHeaderView(HeaderViews* header_views)
 }
 
 HeaderViews* ConstrainedWindowFrameSimple::CreateDefaultHeaderView() {
+  const int kTitleTopPadding = ConstrainedWindow::kTitleTopPadding -
+      ConstrainedWindow::kCloseButtonPadding;
+  const int kTitleLeftPadding = 0;
+  const int kTitleBottomPadding = 0;
+  const int kTitleRightPadding = 0;
+
   views::View* header_view = new views::View;
 
   views::GridLayout* grid_layout = new views::GridLayout(header_view);
@@ -128,17 +158,20 @@ HeaderViews* ConstrainedWindowFrameSimple::CreateDefaultHeaderView() {
   header_cs->AddColumn(views::GridLayout::CENTER, views::GridLayout::CENTER, 0,
                        views::GridLayout::USE_PREF, 0, 0);  // Title.
   header_cs->AddPaddingColumn(1, views::kUnrelatedControlHorizontalSpacing);
-  header_cs->AddColumn(views::GridLayout::CENTER, views::GridLayout::CENTER, 0,
-                       views::GridLayout::USE_PREF, 0, 0);  // Close Button.
+  header_cs->AddColumn(views::GridLayout::TRAILING, views::GridLayout::LEADING,
+                       0, views::GridLayout::USE_PREF, 0, 0);  // Close Button.
 
   // Header row.
   ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
   grid_layout->StartRow(0, 0);
+
   views::Label* title_label = new views::Label();
   title_label->SetHorizontalAlignment(views::Label::ALIGN_LEFT);
   title_label->SetFont(rb.GetFont(ConstrainedWindow::kTitleFontStyle));
   title_label->SetEnabledColor(ConstrainedWindow::GetTextColor());
   title_label->SetText(container_->widget_delegate()->GetWindowTitle());
+  title_label->set_border(views::Border::CreateEmptyBorder(kTitleTopPadding,
+      kTitleLeftPadding, kTitleBottomPadding, kTitleRightPadding));
   grid_layout->AddView(title_label);
 
   views::Button* close_button = CreateCloseButton();
@@ -148,37 +181,19 @@ HeaderViews* ConstrainedWindowFrameSimple::CreateDefaultHeaderView() {
 }
 
 gfx::Rect ConstrainedWindowFrameSimple::GetBoundsForClientView() const {
-  int horizontal_padding = ConstrainedWindow::kHorizontalPadding;
-  int vertical_padding = ConstrainedWindow::kVerticalPadding;
-  int row_padding = ConstrainedWindow::kRowPadding;
-  gfx::Size header_size =
-      header_views_->header ?
-      header_views_->header->GetPreferredSize() : gfx::Size();
-
-  return gfx::Rect(horizontal_padding,
-                   vertical_padding + header_size.height() + row_padding,
-                   std::max(0, width() - 2 * horizontal_padding),
-                   std::max(0, (height() - 2 * vertical_padding -
-                                header_size.height() - row_padding)));
+  gfx::Rect bounds(GetContentsBounds());
+  if (header_views_->header)
+    bounds.Inset(0, header_views_->header->GetPreferredSize().height(), 0, 0);
+  return bounds;
 }
 
 gfx::Rect ConstrainedWindowFrameSimple::GetWindowBoundsForClientBounds(
     const gfx::Rect& client_bounds) const {
-  int horizontal_padding = ConstrainedWindow::kHorizontalPadding;
-  int vertical_padding = ConstrainedWindow::kVerticalPadding;
-  int row_padding = ConstrainedWindow::kRowPadding;
-  gfx::Size header_size =
-      header_views_->header ?
-      header_views_->header->GetPreferredSize() : gfx::Size();
-
-  int x = client_bounds.x() - horizontal_padding;
-  int y = client_bounds.y() - vertical_padding - header_size.height() -
-      row_padding;
-  int width = client_bounds.width() + 2 * horizontal_padding;
-  int height = client_bounds.height() + 2 * vertical_padding +
-      header_size.height() + row_padding;
-
-  return gfx::Rect(x, y, width, height);
+  gfx::Rect bounds(client_bounds);
+  bounds.Inset(-GetInsets());
+  if (header_views_->header)
+    bounds.Inset(0, -header_views_->header->GetPreferredSize().height(), 0, 0);
+  return bounds;
 }
 
 int ConstrainedWindowFrameSimple::NonClientHitTest(const gfx::Point& point) {
@@ -238,6 +253,6 @@ views::ImageButton* ConstrainedWindowFrameSimple::CreateCloseButton() {
   close_button->SetImage(views::CustomButton::BS_HOT,
                           rb.GetImageSkiaNamed(IDR_SHARED_IMAGES_X_HOVER));
   close_button->SetImage(views::CustomButton::BS_PUSHED,
-                          rb.GetImageSkiaNamed(IDR_SHARED_IMAGES_X_HOVER));
+                          rb.GetImageSkiaNamed(IDR_SHARED_IMAGES_X_PRESSED));
   return close_button;
 }