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 cd8cce684b485da8a5a11408988269b4c35fe58b..0a7c72cd358c39104c54fed52036018332f496d0 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,12 +87,32 @@ 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);
 }
 
+// Utility function to add padding to a view.
+views::View* AddPadding(
+    views::View* viewToPad,

[Peter Kasting, 2012/10/09 19:31:10]

Nit: Use underscores

[please use gerrit instead, 2012/10/10 19:12:58]

I've removed this method entirely in favor of set_border().

+    int top,
+    int left,
+    int bottom,
+    int right) {
+  views::View* result = new views::View();
+  views::GridLayout* layout = new views::GridLayout(result);
+  result->SetLayoutManager(layout);
+  layout->SetInsets(top, left, bottom, right);
+  views::ColumnSet* cs = layout->AddColumnSet(0);
+  cs->AddColumn(views::GridLayout::CENTER, views::GridLayout::CENTER, 0,
+                views::GridLayout::USE_PREF, 0, 0);
+  layout->StartRow(0, 0);
+  layout->AddView(viewToPad);
+  return result;
+}
+
 }  // namespace
 
 ConstrainedWindowFrameSimple::HeaderViews::HeaderViews(
@@ -94,10 +127,15 @@ ConstrainedWindowFrameSimple::HeaderViews::HeaderViews(
 
 ConstrainedWindowFrameSimple::ConstrainedWindowFrameSimple(
     ConstrainedWindowViews* container)
-    : container_(container) {
+    : container_(container),
+      client_insets_(ConstrainedWindow::kRowPadding,
+                     ConstrainedWindow::kHorizontalPadding,
+                     ConstrainedWindow::kVerticalPadding,
+                     ConstrainedWindow::kHorizontalPadding) {

[Peter Kasting, 2012/10/09 19:31:10]

Instead of constructing an Insets member, do something like:

  set_border(Border::CreateEmptyBorder(T, L, B, R));

This will actually reserve this space for you so you don't need to discount for
it everywhere, which in turn should make your HeaderLayout class and the rest of
the code here simpler.

[Mike Wittman, 2012/10/09 22:55:46]

This is good advice, but if you go this route please verify that it works OK on
Windows.  I've run into some issues getting borders to coexist properly with a
window mask on that platform.

[please use gerrit instead, 2012/10/10 19:12:58]

wittman@: Please try the latest patch set on your Windows build machine.

[please use gerrit instead, 2012/10/10 19:12:58]

I am using set_border now, but I want to keep |client_insets_| member, because
ConstrainedWindow will need to override the insets to be {0, 0, 0, 0}.

   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/09 19:31:10]

Nit: Indent even

[please use gerrit instead, 2012/10/10 19:12:58]

Done.

   SetLayoutManager(layout_);
 
   SetHeaderView(CreateDefaultHeaderView());
@@ -119,6 +157,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,18 +172,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());
-  grid_layout->AddView(title_label);
+  grid_layout->AddView(AddPadding(title_label, kTitleTopPadding,
+      kTitleLeftPadding, kTitleBottomPadding, kTitleRightPadding));
 
   views::Button* close_button = CreateCloseButton();
   grid_layout->AddView(close_button);
@@ -148,35 +194,33 @@ 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 =

[Peter Kasting, 2012/10/09 19:31:10]

Nit: Simpler:

  gfx::Rect bounds(GetContentsBounds());
  if (header_views_->header)
    bounds.Inset(0, header_views_->header->GetPreferredSize().height(), 0, 0);
  return bounds;

This assumes you follow the border suggestion above.

[please use gerrit instead, 2012/10/10 19:12:58]

Done.

       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)));
+  int client_x = client_insets_.left();
+  int client_y = header_size.height() + client_insets_.top();
+  int client_width = width() - client_x - client_insets_.right();
+  int client_height = height() - client_y - client_insets_.bottom();
+
+  client_width = std::max(0, client_width);
+  client_height = std::max(0, client_height);
+
+  return gfx::Rect(client_x, client_y, client_width, client_height);
 }
 
 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 =

[Peter Kasting, 2012/10/09 19:31:10]

Nit: Simplify by rewriting like the function above and making use of something
like:

  bounds.Inset(-GetInsets());

[please use gerrit instead, 2012/10/10 19:12:58]

Done.

       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;
+  int x = client_bounds.x() - client_insets_.left();
+  int y = client_bounds.y() - header_size.height() - client_insets_.top();
+  int width = client_bounds.width() + client_insets_.left() +
+      client_insets_.right();
+  int height = client_bounds.height() + header_size.height() +
+      client_insets_.top() + client_insets_.bottom();
 
   return gfx::Rect(x, y, width, height);
 }