views: align columns in site settings dialog

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

Index: chrome/browser/ui/views/website_settings/permission_selector_row.h
diff --git a/chrome/browser/ui/views/website_settings/permission_selector_row.h b/chrome/browser/ui/views/website_settings/permission_selector_row.h
index 28310c39feca93582f9dc6f9e09b8f8ee3889036..716c53dbd1928a60a08095011fd0c2f1cc2e7ef1 100644
--- a/chrome/browser/ui/views/website_settings/permission_selector_row.h
+++ b/chrome/browser/ui/views/website_settings/permission_selector_row.h
@@ -27,30 +27,35 @@ class PermissionCombobox;
 class PermissionMenuButton;
 }
 
+namespace test {
+class WebsiteSettingsPopupViewTestApi;
+}
+
 namespace views {
 class GridLayout;
 class ImageView;
+class Label;
 }
 
 // A PermissionSelectorRow is a row in the Page Info bubble that shows a
 // permission that a site can have ambient access to, and allows the user to
-// control whether that access is granted.
-class PermissionSelectorRow : public views::View {
+// control whether that access is granted. A PermissionSelectorRow is not itself
+// a View, but creates some Views, which end up owned by the View hierarchy.
+class PermissionSelectorRow {
  public:
+  // The PermissionSelectorRow's constituent views are added to |container|.
   PermissionSelectorRow(Profile* profile,
                         const GURL& url,
-                        const WebsiteSettingsUI::PermissionInfo& permission);
+                        const WebsiteSettingsUI::PermissionInfo& permission,
+                        views::GridLayout* container);

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

The name container implies a view, where as |container| is really a GridLayout.
Maybe grid_layout, or layout which is consistent with other names in this calss.

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

Done.

+  virtual ~PermissionSelectorRow();
 
   void AddObserver(PermissionSelectorRowObserver* observer);
 
   void PermissionChanged(const WebsiteSettingsUI::PermissionInfo& permission);
 
- protected:
-  // Overridden from views::View.
-  void ChildPreferredSizeChanged(View* child) override;
-
  private:
-  ~PermissionSelectorRow() override;
+  friend class test::WebsiteSettingsPopupViewTestApi;
 
   void InitializeMenuButtonView(
       views::GridLayout* layout,
@@ -59,15 +64,19 @@ class PermissionSelectorRow : public views::View {
       views::GridLayout* layout,
       const WebsiteSettingsUI::PermissionInfo& permission);
 
+  views::View* button();

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

Add description of this, it isn't at all obvious that this is one of
menu_button_ or combobox_.

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

Done.

+
   Profile* profile_;
 
   // Model for the permission's menu.
   std::unique_ptr<PermissionMenuModel> menu_model_;
   std::unique_ptr<internal::ComboboxModelAdapter> combobox_model_adapter_;
 
-  views::ImageView* icon_;  // Owned by the views hierachy.
-  internal::PermissionMenuButton* menu_button_;  // Owned by the views hierachy.
-  internal::PermissionCombobox* combobox_;  // Owned by the views hierarchy.
+  // These are all owned by the views hierarchy:
+  views::ImageView* icon_;
+  views::Label* label_;
+  internal::PermissionMenuButton* menu_button_;
+  internal::PermissionCombobox* combobox_;
 
   base::ObserverList<PermissionSelectorRowObserver, false> observer_list_;