Consolidate EulaAcceptedNotifier implementations.

Adds a test and removes the ChromeOS-specific notification
that previously existed only for notifying the
EulaAcceptedNotifierChromeos class.

BUG=245437
TEST=New unit test, no functionality change.

Review URL: https://chromiumcodereview.appspot.com/16129004

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

chrome/browser/chromeos/login/wizard_controller.cc

@@ -471,11 +471,6 @@ void WizardController::OnEulaAccepted() {
   bool uma_enabled =
       OptionsUtil::ResolveMetricsReportingEnabled(usage_statistics_reporting_);
 
-  content::NotificationService::current()->Notify(
-      chrome::NOTIFICATION_WIZARD_EULA_ACCEPTED,
-      content::NotificationSource(content::Source<WizardController>(this)),
-      content::NotificationService::NoDetails());
-
   CrosSettings::Get()->SetBoolean(kStatsReportingPref, uma_enabled);
   if (uma_enabled) {
 #if defined(USE_LINUX_BREAKPAD) && defined(GOOGLE_CHROME_BUILD)

chrome/browser/metrics/variations/eula_accepted_notifier.cc

@@ -4,9 +4,14 @@
 
 #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) {
+EulaAcceptedNotifier::EulaAcceptedNotifier(PrefService* local_state)
+    : local_state_(local_state), observer_(NULL) {
 }
 
 EulaAcceptedNotifier::~EulaAcceptedNotifier() {
@@ -17,13 +22,47 @@ void EulaAcceptedNotifier::Init(Observer* observer) {
   observer_ = observer;
 }
 
-void EulaAcceptedNotifier::NotifyObserver() {
-  observer_->OnEulaAccepted();
+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;
 }
 
-#if !defined(OS_CHROMEOS) && !defined(OS_ANDROID) && !defined(OS_IOS)
 // static
 EulaAcceptedNotifier* EulaAcceptedNotifier::Create() {
+  // First run EULA only exists on ChromeOS, Android and iOS. On ChromeOS, it is
+  // only shown in official builds.
+#if (defined(OS_CHROMEOS) && defined(GOOGLE_CHROME_BUILD)) || \
+    defined(OS_ANDROID) || defined(OS_IOS)
+  PrefService* local_state = g_browser_process->local_state();
+  // Tests that use higher-level classes that use EulaAcceptNotifier may not
+  // register this pref. In this case, return NULL which is equivalent to not
+  // needing to check the EULA.
+  if (local_state->FindPreference(prefs::kEulaAccepted) == NULL)
+    return NULL;
+  return new EulaAcceptedNotifier(local_state);
+#else
   return NULL;
-}
 #endif
+}
+
+void EulaAcceptedNotifier::NotifyObserver() {
+  observer_->OnEulaAccepted();
+}
+
+void EulaAcceptedNotifier::OnPrefChanged() {
+  DCHECK(!registrar_.IsEmpty());
+  registrar_.RemoveAll();
+
+  DCHECK(local_state_->GetBoolean(prefs::kEulaAccepted));
+  observer_->OnEulaAccepted();
+}
+

chrome/browser/metrics/variations/eula_accepted_notifier.h

@@ -6,10 +6,12 @@
 #define CHROME_BROWSER_METRICS_VARIATIONS_EULA_ACCEPTED_NOTIFIER_H_
 
 #include "base/basictypes.h"
+#include "base/prefs/pref_change_registrar.h"
 
-// Interface for querying the EULA accepted state and receiving a notification
-// when the EULA is accepted. This abstracts away the platform-specific logic
-// for EULA notifications on different platforms.
+class PrefService;
+
+// Helper class for querying the EULA accepted state and receiving a
+// notification when the EULA is accepted.
 class EulaAcceptedNotifier {
  public:
   // Observes EULA accepted state changes.
@@ -18,7 +20,7 @@ class EulaAcceptedNotifier {
     virtual void OnEulaAccepted() = 0;
   };
 
-  EulaAcceptedNotifier();
+  explicit EulaAcceptedNotifier(PrefService* local_state);
   virtual ~EulaAcceptedNotifier();
 
   // Initializes this class with the given |observer|. Must be called before
@@ -28,17 +30,26 @@ class EulaAcceptedNotifier {
   // Returns true if the EULA has been accepted. If the EULA has not yet been
   // accepted, begins monitoring the EULA state and will notify the observer
   // once the EULA has been accepted.
-  virtual bool IsEulaAccepted() = 0;
+  virtual bool IsEulaAccepted();
 
   // Factory method for this class.
   static EulaAcceptedNotifier* Create();
 
  protected:
-  // Notifies the observer that the EULA has been updated, to be called by
-  // platform-specific subclasses.
+  // Notifies the observer that the EULA has been updated, made protected for
+  // testing.
   void NotifyObserver();
 
  private:
+  // Callback for EULA accepted pref change notification.
+  void OnPrefChanged();
+
+  // Local state pref service for querying the EULA accepted pref.
+  PrefService* local_state_;
+
+  // Used to listen for the EULA accepted pref change notification.
+  PrefChangeRegistrar registrar_;
+
   // Observer of the EULA accepted notification.
   Observer* observer_;
 

chrome/browser/metrics/variations/eula_accepted_notifier_chromeos.cc

-// Copyright (c) 2013 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 "chrome/browser/metrics/variations/eula_accepted_notifier_chromeos.h"
-
-#include "base/logging.h"
-#include "chrome/browser/chromeos/login/startup_utils.h"
-#include "chrome/common/chrome_notification_types.h"
-#include "content/public/browser/notification_service.h"
-
-EulaAcceptedNotifierChromeos::EulaAcceptedNotifierChromeos() {
-}
-
-EulaAcceptedNotifierChromeos::~EulaAcceptedNotifierChromeos() {
-}
-
-bool EulaAcceptedNotifierChromeos::IsEulaAccepted() {
-  if (chromeos::StartupUtils::IsEulaAccepted())
-    return true;
-
-  // Register for the notification, if this is the first time.
-  if (registrar_.IsEmpty()) {
-    // Note that this must listen on AllSources due to the difficulty in knowing
-    // when the WizardController instance is created, and to avoid over-coupling
-    // the Chrome OS code with the VariationsService by directly attaching as an
-    // observer. This is OK because WizardController is essentially a singleton.
-    registrar_.Add(this, chrome::NOTIFICATION_WIZARD_EULA_ACCEPTED,
-                   content::NotificationService::AllSources());
-  }
-  return false;
-}
-
-void EulaAcceptedNotifierChromeos::Observe(
-    int type,
-    const content::NotificationSource& source,
-    const content::NotificationDetails& details) {
-  DCHECK_EQ(chrome::NOTIFICATION_WIZARD_EULA_ACCEPTED, type);
-  // This should only ever be received once. Remove it after this call.
-  DCHECK(!registrar_.IsEmpty());
-  registrar_.Remove(this, chrome::NOTIFICATION_WIZARD_EULA_ACCEPTED,
-                    content::NotificationService::AllSources());
-
-  NotifyObserver();
-}
-
-// static
-EulaAcceptedNotifier* EulaAcceptedNotifier::Create() {
-#if defined(GOOGLE_CHROME_BUILD)
-  return new EulaAcceptedNotifierChromeos;
-#else
-  return NULL;
-#endif
-}

chrome/browser/metrics/variations/eula_accepted_notifier_chromeos.h

-// Copyright (c) 2013 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.
-
-#ifndef CHROME_BROWSER_METRICS_VARIATIONS_EULA_ACCEPTED_NOTIFIER_CHROMEOS_H_
-#define CHROME_BROWSER_METRICS_VARIATIONS_EULA_ACCEPTED_NOTIFIER_CHROMEOS_H_
-
-#include "chrome/browser/metrics/variations/eula_accepted_notifier.h"
-#include "content/public/browser/notification_observer.h"
-#include "content/public/browser/notification_registrar.h"
-
-// ChromeOS implementation of the EulaAcceptedNotifier.
-class EulaAcceptedNotifierChromeos : public EulaAcceptedNotifier,
-                                     public content::NotificationObserver {
- public:
-  EulaAcceptedNotifierChromeos();
-  virtual ~EulaAcceptedNotifierChromeos();
-
-  // EulaAcceptedNotifier overrides:
-  virtual bool IsEulaAccepted() OVERRIDE;
-
- private:
-  // content::NotificationObserver overrides:
-  virtual void Observe(int type,
-                       const content::NotificationSource& source,
-                       const content::NotificationDetails& details) OVERRIDE;
-
-  // Used to listen for the EULA accepted notification.
-  content::NotificationRegistrar registrar_;
-
-  DISALLOW_COPY_AND_ASSIGN(EulaAcceptedNotifierChromeos);
-};
-
-#endif  // CHROME_BROWSER_METRICS_VARIATIONS_EULA_ACCEPTED_NOTIFIER_CHROMEOS_H_

chrome/browser/metrics/variations/eula_accepted_notifier_mobile.cc

-// Copyright (c) 2013 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 "chrome/browser/metrics/variations/eula_accepted_notifier_mobile.h"
-
-#include "base/bind.h"
-#include "base/prefs/pref_service.h"
-#include "chrome/browser/browser_process.h"
-#include "chrome/common/pref_names.h"
-
-EulaAcceptedNotifierMobile::EulaAcceptedNotifierMobile(
-    PrefService* local_state)
-    : local_state_(local_state) {
-}
-
-EulaAcceptedNotifierMobile::~EulaAcceptedNotifierMobile() {
-}
-
-bool EulaAcceptedNotifierMobile::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(&EulaAcceptedNotifierMobile::OnPrefChanged,
-                              base::Unretained(this)));
-  }
-  return false;
-}
-
-void EulaAcceptedNotifierMobile::OnPrefChanged() {
-  DCHECK(!registrar_.IsEmpty());
-  registrar_.RemoveAll();
-
-  DCHECK(local_state_->GetBoolean(prefs::kEulaAccepted));
-  NotifyObserver();
-}
-
-// static
-EulaAcceptedNotifier* EulaAcceptedNotifier::Create() {
-  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.
-  if (local_state->FindPreference(prefs::kEulaAccepted) == NULL)
-    return NULL;
-  return new EulaAcceptedNotifierMobile(local_state);
-}

chrome/browser/metrics/variations/eula_accepted_notifier_mobile.h

-// Copyright (c) 2013 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.
-
-#ifndef CHROME_BROWSER_METRICS_VARIATIONS_EULA_ACCEPTED_NOTIFIER_MOBILE_H_
-#define CHROME_BROWSER_METRICS_VARIATIONS_EULA_ACCEPTED_NOTIFIER_MOBILE_H_
-
-#include "chrome/browser/metrics/variations/eula_accepted_notifier.h"
-#include "base/prefs/pref_change_registrar.h"
-
-class PrefService;
-
-// Mobile (Android and iOS) implementation of the EulaAcceptedNotifier.
-class EulaAcceptedNotifierMobile : public EulaAcceptedNotifier {
- public:
-  explicit EulaAcceptedNotifierMobile(PrefService* local_state);
-  virtual ~EulaAcceptedNotifierMobile();
-
-  // EulaAcceptedNotifier overrides:
-  virtual bool IsEulaAccepted() OVERRIDE;
-
- private:
-  // Callback for EULA accepted pref change notification.
-  void OnPrefChanged();
-
-  PrefService* local_state_;
-
-  // Used to listen for the EULA accepted pref change notification.
-  PrefChangeRegistrar registrar_;
-
-  DISALLOW_COPY_AND_ASSIGN(EulaAcceptedNotifierMobile);
-};
-
-#endif  // CHROME_BROWSER_METRICS_VARIATIONS_EULA_ACCEPTED_NOTIFIER_MOBILE_H_

chrome/browser/metrics/variations/eula_accepted_notifier_unittest.cc

+// Copyright 2013 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 "chrome/browser/metrics/variations/eula_accepted_notifier.h"
+
+#include "base/prefs/pref_registry_simple.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/test/base/scoped_testing_local_state.h"
+#include "chrome/test/base/testing_browser_process.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+class EulaAcceptedNotifierTest : public testing::Test,
+                                 public EulaAcceptedNotifier::Observer {
+ public:
+  EulaAcceptedNotifierTest() : eula_accepted_called_(false) {
+  }
+
+  // testing::Test overrides.
+  virtual void SetUp() OVERRIDE {
+    local_state_.registry()->RegisterBooleanPref(prefs::kEulaAccepted, false);
+    notifier_.reset(new EulaAcceptedNotifier(&local_state_));
+    notifier_->Init(this);
+  }
+
+  // EulaAcceptedNotifier::Observer overrides.
+  virtual void OnEulaAccepted() OVERRIDE {
+    EXPECT_FALSE(eula_accepted_called_);
+    eula_accepted_called_ = true;
+  }
+
+  void SetEulaAcceptedPref() {
+    local_state_.SetBoolean(prefs::kEulaAccepted, true);
+  }
+
+  EulaAcceptedNotifier* notifier() {
+    return notifier_.get();
+  }
+
+  bool eula_accepted_called() {
+    return eula_accepted_called_;
+  }
+
+ private:
+  TestingPrefServiceSimple local_state_;
+  scoped_ptr<EulaAcceptedNotifier> notifier_;
+  bool eula_accepted_called_;
+
+  DISALLOW_COPY_AND_ASSIGN(EulaAcceptedNotifierTest);
+};
+
+TEST_F(EulaAcceptedNotifierTest, EulaAlreadyAccepted) {
+  SetEulaAcceptedPref();
+  EXPECT_TRUE(notifier()->IsEulaAccepted());
+  EXPECT_FALSE(eula_accepted_called());
+  // Call it a second time, to ensure the answer doesn't change.
+  EXPECT_TRUE(notifier()->IsEulaAccepted());
+  EXPECT_FALSE(eula_accepted_called());
+}
+
+TEST_F(EulaAcceptedNotifierTest, EulaNotAccepted) {
+  EXPECT_FALSE(notifier()->IsEulaAccepted());
+  EXPECT_FALSE(eula_accepted_called());
+  // Call it a second time, to ensure the answer doesn't change.
+  EXPECT_FALSE(notifier()->IsEulaAccepted());
+  EXPECT_FALSE(eula_accepted_called());
+}
+
+TEST_F(EulaAcceptedNotifierTest, EulaNotInitiallyAccepted) {
+  EXPECT_FALSE(notifier()->IsEulaAccepted());
+  SetEulaAcceptedPref();
+  EXPECT_TRUE(notifier()->IsEulaAccepted());
+  EXPECT_TRUE(eula_accepted_called());
+  // Call it a second time, to ensure the answer doesn't change.
+  EXPECT_TRUE(notifier()->IsEulaAccepted());
+}

chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc

@@ -45,7 +45,9 @@ class TestNetworkChangeNotifier : public net::NetworkChangeNotifier {
 // and issuing simulated notifications.
 class TestEulaAcceptedNotifier : public EulaAcceptedNotifier {
  public:
-  TestEulaAcceptedNotifier() : eula_accepted_(false) {
+  TestEulaAcceptedNotifier()
+      : EulaAcceptedNotifier(NULL),
+        eula_accepted_(false) {
   }
   virtual ~TestEulaAcceptedNotifier() {
   }

chrome/chrome_browser.gypi

@@ -1086,10 +1086,6 @@
         'browser/metrics/tracking_synchronizer_observer.h',
         'browser/metrics/variations/eula_accepted_notifier.cc',
         'browser/metrics/variations/eula_accepted_notifier.h',
-        'browser/metrics/variations/eula_accepted_notifier_chromeos.cc',
-        'browser/metrics/variations/eula_accepted_notifier_chromeos.h',
-        'browser/metrics/variations/eula_accepted_notifier_mobile.cc',
-        'browser/metrics/variations/eula_accepted_notifier_mobile.h',
         'browser/metrics/variations/resource_request_allowed_notifier.cc',
         'browser/metrics/variations/resource_request_allowed_notifier.h',
         'browser/metrics/variations/variations_http_header_provider.cc',
@@ -2835,8 +2831,6 @@
           'sources!': [
             'browser/chrome_browser_field_trials_mobile.cc',
             'browser/chrome_browser_field_trials_mobile.h',
-            'browser/metrics/variations/eula_accepted_notifier_mobile.cc',
-            'browser/metrics/variations/eula_accepted_notifier_mobile.h',
             'browser/metrics/variations/variations_request_scheduler_mobile.cc',
             'browser/metrics/variations/variations_request_scheduler_mobile.h',
           ],

chrome/chrome_tests_unit.gypi

@@ -934,6 +934,7 @@
         'browser/metrics/thread_watcher_unittest.cc',
         'browser/metrics/time_ticks_experiment_unittest.cc',
         'browser/metrics/variations/variations_service_unittest.cc',
+        'browser/metrics/variations/eula_accepted_notifier_unittest.cc',
         'browser/metrics/variations/resource_request_allowed_notifier_test_util.cc',
         'browser/metrics/variations/resource_request_allowed_notifier_test_util.h',
         'browser/metrics/variations/resource_request_allowed_notifier_unittest.cc',

chrome/common/chrome_notification_types.h

@@ -957,10 +957,6 @@ enum NotificationType {
   // First paint event after this fires NOTIFICATION_LOGIN_WEBUI_VISIBLE.
   NOTIFICATION_WIZARD_FIRST_SCREEN_SHOWN,
 
-  // Sent when the EULA has been accepted in the first-run wizard. This is never
-  // sent if the EULA was already accepted at startup.
-  NOTIFICATION_WIZARD_EULA_ACCEPTED,
-
   // Sent when the specific part of login WebUI is considered to be visible.
   // That moment is tracked as the first paint event after one of the:
   // 1. NOTIFICATION_LOGIN_USER_IMAGES_LOADED