Correct padding and focus ring for frameless constrained window dialog

chrome/browser/ui/views/constrained_window_frame_simple.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/constrained_window_frame_simple.h"
 
 #include "chrome/browser/ui/constrained_window.h"
 #include "chrome/browser/ui/views/constrained_window_views.h"
 #include "grit/ui_resources.h"
 #include "grit/chromium_strings.h"
@@ skipping 16 matching lines @@
 #include "ui/views/widget/widget_delegate.h"
 
 namespace {
 
 typedef ConstrainedWindowFrameSimple::HeaderViews HeaderViews;
 
 // A layout manager that lays out the header view with proper padding,
 // 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
   virtual void Layout(views::View* host);
   virtual gfx::Size GetPreferredSize(views::View* host);
 
  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();
 
   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(
     views::View* header,
     views::Label* title_label,
     views::Button* close_button)
     : header(header),
       title_label(title_label),
       close_button(close_button) {
   DCHECK(header);
 }
 
 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());
 
   set_background(views::Background::CreateSolidBackground(
       ConstrainedWindow::GetBackgroundColor()));
 }
 
 ConstrainedWindowFrameSimple::~ConstrainedWindowFrameSimple() {
 }
 
 void ConstrainedWindowFrameSimple::SetHeaderView(HeaderViews* header_views)
 {
   RemoveAllChildViews(true);
 
   header_views_.reset(header_views);
 
   AddChildView(header_views_->header);
 }
 
 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);
   header_view->SetLayoutManager(grid_layout);
 
   views::ColumnSet* header_cs = grid_layout->AddColumnSet(0);
   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);
 
   return new HeaderViews(header_view, title_label, close_button);
 }
 
 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);
 }
 
 int ConstrainedWindowFrameSimple::NonClientHitTest(const gfx::Point& point) {
   if (!bounds().Contains(point))
     return HTNOWHERE;
   return HTCLIENT;
 }
 
@@ skipping 38 matching lines @@
   ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
   views::ImageButton* close_button = new views::ImageButton(this);
   close_button->SetImage(views::CustomButton::BS_NORMAL,
                           rb.GetImageSkiaNamed(IDR_SHARED_IMAGES_X));
   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));
   return close_button;
 }