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() :

[Alexei Svitkine (slow), 2012/09/21 15:52:05]

Nit: ":" should go on the next line. Perhaps reorder
|was_waiting_for_user_to_accept_eula_| and |was_waiting_for_network_| so that
the ifdef will be cleaner once you put ":" on the next line.

[SteveT, 2012/09/24 15:38:38]

Done.

+#if defined(OS_CHROMEOS)
+    was_waiting_for_user_to_accept_eula_(false),
+#endif
+    was_waiting_for_network_(false),
+    observer_(NULL) {
+}
+
+ResourceRequestAllowedNotifier::~ResourceRequestAllowedNotifier() {
+  if (observer_)
+    net::NetworkChangeNotifier::RemoveConnectionTypeObserver(this);
+}
+
+void ResourceRequestAllowedNotifier::Init(Observer* observer) {
+  DCHECK(!observer_ && observer);
+  observer_ = observer;
+
+  net::NetworkChangeNotifier::AddConnectionTypeObserver(this);
+
+  // Check this state during initialization. It is not expected to change until
+  // the corresponding notification is received.
+  was_waiting_for_user_to_accept_eula_ = !IsEulaAccepted();
+#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. This is OK because WizardController is essentially a singleton.
+    registrar_.Add(this, chrome::NOTIFICATION_WIZARD_EULA_ACCEPTED,
+                   content::NotificationService::AllSources());
+  }
+#endif
+}
+
+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() {

[Alexei Svitkine (slow), 2012/09/21 15:52:05]

Nit: Should be MaybeNotifyObserver() (singular).

[SteveT, 2012/09/24 15:38:38]

Done.

+  // Need to ensure that all criteria are met before notifying observers.
+  if (ResourceRequestsAllowed()) {
+    DVLOG(1) << "Notifying observer of state change.";
+    observer_->OnResourceRequestsAllowed();
+  }
+}
+
+bool ResourceRequestAllowedNotifier::IsEulaAccepted() {
+  return chromeos::WizardController::IsEulaAccepted();
+}
+
+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();

[Alexei Svitkine (slow), 2012/09/21 15:52:05]

This still doesn't work when net::NetworkChangeNotifier::IsOffline() is out of
sync with the notification, per an earlier comment from mmenke on another
review.

The problem is that if we get notified that network is available but IsOffline()
still returns true, then MaybeNotifyObservers() won't notify the observer and as
long as network doesn't change state, the observer won't get notified again!

Basically, the issue is that the code called by the body of this if statement
shouldn't be checking for net::NetworkChangeNotifier::IsOffline() and should
instead just set |was_waiting_for_network_| based on the notification.

I think one way to solve this would be to only check
net::NetworkChangeNotifier::IsOffline() in Init(), similarly to how
IsEulaAccepted() is done on ChromeOS.

Then, this if body can set was_waiting_for_network_ = true; and
ResourceRequestsAllowed() would only act on the state of the two variables
without making the IsOffline() call.

[SteveT, 2012/09/24 15:38:38]

As discussed offline, we addressed this by shifting the IsOffline logic as you
suggested, but also added a new observer_requested_permission_ field to ensure
that we don't notify the observer/service unless it made a call to
ResourceRequestsAllowed, which denotes its interest in being alerted.

Note that there were some test changes that accompanied this new field.

+  }
+}
+
+#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());
+
+  // This flag should have been set if this was waiting on the EULA
+  // notification.
+  DCHECK(was_waiting_for_user_to_accept_eula_);
+  DVLOG(1) << "EULA was accepted.";
+  was_waiting_for_user_to_accept_eula_ = false;
+  MaybeNotifyObservers();
+}
+#endif