views: align columns in site settings dialog

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

 // 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.
 
 #ifndef CHROME_BROWSER_UI_VIEWS_WEBSITE_SETTINGS_PERMISSION_SELECTOR_ROW_H_
 #define CHROME_BROWSER_UI_VIEWS_WEBSITE_SETTINGS_PERMISSION_SELECTOR_ROW_H_
 
 #include <memory>
 
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/observer_list.h"
 #include "base/strings/string16.h"
 #include "chrome/browser/ui/views/website_settings/permission_selector_row_observer.h"
 #include "chrome/browser/ui/website_settings/permission_menu_model.h"
 #include "chrome/browser/ui/website_settings/website_settings_ui.h"
 #include "components/content_settings/core/common/content_settings.h"
 #include "components/content_settings/core/common/content_settings_types.h"
 #include "ui/views/controls/button/menu_button_listener.h"
 #include "ui/views/view.h"
 
 class Profile;
 
 namespace internal {
 class ComboboxModelAdapter;
 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,
       const WebsiteSettingsUI::PermissionInfo& permission);
   void InitializeComboboxView(
       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_;
 
   DISALLOW_COPY_AND_ASSIGN(PermissionSelectorRow);
 };
 
 #endif  // CHROME_BROWSER_UI_VIEWS_WEBSITE_SETTINGS_PERMISSION_SELECTOR_ROW_H_