Activate the VariationsService for ChromeOS and ensure that it does not ping the server until the E…

chrome/browser/metrics/variations/resource_request_allowed_notifier.cc

+// Copyright (c) 2012 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/resource_request_allowed_notifier.h"
+
+#include "base/metrics/histogram.h"
+#include "chrome/common/chrome_notification_types.h"
+#include "content/public/browser/notification_service.h"
+
+#if defined(OS_CHROMEOS)
+#include "chrome/browser/chromeos/login/wizard_controller.h"
+#endif
+
+ResourceRequestAllowedNotifier::ResourceRequestAllowedNotifier() :
+#if defined(OS_CHROMEOS)
+    // Check this once immediately at creation time. It is not expected to
+    // change until the corresponding notification is received.
+    was_waiting_for_user_to_accept_eula_(
+        !chromeos::WizardController::IsEulaAccepted()),
+#endif
+    was_waiting_for_network_(false),
+    observer_(NULL) {
+  net::NetworkChangeNotifier::AddConnectionTypeObserver(this);
+
+#if defined(OS_CHROMEOS)
+  if (was_waiting_for_user_to_accept_eula_) {
+    // 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.

[Ilya Sherman, 2012/09/19 19:49:01]

nit: Please mention that the WizardController is a singleton, which is why
listening for AllSources() is ok.

[SteveT, 2012/09/20 19:40:18]

Done.

+    registrar_.Add(this, chrome::NOTIFICATION_WIZARD_EULA_ACCEPTED,
+                   content::NotificationService::AllSources());
+  }
+#endif
+}
+
+ResourceRequestAllowedNotifier::~ResourceRequestAllowedNotifier() {
+  net::NetworkChangeNotifier::RemoveConnectionTypeObserver(this);
+}
+
+void ResourceRequestAllowedNotifier::SetObserver(Observer* observer) {
+  DCHECK(!observer_ && observer);

[Alexei Svitkine (slow), 2012/09/19 20:05:35]

Either remove this check or document it in the header.

[SteveT, 2012/09/20 19:40:18]

Will document in header due to the next comment...

+  observer_ = observer;

[Alexei Svitkine (slow), 2012/09/19 20:05:35]

Hmm, I think perhaps the checks that are currently in the constructor should be
done here instead. (i.e. calling !chromeos::WizardController::IsEulaAccepted()).

Here's my reasoning:

If there's some interval between construction and setting the observer, then
notifications during that time may be lost (since the code that checks to notify
the observer will just be a no-op).

Alternatively, perhaps it might be cleaner to just require that the observer is
a parameter to the constructor and offer no way to reset it. That will avoid
these kinds of edge cases and ensure people use this class in a way that is not
error prone.

[SteveT, 2012/09/20 19:40:18]

I like your latter suggestion of setting the observer at construction-time,
which avoids a whole ton of troubles. Plus, it doesn't make sense for an RRAN to
exist without someone to notify - it's raison d'etre, if you will :).

I've made those changes.

+}
+
+void ResourceRequestAllowedNotifier::ClearObserver() {
+  DCHECK(observer_);

[Alexei Svitkine (slow), 2012/09/19 20:05:35]

Either remove this check or document it in the header.

[SteveT, 2012/09/20 19:40:18]

Will document in header due to the previous comment...

+  observer_ = NULL;
+}
+
+bool ResourceRequestAllowedNotifier::ResourceRequestsAllowed() {
+  was_waiting_for_network_ = net::NetworkChangeNotifier::IsOffline();
+
+  return
+#if defined(OS_CHROMEOS)
+      !was_waiting_for_user_to_accept_eula_ &&
+#endif
+      !was_waiting_for_network_;
+}
+
+void ResourceRequestAllowedNotifier::
+    SetWasWaitingForNetworkForTesting(bool waiting) {
+  was_waiting_for_network_ = waiting;
+}
+
+#if defined(OS_CHROMEOS)
+void ResourceRequestAllowedNotifier::
+    SetWasWaitingForEulaForTesting(bool waiting) {
+  was_waiting_for_user_to_accept_eula_ = waiting;
+}
+#endif
+
+void ResourceRequestAllowedNotifier::MaybeNotifyObservers() {
+  // Need to ensure that all criteria are met before notifying observers.
+  if (observer_ && ResourceRequestsAllowed()) {
+    DVLOG(1) << "Notifying observer of state change.";
+    observer_->OnResourceRequestsAllowed();
+  }
+}
+
+#if defined(OS_CHROMEOS)
+void ResourceRequestAllowedNotifier::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());
+
+  // If the observing service did not fail earlier due to an unaccepted EULA,
+  // do not notify it.
+  if (was_waiting_for_user_to_accept_eula_) {

[Alexei Svitkine (slow), 2012/09/19 20:05:35]

DCHECK that this is true instead.

[SteveT, 2012/09/20 19:40:18]

Thanks - done. That caught a few test issues that I fixed.

+    DVLOG(1) << "EULA was accepted.";
+    was_waiting_for_user_to_accept_eula_ = false;
+    MaybeNotifyObservers();
+  }
+}
+#endif
+
+// net::NetworkChangeNotifier::ConnectionTypeObserver implementation.
+void ResourceRequestAllowedNotifier::OnConnectionTypeChanged(
+    net::NetworkChangeNotifier::ConnectionType type) {
+  // Only attempt to notify observers if this was previously waiting for the
+  // network to reconnect, and new network state is actually available. This
+  // prevents the notifier from notifying the observer if the notifier was never
+  // waiting on the network, or if the network changes from one online state
+  // to another (for example, Wifi to 3G, or Wifi to Wifi, if the network were
+  // flaky).
+  if (was_waiting_for_network_ &&
+      type != net::NetworkChangeNotifier::CONNECTION_NONE) {
+    DVLOG(1) << "Network came back online.";
+    MaybeNotifyObservers();
+  }
+}