OS X: Import settings dialog cleanup.

* Make Import settings dialog app modal.
* No longer allow display of multiple dialogs at once.
* For some reason, unit test was present but missing from .gyp file - re-added and freshened up.

BUG=33011
TEST=selecting Chrome->Import Bookmarks and settings multiple times should only open bookmarks once.

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

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

7 files changed, 61 insertions, 71 deletions

diff --git a/chrome/browser/app_controller_mac.mm b/chrome/browser/app_controller_mac.mm
index e3ed47b..8895354 100644
--- a/chrome/browser/app_controller_mac.mm
+++ b/chrome/browser/app_controller_mac.mm
@@ -580,10 +580,8 @@ static bool g_is_opening_new_window = false;
     }
     case IDC_IMPORT_SETTINGS: {
       UserMetrics::RecordAction("Import_ShowDlg", defaultProfile);
-      // Note that this dialog controller cleans itself up when closed
-      // so auto-scoping it here is not necessary.
-      [[[ImportSettingsDialogController alloc]
-          initWithProfile:defaultProfile parentWindow:nil] runModalDialog];
+      [ImportSettingsDialogController
+          showImportSettingsDialogForProfile:defaultProfile];
       break;
     }
     case IDC_SHOW_BOOKMARK_MANAGER:
diff --git a/chrome/browser/cocoa/browser_window_cocoa.mm b/chrome/browser/cocoa/browser_window_cocoa.mm
index a9b6b2bf0..e5a20c9 100644
--- a/chrome/browser/cocoa/browser_window_cocoa.mm
+++ b/chrome/browser/cocoa/browser_window_cocoa.mm
@@ -294,10 +294,8 @@ void BrowserWindowCocoa::ShowClearBrowsingDataDialog() {
 }
 
 void BrowserWindowCocoa::ShowImportDialog() {
-  // Note that the dialog controller takes care of cleaning itself up
-  // upon dismissal so auto-scoping here is not necessary.
-  [[[ImportSettingsDialogController alloc]
-    initWithProfile:browser_->profile() parentWindow:window()] runModalDialog];
+  [ImportSettingsDialogController
+          showImportSettingsDialogForProfile:browser_->profile()];
 }
 
 void BrowserWindowCocoa::ShowSearchEnginesDialog() {
diff --git a/chrome/browser/cocoa/import_settings_dialog.h b/chrome/browser/cocoa/import_settings_dialog.h
index 3f4fce1..908c7de 100644
--- a/chrome/browser/cocoa/import_settings_dialog.h
+++ b/chrome/browser/cocoa/import_settings_dialog.h
@@ -33,17 +33,10 @@ class Profile;
   BOOL searchEnginesAvailable_;
 }
 
-// Initialize the dialog controller with either the default profile or
-// the profile for the current browser. If you wish to hang this dialog
-// off of another window (such as an open browser window) then pass in the
-// |parentWindow|, otherwise pass nil.
-- (id)initWithProfile:(Profile*)profile
-         parentWindow:(NSWindow*)parentWindow;
-
-// Present the sheet/dialog. If parentWindow_ is nil then this will be an
-// app modal dialog, otherwise this will be window modal presented as
-// a sheet.
-- (void)runModalDialog;
+// Show the import settings window.  Window is displayed as an app modal dialog.
+// If the dialog is already being displayed, this method whill return with
+// no error.
++ (void)showImportSettingsDialogForProfile:(Profile*)profile;
 
 // Called when the "Import" button is pressed.
 - (IBAction)ok:(id)sender;
@@ -74,9 +67,8 @@ class Profile;
 @interface ImportSettingsDialogController (TestingAPI)
 
 // Initialize by providing an array of profile dictionaries. Exposed for
-// unit testing but also called by -[initWithProfiles:parentWindow:].
-- (id)initWithProfiles:(NSArray*)profiles
-          parentWindow:(NSWindow*)parentWindow;
+// unit testing but also called by -[initWithProfile:].
+- (id)initWithProfiles:(NSArray*)profiles;
 
 // Return selected services to import as mapped by the ImportItem enum.
 - (uint16)servicesToImport;
diff --git a/chrome/browser/cocoa/import_settings_dialog.mm b/chrome/browser/cocoa/import_settings_dialog.mm
index 245856b..454e8cc 100644
--- a/chrome/browser/cocoa/import_settings_dialog.mm
+++ b/chrome/browser/cocoa/import_settings_dialog.mm
@@ -9,6 +9,12 @@
 #include "chrome/browser/importer/importer_list.h"
 #include "chrome/browser/profile.h"
 
+namespace {
+
+bool importSettingsDialogVisible = false;
+
+}  // namespace
+
 @interface ImportSettingsDialogController ()
 
 @property(assign, readwrite, nonatomic) BOOL historyAvailable;
@@ -58,6 +64,20 @@
 
 @end
 
+@interface ImportSettingsDialogController (Private)
+
+// Initialize the dialog controller with either the default profile or
+// the profile for the current browser.
+- (id)initWithProfile:(Profile*)profile;
+
+// Present the app modal dialog.
+- (void)runModalDialog;
+
+// Close the modal dialog.
+- (void)closeDialog;
+
+@end
+
 @implementation ImportSettingsDialogController
 
 @synthesize sourceBrowserIndex = sourceBrowserIndex_;
@@ -76,8 +96,16 @@
           @"importPasswords", @"importSearchEngines", nil];
 }
 
-- (id)initWithProfile:(Profile*)profile
-         parentWindow:(NSWindow*)parentWindow {
++ (void)showImportSettingsDialogForProfile:(Profile*)profile {
+  // Don't display if already visible.
+  if (importSettingsDialogVisible)
+    return;
+  ImportSettingsDialogController* controller =
+      [[ImportSettingsDialogController alloc] initWithProfile:profile];
+  [controller runModalDialog];
+}
+
+- (id)initWithProfile:(Profile*)profile {
   // Collect profile information (profile name and the services which can
   // be imported from each) into an array of ImportSettingsProfile which
   // are bound to the Browser List array controller and the popup name
@@ -102,16 +130,13 @@
                                      services:browserServices];
     [browserProfiles addObject:settingsProfile];
   }
-  if ((self = [self initWithProfiles:browserProfiles
-                        parentWindow:parentWindow])) {
+  if ((self = [self initWithProfiles:browserProfiles])) {
     profile_ = profile;
-    parentWindow_ = parentWindow;
   }
   return self;
 }
 
-- (id)initWithProfiles:(NSArray*)profiles
-          parentWindow:(NSWindow*)parentWindow {
+- (id)initWithProfiles:(NSArray*)profiles {
   NSString* nibpath =
       [mac_util::MainAppBundle() pathForResource:@"ImportSettingsDialog"
                                           ofType:@"nib"];
@@ -128,7 +153,7 @@
 }
 
 - (id)init {
-  return [self initWithProfile:NULL parentWindow:nil];
+  return [self initWithProfile:NULL];
 }
 
 - (void)awakeFromNib {
@@ -136,17 +161,14 @@
   [self setSourceBrowserIndex:0];
 }
 
+// Run application modal.
 - (void)runModalDialog {
-  // If there is no parentWindow_ then this will present as a regular
-  // modal dialog not hanging off of any other window.
-  [NSApp beginSheet:[self window]
-     modalForWindow:parentWindow_
-      modalDelegate:self
-     didEndSelector:@selector(didEndSheet:returnCode:contextInfo:)
-        contextInfo:nil];
+  importSettingsDialogVisible = true;
+  [NSApp runModalForWindow:[self window]];
 }
 
 - (IBAction)ok:(id)sender {
+  [self closeDialog];
   const ProfileInfo& sourceProfile =
       importerList_.get()->GetSourceProfileInfoAt([self sourceBrowserIndex]);
   uint16 items = sourceProfile.services_supported;
@@ -165,20 +187,16 @@
     LOG(WARNING) << "There were no settings to import from '"
                  << sourceProfile.description << "'.";
   }
-  [NSApp endSheet:[self window]];
 }
 
 - (IBAction)cancel:(id)sender {
-  [NSApp endSheet:[self window]];
-}
-
-- (void)didEndSheet:(NSWindow*)sheet
-         returnCode:(int)returnCode
-        contextInfo:(void*)contextInfo {
-  [sheet close];
+  [self closeDialog];
 }
 
-- (void)windowWillClose:(NSNotification*)notification {
+- (void)closeDialog {
+  importSettingsDialogVisible = false;
+  [[self window] orderOut:self];
+  [NSApp stopModal];
   [self autorelease];
 }
 
diff --git a/chrome/browser/cocoa/import_settings_dialog_unittest.mm b/chrome/browser/cocoa/import_settings_dialog_unittest.mm
index c5585bf..6c55498 100644
--- a/chrome/browser/cocoa/import_settings_dialog_unittest.mm
+++ b/chrome/browser/cocoa/import_settings_dialog_unittest.mm
@@ -24,11 +24,11 @@ class ImportSettingsDialogTest : public CocoaTest {
         [ImportSettingsProfile
          importSettingsProfileWithBrowserName:@"MockSafari"
                                      services:safariServices];
-    unsigned int fairefoxServices = HISTORY | FAVORITES | COOKIES | PASSWORDS;
+    unsigned int firefoxServices = HISTORY | FAVORITES | COOKIES | PASSWORDS;
     ImportSettingsProfile* mockFirefox =
         [ImportSettingsProfile
          importSettingsProfileWithBrowserName:@"MockFirefox"
-                                     services:fairefoxServices];
+                                     services:firefoxServices];
     unsigned int caminoServices = HISTORY | COOKIES | SEARCH_ENGINES;
     ImportSettingsProfile* mockCamino =
         [ImportSettingsProfile
@@ -37,8 +37,7 @@ class ImportSettingsDialogTest : public CocoaTest {
     NSArray* browsers = [NSArray arrayWithObjects:
                          mockSafari, mockFirefox, mockCamino, nil];
     controller_ = [[ImportSettingsDialogController alloc]
-                   initWithProfiles:browsers parentWindow:nil];
-    [controller_ runModalDialog];  // Forces a nib load.
+                   initWithProfiles:browsers];
   }
 
   virtual void TearDown() {
@@ -58,13 +57,11 @@ TEST_F(ImportSettingsDialogTest, ChooseVariousBrowsers) {
   EXPECT_TRUE([controller_ historyAvailable]);
   EXPECT_TRUE([controller_ importFavorites]);
   EXPECT_TRUE([controller_ favoritesAvailable]);
-  EXPECT_TRUE([controller_ importCookies]);
-  EXPECT_TRUE([controller_ cookiesAvailable]);
   EXPECT_TRUE([controller_ importPasswords]);
   EXPECT_TRUE([controller_ passwordsAvailable]);
   EXPECT_TRUE([controller_ importSearchEngines]);
   EXPECT_TRUE([controller_ searchEnginesAvailable]);
-  EXPECT_EQ(HISTORY | FAVORITES | COOKIES | PASSWORDS | SEARCH_ENGINES,
+  EXPECT_EQ(HISTORY | FAVORITES | PASSWORDS | SEARCH_ENGINES,
             [controller_ servicesToImport]);
 
   // Next choice we test is MockCamino.
@@ -73,13 +70,11 @@ TEST_F(ImportSettingsDialogTest, ChooseVariousBrowsers) {
   EXPECT_TRUE([controller_ historyAvailable]);
   EXPECT_FALSE([controller_ importFavorites]);
   EXPECT_FALSE([controller_ favoritesAvailable]);
-  EXPECT_TRUE([controller_ importCookies]);
-  EXPECT_TRUE([controller_ cookiesAvailable]);
   EXPECT_FALSE([controller_ importPasswords]);
   EXPECT_FALSE([controller_ passwordsAvailable]);
   EXPECT_TRUE([controller_ importSearchEngines]);
   EXPECT_TRUE([controller_ searchEnginesAvailable]);
-  EXPECT_EQ(HISTORY | COOKIES | SEARCH_ENGINES, [controller_ servicesToImport]);
+  EXPECT_EQ(HISTORY | SEARCH_ENGINES, [controller_ servicesToImport]);
 
   // Next choice we test is MockFirefox.
   [controller_ setSourceBrowserIndex:1];
@@ -87,13 +82,11 @@ TEST_F(ImportSettingsDialogTest, ChooseVariousBrowsers) {
   EXPECT_TRUE([controller_ historyAvailable]);
   EXPECT_TRUE([controller_ importFavorites]);
   EXPECT_TRUE([controller_ favoritesAvailable]);
-  EXPECT_TRUE([controller_ importCookies]);
-  EXPECT_TRUE([controller_ cookiesAvailable]);
   EXPECT_TRUE([controller_ importPasswords]);
   EXPECT_TRUE([controller_ passwordsAvailable]);
   EXPECT_FALSE([controller_ importSearchEngines]);
   EXPECT_FALSE([controller_ searchEnginesAvailable]);
-  EXPECT_EQ(HISTORY | FAVORITES | COOKIES | PASSWORDS,
+  EXPECT_EQ(HISTORY | FAVORITES | PASSWORDS,
             [controller_ servicesToImport]);
 
   [controller_ cancel:nil];
@@ -103,10 +96,9 @@ TEST_F(ImportSettingsDialogTest, SetVariousSettings) {
   // Leave the choice MockSafari, but toggle the settings.
   [controller_ setImportHistory:NO];
   [controller_ setImportFavorites:NO];
-  [controller_ setImportCookies:NO];
   [controller_ setImportPasswords:NO];
   [controller_ setImportSearchEngines:NO];
-  EXPECT_EQ(0, [controller_ servicesToImport]);
+  EXPECT_EQ(NONE, [controller_ servicesToImport]);
   EXPECT_FALSE([controller_ importSomething]);
 
   [controller_ setImportHistory:YES];
@@ -117,13 +109,8 @@ TEST_F(ImportSettingsDialogTest, SetVariousSettings) {
   [controller_ setImportFavorites:YES];
   EXPECT_EQ(FAVORITES, [controller_ servicesToImport]);
   EXPECT_TRUE([controller_ importSomething]);
-
   [controller_ setImportFavorites:NO];
-  [controller_ setImportCookies:YES];
-  EXPECT_EQ(COOKIES, [controller_ servicesToImport]);
-  EXPECT_TRUE([controller_ importSomething]);
 
-  [controller_ setImportCookies:NO];
   [controller_ setImportPasswords:YES];
   EXPECT_EQ(PASSWORDS, [controller_ servicesToImport]);
   EXPECT_TRUE([controller_ importSomething]);
diff --git a/chrome/browser/cocoa/preferences_window_controller.mm b/chrome/browser/cocoa/preferences_window_controller.mm
index b3d435a..582cb6f 100644
--- a/chrome/browser/cocoa/preferences_window_controller.mm
+++ b/chrome/browser/cocoa/preferences_window_controller.mm
@@ -1151,11 +1151,7 @@ const int kDisabledIndex = 1;
 // Called to import data from other browsers (Safari, Firefox, etc).
 - (IBAction)importData:(id)sender {
   UserMetrics::RecordAction("Import_ShowDlg", profile_);
-
-  // Note that the dialog controller takes care of cleaning itself up
-  // upon dismissal so auto-scoping here is not necessary.
-  [[[ImportSettingsDialogController alloc]
-      initWithProfile:profile_ parentWindow:nil] runModalDialog];
+  [ImportSettingsDialogController showImportSettingsDialogForProfile:profile_];
 }
 
 // Called to clear user's browsing data. This puts up an application-modal
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index c6115d7..a74e318 100755
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -623,6 +623,7 @@
         'browser/cocoa/fullscreen_window_unittest.mm',
         'browser/cocoa/html_dialog_window_controller_unittest.mm',
         'browser/cocoa/hung_renderer_controller_unittest.mm',
+        'browser/cocoa/import_settings_dialog_unittest.mm',
         'browser/cocoa/info_bubble_view_unittest.mm',
         'browser/cocoa/info_bubble_window_unittest.mm',
         'browser/cocoa/infobar_container_controller_unittest.mm',