Fix PageInfoBubbleViewTest.SetPermissionInfo with SecondaryUiMd

Under MD, the changed setting is always read from the combobox selected
index when changed in the UI. PermissionSelectorRow::PermissionChanged()
ignores the argument, except to detect whether it is the default.

Also PermissionMenuModel gets strings using PageInfoUI::
PermissionActionToUIString(..) rather than directly from the
ResourceBundle.

Ensure the unit test acconts for these.

Test exposed a memory leak with SecondaryUiMd, which enables views::
Combobox for the selectors. Combobox needs to be deleted before its
model is destroyed. View::RemoveChildView doesn't delete its argument,
but simply calling delete view does end up calling RemoveChildView().

Bug: 713030, 780408
Change-Id: I373cf0d40316bb2fc42e76bb470d1977c0678e2b
Reviewed-on: https://chromium-review.googlesource.com/748861Reviewed-by: Lucas Garron <lgarron@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513315}

chrome/browser/ui/views/page_info/page_info_bubble_view_unittest.cc

@@ -6,6 +6,7 @@
 
 #include "base/macros.h"
 #include "base/strings/utf_string_conversions.h"
+#include "build/build_config.h"
 #include "chrome/browser/ui/exclusive_access/exclusive_access_manager.h"
 #include "chrome/browser/ui/views/harmony/chrome_layout_provider.h"
 #include "chrome/browser/ui/views/page_info/chosen_object_row.h"
@@ -22,6 +23,7 @@
 #include "device/usb/mock_usb_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/l10n/l10n_util.h"
+#include "ui/base/ui_base_features.h"
 #include "ui/events/event_utils.h"
 #include "ui/views/controls/button/menu_button.h"
 #include "ui/views/controls/combobox/combobox.h"
@@ -95,6 +97,13 @@ class PageInfoBubbleViewTestApi {
     }
   }
 
+  void SimulateUserSelectingComboboxItemAt(int selector_index, int menu_index) {
+    views::View* view = GetPermissionSelectorAt(selector_index)->button();
+    DCHECK_EQ(views::Combobox::kViewClassName, view->GetClassName());
+    views::Combobox* combobox = static_cast<views::Combobox*>(view);
+    combobox->SetSelectedIndex(menu_index);
+  }
+
   // Simulates updating the number of cookies.
   void SetCookieInfo(const CookieInfoList& list) { view_->SetCookieInfo(list); }
 
@@ -178,26 +187,24 @@ class PageInfoBubbleViewTest : public testing::Test {
 
 }  // namespace
 
-// TODO(ellyjones): re-enable this test for OSX.
-// This test exercises PermissionSelectorRow in a way that it is not used in
-// practice. In practice, every setting in PermissionSelectorRow starts off
-// "set", so there is always one option checked in the resulting MenuModel. This
-// test creates settings that are left at their defaults, leading to zero
-// checked options, and checks that the text on the MenuButtons is right. Since
-// the Comboboxes the MacViews version of this dialog uses don't have separate
-// text, this test doesn't work.
-#if defined(OS_MACOSX)
-#define MAYBE_SetPermissionInfo DISABLED_SetPermissionInfo
-#else
-#define MAYBE_SetPermissionInfo SetPermissionInfo
-#endif
-
 // Each permission selector row is like this: [icon] [label] [selector]
 constexpr int kViewsPerPermissionRow = 3;
 
 // Test UI construction and reconstruction via
 // PageInfoBubbleView::SetPermissionInfo().
-TEST_F(PageInfoBubbleViewTest, MAYBE_SetPermissionInfo) {
+TEST_F(PageInfoBubbleViewTest, SetPermissionInfo) {
+  // This test exercises PermissionSelectorRow in a way that it is not used in
+  // practice. In practice, every setting in PermissionSelectorRow starts off
+  // "set", so there is always one option checked in the resulting MenuModel.
+  // This test creates settings that are left at their defaults, leading to zero
+  // checked options, and checks that the text on the MenuButtons is right. On
+  // Mac, the behavior matches, but only when SecondaryUiMd is enabled.
+  const bool is_md = base::FeatureList::IsEnabled(features::kSecondaryUiMd);
+#if defined(OS_MACOSX)
+  if (!is_md)
+    return;
+#endif
+
   PermissionInfoList list(1);
   list.back().type = CONTENT_SETTINGS_TYPE_GEOLOCATION;
   list.back().source = content_settings::SETTING_SOURCE_USER;
@@ -227,16 +234,32 @@ TEST_F(PageInfoBubbleViewTest, MAYBE_SetPermissionInfo) {
 
   // Simulate a user selection via the UI. Note this will also cover logic in
   // PageInfo to update the pref.
-  list.back().setting = CONTENT_SETTING_ALLOW;
-  api_->GetPermissionSelectorAt(0)->PermissionChanged(list.back());
+  if (is_md) {
+    // Under MD, the changed setting is always read from the combobox selected
+    // index when changed in the UI. PermissionSelectorRow::PermissionChanged()
+    // ignores the argument, except to detect whether it is the default.
+    api_->SimulateUserSelectingComboboxItemAt(0, 1);
+  } else {
+    list.back().setting = CONTENT_SETTING_ALLOW;
+    api_->GetPermissionSelectorAt(0)->PermissionChanged(list.back());
+  }
   EXPECT_EQ(num_expected_children, api_->permissions_view()->child_count());
   EXPECT_EQ(base::ASCIIToUTF16("Allow"), api_->GetPermissionButtonTextAt(0));
 
   // Setting to the default via the UI should keep the button around.
-  list.back().setting = CONTENT_SETTING_ASK;
-  api_->GetPermissionSelectorAt(0)->PermissionChanged(list.back());
+  if (is_md) {
+    api_->SimulateUserSelectingComboboxItemAt(0, 0);
+    // Under MD, PermissionMenuModel gets strings using PageInfoUI::
+    // PermissionActionToUIString(..) rather than directly from the
+    // ResourceBundle.
+    EXPECT_EQ(base::ASCIIToUTF16("Ask (default)"),
+              api_->GetPermissionButtonTextAt(0));
+  } else {
+    list.back().setting = CONTENT_SETTING_ASK;
+    api_->GetPermissionSelectorAt(0)->PermissionChanged(list.back());
+    EXPECT_EQ(base::ASCIIToUTF16("Ask"), api_->GetPermissionButtonTextAt(0));
+  }
   EXPECT_EQ(num_expected_children, api_->permissions_view()->child_count());
-  EXPECT_EQ(base::ASCIIToUTF16("Ask"), api_->GetPermissionButtonTextAt(0));
 
   // However, since the setting is now default, recreating the dialog with those
   // settings should omit the permission from the UI.

chrome/browser/ui/views/page_info/permission_selector_row.cc

@@ -345,9 +345,7 @@ PermissionSelectorRow::~PermissionSelectorRow() {
   //
   // Technically, the MenuButton has the same problem, but MenuButton doesn't
   // use its model in its destructor.
-  if (combobox_) {
-    combobox_->parent()->RemoveChildView(combobox_);
-  }
+  delete combobox_;
 }
 
 // static