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

chrome/browser/resource_request_allowed_notifier.h

+// 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.
+
+#ifndef CHROME_BROWSER_RESOURCE_REQUEST_ALLOWED_NOTIFIER_H_
+#define CHROME_BROWSER_RESOURCE_REQUEST_ALLOWED_NOTIFIER_H_

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

nit: Is the variations service currently the only client of this header's
contents?  If so, it should probably be moved to
chrome/browser/metrics/variations.  It can always be extracted back up to a
higher level if it proves useful for other clients.

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

I agree with Ilya here. Let's add it under chrome/browser/metrics/variations for
now. It can be moved later when MAD makes the translate manager use it.

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

Done.

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

Done.

+
+#include "base/observer_list.h"
+#include "net/base/network_change_notifier.h"
+
+#if defined(OS_CHROMEOS)
+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_registrar.h"
+#endif
+
+// Services that need to request resources over the network should use this

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

Can you rephrase the first sentence and/or paragraph to clearly state what
problem this class is solving (i.e. "should use this class" doesn't explain
"why").

Mention up-front (instead of further below) that on ChromeOS resource requests
are not allowed until the EULA is accepted, etc.

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

Done.

+// helper class to check if they can perform their requests. This class wraps
+// such criteria and notifies interested services if state ever changes in such
+// a way that resource access is allowed.
+//
+// Such services should add themselves as an observer of
+// ResourceRequestAllowedNotifier and check ResourceRequestsAllowed() to see if
+// requests are permitted. If it returns true, they can go ahead and make their
+// request. If it returns false, ResourceRequestAllowedNotifier will notify the
+// service when the criteria is met.
+//
+// If the ResourceRequestsAllowed returns true the first time,
+// ResourceRequestAllowedNotifier will not notify the service in the future.
+//
+// Note that this class handles the criteria state for a single service, so
+// services should keep their own instance of this class rather than sharing a
+// global instance.
+//
+// Currently, the criteria for allowing resource requests include:
+//  1. The network is currently available.
+//  2. The EULA was accepted by the user (ChromeOS only).
+class ResourceRequestAllowedNotifier :
+#if defined(OS_CHROMEOS)
+    public content::NotificationObserver,
+#endif
+    public net::NetworkChangeNotifier::ConnectionTypeObserver {
+ public:
+  // Observes resource request allowed state changes.
+  class Observer {
+   public:
+    virtual void OnResourceRequestsAllowed() = 0;
+  };
+
+  ResourceRequestAllowedNotifier();
+  virtual ~ResourceRequestAllowedNotifier();
+
+  void AddObserver(Observer* observer);
+  void RemoveObserver(Observer* observer);
+
+  // Returns true iff all resource request criteria is met. If not, this call
+  // will set some flags so it knows to notify the observer if the criteria
+  // changes.
+  bool ResourceRequestsAllowed();
+
+#if defined(OS_CHROMEOS)
+  // content::NotificationObserver overrides:
+  virtual void Observe(int type,
+                       const content::NotificationSource& source,
+                       const content::NotificationDetails& details) OVERRIDE;

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

nit: Can this have private visibility?

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

Yes! Done.

+#endif
+
+  // net::NetworkChangeNotifier::ConnectionTypeObserver implementation.
+  virtual void OnConnectionTypeChanged(
+      net::NetworkChangeNotifier::ConnectionType type) OVERRIDE;

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

nit: Can this have private visibility?

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

Yeah, done.

+
+  void SetWasOfflineDuringLastRequestAttemptForTesting(bool offline);
+
+ private:
+  // Notifies observers if all criteria needed for resource requests are met.
+  void MaybeNotifyObservers();
+
+#if defined(OS_CHROMEOS)

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

I think this ifdef was meant to go around the next variable, not this one.

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

Yes, done.

+  // Tracks network connectivity criteria.
+  bool was_offline_during_last_request_attempt_;

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

Can you name this consistently with |was_waiting_for_user_to_accept_eula_|.

e.g. both refer to the state during the last ResourceRequestsAllowed(), but only
this variable has suffix "during_last_request_attempt_". Adjust one or both
names so that they're consistent. (The prefix may not be needed if you keep the
"was_" suffix and document the specifics in the comments above the variables.)

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

Sure.

was_waiting_for_network_
and
was_waiting_for_user_to_accept_eula_

Let me know if you prefer otherwise.

+#endif
+
+  // Tracks EULA acceptance criteria.
+  bool was_waiting_for_user_to_accept_eula_;

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

nit: Should this also be in the #if defined(OS_CHROMEOS) conditional?

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

Yes - thanks.

+
+  // List of observers interested in request permissions.
+  ObserverList<Observer> observer_list_;
+
+  content::NotificationRegistrar registrar_;

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

nit: Ditto

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

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(ResourceRequestAllowedNotifier);
+};
+
+#endif  // CHROME_BROWSER_RESOURCE_REQUEST_ALLOWED_NOTIFIER_H_