PermissionSelectorView: use Combobox on MacViews builds.

chrome/browser/ui/views/website_settings/permission_selector_view.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_view.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/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 internal {
 
 // The |PermissionMenuButton| provides a menu for selecting a setting a
@@ skipping 77 matching lines @@
   if (menu_runner_->RunMenuAt(source->GetWidget()->GetTopLevelWidget(),
                               this,
                               gfx::Rect(p, gfx::Size()),
                               views::MENU_ANCHOR_TOPLEFT,
                               ui::MENU_SOURCE_NONE) ==
       views::MenuRunner::MENU_DELETED) {
     return;
   }
 }
 
+// This class adapts a PermissionMenuModel into a ui::ComboboxModel so that

[palmer, 2016/05/25 18:34:38]

Nit: Demarcate identifiers in comments with |foo|.

[Elly Fong-Jones, 2016/05/26 13:40:38]

Done.

+// PermissionCombobox can use it.
+class ComboboxModelAdapter : public ui::ComboboxModel {
+ public:
+  explicit ComboboxModelAdapter(PermissionMenuModel* model) : model_(model) {}
+  ~ComboboxModelAdapter() override {}
+
+  void OnPerformAction(int index);
+
+  // Returns the checked index of the underlying PermissionMenuModel, of which
+  // there must be exactly one. This is used to choose which index is selected
+  // in the PermissionCombobox.
+  int GetCheckedIndex();
+
+  // ui::ComboboxModel:
+  int GetItemCount() const override;
+  base::string16 GetItemAt(int index) override;

[palmer, 2016/05/25 18:34:38]

Nit: Since this is not a collection of string16s, I wouldn't call them the
"items". Since it's a convenience wrapper/way to expose model_->GetLabelAt, I'd
call this |GetLabelAt|, too.

[Elly Fong-Jones, 2016/05/26 13:40:38]

Unfortunately, these two names are defined in ui::ComboboxModel :(

+
+ private:
+  PermissionMenuModel* model_;
+};
+
+void ComboboxModelAdapter::OnPerformAction(int index) {
+  model_->ExecuteCommand(index, 0);
+}
+
+int ComboboxModelAdapter::GetCheckedIndex() {
+  int checked_index = -1;
+  for (int i = 0; i < model_->GetItemCount(); i++) {

[palmer, 2016/05/25 18:34:39]

Nit: I think the Chromium style calls for ++i.

[Elly Fong-Jones, 2016/05/26 13:40:38]

Done.

+    if (model_->IsCommandIdChecked(i)) {
+      DCHECK_EQ(checked_index, -1);
+      checked_index = i;

[palmer, 2016/05/25 18:34:38]

I think you can early-return here instead:

    return i;

and then return -1 at the bottom. And then you don't need (what is currently)
line 145.

[Elly Fong-Jones, 2016/05/26 13:40:38]

I avoided early return so that I could DCHECK that there's only one checked
item, which isn't something the PermissionMenuModel normally guarantees. Do you
think it's better to leave it as is with the DCHECK, or pull the DCHECK and
early return here?

[palmer, 2016/05/26 19:10:52]

Ahh, I see. Maybe add a comment to that effect.
Leave it as-is, I'd say.

+    }
+  }
+  return checked_index;
+}
+
+int ComboboxModelAdapter::GetItemCount() const {
+  DCHECK(model_);
+  return model_->GetItemCount();
+}
+
+base::string16 ComboboxModelAdapter::GetItemAt(int index) {
+  return model_->GetLabelAt(index);
+}
+
+// The |PermissionCombobox| provides a combobox for selecting a permission type.
+// This is only used on platforms where the permission dialog uses a combobox
+// instead of a MenuButton (currently, Mac).
+class PermissionCombobox : public views::Combobox,
+                           public views::ComboboxListener {
+ public:
+  PermissionCombobox(const base::string16& text,
+                     ComboboxModelAdapter* model,
+                     bool enabled);
+  ~PermissionCombobox() override;
+
+  void UpdateSelectedIndex();
+
+ private:
+  // views::ComboboxListener:
+  void OnPerformAction(Combobox* combobox) override;
+
+  ComboboxModelAdapter* model_;
+};
+
+PermissionCombobox::PermissionCombobox(const base::string16& text,
+                                       ComboboxModelAdapter* model,
+                                       bool enabled)
+    : views::Combobox(model), model_(model) {
+  set_listener(this);
+  SetEnabled(enabled);
+  UpdateSelectedIndex();
+}
+
+PermissionCombobox::~PermissionCombobox() {}
+
+void PermissionCombobox::UpdateSelectedIndex() {
+  int index = model_->GetCheckedIndex();
+  DCHECK_NE(index, -1);
+  SetSelectedIndex(index);
+}
+
+void PermissionCombobox::OnPerformAction(Combobox* combobox) {
+  model_->OnPerformAction(combobox->selected_index());
+}
+
 }  // namespace internal
 
 ///////////////////////////////////////////////////////////////////////////////
 // PermissionSelectorView
 ///////////////////////////////////////////////////////////////////////////////
 
 PermissionSelectorView::PermissionSelectorView(
     const GURL& url,
     const WebsiteSettingsUI::PermissionInfo& permission)
-    : icon_(NULL), menu_button_(NULL) {
+    : 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);
@@ skipping 30 matching lines @@
                   1,
                   views::GridLayout::LEADING,
                   views::GridLayout::CENTER);
   // Create the menu model.
   menu_model_.reset(new PermissionMenuModel(
       url,
       permission,
       base::Bind(&PermissionSelectorView::PermissionChanged,
                  base::Unretained(this))));
   // Create the permission menu button.
-  bool button_enabled =
-      permission.source == content_settings::SETTING_SOURCE_USER;
-  menu_button_ = new internal::PermissionMenuButton(
-      WebsiteSettingsUI::PermissionActionToUIString(
-          permission.type, permission.setting, permission.default_setting,
-          permission.source),
-      menu_model_.get(), button_enabled);
-  menu_button_->SetEnabled(button_enabled);
-  menu_button_->SetAccessibleName(
-      WebsiteSettingsUI::PermissionTypeToUIString(permission.type));
-  layout->AddView(menu_button_);
+#if defined(OS_MACOSX)
+  InitializeComboboxView(layout, permission);
+#else
+  InitializeMenuButtonView(layout, permission);
+#endif
 }
 
 void PermissionSelectorView::AddObserver(
     PermissionSelectorViewObserver* observer) {
   observer_list_.AddObserver(observer);
 }
 
 void PermissionSelectorView::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();
 }
 
 PermissionSelectorView::~PermissionSelectorView() {
+  // 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* ~PermissionSelectorView() 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

[palmer, 2016/05/25 18:34:38]

Nit: Blank line before a new paragraph.

[Elly Fong-Jones, 2016/05/26 13:40:38]

Done.

+  // use its model in its destructor.
+  if (combobox_)
+    RemoveChildView(combobox_);
+}
+
+void PermissionSelectorView::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,
+          permission.source),
+      menu_model_.get(), button_enabled);
+  menu_button_->SetEnabled(button_enabled);
+  menu_button_->SetAccessibleName(
+      WebsiteSettingsUI::PermissionTypeToUIString(permission.type));
+  layout->AddView(menu_button_);
+}
+
+void PermissionSelectorView::InitializeComboboxView(
+    views::GridLayout* layout,
+    const WebsiteSettingsUI::PermissionInfo& permission) {
+  bool button_enabled =
+      permission.source == content_settings::SETTING_SOURCE_USER;
+  combobox_model_adaptor_.reset(
+      new internal::ComboboxModelAdapter(menu_model_.get()));
+  combobox_ = new internal::PermissionCombobox(
+      WebsiteSettingsUI::PermissionActionToUIString(
+          permission.type, permission.setting, permission.default_setting,
+          permission.source),
+      combobox_model_adaptor_.get(), button_enabled);
+  combobox_->SetEnabled(button_enabled);
+  combobox_->SetAccessibleName(
+      WebsiteSettingsUI::PermissionTypeToUIString(permission.type));
+  layout->AddView(combobox_);
 }
 
 void PermissionSelectorView::PermissionChanged(
     const WebsiteSettingsUI::PermissionInfo& permission) {
   // 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.
-  menu_button_->SetText(WebsiteSettingsUI::PermissionActionToUIString(
-      permission.type, permission.setting, permission.default_setting,
-      content_settings::SETTING_SOURCE_USER));
-  menu_button_->SizeToPreferredSize();
+  if (menu_button_) {
+    menu_button_->SetText(WebsiteSettingsUI::PermissionActionToUIString(
+        permission.type, permission.setting, permission.default_setting,
+        content_settings::SETTING_SOURCE_USER));
+    menu_button_->SizeToPreferredSize();
+  } else if (combobox_) {
+    combobox_->UpdateSelectedIndex();
+  }
 
   FOR_EACH_OBSERVER(PermissionSelectorViewObserver,
                     observer_list_,
                     OnPermissionChanged(permission));
 }