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

chrome/browser/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 "base/metrics/histogram.h"
+#include "chrome/browser/resource_request_allowed_notifier.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)
+    was_offline_during_last_request_attempt_(false),
+#endif
+    was_waiting_for_user_to_accept_eula_(false) {

[Alexei Svitkine (slow), 2012/09/12 15:15:46]

I think you want the OS_CHROMEOS ifdef over this line, not over the previous
one.

[SteveT, 2012/09/14 20:25:09]

D'oh. You can tell I haven't ran my Non-CrOS tests. Fixed.

+  net::NetworkChangeNotifier::AddConnectionTypeObserver(this);
+
+#if defined(OS_CHROMEOS)
+  registrar_.Add(this, chrome::NOTIFICATION_WIZARD_EULA_ACCEPTED,
+                 content::NotificationService::AllSources());

[Ilya Sherman, 2012/09/11 22:51:27]

nit: Listening to notifications from AllSources() is discouraged.

[SteveT, 2012/09/14 20:25:09]

Did the work to resolve all this.

+#endif
+}
+
+ResourceRequestAllowedNotifier::~ResourceRequestAllowedNotifier() {
+  net::NetworkChangeNotifier::RemoveConnectionTypeObserver(this);
+
+#if defined(OS_CHROMEOS)
+  registrar_.Remove(this, chrome::NOTIFICATION_WIZARD_EULA_ACCEPTED,
+                    content::NotificationService::AllSources());

[Ilya Sherman, 2012/09/11 22:51:27]

nit: Listening to notifications from AllSources() is discouraged.

[SteveT, 2012/09/14 20:25:09]

Done.

+#endif
+}
+
+void ResourceRequestAllowedNotifier::AddObserver(Observer* observer) {
+  DCHECK(observer_list_.size() == 0) << "One service per notifier.";

[Ilya Sherman, 2012/09/11 22:51:27]

nit: Please drop the """<< "One service per notifier."""" -- you can add it as a
comment instead, but printing from DCHECKs is strongly discouraged.

[Alexei Svitkine (slow), 2012/09/12 15:15:46]

If you're enforcing this, why not just make it enforced by the API? e.g.
SetObserver() instead of AddObserver()? And why store them in a list if there's
only going to be one observer? Just use a regular single variable.

[SteveT, 2012/09/14 20:25:09]

Didn't realize that was the case. I'll drop the logs.

[SteveT, 2012/09/14 20:25:09]

No, you're totally right. I've changed this class to just have the one observer
and changed the methods to set or clear it.

+  observer_list_.AddObserver(observer);
+}
+
+void ResourceRequestAllowedNotifier::RemoveObserver(Observer* observer) {
+  observer_list_.RemoveObserver(observer);
+  DCHECK(observer_list_.size() == 0) << "One service per notifier.";

[Ilya Sherman, 2012/09/11 22:51:27]

nit: Please drop the logged line.

[SteveT, 2012/09/14 20:25:09]

Done.

+}
+
+bool ResourceRequestAllowedNotifier::ResourceRequestsAllowed() {
+#if defined(OS_CHROMEOS)
+  was_waiting_for_user_to_accept_eula_ =
+      !chromeos::WizardController::IsEulaAccepted();

[Alexei Svitkine (slow), 2012/09/12 15:15:46]

Do we need to query this each time or can we just query this once in the
constructor and update its value when we get the EULA notification?

[SteveT, 2012/09/14 20:25:09]

I think your suggestion is reasonable, because once it gets set by the
notification, that's it. It'll generally never go false again. Made those
changes.

+#endif
+  was_offline_during_last_request_attempt_ =
+      net::NetworkChangeNotifier::IsOffline();
+
+  // TODO(stevet): Remove this or rename.
+  UMA_HISTOGRAM_BOOLEAN("Variations.NetworkAvailability",

[Alexei Svitkine (slow), 2012/09/12 15:15:46]

Maybe keep this histogram in variations service and have it track "resource
request allowability" now - i.e. network availability for both technical and
eula reasons. Also, especially if this class gets used by multiple consumers,
the histogram results may not be meaningful anymore.

[SteveT, 2012/09/14 20:25:09]

I am considering just sunsetting, since we already got the data we wanted from
it (approximately 2% network downtime).

As for not pinging because the Eula has not been accepted... We sort of have an
understanding of that event already... it'll get hit roughly once per new CrOS
device activation (assuming the user accepts the Eula the first time and doesn't
just reboot a lot).

I've removed it for now. Let me know if you're opposed. I've noted that I should
clean it from histograms.xml as well.

+                        !was_offline_during_last_request_attempt_);
+
+  return
+#if defined(OS_CHROMEOS)
+      !was_waiting_for_user_to_accept_eula_ &&
+#endif
+      !was_offline_during_last_request_attempt_;
+}
+
+#if defined(OS_CHROMEOS)
+void ResourceRequestAllowedNotifier::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  DCHECK(type == chrome::NOTIFICATION_WIZARD_EULA_ACCEPTED);

[Alexei Svitkine (slow), 2012/09/12 15:15:46]

Can you use DCHECK_EQ() here?

[SteveT, 2012/09/14 20:25:09]

Done.

+  // If the observing service did not fail earlier due to an unaccepted EULA,
+  // do not notify it.

[Alexei Svitkine (slow), 2012/09/12 15:15:46]

Wouldn't the condition you describe in this comment be unexpected? If so,
perhaps we should DCHECK on it. Or is it possible for the state go from EULA
accepted -> EULA unaccepted?

[Nikita (slow), 2012/09/12 15:20:23]

No, it is not possible.

+  if (was_waiting_for_user_to_accept_eula_) {
+    DVLOG(1) << "EULA was accepted.";

[Ilya Sherman, 2012/09/11 22:51:27]

Is this DVLOG just to help you while developing, or do you expect it to be
useful longer term?

[SteveT, 2012/09/14 20:25:09]

Well I expect it to be useful in the future as developers can quickly check to
see when events could cause their service to start resource requests.

+    MaybeNotifyObservers();
+  }
+}
+#endif
+
+// net::NetworkChangeNotifier::ConnectionTypeObserver implementation.
+void ResourceRequestAllowedNotifier::OnConnectionTypeChanged(
+    net::NetworkChangeNotifier::ConnectionType type) {
+  if (was_offline_during_last_request_attempt_ &&
+      type != net::NetworkChangeNotifier::CONNECTION_NONE) {

[Alexei Svitkine (slow), 2012/09/12 15:15:46]

Can you add a comment explaining the if statement conditions?

[SteveT, 2012/09/14 20:25:09]

Done.

+    DVLOG(1) << "Network came back online.";
+    MaybeNotifyObservers();
+  }
+}
+
+void ResourceRequestAllowedNotifier::
+    SetWasOfflineDuringLastRequestAttemptForTesting(bool offline) {
+  was_offline_during_last_request_attempt_ = offline;
+}
+
+void ResourceRequestAllowedNotifier::MaybeNotifyObservers() {
+  // Need to ensure that all criteria is met before notifying observers.

[Alexei Svitkine (slow), 2012/09/12 15:15:46]

Nit: "is" -> "are".

[SteveT, 2012/09/14 20:25:09]

Done.

+  if (ResourceRequestsAllowed()) {

[Alexei Svitkine (slow), 2012/09/12 15:15:46]

I think this is problematic per what mmenke said in a past review.

He said that it may be possible for the network notification to be inconsistent
with the net::NetworkChangeNotifier::IsOffline() state under certain
circumstances on Windows.

So there could be a scenario where we get a notification that net is up, but
IsOffline() returns true. In such a case, this notification will never get sent.

Perhaps the code could be restructured such that when we get notified of X
changing, we don't re-check X since we should know it's value from the
notification? This would solve the above problem.

[SteveT, 2012/09/14 20:25:09]

X being one of Eula-accepted or network-online? So you're suggesting that after
the first time the user calls ResourceRequestAllowed(), the criteria flags can
only flip from false to true? And here, instead of calling
ResourceRequestsAllowed, which could reset the flags, I just check all flags
directly?

(note that I haven't had a fix for this yet)

+    DVLOG(1) << "Notifying observer of state change.";
+    FOR_EACH_OBSERVER(Observer, observer_list_, OnResourceRequestsAllowed());
+  }
+}