views: align columns in site settings dialog

chrome/browser/ui/views/website_settings/permission_selector_row.cc

Index: chrome/browser/ui/views/website_settings/permission_selector_row.cc
diff --git a/chrome/browser/ui/views/website_settings/permission_selector_row.cc b/chrome/browser/ui/views/website_settings/permission_selector_row.cc
index 6554daec10b2769623e71d0723b3d44fcfe9f88c..d8083ac59a615d3a8ae92807f2de026966e52bac 100644
--- a/chrome/browser/ui/views/website_settings/permission_selector_row.cc
+++ b/chrome/browser/ui/views/website_settings/permission_selector_row.cc
@@ -26,11 +26,6 @@
 #include "ui/views/view.h"
 #include "ui/views/widget/widget.h"
 
-namespace {
-// Minimum distance between the label and its corresponding menu.
-const int kMinSeparationBetweenLabelAndMenu = 16;
-}  // namespace
-
 namespace internal {
 
 // The |PermissionMenuButton| provides a menu for selecting a setting a
@@ -74,6 +69,12 @@ PermissionMenuButton::PermissionMenuButton(const base::string16& text,
                                            PermissionMenuModel* model,
                                            bool show_menu_marker)
     : MenuButton(text, this, show_menu_marker), menu_model_(model) {
+  // Since PermissionMenuButtons are added to a GridLayout, they are not always
+  // sized to their preferred size. Disclosure arrows are always right-aligned,
+  // so if the text is not right-aligned, awkward space appears between the text
+  // and the arrow.
+  SetHorizontalAlignment(gfx::ALIGN_RIGHT);
+
   // Update the themed border before the NativeTheme is applied. Usually this
   // happens in a call to LabelButton::OnNativeThemeChanged(). However, if
   // PermissionMenuButton called that from its override, the NativeTheme would
@@ -232,40 +233,20 @@ void PermissionCombobox::OnPerformAction(Combobox* combobox) {
 PermissionSelectorRow::PermissionSelectorRow(
     Profile* profile,
     const GURL& url,
-    const WebsiteSettingsUI::PermissionInfo& permission)
+    const WebsiteSettingsUI::PermissionInfo& permission,
+    views::GridLayout* container)
     : profile_(profile), icon_(NULL), menu_button_(NULL), combobox_(NULL) {
-  views::GridLayout* layout = new views::GridLayout(this);
-  SetLayoutManager(layout);
-  const int column_set_id = 0;
-  views::ColumnSet* column_set = layout->AddColumnSet(column_set_id);
-  column_set->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 0,
-                        views::GridLayout::FIXED, kPermissionIconColumnWidth,
-                        0);
-  column_set->AddPaddingColumn(0, kPermissionIconMarginLeft);
-  column_set->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 0,
-                        views::GridLayout::USE_PREF, 0, 0);
-  column_set->AddPaddingColumn(1, kMinSeparationBetweenLabelAndMenu);
-  column_set->AddColumn(views::GridLayout::TRAILING, views::GridLayout::FILL, 0,
-                        views::GridLayout::USE_PREF, 0, 0);
-
-  layout->StartRow(1, column_set_id);
   // Create the permission icon.
   icon_ = new NonAccessibleImageView();
   const gfx::Image& image = WebsiteSettingsUI::GetPermissionIcon(permission);
   icon_->SetImage(image.ToImageSkia());
-  layout->AddView(icon_,
-                  1,
-                  1,
-                  views::GridLayout::CENTER,
-                  views::GridLayout::CENTER);
+  container->AddView(icon_, 1, 1, views::GridLayout::CENTER,
+                     views::GridLayout::CENTER);
   // Create the label that displays the permission type.
-  views::Label* label = new views::Label(
+  label_ = new views::Label(
       WebsiteSettingsUI::PermissionTypeToUIString(permission.type));
-  layout->AddView(label,
-                  1,
-                  1,
-                  views::GridLayout::LEADING,
-                  views::GridLayout::CENTER);
+  container->AddView(label_, 1, 1, views::GridLayout::LEADING,
+                     views::GridLayout::CENTER);
   // Create the menu model.
   menu_model_.reset(new PermissionMenuModel(
       profile, url, permission,
@@ -280,9 +261,9 @@ PermissionSelectorRow::PermissionSelectorRow(
       ui::MaterialDesignController::IsSecondaryUiMaterial();
 #endif
   if (use_real_combobox)
-    InitializeComboboxView(layout, permission);
+    InitializeComboboxView(container, permission);
   else
-    InitializeMenuButtonView(layout, permission);
+    InitializeMenuButtonView(container, permission);
 }
 
 void PermissionSelectorRow::AddObserver(
@@ -290,10 +271,6 @@ void PermissionSelectorRow::AddObserver(
   observer_list_.AddObserver(observer);
 }
 
-void PermissionSelectorRow::ChildPreferredSizeChanged(View* child) {
-  Layout();
-}
-
 PermissionSelectorRow::~PermissionSelectorRow() {
   // Gross. On paper the Combobox and the ComboboxModelAdapter are both owned by
   // this class, but actually, the Combobox is owned by View and will be
@@ -306,7 +283,7 @@ PermissionSelectorRow::~PermissionSelectorRow() {
   // Technically, the MenuButton has the same problem, but MenuButton doesn't
   // use its model in its destructor.
   if (combobox_)
-    RemoveChildView(combobox_);
+    combobox_->parent()->RemoveChildView(combobox_);
 }
 
 void PermissionSelectorRow::InitializeMenuButtonView(
@@ -352,6 +329,7 @@ void PermissionSelectorRow::PermissionChanged(
         profile_, permission.type, permission.setting,
         permission.default_setting, content_settings::SETTING_SOURCE_USER));
     menu_button_->SizeToPreferredSize();
+    menu_button_->InvalidateLayout();

[sky, 2017/03/02 20:05:41]

Who calls Layout?

[Elly Fong-Jones, 2017/03/06 17:17:10]

Done.

   } else if (combobox_) {
     bool use_default = permission.setting == CONTENT_SETTING_DEFAULT;
     combobox_->UpdateSelectedIndex(use_default);
@@ -360,3 +338,8 @@ void PermissionSelectorRow::PermissionChanged(
   for (PermissionSelectorRowObserver& observer : observer_list_)
     observer.OnPermissionChanged(permission);
 }
+
+views::View* PermissionSelectorRow::button() {
+  return menu_button_ ? static_cast<views::View*>(menu_button_)

[sky, 2017/03/02 20:05:41]

Why do you need the cast here?

[Elly Fong-Jones, 2017/03/06 17:17:10]

The types of the arms of a ?: cannot be different types T1 and T2 even if both
T1 and T2 convert to T and the result of the eventual expression is a T. :(

+                      : static_cast<views::View*>(combobox_);
+}