Fixes of settings view layout bugs.

ui/message_center/views/notifier_settings_view.cc

 // Copyright (c) 2013 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 "ui/message_center/views/notifier_settings_view.h"
 
 #include "grit/ui_strings.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
@@ skipping 31 matching lines @@
 // The view to guarantee the 48px height and place the contents at the
 // middle. It also guarantee the left margin.
 class EntryView : public views::View {
  public:
   EntryView(views::View* contents);
   virtual ~EntryView();
 
   // Overridden from views::View:
   virtual void Layout() OVERRIDE;
   virtual gfx::Size GetPreferredSize() OVERRIDE;
+  virtual void OnFocus() OVERRIDE;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(EntryView);
 };
 
 EntryView::EntryView(views::View* contents) {
   AddChildView(contents);
 }
 
 EntryView::~EntryView() {
 }
 
 void EntryView::Layout() {
   DCHECK_EQ(1, child_count());
   views::View* contents = child_at(0);
-  gfx::Size size = contents->GetPreferredSize();
+  int contents_width = width() - kMarginWidth * 2;
+  int contents_height = contents->GetHeightForWidth(contents_width);

[dharcourt, 2013/04/05 16:07:03]

Nit: contents_width/contents_height should be content_width/content_height
(singular, like "horse race" instead of "horses race")... and I know we have
many other places in the code where this is wrong :-).

[Jun Mukai, 2013/04/06 03:14:04]

Done.

   int y = 0;
-  if (size.height() < height())
-    y = (height() - size.height()) / 2;
-  contents->SetBounds(kMarginWidth, y, width() - kMarginWidth, size.height());
+  if (contents_height < height())
+    y = (height() - contents_height) / 2;

[dharcourt, 2013/04/05 16:07:03]

This _might_ be simpler as y = std::max((height() - content_height) / 2, 0),
your call.

[Jun Mukai, 2013/04/06 03:14:04]

Done.

+  contents->SetBounds(kMarginWidth, y, contents_width, contents_height);
 }
 
 gfx::Size EntryView::GetPreferredSize() {
   DCHECK_EQ(1, child_count());
   gfx::Size size = child_at(0)->GetPreferredSize();
   size.ClampToMin(gfx::Size(kMinimumWindowWidth, kEntryHeight));
   return size;
 }
 
+void EntryView::OnFocus() {
+  ScrollRectToVisible(GetLocalBounds());
+}
+
 // The separator line between the title and the scroll view. Currently
 // it is achieved as a top border of the scroll view.
 class Separator : public views::Border {
  public:
   Separator();
   virtual ~Separator();
 
   // Overridden from views::Border:
   virtual void Paint(const views::View& view, gfx::Canvas* canvas) OVERRIDE;
   virtual gfx::Insets GetInsets() const OVERRIDE;
@@ skipping 43 matching lines @@
   NotifierButton(Notifier* notifier, views::ButtonListener* listener)
       : views::CustomButton(listener),
         notifier_(notifier),
         icon_view_(NULL),
         checkbox_(new views::Checkbox(string16())) {
     DCHECK(notifier);
     SetLayoutManager(new views::BoxLayout(
         views::BoxLayout::kHorizontal, 0, 0, kSpaceInButtonComponents));
     checkbox_->SetChecked(notifier_->enabled);
     checkbox_->set_listener(this);
-    checkbox_->set_focusable(true);
+    checkbox_->set_focusable(false);

[dewittj, 2013/04/05 20:48:41]

I believe you could just remove this line, since focusable is default false.

[Jun Mukai, 2013/04/06 03:14:04]

Done.

     AddChildView(checkbox_);
     UpdateIconImage(notifier_->icon);
     AddChildView(new views::Label(notifier_->name));
   }
 
   void UpdateIconImage(const gfx::Image& icon) {
     notifier_->icon = icon;
     if (icon.IsEmpty()) {
       delete icon_view_;
       icon_view_ = NULL;
@@ skipping 102 matching lines @@
   AddChildView(scroller_);
 
   views::View* contents_view = new views::View();
   contents_view->SetLayoutManager(new views::BoxLayout(
       views::BoxLayout::kVertical, 0, 0, 0));
 
   views::Label* top_label = new views::Label(l10n_util::GetStringUTF16(
       IDS_MESSAGE_CENTER_SETTINGS_DIALOG_DESCRIPTION));
   top_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
   top_label->SetMultiLine(true);
-  top_label->SizeToFit(kMinimumWindowWidth - kMarginWidth);
+  top_label->SizeToFit(kMinimumWindowWidth - kMarginWidth * 2);
   contents_view->AddChildView(new EntryView(top_label));
 
   std::vector<Notifier*> notifiers;
   delegate_->GetNotifierList(&notifiers);
   for (size_t i = 0; i < notifiers.size(); ++i) {
     NotifierButton* button = new NotifierButton(notifiers[i], this);
-    contents_view->AddChildView(new EntryView(button));
+    EntryView* entry = new EntryView(button);
+    entry->set_focusable(true);
+    contents_view->AddChildView(entry);
     buttons_.insert(button);
   }
   scroller_->SetContents(contents_view);
 
   contents_view->SetBoundsRect(gfx::Rect(contents_view->GetPreferredSize()));
 }
 
 NotifierSettingsView::~NotifierSettingsView() {
   settings_view_ = NULL;
 }
 
 void NotifierSettingsView::WindowClosing() {
   if (delegate_)
     delegate_->OnNotifierSettingsClosing();
 }
 
 views::View* NotifierSettingsView::GetContentsView() {
   return this;
 }
 
 bool NotifierSettingsView::CanResize() const {
   return true;
 }
 
 void NotifierSettingsView::Layout() {
   int title_height = title_entry_->GetPreferredSize().height();
   title_entry_->SetBounds(0, 0, width(), title_height);
+  views::View* contents_view = scroller_->contents();

[dewittj, 2013/04/05 20:48:41]

You are more familiar with scroll views than me but it strikes me as strange
that we are manually resizing the contents view instead of telling the
scrollview that it has a different amount of room now. 

[Jun Mukai, 2013/04/06 03:14:04]

That is a tricky part of the scroll view.
The size change of the ScrollView changes the visible rectangle size of its
contents, and that has no relationship with the layout of the contents view.
So essentially, if the contents view is wider than the width of scroll view, it
will show the horizontal scroll bar rather than changing the width of contents
view.

This is good to ideal scrollers, but we want to allow the vertical scroll bar
only here and wrap the text properly. This actually needs to change the width of
the contents view properly.

+  int contents_width = width();
+  int contents_height = contents_view->GetHeightForWidth(contents_width);

[dharcourt, 2013/04/05 16:07:03]

Nit: Same contents_width/contents_height -> content_width/content_height.

[Jun Mukai, 2013/04/06 03:14:04]

Done.

+  if (title_height + contents_height > kMinimumWindowHeight) {
+    contents_width -= scroller_->GetScrollBarWidth();
+    contents_height = contents_view->GetHeightForWidth(contents_width);
+  }
+  contents_view->SetBounds(0, 0, contents_width, contents_height);
   scroller_->SetBounds(0, title_height, width(), height() - title_height);
 }
 
 gfx::Size NotifierSettingsView::GetMinimumSize() {
   gfx::Size size(kMinimumWindowWidth, kMinimumWindowHeight);
   int total_height = title_entry_->GetPreferredSize().height() +
       scroller_->contents()->GetPreferredSize().height();
   if (total_height > kMinimumWindowHeight)
     size.Enlarge(scroller_->GetScrollBarWidth(), 0);
   return size;
 }
 
 gfx::Size NotifierSettingsView::GetPreferredSize() {
   return GetMinimumSize();
 }
 
 void NotifierSettingsView::ButtonPressed(views::Button* sender,
                                          const ui::Event& event) {
   std::set<NotifierButton*>::iterator iter = buttons_.find(
       static_cast<NotifierButton*>(sender));
   DCHECK(iter != buttons_.end());
 
   (*iter)->SetChecked(!(*iter)->checked());
   if (delegate_)
     delegate_->SetNotifierEnabled((*iter)->notifier(), (*iter)->checked());
 }
 
 }  // namespace message_center