Consolidate EulaAcceptedNotifier implementations.

chrome/browser/metrics/variations/eula_accepted_notifier.cc

Index: chrome/browser/metrics/variations/eula_accepted_notifier.cc
===================================================================
--- chrome/browser/metrics/variations/eula_accepted_notifier.cc	(revision 203196)
+++ chrome/browser/metrics/variations/eula_accepted_notifier.cc	(working copy)
@@ -4,11 +4,36 @@
 
 #include "chrome/browser/metrics/variations/eula_accepted_notifier.h"
 
+#include "base/bind.h"
 #include "base/logging.h"
+#include "base/prefs/pref_service.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/common/pref_names.h"
 
-EulaAcceptedNotifier::EulaAcceptedNotifier() : observer_(NULL) {
+// static
+EulaAcceptedNotifier* EulaAcceptedNotifier::Create() {
+#if !defined(OS_CHROMEOS) && !defined(OS_ANDROID) && !defined(OS_IOS)
+  // First run EULA only exists on ChromeOS, Android and iOS.
+  return NULL;
+#elif defined(OS_CHROMEOS) && !defined(GOOGLE_CHROME_BUILD)
+  // On ChromeOS, unofficial builds do not show the EULA, whereas on Android
+  // and iOS they do.
+  return NULL;
+#else

[SteveT, 2013/05/31 12:20:08]

Do you think it'd be cleaner if we did

#if (defined(OS_CHROMEOS) && defined(GOOGLE_CHROME_BUILD) || defined(OS_ANDROID)
|| defined(OS_IOS)
  // maybe make the EulaAcceptedNotifier
#else
  return NULL;
#endif

?

The first IF condition is nastier (especially with the extra brackets) and might
wrap, but it means that a reader can easily tell in precisely which situations
the EulaAcceptedNotifier may be created. (I say "may be" here and above because
of your additional FindPreference check below)

Thoughts?

[Alexei Svitkine (slow), 2013/05/31 15:25:17]

Done.

+  PrefService* local_state = g_browser_process->local_state();
+  // If the |kEulaAccepted| pref is not registered, return NULL which is
+  // equivalent to not needing to check the EULA. This is the case for some
+  // tests for higher-level classes that use the EulaAcceptNotifier.

[SteveT, 2013/05/31 12:20:08]

To be a bit more clear in this comment, is the kEulaAccepted pref not registered
JUST in these tests, or is this is a thing that can happen in a real runtime? A
little bit of clarity on that might be nice.

[Alexei Svitkine (slow), 2013/05/31 15:25:17]

Done.

+  if (local_state->FindPreference(prefs::kEulaAccepted) == NULL)
+    return NULL;
+  return new EulaAcceptedNotifier(local_state);
+#endif
 }
 
+EulaAcceptedNotifier::EulaAcceptedNotifier(PrefService* local_state)
+    : local_state_(local_state), observer_(NULL) {
+}
+
 EulaAcceptedNotifier::~EulaAcceptedNotifier() {
 }
 
@@ -17,13 +42,29 @@
   observer_ = observer;
 }
 
+bool EulaAcceptedNotifier::IsEulaAccepted() {
+  if (local_state_->GetBoolean(prefs::kEulaAccepted))
+    return true;
+
+  // Register for the notification, if this is the first time.
+  if (registrar_.IsEmpty()) {
+    registrar_.Init(local_state_);
+    registrar_.Add(prefs::kEulaAccepted,
+                   base::Bind(&EulaAcceptedNotifier::OnPrefChanged,
+                              base::Unretained(this)));
+  }
+  return false;
+}
+
 void EulaAcceptedNotifier::NotifyObserver() {
   observer_->OnEulaAccepted();
 }
 
-#if !defined(OS_CHROMEOS) && !defined(OS_ANDROID) && !defined(OS_IOS)
-// static
-EulaAcceptedNotifier* EulaAcceptedNotifier::Create() {
-  return NULL;
+void EulaAcceptedNotifier::OnPrefChanged() {
+  DCHECK(!registrar_.IsEmpty());
+  registrar_.RemoveAll();
+
+  DCHECK(local_state_->GetBoolean(prefs::kEulaAccepted));
+  observer_->OnEulaAccepted();
 }
-#endif
+