Remove the Allow/Block drop-down from grouped permission requests

chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc

Index: chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc
diff --git a/chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc b/chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc
index 483f7f7182bf3f9318daa2ee1acb16ed0177b672..60c8c60a1707d43c3c42f099e94d8cfb739fa15f 100644
--- a/chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc
+++ b/chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc
@@ -50,102 +50,14 @@ const int kIconSize = 18;
 
 }  // namespace
 
-// This class is a MenuButton which is given a PermissionMenuModel. It
-// shows the current checked item in the menu model, and notifies its listener
-// about any updates to the state of the selection.
-// TODO(gbillock): refactor PermissionMenuButton to work like this and re-use?
-class PermissionCombobox : public views::MenuButton,
-                           public views::MenuButtonListener {
- public:
-  // Get notifications when the selection changes.
-  class Listener {
-   public:
-    virtual void PermissionSelectionChanged(int index, bool allowed) = 0;
-  };
-
-  PermissionCombobox(Profile* profile,
-                     Listener* listener,
-                     int index,
-                     const GURL& url,
-                     ContentSetting setting);
-  ~PermissionCombobox() override;
-
-  int index() const { return index_; }
-
-  void GetAccessibleNodeData(ui::AXNodeData* node_data) override;
-
-  // MenuButtonListener:
-  void OnMenuButtonClicked(views::MenuButton* source,
-                           const gfx::Point& point,
-                           const ui::Event* event) override;
-
-  // Callback when a permission's setting is changed.
-  void PermissionChanged(const PageInfoUI::PermissionInfo& permission);
-
- private:
-  int index_;
-  Listener* listener_;
-  std::unique_ptr<PermissionMenuModel> model_;
-  std::unique_ptr<views::MenuRunner> menu_runner_;
-};
-
-PermissionCombobox::PermissionCombobox(Profile* profile,
-                                       Listener* listener,
-                                       int index,
-                                       const GURL& url,
-                                       ContentSetting setting)
-    : MenuButton(base::string16(), this, true),
-      index_(index),
-      listener_(listener),
-      model_(new PermissionMenuModel(
-          profile,
-          url,
-          setting,
-          base::Bind(&PermissionCombobox::PermissionChanged,
-                     base::Unretained(this)))) {
-  SetText(model_->GetLabelAt(model_->GetIndexOfCommandId(setting)));
-  SizeToPreferredSize();
-}
-
-PermissionCombobox::~PermissionCombobox() {}
-
-void PermissionCombobox::GetAccessibleNodeData(ui::AXNodeData* node_data) {
-  MenuButton::GetAccessibleNodeData(node_data);
-  node_data->SetValue(GetText());
-}
-
-void PermissionCombobox::OnMenuButtonClicked(views::MenuButton* source,
-                                             const gfx::Point& point,
-                                             const ui::Event* event) {
-  menu_runner_.reset(
-      new views::MenuRunner(model_.get(), views::MenuRunner::HAS_MNEMONICS));
-
-  gfx::Point p(point);
-  p.Offset(-source->width(), 0);
-  menu_runner_->RunMenuAt(source->GetWidget()->GetTopLevelWidget(), this,
-                          gfx::Rect(p, gfx::Size()), views::MENU_ANCHOR_TOPLEFT,
-                          ui::MENU_SOURCE_NONE);
-}
-
-void PermissionCombobox::PermissionChanged(
-    const PageInfoUI::PermissionInfo& permission) {
-  SetText(model_->GetLabelAt(model_->GetIndexOfCommandId(permission.setting)));
-  SizeToPreferredSize();
-
-  listener_->PermissionSelectionChanged(
-      index_, permission.setting == CONTENT_SETTING_ALLOW);
-}
-
 ///////////////////////////////////////////////////////////////////////////////
 // View implementation for the permissions bubble.
 class PermissionsBubbleDialogDelegateView
-    : public views::BubbleDialogDelegateView,
-      public PermissionCombobox::Listener {
+    : public views::BubbleDialogDelegateView {
  public:
   PermissionsBubbleDialogDelegateView(
       PermissionPromptImpl* owner,
-      const std::vector<PermissionRequest*>& requests,
-      const std::vector<bool>& accept_state);
+      const std::vector<PermissionRequest*>& requests);
   ~PermissionsBubbleDialogDelegateView() override;
 
   void CloseBubble();
@@ -164,9 +76,6 @@ class PermissionsBubbleDialogDelegateView
   int GetDialogButtons() const override;
   base::string16 GetDialogButtonLabel(ui::DialogButton button) const override;
 
-  // PermissionCombobox::Listener:
-  void PermissionSelectionChanged(int index, bool allowed) override;
-
   // Updates the anchor's arrow and view. Also repositions the bubble so it's
   // displayed in the correct location.
   void UpdateAnchor(views::View* anchor_view,
@@ -175,10 +84,8 @@ class PermissionsBubbleDialogDelegateView
 
  private:
   PermissionPromptImpl* owner_;
-  bool multiple_requests_;
   base::string16 display_origin_;
   std::unique_ptr<PermissionMenuModel> menu_button_model_;

[msw, 2017/06/05 20:22:20]

nit: remove this?

[raymes, 2017/06/05 22:46:28]

Done.

-  std::vector<PermissionCombobox*> customize_comboboxes_;
   views::Checkbox* persist_checkbox_;
 
   DISALLOW_COPY_AND_ASSIGN(PermissionsBubbleDialogDelegateView);
@@ -186,11 +93,8 @@ class PermissionsBubbleDialogDelegateView
 
 PermissionsBubbleDialogDelegateView::PermissionsBubbleDialogDelegateView(
     PermissionPromptImpl* owner,
-    const std::vector<PermissionRequest*>& requests,
-    const std::vector<bool>& accept_state)
-    : owner_(owner),
-      multiple_requests_(requests.size() > 1),
-      persist_checkbox_(nullptr) {
+    const std::vector<PermissionRequest*>& requests)

[msw, 2017/06/05 20:22:19]

q: does it make sense to change this to just one request or update the DCHECK
below to ensure there's exactly one item? (or are there still multiple requests
in the mic+camera case?)

[raymes, 2017/06/05 22:46:28]

There can still be multiple requests for the mic+camera case (2). 

+    : owner_(owner), persist_checkbox_(nullptr) {
   DCHECK(!requests.empty());
 
   set_close_on_deactivate(false);
@@ -206,7 +110,6 @@ PermissionsBubbleDialogDelegateView::PermissionsBubbleDialogDelegateView(
 
   bool show_persistence_toggle = true;
   for (size_t index = 0; index < requests.size(); index++) {
-    DCHECK(index < accept_state.size());
     // The row is laid out containing a leading-aligned label area and a
     // trailing column which will be filled if there are multiple permission
     // requests.
@@ -241,15 +144,7 @@ PermissionsBubbleDialogDelegateView::PermissionsBubbleDialogDelegateView(
     // Only show the toggle if every request wants to show it.
     show_persistence_toggle = show_persistence_toggle &&
                               requests[index]->ShouldShowPersistenceToggle();
-    if (requests.size() > 1) {
-      PermissionCombobox* combobox = new PermissionCombobox(
-          owner->GetProfile(), this, index, requests[index]->GetOrigin(),
-          accept_state[index] ? CONTENT_SETTING_ALLOW : CONTENT_SETTING_BLOCK);
-      row_layout->AddView(combobox);
-      customize_comboboxes_.push_back(combobox);
-    } else {
-      row_layout->AddView(new views::View());
-    }
+    row_layout->AddView(new views::View());

[msw, 2017/06/05 20:22:19]

What's this doing? Should we be just skipping/merging a column, or updating the
layout to remove an unused column?

[raymes, 2017/06/05 22:46:28]

Sorry I'm not a views expert by any means! That makes sense though. I removed
the column and it still seems to work properly. 

 
     AddChildView(row);
   }
@@ -316,10 +211,7 @@ int PermissionsBubbleDialogDelegateView::GetDefaultDialogButton() const {
 }
 
 int PermissionsBubbleDialogDelegateView::GetDialogButtons() const {
-  int buttons = ui::DIALOG_BUTTON_OK;
-  if (!multiple_requests_)
-    buttons |= ui::DIALOG_BUTTON_CANCEL;
-  return buttons;
+  return ui::DIALOG_BUTTON_OK | ui::DIALOG_BUTTON_CANCEL;
 }
 
 base::string16 PermissionsBubbleDialogDelegateView::GetDialogButtonLabel(
@@ -354,12 +246,6 @@ bool PermissionsBubbleDialogDelegateView::Close() {
   return true;
 }
 
-void PermissionsBubbleDialogDelegateView::PermissionSelectionChanged(
-    int index,
-    bool allowed) {
-  owner_->ToggleAccept(index, allowed);
-}
-
 void PermissionsBubbleDialogDelegateView::UpdateAnchor(
     views::View* anchor_view,
     const gfx::Point& anchor_point,
@@ -403,8 +289,8 @@ void PermissionPromptImpl::Show() {
   if (bubble_delegate_)
     bubble_delegate_->CloseBubble();
 
-  bubble_delegate_ = new PermissionsBubbleDialogDelegateView(
-      this, delegate_->Requests(), delegate_->AcceptStates());
+  bubble_delegate_ =
+      new PermissionsBubbleDialogDelegateView(this, delegate_->Requests());
 
   // Set |parent_window| because some valid anchors can become hidden.
   bubble_delegate_->set_parent_window(