Add remaining functionality for popup blocker: popup menu to unblock...

Add remaining functionality for popup blocker: popup menu to unblock individual popups and whitelist sites. Also fixes intermittant leak on valgrind bots from poorly constructed unit test.
BUG=13160, 15818
TEST=unblocking popups, whitelisting sites, and unwhitelisting them. Green valgrind bot.
Review URL: http://codereview.chromium.org/149145

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@19944 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/blocked_popup_container.cc

@@ -21,6 +21,15 @@ BlockedPopupContainer* BlockedPopupContainer::Create(
   return container;
 }
 
+// static
+BlockedPopupContainer* BlockedPopupContainer::Create(
+    TabContents* owner, Profile* profile, BlockedPopupContainerView* view) {
+  BlockedPopupContainer* container =
+      new BlockedPopupContainer(owner, profile->GetPrefs());
+  container->set_view(view);
+  return container;
+}
+
 // static
 void BlockedPopupContainer::RegisterUserPrefs(PrefService* prefs) {
   prefs->RegisterListPref(prefs::kPopupWhitelistedHosts);

chrome/browser/blocked_popup_container.h

@@ -198,6 +198,12 @@ class BlockedPopupContainer : public TabContentsDelegate,
   // string is hostname.  bool is whitelisted status.
   typedef std::map<std::string, bool> PopupHosts;
 
+  // Creates a BlockedPopupContainer, anchoring the container to the lower
+  // right corner using the given BlockedPopupContainerView. Use only for
+  // testing.
+  static BlockedPopupContainer* Create(TabContents* owner, Profile* profile,
+                                       BlockedPopupContainerView* view);
+
   // Hides the UI portion of the container.
   void HideSelf();
 
@@ -217,6 +223,7 @@ class BlockedPopupContainer : public TabContentsDelegate,
  private:
   friend class BlockedPopupContainerImpl;
   friend class BlockedPopupContainerTest;
+  friend class BlockedPopupContainerControllerTest;
 
   // string is hostname.
   typedef std::set<std::string> Whitelist;

chrome/browser/cocoa/blocked_popup_container_controller.h

@@ -26,7 +26,7 @@
   scoped_ptr<BlockedPopupContainerView> bridge_;
   BlockedPopupContainer* container_;  // Weak. "owns" me.
   scoped_nsobject<NSView> view_;
-  IBOutlet NSTextField* label_;
+  IBOutlet NSPopUpButton* popupButton_;
 }
 
 // Initialize with the given popup container. Creates the C++ bridge object
@@ -45,8 +45,10 @@
 
 @interface BlockedPopupContainerController(ForTesting)
 - (NSView*)view;
-- (NSView*)label;
+- (NSPopUpButton*)popupButton;
 - (IBAction)closePopup:(id)sender;
+- (NSMenu*)buildMenu;
+- (void)setContainer:(BlockedPopupContainer*)container;
 @end
 
 #endif  // CHROME_BROWSER_VIEWS_BLOCKED_POPUP_CONTAINER_CONTROLLER_H_

chrome/browser/cocoa/blocked_popup_container_controller.mm

@@ -50,6 +50,7 @@ class BlockedPopupContainerViewBridge : public BlockedPopupContainerView {
 
 - (void)dealloc {
   [view_ removeFromSuperview];
+  [[NSNotificationCenter defaultCenter] removeObserver:self];
   [super dealloc];
 }
 
@@ -77,13 +78,29 @@ class BlockedPopupContainerViewBridge : public BlockedPopupContainerView {
                                  0,
                                  startFrame.size.width - kCloseBoxSize,
                                  startFrame.size.height);
-  label_ = [[[NSTextField alloc] initWithFrame:labelFrame] autorelease];
-  [label_ setSelectable:NO];
-  [label_ setAutoresizingMask:NSViewWidthSizable];
-  [label_ setBordered:NO];
-  [label_ setBezeled:NO];
-  [label_ setDrawsBackground:NO];
-  [view_ addSubview:label_];
+  popupButton_ = [[[NSPopUpButton alloc] initWithFrame:labelFrame] autorelease];
+  [popupButton_ setAutoresizingMask:NSViewWidthSizable];
+  [popupButton_ setBordered:NO];
+  [popupButton_ setBezelStyle:NSTexturedRoundedBezelStyle];
+  [popupButton_ setPullsDown:YES];
+  // TODO(pinkerton): this doesn't work, not sure why.
+  [popupButton_ setPreferredEdge:NSMaxYEdge];
+  // TODO(pinkerton): no matter what, the arrows always draw in the middle
+  // of the button. We can turn off the arrows entirely, but then will the
+  // user ever know to click it? Leave them on for now.
+  //[[popupButton_ cell] setArrowPosition:NSPopUpNoArrow];
+  [[popupButton_ cell] setAltersStateOfSelectedItem:NO];
+  // If we don't add this, no title will ever display.
+  [popupButton_ addItemWithTitle:@"placeholder"];
+  [view_ addSubview:popupButton_];
+
+  // Register for notifications that the menu is about to display so we can
+  // fill it in lazily
+  [[NSNotificationCenter defaultCenter]
+      addObserver:self
+         selector:@selector(showMenu:)
+             name:NSPopUpButtonCellWillPopUpNotification
+           object:nil];
 
   // Create the close box and position at the left of the view.
   NSRect closeFrame = NSMakeRect(startFrame.size.width - kCloseBoxSize,
@@ -139,9 +156,14 @@ class BlockedPopupContainerViewBridge : public BlockedPopupContainerView {
 // Resize the view based on the new label contents. The autoresize mask will
 // take care of resizing everything else.
 - (void)resizeWithLabel:(NSString*)label {
+// TODO(pinkerton): fix this so that it measures the text so that it can
+// be localized.
 #if 0
-// TODO(pinkerton): fix this once the popup gets put in.
-  NSSize stringSize = [label sizeWithAttributes:nil];
+  NSDictionary* attributes =
+      [NSDictionary dictionaryWithObjectsAndKeys:
+        NSFontAttributeName, [NSFont systemFontOfSize:25],
+        nil];
+  NSSize stringSize = [label sizeWithAttributes:attributes];
   NSRect frame = [view_ frame];
   float originalWidth = frame.size.width;
   frame.size.width = stringSize.width + 16 + 5;
@@ -162,15 +184,116 @@ class BlockedPopupContainerViewBridge : public BlockedPopupContainerView {
         l10n_util::GetStringUTF16(IDS_POPUPS_UNBLOCKED));
   }
   [self resizeWithLabel:label];
-  [label_ setStringValue:label];
+  [popupButton_ setTitle:label];
+}
+
+// Called when the user selects an item from the popup menu. The tag, if below
+// |kImpossibleNumberOfPopups| will be the index into the container's popup
+// array. In that case, we should display the popup. If >=
+// |kImpossibleNumberOfPopups|, it represents a host that we should whitelist.
+// |sender| is the NSMenuItem that was chosen.
+- (void)menuAction:(id)sender {
+  size_t tag = static_cast<size_t>([sender tag]);
+  if (tag < BlockedPopupContainer::kImpossibleNumberOfPopups) {
+    container_->LaunchPopupAtIndex(tag);
+  } else {
+    size_t hostIndex = tag - BlockedPopupContainer::kImpossibleNumberOfPopups;
+    container_->ToggleWhitelistingForHost(hostIndex);
+  }
+}
+
+namespace {
+void GetURLAndTitleForPopup(
+    const BlockedPopupContainer* container,
+    size_t index,
+    string16* url,
+    string16* title) {
+  DCHECK(url);
+  DCHECK(title);
+  TabContents* tab_contents = container->GetTabContentsAt(index);
+  const GURL& tab_contents_url = tab_contents->GetURL().GetOrigin();
+  *url = UTF8ToUTF16(tab_contents_url.possibly_invalid_spec());
+  *title = tab_contents->GetTitle();
+}
+}  // namespace
+
+// Build a new popup menu from scratch. The menu contains the blocked popups
+// (tags being the popup's index), followed by the list of hosts from which
+// the popups were blocked (tags being |kImpossibleNumberOfPopups| + host
+// index). The hosts are used to toggle whitelisting for a site.
+- (NSMenu*)buildMenu {
+  NSMenu* menu = [[[NSMenu alloc] init] autorelease];
+
+  // For pop-down menus, the first item is what is displayed while tracking the
+  // menu and it remains there if nothing is selected. Set it to the
+  // current title.
+  NSString* currentTitle = [popupButton_ title];
+  scoped_nsobject<NSMenuItem> dummy(
+      [[NSMenuItem alloc] initWithTitle:currentTitle
+                                 action:nil
+                          keyEquivalent:@""]);
+  [menu addItem:dummy.get()];
+
+  // Add the list of blocked popups titles to the menu. We set the array index
+  // as the tag for use in the menu action rather than relying on the menu item
+  // index.
+  const size_t count = container_->GetBlockedPopupCount();
+  for (size_t i = 0; i < count; ++i) {
+    string16 url, title;
+    GetURLAndTitleForPopup(container_, i, &url, &title);
+    NSString* titleStr = base::SysUTF16ToNSString(
+        l10n_util::GetStringFUTF16(IDS_POPUP_TITLE_FORMAT, url, title));
+    scoped_nsobject<NSMenuItem> item(
+        [[NSMenuItem alloc] initWithTitle:titleStr
+                                   action:@selector(menuAction:)
+                            keyEquivalent:@""]);
+    [item setTag:i];
+    [item setTarget:self];
+    [menu addItem:item.get()];
+  }
+
+  // Add the list of hosts. We begin tagging these at
+  // |kImpossibleNumberOfPopups|. If whitelisting has already been enabled
+  // for a site, mark it with a checkmark.
+  std::vector<std::string> hosts(container_->GetHosts());
+  if (!hosts.empty() && count)
+    [menu addItem:[NSMenuItem separatorItem]];
+  for (size_t i = 0; i < hosts.size(); ++i) {
+    NSString* titleStr = base::SysUTF8ToNSString(
+        l10n_util::GetStringFUTF8(IDS_POPUP_HOST_FORMAT,
+                                  UTF8ToUTF16(hosts[i])));
+    scoped_nsobject<NSMenuItem> item(
+        [[NSMenuItem alloc] initWithTitle:titleStr
+                                   action:@selector(menuAction:)
+                            keyEquivalent:@""]);
+    if (container_->IsHostWhitelisted(i))
+      [item setState:NSOnState];
+    [item setTag:BlockedPopupContainer::kImpossibleNumberOfPopups + i];
+    [item setTarget:self];
+    [menu addItem:item.get()];
+  }
+
+  return menu;
+}
+
+// Called when the popup button is about to display the menu, giving us a
+// chance to fill in the contents.
+- (void)showMenu:(NSNotification*)notify {
+  NSMenu* menu = [self buildMenu];
+  [[notify object] setMenu:menu];
 }
 
 - (NSView*)view {
   return view_.get();
 }
 
-- (NSView*)label {
-  return label_;
+- (NSPopUpButton*)popupButton {
+  return popupButton_;
+}
+
+// Only used for testing.
+- (void)setContainer:(BlockedPopupContainer*)container {
+  container_ = container;
 }
 
 @end

chrome/browser/cocoa/blocked_popup_container_controller_unittest.mm

@@ -6,6 +6,7 @@
 
 #include "app/app_paths.h"
 #include "base/path_service.h"
+#include "base/scoped_nsautorelease_pool.h"
 #import "chrome/browser/cocoa/blocked_popup_container_controller.h"
 #include "chrome/browser/cocoa/browser_test_helper.h"
 #import "chrome/browser/cocoa/cocoa_test_helper.h"
@@ -21,12 +22,18 @@ const std::string host1 = "host1";
 class BlockedPopupContainerControllerTest : public RenderViewHostTestHarness {
  public:
   virtual void SetUp() {
+    // This is all a bit convoluted because the standard factory Create() call
+    // doesn't give us access to the cocoa controller for testing (since it's
+    // an internal implementation detail). As a result, we need to create one
+    // separately and inject the bridge with a test-only Create() call.
+    // Unfortunate, but no way around it.
     RenderViewHostTestHarness::SetUp();
-    container_ = BlockedPopupContainer::Create(contents(), profile());
     cocoa_controller_ = [[BlockedPopupContainerController alloc]
-                          initWithContainer:container_];
+                          initWithContainer:nil];
     EXPECT_TRUE([cocoa_controller_ bridge]);
-    container_->set_view([cocoa_controller_ bridge]);
+    container_ = BlockedPopupContainer::Create(contents(), profile(),
+                                               [cocoa_controller_ bridge]);
+    [cocoa_controller_ setContainer:container_];
     contents_->set_blocked_popup_container(container_);
   }
 
@@ -54,6 +61,7 @@ class BlockedPopupContainerControllerTest : public RenderViewHostTestHarness {
     return net::FilePathToFileURL(filename);
   }
 
+  base::ScopedNSAutoreleasePool pool;
   BlockedPopupContainer* container_;
   BlockedPopupContainerController* cocoa_controller_;
 };
@@ -70,12 +78,24 @@ TEST_F(BlockedPopupContainerControllerTest, BasicPopupBlock) {
   EXPECT_FALSE(container_->IsHostWhitelisted(0));
 
   // Ensure the view has been displayed. If it has a superview, then ShowView()
-  // has been called on the bridge. If the label has a string, then
+  // has been called on the bridge. If the button has a string, then
   // UpdateLabel() has been called.
   EXPECT_TRUE([cocoa_controller_ view]);
   EXPECT_TRUE([[cocoa_controller_ view] superview]);
-  EXPECT_TRUE([[(NSTextField*)[cocoa_controller_ label]
-                stringValue] length] > 0);
+  EXPECT_TRUE([[[cocoa_controller_ popupButton] title] length] > 0);
+
+  // Validate the menu. It should have 4 items (the dummy title item, 1 poupup,
+  // a separator, 1 host).
+  NSMenu* menu = [cocoa_controller_ buildMenu];
+  EXPECT_TRUE(menu);
+  EXPECT_EQ([menu numberOfItems], 4);
+
+  // Change the whitelisting and make sure the host is checked.
+  container_->ToggleWhitelistingForHost(0);
+  menu = [cocoa_controller_ buildMenu];
+  EXPECT_TRUE(menu);
+  EXPECT_EQ([menu numberOfItems], 2);
+  EXPECT_EQ([[menu itemAtIndex:1] state], NSOnState);
 
   // Close the popup and verify it's no longer in the view hierarchy. This
   // means HideView() has been called.