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"
 #include "base/command_line.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/metrics/field_trial.h"
-#include "base/metrics/histogram.h"
 #include "base/version.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/metrics/proto/trials_seed.pb.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/metrics/variations/variations_util.h"
 #include "chrome/common/pref_names.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/common/url_fetcher.h"
 #include "googleurl/src/gurl.h"
@@ skipping 67 matching lines @@
     server_url = kDefaultVariationsServerURL;
   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),
-      was_offline_during_last_request_attempt_(false) {
-  net::NetworkChangeNotifier::AddConnectionTypeObserver(this);
+      create_trials_from_seed_called_(false) {
+  resource_request_allowed_notifier_.AddObserver(this);
 }
 
 VariationsService::~VariationsService() {
-  net::NetworkChangeNotifier::RemoveConnectionTypeObserver(this);
+  resource_request_allowed_notifier_.RemoveObserver(this);
 }
 
 bool VariationsService::CreateTrialsFromSeed(PrefService* local_prefs) {
   create_trials_from_seed_called_ = true;
 
   TrialsSeed seed;
   if (!LoadTrialsSeedFromPref(local_prefs, &seed))
     return false;
 
   const int64 date_value = local_prefs->GetInt64(prefs::kVariationsSeedDate);
@@ skipping 28 matching lines @@
   FetchVariationsSeed();
 
   // Repeat this periodically.
   timer_.Start(FROM_HERE, base::TimeDelta::FromHours(kSeedFetchPeriodHours),
                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_) {
-    DVLOG(1) << "Network was offline.";
+  if (!resource_request_allowed_notifier_.ResourceRequestsAllowed()) {
+    DVLOG(1) << "Resource requests were not allowed. Waiting for notification.";
     return;
   }
 
   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(

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

I don't see your tests calling this anymore.

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

Removed.

     bool offline) {
-  was_offline_during_last_request_attempt_ = offline;
+  resource_request_allowed_notifier_.
+      SetWasOfflineDuringLastRequestAttemptForTesting(offline);
+}
+
+void VariationsService::OnResourceRequestsAllowed() {

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

Does this guarantee that we won't fetch more than once every 5 hours if e.g. the
network is flapping?

We've had this discussion before and I think the answer may be "yes" due to how
the code is structured. If that's the case, please document it both here as well
as making sure that it's also clear in the comments for the new class (e.g. that
it only notifies when the last "is allowed" returned false, but not otherwise.)

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

Done here, and already done in the header of the RRAN class (see the 3rd
paragraph).

+  VLOG(1) << "Retrying fetch.";

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

nit: DVLOG?

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

Done.

+  FetchVariationsSeed();
+  if (timer_.IsRunning())
+    timer_.Reset();
 }
 
 // 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) {
@@ skipping 16 matching lines @@
   bool success = request->GetResponseAsString(&seed_data);
   DCHECK(success);
 
   base::Time response_date;
   success = request->GetResponseHeaders()->GetDateValue(&response_date);
   DCHECK(success || response_date.is_null());
 
   StoreSeedData(seed_data, response_date, g_browser_process->local_state());
 }
 
-void VariationsService::OnConnectionTypeChanged(
-    net::NetworkChangeNotifier::ConnectionType type) {
-  // If the connection type is back online, start a request if the last request
-  // failed due to being offline.
-  if (was_offline_during_last_request_attempt_ &&
-      type != net::NetworkChangeNotifier::CONNECTION_NONE) {
-    VLOG(1) << "Retrying fetch due to network reconnect.";
-    FetchVariationsSeed();
-
-    // Since FetchVariationsSeed was explicitly called here, reset the timer to
-    // avoid retrying for a full period.
-    // net::NetworkChangeNotifier::IsOffline may be inconsistent with |type|, so
-    // we check if FetchVariationsSeed set
-    // |was_offline_during_last_request_attempt_| to true before we reset the
-    // timer.
-    if (!was_offline_during_last_request_attempt_ && timer_.IsRunning())
-      timer_.Reset();
-  }
-}
-
 bool VariationsService::StoreSeedData(const std::string& seed_data,
                                       const base::Time& seed_date,
                                       PrefService* local_prefs) {
   // Only store the seed data if it parses correctly.
   TrialsSeed seed;
   if (!seed.ParseFromString(seed_data)) {
     VLOG(1) << "Variations Seed data from server is not in valid proto format, "
             << "rejecting the seed.";
     return false;
   }
@@ skipping 246 matching lines @@
                                       variation_id);
     }
   }
 
   trial->SetForced();
   if (IsStudyExpired(study, reference_date))
     trial->Disable();
 }
 
 }  // namespace chrome_variations