[Mac] Polish the search engine manager

* Make the edit search engine window a sheet.
* Only allow one instance of the search engine manager to be opened at once.
* The search engine manager now remembers its position.
* Create NSWindow(LocalStateAdditions) category to assist with storing window
  position in Chromium's local state.

BUG=21761,21762,21883,21996
TEST=Editing/adding a search engine happens in a sheet. Press [Manage] multiple times and only 1 window should open. Press [Manage] and the window should be at its last position.

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

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

10 files changed, 198 insertions, 62 deletions

diff --git a/chrome/browser/cocoa/edit_search_engine_cocoa_controller.mm b/chrome/browser/cocoa/edit_search_engine_cocoa_controller.mm
index 00e4913..d3e2131 100644
--- a/chrome/browser/cocoa/edit_search_engine_cocoa_controller.mm
+++ b/chrome/browser/cocoa/edit_search_engine_cocoa_controller.mm
@@ -102,8 +102,18 @@ void ShiftOriginY(NSView* view, CGFloat amount) {
   [self autorelease];
 }
 
+// Performs the logic of closing the window. If we are a sheet, then it ends the
+// modal session; otherwise, it closes the window.
+- (void)doClose {
+  if ([[self window] isSheet]) {
+    [NSApp endSheet:[self window]];
+  } else {
+    [[self window] close];
+  }
+}
+
 - (IBAction)cancel:(id)sender {
-  [[self window] close];
+  [self doClose];
 }
 
 - (IBAction)save:(id)sender {
@@ -112,7 +122,7 @@ void ShiftOriginY(NSView* view, CGFloat amount) {
   std::wstring keyword = base::SysNSStringToWide([keywordField_ stringValue]);
   std::wstring url = base::SysNSStringToWide([urlField_ stringValue]);
   controller_->AcceptAddOrEdit(title, keyword, url);
-  [[self window] close];
+  [self doClose];
 }
 
 // Delegate method for the text fields.
diff --git a/chrome/browser/cocoa/keyword_editor_cocoa_controller.mm b/chrome/browser/cocoa/keyword_editor_cocoa_controller.mm
index d564b6b..4d90f15 100644
--- a/chrome/browser/cocoa/keyword_editor_cocoa_controller.mm
+++ b/chrome/browser/cocoa/keyword_editor_cocoa_controller.mm
@@ -6,10 +6,14 @@
 
 #import "base/mac_util.h"
 #include "base/sys_string_conversions.h"
+#include "chrome/browser/browser_process.h"
 #import "chrome/browser/cocoa/edit_search_engine_cocoa_controller.h"
+#import "chrome/browser/cocoa/nswindow_local_state.h"
 #import "chrome/browser/cocoa/keyword_editor_cocoa_controller.h"
 #include "chrome/browser/profile.h"
 #include "chrome/browser/search_engines/template_url_table_model.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/common/pref_service.h"
 #include "grit/generated_resources.h"
 #include "third_party/GTM/AppKit/GTMUILocalizerAndLayoutTweaker.h"
 
@@ -76,6 +80,14 @@ void KeywordEditorModelObserver::OnEditedKeyword(
   size.height = NSHeight([addButton_ frame]);
   [makeDefaultButton_ setFrameSize:size];
 
+  // Restore the window position.
+  if (g_browser_process && g_browser_process->local_state()) {
+    PrefService* prefs = g_browser_process->local_state();
+    NSWindow* window = [self window];
+    [window restoreWindowPositionFromPrefs:prefs
+                                withPath:prefs::kKeywordEditorWindowPlacement];
+  }
+
   [self adjustEditingButtons];
   [tableView_ setDoubleAction:@selector(editKeyword:)];
   [tableView_ setTarget:self];
@@ -86,6 +98,16 @@ void KeywordEditorModelObserver::OnEditedKeyword(
   [self autorelease];
 }
 
+// The last page info window that was moved will determine the location of the
+// next new one.
+- (void)windowDidMove:(NSNotification*)notif {
+  if (g_browser_process && g_browser_process->local_state()) {
+    NSWindow* window = [self window];
+    [window saveWindowPositionToPrefs:g_browser_process->local_state()
+                             withPath:prefs::kKeywordEditorWindowPlacement];
+  }
+}
+
 - (void)modelChanged {
   [tableView_ reloadData];
 }
@@ -94,13 +116,23 @@ void KeywordEditorModelObserver::OnEditedKeyword(
   return controller_.get();
 }
 
+- (void)sheetDidEnd:(NSWindow*)sheet
+         returnCode:(NSInteger)code
+            context:(void*)context {
+  [sheet orderOut:self];
+}
+
 - (IBAction)addKeyword:(id)sender {
   // The controller will release itself when the window closes.
   EditSearchEngineCocoaController* editor =
       [[EditSearchEngineCocoaController alloc] initWithProfile:profile_
                                                       delegate:observer_.get()
                                                    templateURL:NULL];
-  [[editor window] makeKeyAndOrderFront:self];
+  [NSApp beginSheet:[editor window]
+     modalForWindow:[self window]
+      modalDelegate:self
+     didEndSelector:@selector(sheetDidEnd:returnCode:context:)
+        contextInfo:NULL];
 }
 
 - (void)editKeyword:(id)sender {
@@ -113,7 +145,11 @@ void KeywordEditorModelObserver::OnEditedKeyword(
       [[EditSearchEngineCocoaController alloc] initWithProfile:profile_
                                                       delegate:observer_.get()
                                                    templateURL:url];
-  [[editor window] makeKeyAndOrderFront:self];
+  [NSApp beginSheet:[editor window]
+     modalForWindow:[self window]
+      modalDelegate:self
+     didEndSelector:@selector(sheetDidEnd:returnCode:context:)
+        contextInfo:NULL];
 }
 
 - (IBAction)deleteKeyword:(id)sender {
diff --git a/chrome/browser/cocoa/nswindow_local_state.h b/chrome/browser/cocoa/nswindow_local_state.h
new file mode 100644
index 0000000..3a3394c
--- /dev/null
+++ b/chrome/browser/cocoa/nswindow_local_state.h
@@ -0,0 +1,21 @@
+// Copyright (c) 2009 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.
+
+class PrefService;
+@class NSWindow;
+
+// These additions to NSWindow assist in saving and restoring a window's
+// position to Chromium's local state preferences.
+
+@interface NSWindow (LocalStateAdditions)
+
+// Saves the window's origin into the given PrefService. Caller is responsible
+// for making sure |prefs| is not NULL.
+- (void)saveWindowPositionToPrefs:(PrefService*)prefs 
+                         withPath:(const wchar_t*)path;
+
+- (void)restoreWindowPositionFromPrefs:(PrefService*)prefs
+                              withPath:(const wchar_t*)path;
+
+@end
diff --git a/chrome/browser/cocoa/nswindow_local_state.mm b/chrome/browser/cocoa/nswindow_local_state.mm
new file mode 100644
index 0000000..7fdfff6
--- /dev/null
+++ b/chrome/browser/cocoa/nswindow_local_state.mm
@@ -0,0 +1,37 @@
+// Copyright (c) 2009 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.
+
+#import <Cocoa/Cocoa.h>
+#include "chrome/common/pref_service.h"
+
+@implementation NSWindow (LocalStateAdditions)
+
+- (void)saveWindowPositionToPrefs:(PrefService*)prefs 
+                         withPath:(const wchar_t*)path {
+  DCHECK(prefs);
+  // Save the origin of the window.
+  DictionaryValue* windowPrefs = prefs->GetMutableDictionary(path);
+  NSRect frame = [self frame];
+  windowPrefs->SetInteger(L"x", frame.origin.x);
+  windowPrefs->SetInteger(L"y", frame.origin.y);
+}
+
+- (void)restoreWindowPositionFromPrefs:(PrefService*)prefs
+                              withPath:(const wchar_t*)path {
+  DCHECK(prefs);
+  // Get the positioning information.
+  DictionaryValue* windowPrefs = prefs->GetMutableDictionary(path);
+  int x = 0, y = 0;
+  windowPrefs->GetInteger(L"x", &x);
+  windowPrefs->GetInteger(L"y", &y);
+  // Turn the origin (lower-left) into an upper-left window point.
+  NSPoint upperLeft = NSMakePoint(x, y + [self frame].size.height);
+  NSPoint cascadePoint = [self cascadeTopLeftFromPoint:upperLeft];
+  // Cascade again to get the offset when opening new windows.
+  [self cascadeTopLeftFromPoint:cascadePoint];
+  // Force a save of the pref.
+  [self saveWindowPositionToPrefs:prefs withPath:path];
+}
+
+@end
diff --git a/chrome/browser/cocoa/nswindow_local_state_unittest.mm b/chrome/browser/cocoa/nswindow_local_state_unittest.mm
new file mode 100644
index 0000000..6d4fab1
--- /dev/null
+++ b/chrome/browser/cocoa/nswindow_local_state_unittest.mm
@@ -0,0 +1,66 @@
+// Copyright (c) 2009 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.
+
+#import <Cocoa/Cocoa.h>
+
+#include "base/scoped_nsobject.h"
+#include "chrome/browser/cocoa/browser_test_helper.h"
+#import "chrome/browser/cocoa/cocoa_test_helper.h"
+#import "chrome/browser/cocoa/nswindow_local_state.h"
+#include "chrome/common/pref_service.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "testing/platform_test.h"
+
+class NSWindowLocalStateTest : public PlatformTest {
+  virtual void SetUp() {
+    path_ = L"NSWindowLocalStateTest";
+    window_.reset(
+        [[NSWindow alloc] initWithContentRect:NSMakeRect(100, 100, 20, 20)
+                                    styleMask:NSTitledWindowMask
+                                      backing:NSBackingStoreBuffered
+                                        defer:NO]);
+    browser_helper_.profile()->GetPrefs()->RegisterDictionaryPref(path_);
+  }
+
+ public:
+  CocoaTestHelper cocoa_helper_;
+  BrowserTestHelper browser_helper_;
+  scoped_nsobject<NSWindow> window_;
+  const wchar_t* path_;
+};
+
+TEST_F(NSWindowLocalStateTest, SaveWindowPlacement) {
+  PrefService* prefs = browser_helper_.profile()->GetPrefs();
+  ASSERT_TRUE(prefs != NULL);
+
+  // Check to make sure there is no existing pref for window placement.
+  ASSERT_TRUE(prefs->GetDictionary(path_) == NULL);
+
+  // Ask the window to save its position, then check that a preference
+  // exists.  We're technically passing in a pointer to the user prefs
+  // and not the local state prefs, but a PrefService* is a
+  // PrefService*, and this is a unittest.
+  [window_ saveWindowPositionToPrefs:prefs withPath:path_];
+  EXPECT_TRUE(prefs->GetDictionary(path_) != NULL);
+  int x, y;
+  DictionaryValue* windowPrefs = prefs->GetMutableDictionary(path_);
+  windowPrefs->GetInteger(L"x", &x);
+  windowPrefs->GetInteger(L"y", &y);
+  EXPECT_EQ(x, [window_ frame].origin.x);
+  EXPECT_EQ(y, [window_ frame].origin.y);
+}
+
+TEST_F(NSWindowLocalStateTest, RestoreWindowPlacement) {
+  PrefService* prefs = browser_helper_.profile()->GetPrefs();
+  DictionaryValue* windowPrefs = prefs->GetMutableDictionary(path_);
+
+  // Large enough so that the window is on screen without cascasding to a
+  // totally new location.
+  const int value = 420;
+  windowPrefs->SetInteger(L"x", value);
+  windowPrefs->SetInteger(L"y", value);
+  [window_ restoreWindowPositionFromPrefs:prefs withPath:path_];
+  EXPECT_LT(value, [window_ frame].origin.x);
+  EXPECT_GT(value, [window_ frame].origin.y);
+}
diff --git a/chrome/browser/cocoa/page_info_window_controller.h b/chrome/browser/cocoa/page_info_window_controller.h
index c62893b..5ad689b 100644
--- a/chrome/browser/cocoa/page_info_window_controller.h
+++ b/chrome/browser/cocoa/page_info_window_controller.h
@@ -59,11 +59,3 @@ class PrefService;
 - (void)setShowHistoryBox:(BOOL)show;
 
 @end
-
-@interface PageInfoWindowController (ExposedForTesting)
-
-// Saves the window's origin into the given PrefService. Caller is responsible
-// for making sure |prefs| is not NULL.
-- (void)saveWindowPositionToPrefs:(PrefService*)prefs;
-
-@end
diff --git a/chrome/browser/cocoa/page_info_window_controller.mm b/chrome/browser/cocoa/page_info_window_controller.mm
index 9f3b813..d8eb7c7 100644
--- a/chrome/browser/cocoa/page_info_window_controller.mm
+++ b/chrome/browser/cocoa/page_info_window_controller.mm
@@ -7,6 +7,7 @@
 #include "base/mac_util.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
+#import "chrome/browser/cocoa/nswindow_local_state.h"
 #include "chrome/browser/cocoa/page_info_window_mac.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/pref_service.h"
@@ -45,17 +46,8 @@
   if (g_browser_process && g_browser_process->local_state()) {
     // Get the positioning information.
     PrefService* prefs = g_browser_process->local_state();
-    DictionaryValue* windowPrefs =
-        prefs->GetMutableDictionary(prefs::kPageInfoWindowPlacement);
-    int x = 0, y = 0;
-    windowPrefs->GetInteger(L"x", &x);
-    windowPrefs->GetInteger(L"y", &y);
-    // Turn the origin (lower-left) into an upper-left window point.
-    NSPoint upperLeft = NSMakePoint(x, y + [[self window] frame].size.height);
-    NSPoint cascadePoint = [[self window] cascadeTopLeftFromPoint:upperLeft];
-    // Cascade again to get the offset when opening new windows.
-    [[self window] cascadeTopLeftFromPoint:cascadePoint];
-    [self saveWindowPositionToLocalState];  // Force a save of the pref.
+    [[self window] restoreWindowPositionFromPrefs:prefs
+                                   withPath:prefs::kPageInfoWindowPlacement];
   }
 
   // By default, assume we have no history information.
@@ -116,25 +108,9 @@
 // The last page info window that was moved will determine the location of the
 // next new one.
 - (void)windowDidMove:(NSNotification*)notif {
-  [self saveWindowPositionToLocalState];
-}
-
-// Saves the window preference to the local state.
-- (void)saveWindowPositionToLocalState {
-  if (!g_browser_process || !g_browser_process->local_state())
-    return;
-  [self saveWindowPositionToPrefs:g_browser_process->local_state()];
-}
-
-// Saves the window's origin into the given PrefService. Caller is responsible
-// for making sure |prefs| is not NULL.
-- (void)saveWindowPositionToPrefs:(PrefService*)prefs {
-  // Save the origin of the window.
-  DictionaryValue* windowPrefs = prefs->GetMutableDictionary(
-      prefs::kPageInfoWindowPlacement);
-  NSRect frame = [[self window] frame];
-  windowPrefs->SetInteger(L"x", frame.origin.x);
-  windowPrefs->SetInteger(L"y", frame.origin.y);
+  if (g_browser_process && g_browser_process->local_state())
+    [[self window] saveWindowPositionToPrefs:g_browser_process->local_state()
+                                    withPath:prefs::kPageInfoWindowPlacement];
 }
 
 @end
diff --git a/chrome/browser/cocoa/page_info_window_controller_unittest.mm b/chrome/browser/cocoa/page_info_window_controller_unittest.mm
index 66018a2..8c92098 100644
--- a/chrome/browser/cocoa/page_info_window_controller_unittest.mm
+++ b/chrome/browser/cocoa/page_info_window_controller_unittest.mm
@@ -50,20 +50,3 @@ TEST_F(PageInfoWindowControllerTest, TestShrink) {
   EXPECT_LE(newFrame.size.height, frame.size.height);
   EXPECT_GE(newFrame.origin.y, frame.origin.y);
 }
-
-
-TEST_F(PageInfoWindowControllerTest, TestSaveWindowPlacement) {
-  PrefService* prefs = helper_.profile()->GetPrefs();
-  ASSERT_TRUE(prefs != NULL);
-
-  // Check to make sure there is no existing pref for window placement.
-  ASSERT_TRUE(prefs->GetDictionary(prefs::kPageInfoWindowPlacement) == NULL);
-
-  // Ask the window to save its position, then check that a preference
-  // exists.  We're technically passing in a pointer to the user prefs
-  // and not the local state prefs, but a PrefService* is a
-  // PrefService*, and this is a unittest.
-  [controller_ saveWindowPositionToPrefs:prefs];
-  EXPECT_TRUE(prefs->GetDictionary(prefs::kPageInfoWindowPlacement) != NULL);
-}
-
diff --git a/chrome/browser/cocoa/preferences_window_controller.h b/chrome/browser/cocoa/preferences_window_controller.h
index 7b333ee..5aa8b95 100644
--- a/chrome/browser/cocoa/preferences_window_controller.h
+++ b/chrome/browser/cocoa/preferences_window_controller.h
@@ -9,6 +9,7 @@
 #include "chrome/common/pref_member.h"
 
 @class CustomHomePagesModel;
+@class KeywordEditorCocoaController;
 class PrefObserverBridge;
 class PrefService;
 class Profile;
@@ -42,6 +43,7 @@ class Profile;
   BooleanPrefMember showHomeButton_;
   BooleanPrefMember showPageOptionButtons_;
   scoped_nsobject<SearchEngineListModel> searchEngineModel_;
+  KeywordEditorCocoaController* keywordEditorController_;  // weak
   // Used when creating a new home page url to make the new cell editable.
   BOOL pendingSelectForEdit_;
 
diff --git a/chrome/browser/cocoa/preferences_window_controller.mm b/chrome/browser/cocoa/preferences_window_controller.mm
index 26a82bb..b3f234d 100644
--- a/chrome/browser/cocoa/preferences_window_controller.mm
+++ b/chrome/browser/cocoa/preferences_window_controller.mm
@@ -540,11 +540,24 @@ enum { kHomepageNewTabPage, kHomepageURL };
   [self setSearchEngineSelectedIndex:[self searchEngineSelectedIndex]];
 }
 
+// Notification receiver for when the keyword editor window closes. The
+// controller will clean up the window, but we need to clean up.
+- (void)keywordEditorClosed:(NSNotification*)notif {
+  keywordEditorController_ = nil;
+}
+
 // Brings up the edit search engines window.
 - (IBAction)manageSearchEngines:(id)sender {
-  KeywordEditorCocoaController* controller =
-      [[KeywordEditorCocoaController alloc] initWithProfile:profile_];
-  [[controller window] makeKeyAndOrderFront:sender];
+  if (!keywordEditorController_) {
+    keywordEditorController_ =
+        [[KeywordEditorCocoaController alloc] initWithProfile:profile_];
+    NSNotificationCenter* center = [NSNotificationCenter defaultCenter];
+    [center addObserver:self
+               selector:@selector(keywordEditorClosed:)
+                   name:NSWindowWillCloseNotification
+                 object:[keywordEditorController_ window]];
+  }
+  [[keywordEditorController_ window] makeKeyAndOrderFront:sender];
 }
 
 // Called when the user clicks the button to make Chromium the default