views: align columns in site settings dialog

chrome/browser/ui/views/website_settings/website_settings_popup_view_unittest.cc

 // Copyright 2015 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/website_settings_popup_view.h"
 
 #include "base/macros.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/ui/exclusive_access/exclusive_access_manager.h"
 #include "chrome/browser/ui/views/website_settings/chosen_object_row.h"
@@ skipping 38 matching lines @@
     views::View* anchor_view = nullptr;
     view_ =
         new WebsiteSettingsPopupView(anchor_view, parent_, profile_,
                                      web_contents_, GURL(kUrl), security_info);
   }
 
   WebsiteSettingsPopupView* view() { return view_; }
   views::View* permissions_view() { return view_->permissions_view_; }
 
   PermissionSelectorRow* GetPermissionSelectorAt(int index) {
     return static_cast<PermissionSelectorRow*>(

[sky, 2017/03/07 22:03:40]

Do you need this cast?

[Elly Fong-Jones, 2017/03/08 18:41:09]

Nope, deleted.

-        permissions_view()->child_at(index));
+        view_->selector_rows_.at(index).get());

[sky, 2017/03/07 22:03:40]

.at(index) -> [index] ?

[Elly Fong-Jones, 2017/03/08 18:41:09]

Done.

+  }
+
+  base::string16 GetPermissionLabelTextAt(int index) {
+    views::View* view = GetPermissionSelectorAt(index)->label_;
+    if (view->GetClassName() == views::Label::kViewClassName)

[sky, 2017/03/07 22:03:40]

This is subtle and worth a comment.

[Elly Fong-Jones, 2017/03/08 18:41:09]

Done.

+      return static_cast<views::Label*>(view)->text();
+    return base::string16();
   }
 
   base::string16 GetPermissionButtonTextAt(int index) {
-    const int kButtonIndex = 2;  // Button should be the third child.
-    views::View* view = GetPermissionSelectorAt(index)->child_at(kButtonIndex);
+    views::View* view = GetPermissionSelectorAt(index)->button();
     if (view->GetClassName() == views::MenuButton::kViewClassName) {
       return static_cast<views::MenuButton*>(view)->GetText();
     } else if (view->GetClassName() == views::Combobox::kViewClassName) {
       views::Combobox* combobox = static_cast<views::Combobox*>(view);
       return combobox->GetTextForRow(combobox->GetSelectedRow());
     } else {
       NOTREACHED() << "Unknown class " << view->GetClassName();
       return base::string16();
     }
   }
@@ skipping 92 matching lines @@
 // Test UI construction and reconstruction via
 // WebsiteSettingsPopupView::SetPermissionInfo().
 TEST_F(WebsiteSettingsPopupViewTest, MAYBE_SetPermissionInfo) {
   PermissionInfoList list(1);
   list.back().type = CONTENT_SETTINGS_TYPE_GEOLOCATION;
   list.back().source = content_settings::SETTING_SOURCE_USER;
   list.back().is_incognito = false;
   list.back().setting = CONTENT_SETTING_DEFAULT;
 
   const int kExpectedChildren =
-      ExclusiveAccessManager::IsSimplifiedFullscreenUIEnabled() ? 11 : 13;
+      3 * (ExclusiveAccessManager::IsSimplifiedFullscreenUIEnabled() ? 11 : 13);

[sky, 2017/03/07 22:03:40]

The 3 is subtle and worth a comment. I suggest a constant that is shared with
the 3 below that has the comment.

[Elly Fong-Jones, 2017/03/08 18:41:09]

Done.

   EXPECT_EQ(kExpectedChildren, api_->permissions_view()->child_count());
 
   list.back().setting = CONTENT_SETTING_ALLOW;
   api_->SetPermissionInfo(list);
   EXPECT_EQ(kExpectedChildren, api_->permissions_view()->child_count());
 
   PermissionSelectorRow* selector = api_->GetPermissionSelectorAt(0);
-  EXPECT_EQ(3, selector->child_count());
+  EXPECT_NE(nullptr, selector);

[sky, 2017/03/07 22:03:40]

EXPECT(selector)?

[Elly Fong-Jones, 2017/03/08 18:41:09]

Done.

 
   // Verify labels match the settings on the PermissionInfoList.
-  const int kLabelIndex = 1;
-  EXPECT_EQ(views::Label::kViewClassName,
-            selector->child_at(kLabelIndex)->GetClassName());
-  views::Label* label =
-      static_cast<views::Label*>(selector->child_at(kLabelIndex));
-  EXPECT_EQ(base::ASCIIToUTF16("Location"), label->text());
+  EXPECT_EQ(base::ASCIIToUTF16("Location"), api_->GetPermissionLabelTextAt(0));
   EXPECT_EQ(base::ASCIIToUTF16("Allow"), api_->GetPermissionButtonTextAt(0));
 
   // Verify calling SetPermisisonInfo() directly updates the UI.
   list.back().setting = CONTENT_SETTING_BLOCK;
   api_->SetPermissionInfo(list);
   EXPECT_EQ(base::ASCIIToUTF16("Block"), api_->GetPermissionButtonTextAt(0));
 
   // Simulate a user selection via the UI. Note this will also cover logic in
   // WebsiteSettings to update the pref.
   list.back().setting = CONTENT_SETTING_ALLOW;
   api_->GetPermissionSelectorAt(0)->PermissionChanged(list.back());
   EXPECT_EQ(kExpectedChildren, 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());
   EXPECT_EQ(kExpectedChildren, 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.
   api_->SetPermissionInfo(list);
   EXPECT_EQ(kExpectedChildren, api_->permissions_view()->child_count());
 }
 
 // Test UI construction and reconstruction with USB devices.
 TEST_F(WebsiteSettingsPopupViewTest, SetPermissionInfoWithUsbDevice) {
   const int kExpectedChildren =
-      ExclusiveAccessManager::IsSimplifiedFullscreenUIEnabled() ? 11 : 13;
+      3 * (ExclusiveAccessManager::IsSimplifiedFullscreenUIEnabled() ? 11 : 13);
   EXPECT_EQ(kExpectedChildren, api_->permissions_view()->child_count());
 
   const GURL origin = GURL(kUrl).GetOrigin();
   scoped_refptr<device::UsbDevice> device =
       new device::MockUsbDevice(0, 0, "Google", "Gizmo", "1234567890");
   device_client_.usb_service()->AddDevice(device);
   UsbChooserContext* store =
       UsbChooserContextFactory::GetForProfile(web_contents_helper_.profile());
   store->GrantDevicePermission(origin, origin, device->guid());
 
@@ skipping 16 matching lines @@
 
   const ui::MouseEvent event(ui::ET_MOUSE_PRESSED, gfx::Point(), gfx::Point(),
                              ui::EventTimeForNow(), 0, 0);
   views::ButtonListener* button_listener =
       static_cast<views::ButtonListener*>(object_view);
   button_listener->ButtonPressed(button, event);
   api_->SetPermissionInfo(list);
   EXPECT_EQ(kExpectedChildren, api_->permissions_view()->child_count());
   EXPECT_FALSE(store->HasDevicePermission(origin, origin, device));
 }