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

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/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_view_state.h"
 #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"
 #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/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
 // 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 182 matching lines @@
 }
 
 }  // namespace internal
 
 ///////////////////////////////////////////////////////////////////////////////
 // PermissionSelectorRow
 ///////////////////////////////////////////////////////////////////////////////
 
 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,
       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);
+    InitializeComboboxView(wrapper_layout, permission);
   else
-    InitializeMenuButtonView(layout, permission);
+    InitializeMenuButtonView(wrapper_layout, permission);
 }
 
 void PermissionSelectorRow::AddObserver(
     PermissionSelectorRowObserver* observer) {
   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(
     views::GridLayout* layout,
     const WebsiteSettingsUI::PermissionInfo& permission) {
   bool button_enabled =
       permission.source == content_settings::SETTING_SOURCE_USER;
   menu_button_ = new internal::PermissionMenuButton(
       WebsiteSettingsUI::PermissionActionToUIString(
           permission.type, permission.setting, permission.default_setting,
@@ skipping 38 matching lines @@
     menu_button_->SizeToPreferredSize();
   } else if (combobox_) {
     bool use_default = permission.setting == CONTENT_SETTING_DEFAULT;
     combobox_->UpdateSelectedIndex(use_default);
   }
 
   FOR_EACH_OBSERVER(PermissionSelectorRowObserver,
                     observer_list_,
                     OnPermissionChanged(permission));
 }