Make Web Intents picker in Views conform to latest mocks

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 4b480cdef40434c217bb491d18282b1995b731be..aa684342846794cf76d72d20e853f60c59d4d9a5 100644
--- a/chrome/browser/ui/views/constrained_window_frame_simple.cc
+++ b/chrome/browser/ui/views/constrained_window_frame_simple.cc
@@ -27,149 +27,97 @@
 #include "ui/views/widget/widget.h"
 #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() {}
-  virtual ~HeaderLayout() {}
-
-  // Overridden from LayoutManager
-  virtual void Layout(views::View* host);
-  virtual gfx::Size GetPreferredSize(views::View* host);
-
-  DISALLOW_COPY_AND_ASSIGN(HeaderLayout);
-};
-
-void HeaderLayout::Layout(views::View* host) {
-  if (!host->has_children())
-    return;
-
-  int top_padding = ConstrainedWindowConstants::kCloseButtonPadding;
-  int left_padding = ConstrainedWindowConstants::kHorizontalPadding;
-  int right_padding = ConstrainedWindowConstants::kCloseButtonPadding;
-
-  views::View* header = host->child_at(0);
-  gfx::Size preferred_size = GetPreferredSize(host);
-  int width = preferred_size.width() - left_padding - right_padding;
-  int height = preferred_size.height() - top_padding;
-
-  header->SetBounds(left_padding, top_padding, width, height);
-}
-
-gfx::Size HeaderLayout::GetPreferredSize(views::View* host) {
-  int top_padding = ConstrainedWindowConstants::kCloseButtonPadding;
-  int left_padding = ConstrainedWindowConstants::kHorizontalPadding;
-  int right_padding = ConstrainedWindowConstants::kCloseButtonPadding;
-
-  views::View* header = host->child_at(0);
-  gfx::Size header_size = header ? header->GetPreferredSize() : gfx::Size();
-  int width = std::max(host->GetPreferredSize().width(),
-      left_padding + header_size.width() + right_padding);
-  int height = header_size.height() + top_padding;
-
-  return gfx::Size(width, height);
-}
-
-}  // 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);
-}
+using views::ImageButton;
+using views::Label;
+using views::View;

[Peter Kasting, 2012/10/16 17:28:59]

Nit: These are perfectly legal, but I normally avoid them if they don't save a
significant amount of space.  (I'm not even sure where the last using statement
helps you at all.)

[Ben Goodger (Google), 2012/10/16 18:07:56]

+1. I'd prefer not to do this if you're prefixing other views-types within this
file (e.g. views::GridLayout). Just makes things confusing. We also don't tend
to do this unless we have real issues fitting stuff within 80 cols.

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

Removed. Done.

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

Roger that. Done.

 
 ConstrainedWindowFrameSimple::ConstrainedWindowFrameSimple(
     ConstrainedWindowViews* container)
-    : container_(container) {
+    : container_(container),
+      title_label_(new Label(container->widget_delegate()->GetWindowTitle())),
+      ALLOW_THIS_IN_INITIALIZER_LIST(close_button_(new ImageButton(this))),
+      min_width_(0) {
   container_->set_frame_type(views::Widget::FRAME_TYPE_FORCE_CUSTOM);
 
-  layout_ = new HeaderLayout();
-  SetLayoutManager(layout_);
+  views::GridLayout* layout = new views::GridLayout(this);
+  const int kHeaderTopPadding = std::min(
+      ConstrainedWindowConstants::kCloseButtonPadding,
+      ConstrainedWindowConstants::kTitleTopPadding);
+  layout->SetInsets(kHeaderTopPadding,
+                    ConstrainedWindowConstants::kHorizontalPadding,
+                    0,
+                    ConstrainedWindowConstants::kCloseButtonPadding);
+  SetLayoutManager(layout);
+  views::ColumnSet* cs = layout->AddColumnSet(0);
+  cs->AddColumn(views::GridLayout::FILL, views::GridLayout::LEADING, 1,
+                views::GridLayout::USE_PREF, 0, 0);  // Title.
+  cs->AddPaddingColumn(0, ConstrainedWindowConstants::kCloseButtonPadding);
+  cs->AddColumn(views::GridLayout::TRAILING, views::GridLayout::LEADING, 0,
+                views::GridLayout::USE_PREF, 0, 0);  // Close Button.
+
+  layout->StartRow(0, 0);
+
+  ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
+  title_label_->SetFont(rb.GetFont(
+      ConstrainedWindowConstants::kTitleFontStyle));
+  title_label_->SetHorizontalAlignment(Label::ALIGN_LEFT);
+  title_label_->SetEnabledColor(ConstrainedWindow::GetTextColor());
+  title_label_->set_border(views::Border::CreateEmptyBorder(
+      ConstrainedWindowConstants::kTitleTopPadding - kHeaderTopPadding,
+      0, 0, 0));
+  layout->AddView(title_label_);
+
+  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_PRESSED));
+  close_button_->set_border(views::Border::CreateEmptyBorder(
+      ConstrainedWindowConstants::kCloseButtonPadding - kHeaderTopPadding,
+      0, 0, 0));
+  layout->AddView(close_button_);
+
+  Layout();

[Peter Kasting, 2012/10/16 17:28:59]

Tiny nit: It seems less error-prone to make this the last call in this function,
even if adding borders and backgrounds doesn't end up influencing anything. 
Maybe someone comes along later and tacks something on to the bottom of this
function, missing this Layout() call.

[Ben Goodger (Google), 2012/10/16 18:07:56]

My question is why do you have to explicitly call this at all. Seems like the
initial size of the widget should cause the view to get laid out?

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

Removed as ben@ suggested. It still works correctly. Done.

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

Removed. It still works correctly. Done.

 
-  SetHeaderView(CreateDefaultHeaderView());
+  min_width_ = ConstrainedWindowConstants::kHorizontalPadding +

[Peter Kasting, 2012/10/16 17:28:59]

Nit: Seems like this doesn't need to be a member, you could just move this
calculation to GetWindowBoundsForClientBounds(), no?

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

Moved the calculation and removed the member. Done.

+      close_button_->GetPreferredSize().width() +
+      2 * ConstrainedWindowConstants::kCloseButtonPadding;
 
   set_background(views::Background::CreateSolidBackground(
       ConstrainedWindow::GetBackgroundColor()));
 
-  set_border(views::Border::CreateEmptyBorder(
-      ConstrainedWindowConstants::kClientTopPadding,
-      ConstrainedWindowConstants::kHorizontalPadding,
-      ConstrainedWindowConstants::kClientBottomPadding,
-      ConstrainedWindowConstants::kHorizontalPadding));
+  switch (container->chrome_style_client_insets()) {

[Peter Kasting, 2012/10/16 17:28:59]

Nit: I'd just do "if (container->chrome_style_client_insets() ==
ConstrainedWindowViews::DEFAULT_INSETS) { ... }" and drop the other two switch
arms.

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

Using if instead of switch statement. Done.

+    case ConstrainedWindowViews::DEFAULT_INSETS:
+      set_border(views::Border::CreateEmptyBorder(
+          ConstrainedWindowConstants::kClientTopPadding +
+              std::max(close_button_->GetPreferredSize().height(),
+                       title_label_->GetPreferredSize().height()),
+          ConstrainedWindowConstants::kHorizontalPadding,

[Peter Kasting, 2012/10/16 17:28:59]

Are the values in this CreateEmptyBorder() call added to the values in the
SetInsets() call above or not?

If yes, then it seems like instead of kHorizontalPadding twice here we should
have something like "0" and "kHorizontalPadding - kCloseButtonPadding".

If no, then it seems like we need to add kHeaderTopPadding to the top padding
value.  And it makes me wonder how NO_INSETS consumers avoid drawing atop the
views above.

You may want to add a comment noting how the border here interacts with the
insets above.

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

Added kHeaderTopPadding to the calculation to make it correct.

Added the following comment to answer your question:

  // Client insets have no relation to header insets:
  // - The client insets are the distance from the window border to the client
  //   view.
  // - The header insets are the distance from the window border to the header
  //   elements.
  //
  // The NO_ISNETS consumers draw atop the views above.

This poses a follow-on question: should we not add views at all for NO_INSETS. I
am committing and will address your further comments, if any, in a follow up CL.

[Peter Kasting, 2012/10/16 19:20:04]

Yeah, that's what I was thinking: presumably NO_INSETS consumers don't want the
header at all?

+          ConstrainedWindowConstants::kClientBottomPadding,
+          ConstrainedWindowConstants::kHorizontalPadding));
+      break;
+    case ConstrainedWindowViews::NO_INSETS:
+      break;
+    default:
+      NOTREACHED();
+  }
 }
 
 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 = ConstrainedWindowConstants::kTitleTopPadding -
-      ConstrainedWindowConstants::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::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(ConstrainedWindowConstants::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();
-  grid_layout->AddView(close_button);
-
-  return new HeaderViews(header_view, title_label, close_button);
-}
-
 gfx::Rect ConstrainedWindowFrameSimple::GetBoundsForClientView() const {
-  gfx::Rect bounds(GetContentsBounds());
-  if (header_views_->header)
-    bounds.Inset(0, header_views_->header->GetPreferredSize().height(), 0, 0);
-  return bounds;
+  return GetContentsBounds();
 }
 
 gfx::Rect ConstrainedWindowFrameSimple::GetWindowBoundsForClientBounds(
     const gfx::Rect& client_bounds) const {
   gfx::Rect bounds(client_bounds);
   bounds.Inset(-GetInsets());
-  if (header_views_->header)
-    bounds.Inset(0, -header_views_->header->GetPreferredSize().height(), 0, 0);
+  bounds.set_width(std::max(
+       bounds.width(),
+       min_width_ + title_label_->GetPreferredSize().width()));
   return bounds;
 }
 
@@ -204,32 +152,16 @@ void ConstrainedWindowFrameSimple::UpdateWindowIcon() {
 }
 
 void ConstrainedWindowFrameSimple::UpdateWindowTitle() {
-  if (!header_views_->title_label)
-    return;
-
-  string16 text = container_->widget_delegate()->GetWindowTitle();
-  header_views_->title_label->SetText(text);
+  title_label_->SetText(container_->widget_delegate()->GetWindowTitle());
 }
 
 gfx::Size ConstrainedWindowFrameSimple::GetPreferredSize() {
-  return container_->non_client_view()->GetWindowBoundsForClientBounds(
+  return GetWindowBoundsForClientBounds(
       gfx::Rect(container_->client_view()->GetPreferredSize())).size();
 }
 
 void ConstrainedWindowFrameSimple::ButtonPressed(views::Button* sender,
                                                  const ui::Event& event) {
-  if (header_views_->close_button && sender == header_views_->close_button)
+  if (sender == close_button_)
     sender->GetWidget()->Close();
 }
-
-views::ImageButton* ConstrainedWindowFrameSimple::CreateCloseButton() {
-  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_PRESSED));
-  return close_button;
-}