(Views only) Resize the menu buttons on the Website Settings UI to fit the button text

chrome/browser/ui/views/website_settings/permission_selector_view.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/website_settings/permission_selector_view.h"
 
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/ui/website_settings/website_settings_ui.h"
 #include "grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
@@ skipping 76 matching lines @@
 class PermissionMenuButton : public views::MenuButton,
                              public views::MenuButtonListener {
  public:
   // Creates a new |PermissionMenuButton| with the passed |text|. The ownership
   // of the |model| remains with the caller and is not transfered to the
   // |PermissionMenuButton|.
   PermissionMenuButton(const string16& text,
                        PermissionMenuModel* model);
   virtual ~PermissionMenuButton();
 
+  // Overridden from views::MenuButton.
+  virtual gfx::Size GetPreferredSize() OVERRIDE;
+
+  // Overridden from views::TextButton.
+  virtual void SetText(const string16& text) OVERRIDE;
+
  private:
-  // Overridden from views::MenuButtonListener:
+  // Overridden from views::MenuButtonListener.
   virtual void OnMenuButtonClicked(View* source,
                                    const gfx::Point& point) OVERRIDE;
 
   PermissionMenuModel* menu_model_;  // Owned by |PermissionSelectorView|.
   scoped_ptr<views::MenuRunner> menu_runner_;
 
   DISALLOW_COPY_AND_ASSIGN(PermissionMenuButton);
 };
 
 ///////////////////////////////////////////////////////////////////////////////
@@ skipping 42 matching lines @@
   // Accelerators are not supported.
   return false;
 }
 
 void PermissionMenuModel::ExecuteCommand(int command_id) {
   current_setting_ = kSettingsForCommandIDs[command_id];
   permission_selector_->SelectionChanged();
 }
 
 ///////////////////////////////////////////////////////////////////////////////
-// PermissionMenuModel
+// PermissionMenuButton
 ///////////////////////////////////////////////////////////////////////////////
 
 PermissionMenuButton::PermissionMenuButton(const string16& text,
                                            PermissionMenuModel* model)
     : ALLOW_THIS_IN_INITIALIZER_LIST(MenuButton(NULL, text, this, true)),
       menu_model_(model) {
 }
 
 PermissionMenuButton::~PermissionMenuButton() {
 }
 
+gfx::Size PermissionMenuButton::GetPreferredSize() {
+  gfx::Insets insets = GetInsets();
+  // Scale the button to the current text size.

[msw, 2012/07/26 20:01:33]

Should there be a minimum size? What does this look like for empty strings?

[markusheintz_, 2012/07/30 12:55:37]

The minimum size is the size to fit the menu marker. For empty strings the
button is sized to only fit the marker.

Using a minimum size is tricky. In theory I want the button to resize perfectly
to a single character string. In real life there won't be any empty strings. The
PermissionMenuButton is only meant to be used here.

[msw, 2012/07/30 15:48:42]

OK, that seems fine.

+  gfx::Size prefsize(text_size_.width() + insets.width(),
+                     text_size_.height() + insets.height());
+  if (max_width_ > 0)
+    prefsize.set_width(std::min(max_width_, prefsize.width()));
+  if (show_menu_marker()) {
+    prefsize.Enlarge(menu_marker()->width() + 2 /*marke padding left + right*/,

[msw, 2012/07/26 20:01:33]

Shouldn't this use/match kMenuMarkerPaddingLeft and kMenuMarkerPaddingRight?
Also, nit: "marke" and move the comment to the line above if it's still needed.

[markusheintz_, 2012/07/30 12:55:37]

Uups done.


On 2012/07/26 20:01:33, msw wrote:

+                     0);
+  }
+  return prefsize;
+}
+
+void PermissionMenuButton::SetText(const string16& text) {
+  MenuButton::SetText(text);
+  SizeToPreferredSize();
+}
+
 void PermissionMenuButton::OnMenuButtonClicked(View* source,
                                                const gfx::Point& point) {
   views::MenuModelAdapter menu_model_adapter(menu_model_);
   menu_runner_.reset(new views::MenuRunner(menu_model_adapter.CreateMenu()));
 
   gfx::Point p(point);
   p.Offset(-source->width(), 0);
   if (menu_runner_->RunMenuAt(
           source->GetWidget()->GetTopLevelWidget(),
           this,
@@ skipping 96 matching lines @@
 }
 
 ContentSetting PermissionSelectorView::GetSelectedSetting() const {
   return menu_button_model_->current_setting();
 }
 
 ContentSettingsType PermissionSelectorView::GetPermissionType() const {
   return menu_button_model_->site_permission();
 }
 
+void PermissionSelectorView::ChildPreferredSizeChanged(View* child) {
+  SizeToPreferredSize();
+  // The parent is only a plain |View| that is used as a container/box/panel.
+  // In order to not implment a custom |View| class with its own implementation

[msw, 2012/07/26 20:01:33]

nit: use indirect verbiage here: "Call SizeToPreferredSize on the parent view
here to avoid subclassing the Parent." (or similar)

[markusheintz_, 2012/07/30 12:55:37]

Done.

+  // of the ChildPreferredSizeChanged method, we call the SizeToPreferredSize
+  // method of the parent directly from here.
+  parent()->SizeToPreferredSize();

[msw, 2012/07/26 20:01:33]

It's a little odd (perhaps bad) to make assumptions about the parent view. If
the parent and this class were using LayoutManagers, I think the preferred sizes
would change automatically with their respective children. Perhaps give that a
shot?

[markusheintz_, 2012/07/30 12:55:37]

I totally agree with you this is odd.

It would be great if I could make the parent just occupy all the space
available. But it looks like I must provide a proper preferred size.
So I could:
1) Implement the GetMaximumSize method of the PermissionMenuButton
2) Initialize the max size of the PermissionMenuButton
3) Override the GetPreferredSize method of the |PermissionSelectorView|

But that seem like an overkill compared to the one line solution. I will try to
get some more opinions on this.

On 2012/07/26 20:01:33, msw wrote:

+}
+
 PermissionSelectorView::~PermissionSelectorView() {
 }