Material Page Info (Views, 3/3): Update site settings section.

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 780e1f2c28b2ce3778ce720ab000e8d0dbe14317..6695d7c4a61b4ca089aac5e42211b2bf1b14339c 100644
--- a/chrome/browser/ui/views/website_settings/permission_selector_row.cc
+++ b/chrome/browser/ui/views/website_settings/permission_selector_row.cc
@@ -15,6 +15,8 @@
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/material_design/material_design_controller.h"
 #include "ui/base/models/combobox_model.h"
+#include "ui/base/resource/resource_bundle.h"
+#include "ui/gfx/font_list.h"
 #include "ui/gfx/image/image.h"
 #include "ui/views/controls/button/menu_button.h"
 #include "ui/views/controls/combobox/combobox.h"
@@ -22,10 +24,16 @@
 #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/box_layout.h"

[msw, 2016/09/26 20:47:55]

Remove; not used.

[lgarron, 2016/09/28 21:11:46]

Done.

 #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.

[msw, 2016/09/26 20:47:55]

nit: '... in DIPs.' or similar.

[lgarron, 2016/09/28 21:11:46]

Aren't all layout constants using the same scale?

[msw, 2016/09/30 00:37:04]

Yes, most Views code uses DIPs, so specifying isn't needed.

+int kMinSeparationBetweenLabelAndMenu = 16;
+}
+
 namespace internal {
 
 // The |PermissionMenuButton| provides a menu for selecting a setting a
@@ -228,51 +236,40 @@ void PermissionCombobox::OnPerformAction(Combobox* combobox) {
 
 PermissionSelectorRow::PermissionSelectorRow(
     const GURL& url,
-    const WebsiteSettingsUI::PermissionInfo& permission)
-    : 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,
-                        1,
-                        views::GridLayout::FIXED,
-                        kPermissionIconColumnWidth,
-                        0);
-  column_set->AddPaddingColumn(0, kPermissionIconMarginLeft);
-  column_set->AddColumn(views::GridLayout::FILL,
-                        views::GridLayout::FILL,
-                        1,
-                        views::GridLayout::USE_PREF,
-                        0,
-                        0);
-  column_set->AddColumn(views::GridLayout::FILL,
-                        views::GridLayout::FILL,
-                        1,
-                        views::GridLayout::USE_PREF,
-                        0,
-                        0);
-
-  layout->StartRow(1, column_set_id);
-  // Create the permission icon.
+    const WebsiteSettingsUI::PermissionInfo& permission,
+    views::GridLayout* layout)
+    : icon_(NULL),

[msw, 2016/09/26 20:47:55]

nit: nullptr here and elsewhere

[lgarron, 2016/09/28 21:11:46]

Done.

+      menu_button_(NULL),
+      combobox_(NULL) {  // Create the permission icon.
   icon_ = new views::ImageView();
   const gfx::Image& image = WebsiteSettingsUI::GetPermissionIcon(permission);
   icon_->SetImage(image.ToImageSkia());
-  layout->AddView(icon_,
-                  1,
-                  1,
-                  views::GridLayout::CENTER,
-                  views::GridLayout::CENTER);
+  layout->AddView(icon_);
+
+  // Create a wrapper to hold the label and the menu.
+  views::View* wrapper = new views::View();

[msw, 2016/09/26 20:47:55]

Ditto: it'd be nice to avoid this view, but not necessary, I suppose.

[lgarron, 2016/09/28 21:11:46]

I tried hard to avoid this, but it seems to be necessary.

Each row needs to contain:
1. An icon.
2. A left-aligned label.
3. A right-aligned menu dropdown.

1. should be shared for all rows, but the location of the horizontal padding
between 2. and 3. needs to be unique to each row (see the Flash row at [1]).

I only know of two practical solutions:
- Use a separate column set for each row.
- This.

I *think* this implementation is the most reasonable.

[1] https://crbug.com/512442#c87

[msw, 2016/09/30 00:37:04]

Ah, so all rows would share the same width for a given column if they use the
same column set... I guess this is fine. Could you instead make this wrapper
just use BoxLayout?

[lgarron, 2016/09/30 05:05:55]

BoxLayout doesn't support MAIN_AXIS_ALIGNMENT_JUSTIFY [1]. :-(
I spent some time trying to see if I could implement it, but I decided it wasn't
worth my time if GridLayout already works.

https://cs.chromium.org/chromium/src/ui/views/layout/box_layout.h?q=boxlayout...

+  views::GridLayout* wrapper_layout = new views::GridLayout(wrapper);
+  wrapper->SetLayoutManager(wrapper_layout);
+  layout->AddView(wrapper);
+
+  const int column_set_id = 0;
+  views::ColumnSet* column_set = wrapper_layout->AddColumnSet(column_set_id);
+  column_set->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 0,
+                        views::GridLayout::USE_PREF, 0, 0);
+  // We set the resize value to 1 to absorb all extra space.
+  // This keep the subviews aligned at their respective ends.
+  column_set->AddPaddingColumn(1, kMinSeparationBetweenLabelAndMenu);
+  column_set->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 0,
+                        views::GridLayout::USE_PREF, 0, 0);
+  wrapper_layout->StartRow(1, column_set_id);
+
   // Create the label that displays the permission type.
-  views::Label* label = new views::Label(l10n_util::GetStringFUTF16(
-      IDS_WEBSITE_SETTINGS_PERMISSION_TYPE,

[msw, 2016/09/26 20:47:55]

Remove string resources if they are no longer used.

[lgarron, 2016/09/28 21:11:46]

*woo, another one finally bites the dust*

Done.

-      WebsiteSettingsUI::PermissionTypeToUIString(permission.type)));
-  layout->AddView(label,
-                  1,
-                  1,
-                  views::GridLayout::LEADING,
-                  views::GridLayout::CENTER);
+  const gfx::FontList& font_list =
+      ui::ResourceBundle::GetSharedInstance().GetFontListWithDelta(1);

[msw, 2016/09/26 20:47:55]

Ditto: consider adding constants for text sizes.

+  views::Label* label = new views::Label(
+      WebsiteSettingsUI::PermissionTypeToUIString(permission.type), font_list);
+  wrapper_layout->AddView(label);
+
   // Create the menu model.
   menu_model_.reset(new PermissionMenuModel(
       url,
@@ -288,9 +285,9 @@ PermissionSelectorRow::PermissionSelectorRow(
       ui::MaterialDesignController::IsSecondaryUiMaterial();
 #endif
   if (use_real_combobox)
-    InitializeComboboxView(layout, permission);
+    InitializeComboboxView(wrapper_layout, permission);
   else
-    InitializeMenuButtonView(layout, permission);
+    InitializeMenuButtonView(wrapper_layout, permission);
 }
 
 void PermissionSelectorRow::AddObserver(
@@ -298,28 +295,7 @@ void PermissionSelectorRow::AddObserver(
   observer_list_.AddObserver(observer);
 }
 
-void PermissionSelectorRow::ChildPreferredSizeChanged(View* child) {
-  SizeToPreferredSize();
-  // FIXME: The parent is only a plain |View| that is used as a
-  // container/box/panel. The SizeToPreferredSize method of the parent is
-  // called here directly in order not to implement a custom |View| class with
-  // its own implementation of the ChildPreferredSizeChanged method.
-  parent()->SizeToPreferredSize();
-}
-
 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_);
 }
 
 void PermissionSelectorRow::InitializeMenuButtonView(