Mac: Fix another crash in content exceptions dialog.

Also add a couple unit tests. Also fix another potential crash and other problems uncovered by the tests.

BUG=37208
TEST=See bug. Also, unittests.

Review URL: http://codereview.chromium.org/668046

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

3 files changed, 265 insertions, 28 deletions

diff --git a/chrome/browser/cocoa/content_exceptions_window_controller.h b/chrome/browser/cocoa/content_exceptions_window_controller.h
index e7eab5e..a42510e0 100644
--- a/chrome/browser/cocoa/content_exceptions_window_controller.h
+++ b/chrome/browser/cocoa/content_exceptions_window_controller.h
@@ -40,7 +40,8 @@ class UpdatingContentSettingsObserver;
   // is used to suppress updates at bad times.
   BOOL updatesEnabled_;
 
-  // This is non-NULL only while a new element is being added.
+  // This is non-NULL only while a new element is being added and its host
+  // is being edited.
   scoped_ptr<HostContentSettingsMap::HostSettingPair> newException_;
 }
 
@@ -52,3 +53,8 @@ class UpdatingContentSettingsObserver;
 - (IBAction)removeAllExceptions:(id)sender;
 
 @end
+
+@interface ContentExceptionsWindowController(VisibleForTesting)
+- (void)cancel:(id)sender;
+- (BOOL)editingNewException;
+@end
diff --git a/chrome/browser/cocoa/content_exceptions_window_controller.mm b/chrome/browser/cocoa/content_exceptions_window_controller.mm
index 741a907..4c4c237 100644
--- a/chrome/browser/cocoa/content_exceptions_window_controller.mm
+++ b/chrome/browser/cocoa/content_exceptions_window_controller.mm
@@ -19,6 +19,8 @@
 @interface ContentExceptionsWindowController(Private)
 - (id)initWithType:(ContentSettingsType)settingsType
        settingsMap:(HostContentSettingsMap*)settingsMap;
+- (void)updateRow:(NSInteger)row
+        withEntry:(const HostContentSettingsMap::HostSettingPair&)entry;
 - (void)adjustEditingButtons;
 - (void)modelDidChange;
 - (size_t)menuItemCount;
@@ -160,6 +162,7 @@ static ContentExceptionsWindowController*
     model_.reset(new ContentExceptionsTableModel(settingsMap_, settingsType_));
     showAsk_ = settingsType_ == CONTENT_SETTINGS_TYPE_COOKIES;
     tableObserver_.reset(new UpdatingContentSettingsObserver(self));
+    updatesEnabled_ = YES;
 
     // TODO(thakis): autoremember window rect.
     // TODO(thakis): sorting support.
@@ -207,6 +210,10 @@ static ContentExceptionsWindowController*
   [self autorelease];
 }
 
+- (BOOL)editingNewException {
+  return newException_.get() != NULL;
+}
+
 // Let esc close the window.
 - (void)cancel:(id)sender {
   if ([tableView_ currentEditor] != nil) {
@@ -248,15 +255,21 @@ static ContentExceptionsWindowController*
 }
 
 - (IBAction)addException:(id)sender {
+  if (newException_.get()) {
+    // The invariant is that |newException_| is non-NULL exactly if the host of
+    // a new exception is currently being edited - so there's nothing to do in
+    // that case.
+    return;
+  }
   newException_.reset(new HostContentSettingsMap::HostSettingPair);
   newException_->first = "example.com";
   newException_->second = CONTENT_SETTING_BLOCK;
   [tableView_ reloadData];
-  [self adjustEditingButtons];
 
+  [self adjustEditingButtons];
   int index = model_->RowCount();
-  [tableView_ selectRowIndexes:[NSIndexSet indexSetWithIndex:index]
-          byExtendingSelection:NO];
+  NSIndexSet* selectedSet = [NSIndexSet indexSetWithIndex:index];
+  [tableView_ selectRowIndexes:selectedSet byExtendingSelection:NO];
   [tableView_ editColumn:0 row:index withEvent:nil select:YES];
 }
 
@@ -313,10 +326,40 @@ static ContentExceptionsWindowController*
   return result;
 }
 
+// Updates exception at |row| to contain the data in |entry|.
+- (void)updateRow:(NSInteger)row
+        withEntry:(const HostContentSettingsMap::HostSettingPair&)entry {
+  // TODO(thakis): This apparently moves an edited row to the back of the list.
+  // It's what windows and linux do, but it's kinda sucky. Fix.
+  // http://crbug.com/36904
+  updatesEnabled_ = NO;
+  if (row < model_->RowCount())
+    model_->RemoveException(row);
+  model_->AddException(entry.first, entry.second);
+  updatesEnabled_ = YES;
+  [self modelDidChange];
+
+  // For now, at least re-select the edited element.    
+  int newIndex = model_->IndexOfExceptionByHost(entry.first);
+  DCHECK(newIndex != -1);
+  [tableView_ selectRowIndexes:[NSIndexSet indexSetWithIndex:newIndex]
+          byExtendingSelection:NO];
+}
+
 - (void) tableView:(NSTableView*)tv
     setObjectValue:(id)object
     forTableColumn:(NSTableColumn*)tableColumn
                row:(NSInteger)row {
+  // |-remove:| and |-removeAll:| both call |tableView_ deselectAll:|, which
+  // calls this method if a cell is currently being edited. Do not commit edits
+  // of rows that are about to be deleted.
+  if (!updatesEnabled_) {
+    // If this method gets called, the host filed of the new exception can no
+    // longer be being edited. Reset |newException_| to keep the invariant true.
+    newException_.reset();
+    return;
+  }
+
   // Get model object.
   bool isNewRow = newException_.get() && row >= model_->RowCount();
   const HostContentSettingsMap::HostSettingPair* originalEntry =
@@ -334,32 +377,26 @@ static ContentExceptionsWindowController*
   }
 
   // Commit modification, if any.
-  // TODO(thakis): This apparently moves an edited row to the back of the list.
-  // It's what windows and linux do, but it's kinda sucky. Fix.
-  // http://crbug.com/36904
-  if (entry != *originalEntry) {
-    updatesEnabled_ = NO;
-    if (!isNewRow) {
-      model_->RemoveException(row);
-    } else {
-      newException_.reset();
-      if (![identifier isEqualToString:@"hostname"]) {
-        [tableView_ reloadData];
-        [self adjustEditingButtons];
-        return;  // Commit new rows only when the hostname has been set.
-      }
-    }      
-
-    model_->AddException(entry.first, entry.second);
-    updatesEnabled_ = YES;
-    [self modelDidChange];
-
-    // For now, at least re-select the edited element.    
+  if (isNewRow) {
+    newException_.reset();
+    if (![identifier isEqualToString:@"hostname"]) {
+      [tableView_ reloadData];
+      [self adjustEditingButtons];
+      return;  // Commit new rows only when the hostname has been set.
+    }
     int newIndex = model_->IndexOfExceptionByHost(entry.first);
-    DCHECK(newIndex != -1);
-    [tableView_ selectRowIndexes:[NSIndexSet indexSetWithIndex:newIndex]
-            byExtendingSelection:NO];
+    if (newIndex != -1) {
+      // The new host was already in the table. Focus existing row instead of
+      // overwriting it with a new one.
+      [tableView_ selectRowIndexes:[NSIndexSet indexSetWithIndex:newIndex]
+              byExtendingSelection:NO];
+      [tableView_ reloadData];
+      [self adjustEditingButtons];
+      return;
+    }
   }
+  if (entry != *originalEntry || isNewRow)
+    [self updateRow:row withEntry:entry];
 }
 
 
diff --git a/chrome/browser/cocoa/content_exceptions_window_controller_unittest.mm b/chrome/browser/cocoa/content_exceptions_window_controller_unittest.mm
index d321dc2..8e142c6c 100644
--- a/chrome/browser/cocoa/content_exceptions_window_controller_unittest.mm
+++ b/chrome/browser/cocoa/content_exceptions_window_controller_unittest.mm
@@ -17,6 +17,36 @@
 
 namespace {
 
+void ProcessEvents() {
+  for (;;) {
+    base::ScopedNSAutoreleasePool pool;
+    NSEvent* next_event = [NSApp nextEventMatchingMask:NSAnyEventMask
+                                             untilDate:nil
+                                                inMode:NSDefaultRunLoopMode
+                                               dequeue:YES];
+    if (!next_event) break;
+    [NSApp sendEvent:next_event];
+  }
+}
+
+void SendKeyEvents(NSString* characters) {
+  for (NSUInteger i = 0; i < [characters length]; ++i) {
+    unichar character = [characters characterAtIndex:i];
+    NSString* charAsString = [NSString stringWithCharacters:&character length:1];
+    NSEvent* event = [NSEvent keyEventWithType:NSKeyDown
+                                      location:NSZeroPoint
+                                 modifierFlags:0
+                                     timestamp:0.0
+                                  windowNumber:0
+                                       context:nil
+                                    characters:charAsString
+                   charactersIgnoringModifiers:charAsString
+                                     isARepeat:NO
+                                       keyCode:0];
+    [NSApp sendEvent:event];
+  }
+}
+
 class ContentExceptionsWindowControllerTest : public CocoaTest {
  public:
   virtual void SetUp() {
@@ -25,6 +55,37 @@ class ContentExceptionsWindowControllerTest : public CocoaTest {
     settingsMap_ = new HostContentSettingsMap(profile);
   }
 
+  ContentExceptionsWindowController* GetController(ContentSettingsType type) {
+    return [ContentExceptionsWindowController showForType:type
+                                              settingsMap:settingsMap_.get()];
+
+  }
+
+  void ClickAdd(ContentExceptionsWindowController* controller) {
+    [controller addException:nil];
+    ProcessEvents();
+  }
+
+  void ClickRemove(ContentExceptionsWindowController* controller) {
+    [controller removeException:nil];
+    ProcessEvents();
+  }
+
+  void ClickRemoveAll(ContentExceptionsWindowController* controller) {
+    [controller removeAllExceptions:nil];
+    ProcessEvents();
+  }
+
+  void EnterText(NSString* str) {
+    SendKeyEvents(str);
+    ProcessEvents();
+  }
+
+  void HitEscape(ContentExceptionsWindowController* controller) {
+    [controller cancel:nil];
+    ProcessEvents();
+  }
+
  protected:
   BrowserTestHelper browser_helper_;
   scoped_refptr<HostContentSettingsMap> settingsMap_;
@@ -38,4 +99,137 @@ TEST_F(ContentExceptionsWindowControllerTest, Construction) {
   [controller close];  // Should autorelease.
 }
 
+// Regression test for http://crbug.com/37137
+TEST_F(ContentExceptionsWindowControllerTest, AddRemove) {
+  ContentExceptionsWindowController* controller =
+      GetController(CONTENT_SETTINGS_TYPE_PLUGINS);
+
+  HostContentSettingsMap::SettingsForOneType settings;
+
+  ClickAdd(controller);
+  settingsMap_->GetSettingsForOneType(CONTENT_SETTINGS_TYPE_PLUGINS,
+                                      &settings);
+  EXPECT_EQ(0u, settings.size());
+
+  ClickRemove(controller);
+
+  EXPECT_FALSE([controller editingNewException]);
+  [controller close];
+
+  settingsMap_->GetSettingsForOneType(CONTENT_SETTINGS_TYPE_PLUGINS,
+                                      &settings);
+  EXPECT_EQ(0u, settings.size());
+}
+
+// Regression test for http://crbug.com/37137
+TEST_F(ContentExceptionsWindowControllerTest, AddRemoveAll) {
+  ContentExceptionsWindowController* controller =
+      GetController(CONTENT_SETTINGS_TYPE_PLUGINS);
+  ClickAdd(controller);
+  ClickRemoveAll(controller);
+
+  EXPECT_FALSE([controller editingNewException]);
+  [controller close];
+
+  HostContentSettingsMap::SettingsForOneType settings;
+  settingsMap_->GetSettingsForOneType(CONTENT_SETTINGS_TYPE_PLUGINS,
+                                      &settings);
+  EXPECT_EQ(0u, settings.size());
+}
+
+TEST_F(ContentExceptionsWindowControllerTest, Add) {
+  ContentExceptionsWindowController* controller =
+      GetController(CONTENT_SETTINGS_TYPE_PLUGINS);
+
+  ClickAdd(controller);
+  EnterText(@"addedhost\n");
+
+  EXPECT_FALSE([controller editingNewException]);
+  [controller close];
+
+  HostContentSettingsMap::SettingsForOneType settings;
+  settingsMap_->GetSettingsForOneType(CONTENT_SETTINGS_TYPE_PLUGINS,
+                                      &settings);
+  EXPECT_EQ(1u, settings.size());
+  EXPECT_EQ("addedhost", settings[0].first);
+}
+
+TEST_F(ContentExceptionsWindowControllerTest, AddEscDoesNotAdd) {
+  ContentExceptionsWindowController* controller =
+      GetController(CONTENT_SETTINGS_TYPE_PLUGINS);
+
+  ClickAdd(controller);
+  EnterText(@"addedhost");  // but do not press enter
+  HitEscape(controller);
+
+  HostContentSettingsMap::SettingsForOneType settings;
+  settingsMap_->GetSettingsForOneType(CONTENT_SETTINGS_TYPE_PLUGINS,
+                                      &settings);
+  EXPECT_EQ(0u, settings.size());
+  EXPECT_FALSE([controller editingNewException]);
+
+  [controller close];
+}
+
+// Regression test for http://crbug.com/37208
+TEST_F(ContentExceptionsWindowControllerTest, AddEditAddAdd) {
+  ContentExceptionsWindowController* controller =
+      GetController(CONTENT_SETTINGS_TYPE_PLUGINS);
+
+  ClickAdd(controller);
+  EnterText(@"testtesttest");  // but do not press enter
+  ClickAdd(controller);
+  ClickAdd(controller);
+
+  EXPECT_TRUE([controller editingNewException]);
+  [controller close];
+
+  HostContentSettingsMap::SettingsForOneType settings;
+  settingsMap_->GetSettingsForOneType(CONTENT_SETTINGS_TYPE_PLUGINS,
+                                      &settings);
+  EXPECT_EQ(0u, settings.size());
+}
+
+TEST_F(ContentExceptionsWindowControllerTest, AddExistingEditAdd) {
+  settingsMap_->SetContentSetting(
+      "myhost", CONTENT_SETTINGS_TYPE_PLUGINS, CONTENT_SETTING_BLOCK);
+
+  ContentExceptionsWindowController* controller =
+      GetController(CONTENT_SETTINGS_TYPE_PLUGINS);
+
+  ClickAdd(controller);
+  EnterText(@"myhost");  // but do not press enter
+  ClickAdd(controller);
+
+  EXPECT_TRUE([controller editingNewException]);
+  [controller close];
+
+
+  HostContentSettingsMap::SettingsForOneType settings;
+  settingsMap_->GetSettingsForOneType(CONTENT_SETTINGS_TYPE_PLUGINS,
+                                      &settings);
+  EXPECT_EQ(1u, settings.size());
+}
+
+TEST_F(ContentExceptionsWindowControllerTest, AddExistingDoesNotOverwrite) {
+  settingsMap_->SetContentSetting(
+      "myhost", CONTENT_SETTINGS_TYPE_COOKIES, CONTENT_SETTING_ASK);
+
+  ContentExceptionsWindowController* controller =
+      GetController(CONTENT_SETTINGS_TYPE_COOKIES);
+
+  ClickAdd(controller);
+  EnterText(@"myhost\n");
+
+  EXPECT_FALSE([controller editingNewException]);
+  [controller close];
+
+  HostContentSettingsMap::SettingsForOneType settings;
+  settingsMap_->GetSettingsForOneType(CONTENT_SETTINGS_TYPE_COOKIES,
+                                      &settings);
+  EXPECT_EQ(1u, settings.size());
+  EXPECT_EQ(CONTENT_SETTING_ASK, settings[0].second);
+}
+
+
 }  // namespace