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

chrome/browser/metrics/variations/variations_service.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/variations_service.h"
 
 #include <set>
 
 #include "base/base64.h"
 #include "base/build_time.h"
@@ skipping 82 matching lines @@
   GURL url_as_gurl = GURL(server_url);
   DCHECK(url_as_gurl.is_valid());
   return url_as_gurl;
 }
 
 }  // namespace
 
 VariationsService::VariationsService()
     : variations_server_url_(GetVariationsServerURL()),
       create_trials_from_seed_called_(false),
+#if defined(OS_CHROMEOS)
+      waiting_for_user_to_accept_eula_(false),
+#endif
       was_offline_during_last_request_attempt_(false) {
   net::NetworkChangeNotifier::AddConnectionTypeObserver(this);
 }
 
 VariationsService::~VariationsService() {
   net::NetworkChangeNotifier::RemoveConnectionTypeObserver(this);
 }
 
 bool VariationsService::CreateTrialsFromSeed(PrefService* local_prefs) {
   create_trials_from_seed_called_ = true;
@@ skipping 38 matching lines @@
                this, &VariationsService::FetchVariationsSeed);
 }
 
 void VariationsService::FetchVariationsSeed() {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
   was_offline_during_last_request_attempt_ =
       net::NetworkChangeNotifier::IsOffline();
   UMA_HISTOGRAM_BOOLEAN("Variations.NetworkAvailability",
                         !was_offline_during_last_request_attempt_);
   if (was_offline_during_last_request_attempt_) {

[Alexei Svitkine (slow), 2012/09/10 16:50:11]

I feel like it would better to have a helper class that abstracts both network
availability and ChromeOS eula acceptance.

Then, this can just call AreNetworkRequestAllowed() on an instance of that class
and should be notified when that setting changes to the correct state.

That helper class would itself observe the CrOS eula and the network adapter
state and provide its own simple observer interface.

[SteveT, 2012/09/10 17:25:28]

So you're suggesting that I extract the Observer bits in VariationsService to
better decouple it from that logic? It still needs to be notified by this new
VSEventNotifier class, so we don't clean up all those relationships.

Is there an example of another class somewhere that used to directly observe
such notifications, but was refactored to delegate that to a helper class?

I am happy to make this change, but I want to better understand the benefits of
creating this new class.

[MAD, 2012/09/10 17:35:03]

Maybe we could share code with the translate manager?

[Alexei Svitkine (slow), 2012/09/10 17:38:39]

The benefits is that variations_service will not have ChromeOS specific code and
will only need to act on one type of state and one type of notification instead
of two.

Additionally, this helper class could be re-used by the translate manager.

[SteveT, 2012/09/10 19:33:52]

I am convinced by these arguments and will do the work to extract that logic
into this new ResourceRequestChecks helper class (or some better, shorter name).

[Alexei Svitkine (slow), 2012/09/10 21:47:35]

Maybe NetworkAllowedNotifier or something variation thereof? Since the class
basically tells us whether or not we're allowed to use the network / make
requests and would do so through a notification interface.

[SteveT, 2012/09/10 21:57:07]

It's a bit more like a ResourceRequestAllowedNotifier, since it's not just the
network, but also the EULA permission that needs to be set before we are allowed
to make requests for our resources.

     DVLOG(1) << "Network was offline.";
     return;
   }
 
+#if defined(OS_CHROMEOS)
+  if (!chromeos::WizardController::IsEulaAccepted()) {
+    // The WizardController should notify this class when the EULA has been
+    // accepted.

[Alexei Svitkine (slow), 2012/09/10 16:50:11]

What about the case of a second start up when there's no wizard? How does that
work? Can you expand the comment to explain this.

[SteveT, 2012/09/10 17:25:28]

Comment added below to explain how we NO-OP if the Eula was accepted already.

+    VLOG(1) << "EULA was not accepted.";
+    waiting_for_user_to_accept_eula_ = true;
+    return;
+  }
+#endif
+
   pending_seed_request_.reset(net::URLFetcher::Create(
       variations_server_url_, net::URLFetcher::GET, this));
   pending_seed_request_->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
                                       net::LOAD_DO_NOT_SAVE_COOKIES);
   pending_seed_request_->SetRequestContext(
       g_browser_process->system_request_context());
   pending_seed_request_->SetMaxRetries(kMaxRetrySeedFetch);
   if (!variations_serial_number_.empty()) {
     pending_seed_request_->AddExtraRequestHeader("If-Match:" +
                                                  variations_serial_number_);
   }
   pending_seed_request_->Start();
 }
 
 void VariationsService::SetWasOfflineDuringLastRequestAttemptForTesting(
     bool offline) {
   was_offline_during_last_request_attempt_ = offline;
 }
 
+#if defined(OS_CHROMEOS)
+  void VariationsService::OnScreenChanged(chromeos::WizardScreen* next_screen) {

[Alexei Svitkine (slow), 2012/09/10 16:50:11]

Don't indent these. Please add a comment to the no-op methods to explain we only
implement them to follow the Observer interface.

[SteveT, 2012/09/10 17:25:28]

Done and done. Let me know if the comments are fine.

+  }
+
+  void VariationsService::OnSessionStart() {
+  }
+
+  void VariationsService::OnEulaAccepted() {
+    // If an earlier request attempt was aborted because the EULA was not
+    // accepted, retry the request now.
+    if (!waiting_for_user_to_accept_eula_)
+      return;
+    DCHECK(chromeos::WizardController::IsEulaAccepted());
+    waiting_for_user_to_accept_eula_ = false;
+
+    DCHECK(chromeos::WizardController::default_controller());
+    chromeos::WizardController::default_controller()->RemoveObserver(this);
+
+    VLOG(1) << "Starting ping because the EULA was accepted.";
+    FetchVariationsSeed();
+
+    // Since FetchVariationsSeed was explicitly called here, reset the timer to
+    // avoid retrying for a full period.
+    if (timer_.IsRunning())
+      timer_.Reset();
+  }
+#endif
+
 // static
 void VariationsService::RegisterPrefs(PrefService* prefs) {
   prefs->RegisterStringPref(prefs::kVariationsSeed, std::string());
   prefs->RegisterInt64Pref(prefs::kVariationsSeedDate,
                            base::Time().ToInternalValue());
 }
 
 void VariationsService::OnURLFetchComplete(const net::URLFetcher* source) {
   DCHECK_EQ(pending_seed_request_.get(), source);
   // The fetcher will be deleted when the request is handled.
@@ skipping 300 matching lines @@
                                       variation_id);
     }
   }
 
   trial->SetForced();
   if (IsStudyExpired(study, reference_date))
     trial->Disable();
 }
 
 }  // namespace chrome_variations