Add a scoped notifier for pref dictionaries / lists.

Other options included subclassing the pref value types, making the firenotifications public, or just friend classing.

Since values can contain other values, such as dictionary within dictionary, it didn't seem like a good idea to subclass. We might want to consider combining the scoped lookup with the GetMutableList call at some point, but some callers keep a long reference so that might be a bad idea.

BUG=38557
TEST=manual

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

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

7 files changed, 55 insertions, 7 deletions

diff --git a/chrome/browser/cocoa/preferences_window_controller.mm b/chrome/browser/cocoa/preferences_window_controller.mm
index 3503acc..cfe80bf 100644
--- a/chrome/browser/cocoa/preferences_window_controller.mm
+++ b/chrome/browser/cocoa/preferences_window_controller.mm
@@ -949,11 +949,14 @@ void PersonalDataManagerObserver::ShowAutoFillDialog(
   //             never match. Once support for broadcasting such updates is
   //             added, this will automagically start to work, and this comment
   //             can be removed.
-  if (*prefName == prefs::kURLsToRestoreOnStartup) {
-    const SessionStartupPref startupPref =
-        SessionStartupPref::GetStartupPref(prefs_);
-    [customPagesSource_ setURLs:startupPref.urls];
-  }
+  // TODO(chron): We comment out this block right now because we have put in
+  //              broadcast for notifications, but there's some workaround
+  //              currently present that causes an infinite loop.
+  // if (*prefName == prefs::kURLsToRestoreOnStartup) {
+  //  const SessionStartupPref startupPref =
+  //      SessionStartupPref::GetStartupPref(prefs_);
+  //  [customPagesSource_ setURLs:startupPref.urls];
+  // }
 
   if (*prefName == prefs::kHomePageIsNewTabPage) {
     NSInteger useNewTabPage = newTabPageIsHomePage_.GetValue() ? 0 : 1;
diff --git a/chrome/browser/pref_service.h b/chrome/browser/pref_service.h
index 85dd42c..f1819ad 100644
--- a/chrome/browser/pref_service.h
+++ b/chrome/browser/pref_service.h
@@ -27,6 +27,7 @@
 
 class NotificationObserver;
 class Preference;
+class ScopedPrefUpdate;
 
 class PrefService : public NonThreadSafe,
                     public ImportantFileWriter::DataSerializer {
@@ -158,7 +159,8 @@ class PrefService : public NonThreadSafe,
   // This method returns NULL only if you're requesting an unregistered pref or
   // a non-dict/non-list pref.
   // WARNING: Changes to the dictionary or list will not automatically notify
-  // pref observers. TODO(tc): come up with a way to still fire observers.
+  // pref observers.
+  // Use a ScopedPrefUpdate to update observers on changes.
   DictionaryValue* GetMutableDictionary(const wchar_t* path);
   ListValue* GetMutableList(const wchar_t* path);
 
@@ -216,6 +218,8 @@ class PrefService : public NonThreadSafe,
       PrefObserverMap;
   PrefObserverMap pref_observers_;
 
+  friend class ScopedPrefUpdate;
+
   DISALLOW_COPY_AND_ASSIGN(PrefService);
 };
 
diff --git a/chrome/browser/scoped_pref_update.cc b/chrome/browser/scoped_pref_update.cc
new file mode 100644
index 0000000..36d2306
--- /dev/null
+++ b/chrome/browser/scoped_pref_update.cc
@@ -0,0 +1,15 @@
+// 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/values.h"
+#include "chrome/browser/pref_service.h"
+#include "chrome/browser/scoped_pref_update.h"
+
+ScopedPrefUpdate::ScopedPrefUpdate(PrefService* service, const wchar_t* path)
+    : service_(service),
+      path_(path) {}
+
+ScopedPrefUpdate::~ScopedPrefUpdate() {
+  service_->FireObservers(path_.c_str());
+}
diff --git a/chrome/browser/scoped_pref_update.h b/chrome/browser/scoped_pref_update.h
new file mode 100644
index 0000000..661a99a
--- /dev/null
+++ b/chrome/browser/scoped_pref_update.h
@@ -0,0 +1,23 @@
+// 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.
+//
+// A helper class that assists preferences in firing notifications when lists
+// are changed.
+
+#ifndef CHROME_BROWSER_SCOPED_PREF_UPDATE_H_
+#define CHROME_BROWSER_SCOPED_PREF_UPDATE_H_
+
+#include "chrome/browser/pref_service.h"
+
+class ScopedPrefUpdate {
+ public:
+  ScopedPrefUpdate(PrefService* service, const wchar_t* path);
+  ~ScopedPrefUpdate();
+
+ private:
+  PrefService* service_;
+  std::wstring path_;
+};
+
+#endif
diff --git a/chrome/browser/session_startup_pref.cc b/chrome/browser/session_startup_pref.cc
index ff919d0..1da5802 100644
--- a/chrome/browser/session_startup_pref.cc
+++ b/chrome/browser/session_startup_pref.cc
@@ -10,6 +10,7 @@
 #include "chrome/browser/defaults.h"
 #include "chrome/browser/pref_service.h"
 #include "chrome/browser/profile.h"
+#include "chrome/browser/scoped_pref_update.h"
 #include "chrome/common/pref_names.h"
 
 namespace {
@@ -64,6 +65,7 @@ void SessionStartupPref::SetStartupPref(PrefService* prefs,
   // Always save the URLs, that way the UI can remain consistent even if the
   // user changes the startup type pref.
   // Ownership of the ListValue retains with the pref service.
+  ScopedPrefUpdate update(prefs, prefs::kURLsToRestoreOnStartup);
   ListValue* url_pref_list =
       prefs->GetMutableList(prefs::kURLsToRestoreOnStartup);
   DCHECK(url_pref_list);
diff --git a/chrome/browser/views/options/general_page_view.cc b/chrome/browser/views/options/general_page_view.cc
index 9cfc14f..5b86c35 100644
--- a/chrome/browser/views/options/general_page_view.cc
+++ b/chrome/browser/views/options/general_page_view.cc
@@ -156,7 +156,6 @@ void CustomHomePagesTableModel::Add(int index, const GURL& url) {
   DCHECK(index >= 0 && index <= RowCount());
   entries_.insert(entries_.begin() + static_cast<size_t>(index), Entry());
   entries_[index].url = url;
-  LoadFavIcon(&(entries_[index]));
   if (observer_)
     observer_->OnItemsAdded(index, 1);
 }
diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi
index 96ba447..08bc4ae 100755..100644
--- a/chrome/chrome_browser.gypi
+++ b/chrome/chrome_browser.gypi
@@ -1860,6 +1860,8 @@
         'browser/safe_browsing/safe_browsing_store_sqlite.h',
         'browser/safe_browsing/safe_browsing_util.cc',
         'browser/safe_browsing/safe_browsing_util.h',
+        'browser/scoped_pref_update.cc',
+        'browser/scoped_pref_update.h',
         'browser/search_engines/edit_search_engine_controller.cc',
         'browser/search_engines/edit_search_engine_controller.h',
         'browser/search_engines/keyword_editor_controller.cc',