[Mac] Fix the custom homepages preferences to:

1) Make it actually work again.
2) Change the model so that it doesn't get into an infinite recursion cycle
   trying to update the model, notify observers, and then re-update.

BUG=49320
TEST=Chromium-->Preferences. Add custom home pages. Close Preferences and reopen. They are still there.
TEST=Open 3 web pages in tabs. Chromium-->Preferences. Use Current homepages. Close Preferences and reopen. They are still there.
TEST=With custom homepages set, go to Chromium-->Preferences-->UtH-->Reset to Defaults. Go back to Basics. No more custom homepages.

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

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

4 files changed, 117 insertions, 42 deletions

diff --git a/chrome/browser/cocoa/custom_home_pages_model.h b/chrome/browser/cocoa/custom_home_pages_model.h
index ac40c45..962bc79 100644
--- a/chrome/browser/cocoa/custom_home_pages_model.h
+++ b/chrome/browser/cocoa/custom_home_pages_model.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -10,6 +10,7 @@
 
 #include <vector>
 #include "base/scoped_nsobject.h"
+#include "chrome/browser/history/history.h"
 #include "googleurl/src/gurl.h"
 
 class Profile;
@@ -36,6 +37,10 @@ class Profile;
 - (std::vector<GURL>)URLs;
 - (void)setURLs:(const std::vector<GURL>&)urls;
 
+// Reloads the URLs from their stored state. This will notify using KVO
+// |customHomePages|.
+- (void)reloadURLs;
+
 // Validates the set of URLs stored in the model. The user may have input bad
 // data. This function removes invalid entries from the model, which will result
 // in anyone observing being updated.
@@ -48,11 +53,31 @@ class Profile;
 - (void)removeObjectFromCustomHomePagesAtIndex:(NSUInteger)index;
 @end
 
-@interface CustomHomePagesModel(InternalOrTestingAPI)
+////////////////////////////////////////////////////////////////////////////////
+
+// An entry representing a single item in the custom home page model. Stores
+// a url and a favicon.
+@interface CustomHomePageEntry : NSObject {
+ @private
+  scoped_nsobject<NSString> url_;
+  scoped_nsobject<NSImage> icon_;
+
+  // If non-zero, indicates we're loading the favicon for the page.
+  HistoryService::Handle icon_handle_;
+}
+
+@property(nonatomic, copy) NSString* URL;
+@property(nonatomic, retain) NSImage* image;
+
+@end
+
+////////////////////////////////////////////////////////////////////////////////
+
+@interface CustomHomePagesModel (InternalOrTestingAPI)
 
 // Clears the URL string at the specified index. This constitutes bad data. The
 // validator should scrub the entry from the list the next time it is run.
-- (void) setURLStringEmptyAt:(NSUInteger)index;
+- (void)setURLStringEmptyAt:(NSUInteger)index;
 
 @end
 
diff --git a/chrome/browser/cocoa/custom_home_pages_model.mm b/chrome/browser/cocoa/custom_home_pages_model.mm
index 084cf11..9d47c415 100644
--- a/chrome/browser/cocoa/custom_home_pages_model.mm
+++ b/chrome/browser/cocoa/custom_home_pages_model.mm
@@ -5,29 +5,16 @@
 #import "chrome/browser/cocoa/custom_home_pages_model.h"
 
 #include "base/sys_string_conversions.h"
-#include "chrome/browser/history/history.h"
 #include "chrome/browser/net/url_fixer_upper.h"
+#include "chrome/browser/session_startup_pref.h"
 
 NSString* const kHomepageEntryChangedNotification =
     @"kHomepageEntryChangedNotification";
 
-// An entry representing a single item in the custom home page model. Stores
-// a url and a favicon.
-@interface CustomHomePageEntry : NSObject {
- @private
-  scoped_nsobject<NSString> url_;
-  scoped_nsobject<NSImage> icon_;
-
-  // If non-zero, indicates we're loading the favicon for the page.
-  HistoryService::Handle icon_handle_;
-}
-@property(nonatomic, copy) NSString* URL;
-@property(nonatomic, retain) NSImage* image;
+@interface CustomHomePagesModel (Private)
+- (void)setURLsInternal:(const std::vector<GURL>&)urls;
 @end
 
-//----------------------------------------------------------------------------
-
-
 @implementation CustomHomePagesModel
 
 - (id)initWithProfile:(Profile*)profile {
@@ -52,6 +39,8 @@ NSString* const kHomepageEntryChangedNotification =
 
 - (void)removeObjectFromCustomHomePagesAtIndex:(NSUInteger)index {
   [entries_ removeObjectAtIndex:index];
+  // Force a save.
+  [self validateURLs];
 }
 
 // Get/set the urls the model currently contains as a group. These will weed
@@ -70,6 +59,15 @@ NSString* const kHomepageEntryChangedNotification =
 
 - (void)setURLs:(const std::vector<GURL>&)urls {
   [self willChangeValueForKey:@"customHomePages"];
+  [self setURLsInternal:urls];
+  SessionStartupPref pref(SessionStartupPref::GetStartupPref(profile_));
+  pref.urls = urls;
+  SessionStartupPref::SetStartupPref(profile_, pref);
+  [self didChangeValueForKey:@"customHomePages"];
+}
+
+// Converts the C++ URLs to Cocoa objects without notifying KVO.
+- (void)setURLsInternal:(const std::vector<GURL>&)urls {
   [entries_ removeAllObjects];
   for (size_t i = 0; i < urls.size(); ++i) {
     scoped_nsobject<CustomHomePageEntry> entry(
@@ -81,6 +79,12 @@ NSString* const kHomepageEntryChangedNotification =
       [entries_ addObject:entry];
     }
   }
+}
+
+- (void)reloadURLs {
+  [self willChangeValueForKey:@"customHomePages"];
+  SessionStartupPref pref(SessionStartupPref::GetStartupPref(profile_));
+  [self setURLsInternal:pref.urls];
   [self didChangeValueForKey:@"customHomePages"];
 }
 
@@ -129,4 +133,8 @@ NSString* const kHomepageEntryChangedNotification =
   return icon_.get();
 }
 
+- (NSString*)description {
+  return url_.get();
+}
+
 @end
diff --git a/chrome/browser/cocoa/custom_home_pages_model_unittest.mm b/chrome/browser/cocoa/custom_home_pages_model_unittest.mm
index 3d7a32b..addb0e0 100644
--- a/chrome/browser/cocoa/custom_home_pages_model_unittest.mm
+++ b/chrome/browser/cocoa/custom_home_pages_model_unittest.mm
@@ -1,10 +1,11 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
 #include "base/scoped_nsobject.h"
 #include "chrome/browser/cocoa/browser_test_helper.h"
 #import "chrome/browser/cocoa/custom_home_pages_model.h"
+#include "chrome/browser/session_startup_pref.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/platform_test.h"
 
@@ -29,6 +30,24 @@
 }
 @end
 
+@interface NSObject ()
+- (void)setURL:(NSString*)url;
+@end
+
+namespace {
+
+// Helper that creates an autoreleased entry.
+CustomHomePageEntry* MakeEntry(NSString* url) {
+  CustomHomePageEntry* entry = [[[CustomHomePageEntry alloc] init] autorelease];
+  [entry setURL:url];
+  return entry;
+}
+
+// Helper that casts from |id| to the Entry type and returns the URL string.
+NSString* EntryURL(id entry) {
+  return [static_cast<CustomHomePageEntry*>(entry) URL];
+}
+
 class CustomHomePagesModelTest : public PlatformTest {
  public:
   CustomHomePagesModelTest() {
@@ -78,9 +97,9 @@ TEST_F(CustomHomePagesModelTest, KVOObserveWhenListChanges) {
   scoped_nsobject<CustomHomePageHelper> kvo_helper(
       [[CustomHomePageHelper alloc] init]);
   [model_ addObserver:kvo_helper
-          forKeyPath:@"customHomePages"
-             options:0L
-             context:NULL];
+           forKeyPath:@"customHomePages"
+              options:0L
+              context:NULL];
   EXPECT_FALSE(kvo_helper.get()->sawNotification_);
 
   std::vector<GURL> urls;
@@ -99,38 +118,35 @@ TEST_F(CustomHomePagesModelTest, KVO) {
   scoped_nsobject<CustomHomePageHelper> kvo_helper(
       [[CustomHomePageHelper alloc] init]);
   [model_ addObserver:kvo_helper
-          forKeyPath:@"customHomePages"
-             options:0L
-             context:NULL];
+           forKeyPath:@"customHomePages"
+              options:0L
+              context:NULL];
   EXPECT_FALSE(kvo_helper.get()->sawNotification_);
 
   // Cheat and insert NSString objects into the array. As long as we don't
   // call -URLs, we'll be ok.
-  [model_ insertObject:@"www.google.com" inCustomHomePagesAtIndex:0];
+  [model_ insertObject:MakeEntry(@"www.google.com") inCustomHomePagesAtIndex:0];
   EXPECT_TRUE(kvo_helper.get()->sawNotification_);
-  [model_ insertObject:@"www.yahoo.com" inCustomHomePagesAtIndex:1];
-  [model_ insertObject:@"dev.chromium.org" inCustomHomePagesAtIndex:2];
+  [model_ insertObject:MakeEntry(@"www.yahoo.com") inCustomHomePagesAtIndex:1];
+  [model_ insertObject:MakeEntry(@"dev.chromium.org")
+      inCustomHomePagesAtIndex:2];
   EXPECT_EQ([model_ countOfCustomHomePages], 3U);
 
-  EXPECT_TRUE([[model_ objectInCustomHomePagesAtIndex:1]
-                  isEqualToString:@"www.yahoo.com"]);
+  EXPECT_TRUE([EntryURL([model_ objectInCustomHomePagesAtIndex:1])
+                  isEqualToString:@"http://www.yahoo.com/"]);
 
   kvo_helper.get()->sawNotification_ = NO;
   [model_ removeObjectFromCustomHomePagesAtIndex:1];
   EXPECT_TRUE(kvo_helper.get()->sawNotification_);
   EXPECT_EQ([model_ countOfCustomHomePages], 2U);
-  EXPECT_TRUE([[model_ objectInCustomHomePagesAtIndex:1]
-                  isEqualToString:@"dev.chromium.org"]);
-  EXPECT_TRUE([[model_ objectInCustomHomePagesAtIndex:0]
-                  isEqualToString:@"www.google.com"]);
+  EXPECT_TRUE([EntryURL([model_ objectInCustomHomePagesAtIndex:1])
+                  isEqualToString:@"http://dev.chromium.org/"]);
+  EXPECT_TRUE([EntryURL([model_ objectInCustomHomePagesAtIndex:0])
+                  isEqualToString:@"http://www.google.com/"]);
 
   [model_ removeObserver:kvo_helper forKeyPath:@"customHomePages"];
 }
 
-@interface NSObject()
-- (void)setURL:(NSString*)url;
-@end
-
 // Test that when individual items are changed that they broadcast a message.
 TEST_F(CustomHomePagesModelTest, ModelChangedNotification) {
   scoped_nsobject<CustomHomePageHelper> kvo_helper(
@@ -149,3 +165,31 @@ TEST_F(CustomHomePagesModelTest, ModelChangedNotification) {
   EXPECT_TRUE(kvo_helper.get()->sawNotification_);
   [[NSNotificationCenter defaultCenter] removeObserver:kvo_helper];
 }
+
+TEST_F(CustomHomePagesModelTest, ReloadURLs) {
+  scoped_nsobject<CustomHomePageHelper> kvo_helper(
+      [[CustomHomePageHelper alloc] init]);
+  [model_ addObserver:kvo_helper
+           forKeyPath:@"customHomePages"
+              options:0L
+              context:NULL];
+  EXPECT_FALSE(kvo_helper.get()->sawNotification_);
+  EXPECT_EQ([model_ countOfCustomHomePages], 0U);
+
+  std::vector<GURL> urls;
+  urls.push_back(GURL("http://www.google.com"));
+  SessionStartupPref pref;
+  pref.urls = urls;
+  SessionStartupPref::SetStartupPref(helper_.profile(), pref);
+
+  [model_ reloadURLs];
+
+  EXPECT_TRUE(kvo_helper.get()->sawNotification_);
+  EXPECT_EQ([model_ countOfCustomHomePages], 1U);
+  EXPECT_TRUE([EntryURL([model_ objectInCustomHomePagesAtIndex:0])
+                  isEqualToString:@"http://www.google.com/"]);
+
+  [model_ removeObserver:kvo_helper.get() forKeyPath:@"customHomePages"];
+}
+
+}  // namespace
diff --git a/chrome/browser/cocoa/preferences_window_controller.mm b/chrome/browser/cocoa/preferences_window_controller.mm
index d7ec2de..cbf7b3f 100644
--- a/chrome/browser/cocoa/preferences_window_controller.mm
+++ b/chrome/browser/cocoa/preferences_window_controller.mm
@@ -884,9 +884,7 @@ class ManagedPrefsBannerState : public ManagedPrefsBannerBase {
   }
 
   if (*prefName == prefs::kURLsToRestoreOnStartup) {
-    const SessionStartupPref startupPref =
-        SessionStartupPref::GetStartupPref(prefs_);
-    [customPagesSource_ setURLs:startupPref.urls];
+    [customPagesSource_ reloadURLs];
   }
 
   if (*prefName == prefs::kHomePageIsNewTabPage) {