views: align columns in site settings dialog

chrome/browser/ui/views/website_settings/permission_selector_row.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_row.h"
 
 #include "base/i18n/rtl.h"
 #include "base/macros.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/ui/views/website_settings/non_accessible_image_view.h"
 #include "chrome/browser/ui/views/website_settings/website_settings_popup_view.h"
 #include "chrome/browser/ui/website_settings/permission_menu_model.h"
 #include "chrome/browser/ui/website_settings/website_settings_ui.h"
 #include "chrome/grit/generated_resources.h"
 #include "ui/accessibility/ax_node_data.h"
 #include "ui/base/material_design/material_design_controller.h"
 #include "ui/base/models/combobox_model.h"
 #include "ui/gfx/image/image.h"
 #include "ui/views/controls/button/menu_button.h"
 #include "ui/views/controls/combobox/combobox.h"
 #include "ui/views/controls/combobox/combobox_listener.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/menu/menu_runner.h"
 #include "ui/views/layout/grid_layout.h"
 #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
 // permissions type.
 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|. If the |show_menu_marker| flag is true, then a
@@ skipping 23 matching lines @@
 };
 
 ///////////////////////////////////////////////////////////////////////////////
 // PermissionMenuButton
 ///////////////////////////////////////////////////////////////////////////////
 
 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
   // be available, and the button would get native GTK styling on Linux.
   UpdateThemedBorder();
 
   SetFocusForPlatform();
   set_request_focus_on_press(true);
   is_rtl_display_ =
       base::i18n::RIGHT_TO_LEFT == base::i18n::GetStringDirection(text);
@@ skipping 138 matching lines @@
 
 }  // namespace internal
 
 ///////////////////////////////////////////////////////////////////////////////
 // PermissionSelectorRow
 ///////////////////////////////////////////////////////////////////////////////
 
 PermissionSelectorRow::PermissionSelectorRow(
     Profile* profile,
     const GURL& url,
-    const WebsiteSettingsUI::PermissionInfo& permission)
+    const WebsiteSettingsUI::PermissionInfo& permission,
+    views::GridLayout* layout)
     : 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,
+  layout->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,
+  layout->AddView(label_, 1, 1, views::GridLayout::LEADING,
                   views::GridLayout::CENTER);
   // Create the menu model.
   menu_model_.reset(new PermissionMenuModel(
       profile, url, permission,
       base::Bind(&PermissionSelectorRow::PermissionChanged,
                  base::Unretained(this))));
 
   // Create the permission menu button.
 #if defined(OS_MACOSX)
   bool use_real_combobox = true;
 #else
   bool use_real_combobox =
       ui::MaterialDesignController::IsSecondaryUiMaterial();
 #endif
   if (use_real_combobox)
     InitializeComboboxView(layout, permission);
   else
     InitializeMenuButtonView(layout, permission);
 }
 
 void PermissionSelectorRow::AddObserver(
     PermissionSelectorRowObserver* observer) {
   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
   // destroyed in ~View(), which runs *after* ~PermissionSelectorRow() is done,
   // which means the Combobox gets destroyed after its ComboboxModel, which
   // causes an explosion when the Combobox attempts to stop observing the
   // ComboboxModel. This hack ensures the Combobox is deleted before its
   // ComboboxModel.
   //
   // 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(
     views::GridLayout* layout,
     const WebsiteSettingsUI::PermissionInfo& permission) {
   bool button_enabled =
       permission.source == content_settings::SETTING_SOURCE_USER;
   menu_button_ = new internal::PermissionMenuButton(
       WebsiteSettingsUI::PermissionActionToUIString(
           profile_, permission.type, permission.setting,
@@ skipping 25 matching lines @@
   // Change the permission icon to reflect the selected setting.
   const gfx::Image& image = WebsiteSettingsUI::GetPermissionIcon(permission);
   icon_->SetImage(image.ToImageSkia());
 
   // Update the menu button text to reflect the new setting.
   if (menu_button_) {
     menu_button_->SetText(WebsiteSettingsUI::PermissionActionToUIString(
         profile_, permission.type, permission.setting,
         permission.default_setting, content_settings::SETTING_SOURCE_USER));
     menu_button_->SizeToPreferredSize();
+    // Re-layout will be done at the WebsiteSettingsPopupView level, since the
+    // WebsiteSettingsPopupView may need to resize itself to accomodate the new

[palmer, 2017/03/06 19:16:10]

Nit: "...the |WebsiteSettingsPopupView| level, since that view may..."

[Elly Fong-Jones, 2017/03/07 18:28:48]

Done.

+    // sizes of its contents.
+    menu_button_->InvalidateLayout();
   } else if (combobox_) {
     bool use_default = permission.setting == CONTENT_SETTING_DEFAULT;
     combobox_->UpdateSelectedIndex(use_default);
   }
 
   for (PermissionSelectorRowObserver& observer : observer_list_)
     observer.OnPermissionChanged(permission);
 }
+
+views::View* PermissionSelectorRow::button() {
+  return menu_button_ ? static_cast<views::View*>(menu_button_)
+                      : static_cast<views::View*>(combobox_);
+}