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..0278642b8357d935f168b0110791bd75a45360dc 100644
--- a/chrome/browser/ui/views/constrained_window_frame_simple.cc
+++ b/chrome/browser/ui/views/constrained_window_frame_simple.cc
@@ -29,138 +29,86 @@
 
 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);
+views::Label* CreateTitleLabel(const string16& title) {
+  const int kTitleTopPadding = ConstrainedWindowConstants::kTitleTopPadding -
+      ConstrainedWindowConstants::kCloseButtonPadding;

[Peter Kasting, 2012/10/15 21:46:54]

Nit: Add a comment about why we subtract kCloseButtonPadding here, since it's
not obvious.

It might make sense to create the title label inside CreateHeader() instead of
passing it in as an argument, in which case this becomes a lot clearer since the
insets are also set in that function.  That in turn suggests that maybe
CreateHeader()'s contents should be inlined into the
ConstrainedWindowFrameSimple constructor so it's convenient to init members like
|title_label_| and |header_|.  Another way would be to make these private member
functions as opposed to file statics.  Personally I generally tend to inline
anything that doesn't have multiple callers :).

[please use gerrit instead, 2012/10/16 00:12:23]

Inlined the whole thing and added a comment for subtracting the
kCloseButtonPadding.

+  ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
+  views::Label* title_label = new views::Label(title);
+  title_label->SetHorizontalAlignment(views::Label::ALIGN_LEFT);
+  title_label->SetFont(rb.GetFont(ConstrainedWindowConstants::kTitleFontStyle));
+  title_label->SetEnabledColor(ConstrainedWindow::GetTextColor());
+  title_label->set_border(views::Border::CreateEmptyBorder(
+      kTitleTopPadding, 0, 0, 0));
+  return title_label;
 }
 
-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;
+views::ImageButton* CreateCloseButton(views::ButtonListener* listener) {
+  ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
+  views::ImageButton* close_button = new views::ImageButton(listener);
+  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;
+}
 
-  return gfx::Size(width, height);
+views::View* CreateHeader(
+    views::View* header_child,

[Peter Kasting, 2012/10/15 21:46:54]

Nit: Place on previous line, indent next line even

[please use gerrit instead, 2012/10/16 00:12:23]

Not applicable after inlining that you requested.

+    views::ImageButton* close_button) {
+  const int kHeaderBottomPadding = 0;

[Peter Kasting, 2012/10/15 21:46:54]

Nit: I'd just write 0 in the SetInsets() call

[please use gerrit instead, 2012/10/16 00:12:23]

Done.

+  views::View* header = new views::View();
+  views::GridLayout* header_layout = new views::GridLayout(header);
+  header_layout->SetInsets(ConstrainedWindowConstants::kCloseButtonPadding,
+                           ConstrainedWindowConstants::kHorizontalPadding,
+                           kHeaderBottomPadding,
+                           ConstrainedWindowConstants::kCloseButtonPadding);
+  header->SetLayoutManager(header_layout);
+  views::ColumnSet* cs = header_layout->AddColumnSet(0);
+  cs->AddColumn(views::GridLayout::FILL, views::GridLayout::CENTER, 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.
+  header_layout->StartRow(0, 0);
+  header_layout->AddView(header_child);
+  header_layout->AddView(close_button);
+  header->Layout();

[Peter Kasting, 2012/10/15 21:46:54]

Is this call necessary?  Shouldn't this happen automatically when the
constructor does Layout()?

[please use gerrit instead, 2012/10/16 00:12:23]

Removed and the UI still works as expected. You're correct: it's not necessary.

+  return header;
 }
 
 }  // 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),
+      header_(NULL),
+      title_label_(CreateTitleLabel(
+          container->widget_delegate()->GetWindowTitle())),
+      ALLOW_THIS_IN_INITIALIZER_LIST(close_button_(CreateCloseButton(this))) {
   container_->set_frame_type(views::Widget::FRAME_TYPE_FORCE_CUSTOM);
 
-  layout_ = new HeaderLayout();
-  SetLayoutManager(layout_);
+  header_ = CreateHeader(title_label_, close_button_);
 
-  SetHeaderView(CreateDefaultHeaderView());
+  views::GridLayout* layout = new views::GridLayout(this);
+  SetLayoutManager(layout);
+  layout->AddColumnSet(0)->AddColumn(
+      views::GridLayout::FILL, views::GridLayout::LEADING, 1,
+      views::GridLayout::USE_PREF, 0, 0);
+  layout->StartRow(0, 0);
+  layout->AddView(header_);
+  Layout();
 
   set_background(views::Background::CreateSolidBackground(
       ConstrainedWindow::GetBackgroundColor()));
-
-  set_border(views::Border::CreateEmptyBorder(
-      ConstrainedWindowConstants::kClientTopPadding,
-      ConstrainedWindowConstants::kHorizontalPadding,
-      ConstrainedWindowConstants::kClientBottomPadding,
-      ConstrainedWindowConstants::kHorizontalPadding));
 }
 
 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;
 }
 
@@ -168,8 +116,8 @@ 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);
+  gfx::Size header_size = header_->GetPreferredSize();

[Peter Kasting, 2012/10/15 21:46:54]

Nit: Or just inline into next line

[please use gerrit instead, 2012/10/16 00:12:23]

Done.

+  bounds.set_width(std::max(bounds.width(), header_size.width()));
   return bounds;
 }
 
@@ -204,32 +152,30 @@ void ConstrainedWindowFrameSimple::UpdateWindowIcon() {
 }
 
 void ConstrainedWindowFrameSimple::UpdateWindowTitle() {
-  if (!header_views_->title_label)
+  if (!title_label_)
     return;
 
   string16 text = container_->widget_delegate()->GetWindowTitle();
-  header_views_->title_label->SetText(text);
+  title_label_->SetText(text);
 }
 
-gfx::Size ConstrainedWindowFrameSimple::GetPreferredSize() {
-  return container_->non_client_view()->GetWindowBoundsForClientBounds(
-      gfx::Rect(container_->client_view()->GetPreferredSize())).size();
+void ConstrainedWindowFrameSimple::OnBoundsChanged(
+    const gfx::Rect& previous_bounds) {
+  gfx::Rect header_bounds(header_->bounds());
+  header_bounds.set_width(std::max(header_->width(), bounds().width()));

[Peter Kasting, 2012/10/15 21:46:54]

Is it right to use header_->width() instead of
header_->GetPreferredSize().width()?  Seems like this way will prevent the
header from shrinking if the overall bounds shrink to a size that's still bigger
than the header's preferred size.

[please use gerrit instead, 2012/10/16 00:12:23]

Great catch! I've been looking for a way to resolve this potential issue. Done.

+  header_->SetBounds(header_bounds.x(), header_bounds.y(),
+                     header_bounds.width(), header_bounds.height());
+  views::NonClientFrameView::OnBoundsChanged(previous_bounds);
 }
 
-void ConstrainedWindowFrameSimple::ButtonPressed(views::Button* sender,
-                                                 const ui::Event& event) {
-  if (header_views_->close_button && sender == header_views_->close_button)
-    sender->GetWidget()->Close();
+gfx::Size ConstrainedWindowFrameSimple::GetPreferredSize() {
+  return GetWindowBoundsForClientBounds(
+      gfx::Rect(container_->client_view()->GetPreferredSize())).size();
 }
 
-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;
+void ConstrainedWindowFrameSimple::ButtonPressed(

[Peter Kasting, 2012/10/15 21:46:54]

Nit: Original wrapping here was fine

[please use gerrit instead, 2012/10/16 00:12:23]

Returned to original wrapping as you requested.

+    views::Button* sender,
+    const ui::Event& event) {
+  if (close_button_ && sender == close_button_)

[Peter Kasting, 2012/10/15 21:46:54]

Nit: NULL-checking |close_button_| is presumably not necessary since |sender|
ought not to be NULL, right?

[please use gerrit instead, 2012/10/16 00:12:23]

Right. Also, |close_button_| should always be non-null. I've removed the
null-check.

+      sender->GetWidget()->Close();

[Peter Kasting, 2012/10/15 21:46:54]

Nit: Indent 2

[please use gerrit instead, 2012/10/16 00:12:23]

Done.

 }