[Mac] Website settings: Enlarge window when permission strings are too long.

[Mac] Website settings: Enlarge window when permission strings are too long.

The website settings window was using a fixed width. This was fine for most
text since it would wrap, but the permissions can't wrap, so the window should
be made wide enough to fit the largest permission string.

BUG=159156
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=169678

[Patrick Dubroy, 2012-11-20 14:45:21]

Hi Robert, please take a look. We probably  want to merge this to stable, so
it's somewhat urgent.

[sail, 2012-11-20 15:51:44]

(Drive by review. This CL needs a regression test.)

[Patrick Dubroy, 2012-11-23 15:50:49]

Added a unit test.

[Robert Sesek, 2012-11-26 17:06:55]

LGTM

https://codereview.chromium.org/11417082/diff/3001/chrome/browser/ui/cocoa/we...
File chrome/browser/ui/cocoa/website_settings_bubble_controller.mm (right):

https://codereview.chromium.org/11417082/diff/3001/chrome/browser/ui/cocoa/we...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:292: // Return the
default width of the window. It may be wider to fit the content.
Move this to the .h since it's a public member.

https://codereview.chromium.org/11417082/diff/3001/chrome/browser/ui/cocoa/we...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:914: for
(NSInteger i = 0; i < [button numberOfItems]; i++) {
nit: use prefix operator: ++i

[commit-bot: I haz the power, 2012-11-27 10:50:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dubroy@chromium.org/11417082/10001

[commit-bot: I haz the power, 2012-11-27 10:50:42]

Failed to apply patch for
chrome/browser/ui/cocoa/website_settings_bubble_controller_unittest.mm:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
chrome/browser/ui/cocoa/website_settings_bubble_controller_unittest.mm
  Hunk #3 FAILED at 151.
  1 out of 6 hunks FAILED -- saving rejects to file
chrome/browser/ui/cocoa/website_settings_bubble_controller_unittest.mm.rej

Patch:      
chrome/browser/ui/cocoa/website_settings_bubble_controller_unittest.mm
Index: chrome/browser/ui/cocoa/website_settings_bubble_controller_unittest.mm
diff --git
a/chrome/browser/ui/cocoa/website_settings_bubble_controller_unittest.mm
b/chrome/browser/ui/cocoa/website_settings_bubble_controller_unittest.mm
index
1f8ebb752a0db0b63f8811daf197a5d69eebe82e..392ac54d3feca6301a593ffa4fa3247dc11d946f
100644
--- a/chrome/browser/ui/cocoa/website_settings_bubble_controller_unittest.mm
+++ b/chrome/browser/ui/cocoa/website_settings_bubble_controller_unittest.mm
@@ -61,6 +61,24 @@
 }
 @end
 
+@interface WebsiteSettingsBubbleControllerForTesting
+    : WebsiteSettingsBubbleController {
+ @private
+  CGFloat defaultWindowWidth_;
+}
+@end
+
+@implementation WebsiteSettingsBubbleControllerForTesting
+- (void)setDefaultWindowWidth:(CGFloat)width {
+  defaultWindowWidth_ = width;
+}
+- (CGFloat)defaultWindowWidth {
+  // If |defaultWindowWidth_| is 0, use the superclass implementation.
+  return defaultWindowWidth_ ?
+      defaultWindowWidth_ : [super defaultWindowWidth];
+}
+@end
+
 namespace {
 
 // Indices of the menu items in the permission menu.
@@ -70,6 +88,50 @@ enum PermissionMenuIndices {
   kMenuIndexContentSettingDefault
 };
 
+const ContentSettingsType kTestPermissionTypes[] = {
+  // NOTE: FULLSCREEN does not support "Always block", so it must appear as
+  // one of the first three permissions.
+  CONTENT_SETTINGS_TYPE_FULLSCREEN,
+  CONTENT_SETTINGS_TYPE_IMAGES,
+  CONTENT_SETTINGS_TYPE_JAVASCRIPT,
+  CONTENT_SETTINGS_TYPE_PLUGINS,
+  CONTENT_SETTINGS_TYPE_POPUPS,
+  CONTENT_SETTINGS_TYPE_GEOLOCATION,
+  CONTENT_SETTINGS_TYPE_NOTIFICATIONS,
+  CONTENT_SETTINGS_TYPE_MOUSELOCK,
+  CONTENT_SETTINGS_TYPE_MEDIASTREAM,
+};
+
+const ContentSetting kTestSettings[] = {
+  CONTENT_SETTING_DEFAULT,
+  CONTENT_SETTING_DEFAULT,
+  CONTENT_SETTING_DEFAULT,
+  CONTENT_SETTING_ALLOW,
+  CONTENT_SETTING_BLOCK,
+  CONTENT_SETTING_ALLOW,
+  CONTENT_SETTING_BLOCK,
+  CONTENT_SETTING_ALLOW,
+  CONTENT_SETTING_BLOCK,
+};
+
+const ContentSetting kTestDefaultSettings[] = {
+  CONTENT_SETTING_ALLOW,
+  CONTENT_SETTING_BLOCK,
+  CONTENT_SETTING_ASK
+};
+
+const content_settings::SettingSource kTestSettingSources[] = {
+  content_settings::SETTING_SOURCE_USER,
+  content_settings::SETTING_SOURCE_USER,
+  content_settings::SETTING_SOURCE_USER,
+  content_settings::SETTING_SOURCE_USER,
+  content_settings::SETTING_SOURCE_USER,
+  content_settings::SETTING_SOURCE_POLICY,
+  content_settings::SETTING_SOURCE_POLICY,
+  content_settings::SETTING_SOURCE_EXTENSION,
+  content_settings::SETTING_SOURCE_EXTENSION,
+};
+
 class WebsiteSettingsBubbleControllerTest : public CocoaTest {
  public:
   WebsiteSettingsBubbleControllerTest() {
@@ -89,20 +151,26 @@ class WebsiteSettingsBubbleControllerTest : public
CocoaTest {
     TEXT_NOT_EQUAL
   };
 
-  void CreateBubble() {
+  // Creates a new website settings bubble, with the given default width.
+  // If |default_width| is 0, the *default* default width will be used.
+  void CreateBubbleWithWidth(CGFloat default_width) {
     bridge_ = new WebsiteSettingsUIBridge();
 
     // The controller cleans up after itself when the window closes.
-    controller_ =
-        [[WebsiteSettingsBubbleController alloc]
-            initWithParentWindow:test_window()
-            websiteSettingsUIBridge:bridge_
-            tabContents:nil
-            isInternalPage:NO];
+    controller_ = [WebsiteSettingsBubbleControllerForTesting alloc];
+    [controller_ setDefaultWindowWidth:default_width];
+    [controller_ initWithParentWindow:test_window()
+                 websiteSettingsUIBridge:bridge_
+                 tabContents:nil
+                 isInternalPage:NO];
     window_ = [controller_ window];
     [controller_ showWindow:nil];
   }
 
+  void CreateBubble() {
+    CreateBubbleWithWidth(0.0);
+  }
+
   // Return a pointer to the first NSTextField found that either matches, or
   // doesn't match, the given text.
   NSTextField* FindTextField(MatchType match_type, NSString* text) {
@@ -137,7 +205,26 @@ class WebsiteSettingsBubbleControllerTest : public
CocoaTest {
     return views;
   }
 
-  WebsiteSettingsBubbleController* controller_;  // Weak, owns self.
+  // Sets up the dialog with some test permission settings.
+  void SetTestPermissions() {
+    // Create a list of 5 different permissions, corresponding to all the
+    // possible settings:
+    // - [allow, block, ask] by default
+    // - [block, allow] * [by user, by policy, by extension]
+    PermissionInfoList list;
+    WebsiteSettingsUI::PermissionInfo info;
+    for (size_t i = 0; i < arraysize(kTestPermissionTypes); ++i) {
+      info.type = kTestPermissionTypes[i];
+      info.setting = kTestSettings[i];
+      if (info.setting == CONTENT_SETTING_DEFAULT)
+        info.default_setting = kTestDefaultSettings[i];
+      info.source = kTestSettingSources[i];
+      list.push_back(info);
+    }
+    bridge_->SetPermissionInfo(list);
+  }
+
+  WebsiteSettingsBubbleControllerForTesting* controller_;  // Weak, owns self.
   NSWindow* window_;  // Weak, owned by controller.
 };
 
@@ -238,66 +325,7 @@ TEST_F(WebsiteSettingsBubbleControllerTest, SetFirstVisit)
{
 
 TEST_F(WebsiteSettingsBubbleControllerTest, SetPermissionInfo) {
   CreateBubble();
-
-  const ContentSettingsType kTestPermissionTypes[] = {
-    // NOTE: FULLSCREEN does not support "Always block", so it must appear as
-    // one of the first three permissions.
-    CONTENT_SETTINGS_TYPE_FULLSCREEN,
-    CONTENT_SETTINGS_TYPE_IMAGES,
-    CONTENT_SETTINGS_TYPE_JAVASCRIPT,
-    CONTENT_SETTINGS_TYPE_PLUGINS,
-    CONTENT_SETTINGS_TYPE_POPUPS,
-    CONTENT_SETTINGS_TYPE_GEOLOCATION,
-    CONTENT_SETTINGS_TYPE_NOTIFICATIONS,
-    CONTENT_SETTINGS_TYPE_MOUSELOCK,
-    CONTENT_SETTINGS_TYPE_MEDIASTREAM,
-  };
-
-  const ContentSetting kTestSettings[] = {
-    CONTENT_SETTING_DEFAULT,
-    CONTENT_SETTING_DEFAULT,
-    CONTENT_SETTING_DEFAULT,
-    CONTENT_SETTING_ALLOW,
-    CONTENT_SETTING_BLOCK,
-    CONTENT_SETTING_ALLOW,
-    CONTENT_SETTING_BLOCK,
-    CONTENT_SETTING_ALLOW,
-    CONTENT_SETTING_BLOCK,
-  };
-
-  const ContentSetting kTestDefaultSettings[] = {
-    CONTENT_SETTING_ALLOW,
-    CONTENT_SETTING_BLOCK,
-    CONTENT_SETTING_ASK
-  };
-
-  const content_settings::SettingSource kTestSettingSources[] = {
-    content_settings::SETTING_SOURCE_USER,
-    content_settings::SETTING_SOURCE_USER,
-    content_settings::SETTING_SOURCE_USER,
-    content_settings::SETTING_SOURCE_USER,
-    content_settings::SETTING_SOURCE_USER,
-    content_settings::SETTING_SOURCE_POLICY,
-    content_settings::SETTING_SOURCE_POLICY,
-    content_settings::SETTING_SOURCE_EXTENSION,
-    content_settings::SETTING_SOURCE_EXTENSION,
-  };
-
-  // Create a list of 5 different permissions, corresponding to all the
-  // possible settings:
-  // - [allow, block, ask] by default
-  // - [block, allow] * [by user, by policy, by extension]
-  PermissionInfoList list;
-  WebsiteSettingsUI::PermissionInfo info;
-  for (size_t i = 0; i < arraysize(kTestPermissionTypes); ++i) {
-    info.type = kTestPermissionTypes[i];
-    info.setting = kTestSettings[i];
-    if (info.setting == CONTENT_SETTING_DEFAULT)
-      info.default_setting = kTestDefaultSettings[i];
-    info.source = kTestSettingSources[i];
-    list.push_back(info);
-  }
-  bridge_->SetPermissionInfo(list);
+  SetTestPermissions();
 
   // There should be three subviews per permission (an icon, a label and a
   // select box), plus a text label for the Permission section.
@@ -349,4 +377,24 @@ TEST_F(WebsiteSettingsBubbleControllerTest, SetSelectedTab)
{
   EXPECT_EQ(1, [tabView indexOfTabViewItem:[tabView selectedTabViewItem]]);
 }
 
+TEST_F(WebsiteSettingsBubbleControllerTest, WindowWidth) {
+  // Try creating a window that is obviously too small.
+  CreateBubbleWithWidth(30.0);
+  SetTestPermissions();
+
+  CGFloat window_width = NSWidth([[controller_ window] frame]);
+
+  // Check the window was made bigger to fit the content.
+  EXPECT_LT(30.0, window_width);
+
+  // Check that the window is wider than the right edge of all the permission
+  // popup buttons.
+  for (NSView* view in [[controller_ permissionsView] subviews]) {
+    if ([view isKindOfClass:[NSPopUpButton class]]) {
+      NSPopUpButton* button = static_cast<NSPopUpButton*>(view);
+      EXPECT_LT(NSMaxX([button frame]), window_width);
+    }
+  }
+}
+
 }  // namespace

[Patrick Dubroy, 2012-11-27 12:11:30]

https://codereview.chromium.org/11417082/diff/3001/chrome/browser/ui/cocoa/we...
File chrome/browser/ui/cocoa/website_settings_bubble_controller.mm (right):

https://codereview.chromium.org/11417082/diff/3001/chrome/browser/ui/cocoa/we...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:292: // Return the
default width of the window. It may be wider to fit the content.
On 2012/11/26 17:06:56, rsesek wrote:
> Move this to the .h since it's a public member.

Done.

https://codereview.chromium.org/11417082/diff/3001/chrome/browser/ui/cocoa/we...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:914: for
(NSInteger i = 0; i < [button numberOfItems]; i++) {
On 2012/11/26 17:06:56, rsesek wrote:
> nit: use prefix operator: ++i

Done.

[commit-bot: I haz the power, 2012-11-27 12:47:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dubroy@chromium.org/11417082/12001

[commit-bot: I haz the power, 2012-11-27 17:20:30]

Retried try job too often for step(s) content_browsertests