Domain Reliability: Don't upload when metrics reporting is off.

components/domain_reliability/monitor.cc

 // Copyright 2014 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 "components/domain_reliability/monitor.h"
 
 #include "base/command_line.h"
 #include "base/logging.h"
-#include "base/message_loop/message_loop.h"
 #include "base/single_thread_task_runner.h"
 #include "base/task_runner.h"
 #include "base/threading/thread_checker.h"
 #include "base/time/time.h"
 #include "components/domain_reliability/baked_in_configs.h"
 #include "net/base/load_flags.h"
 #include "net/http/http_response_headers.h"
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_context_getter.h"
 
 namespace domain_reliability {
 
 DomainReliabilityMonitor::DomainReliabilityMonitor(
     const std::string& upload_reporter_string)
     : time_(new ActualTime()),
       upload_reporter_string_(upload_reporter_string),
       scheduler_params_(
           DomainReliabilityScheduler::Params::GetFromFieldTrialsOrDefaults()),
       dispatcher_(time_.get()),
+      reporting_pref_initialized_(false),
       weak_factory_(this) {}
 
 DomainReliabilityMonitor::DomainReliabilityMonitor(
     const std::string& upload_reporter_string,
     scoped_ptr<MockableTime> time)
     : time_(time.Pass()),
       upload_reporter_string_(upload_reporter_string),
       scheduler_params_(
           DomainReliabilityScheduler::Params::GetFromFieldTrialsOrDefaults()),
       dispatcher_(time_.get()),
+      reporting_pref_initialized_(false),
       weak_factory_(this) {}
 
 DomainReliabilityMonitor::~DomainReliabilityMonitor() {
+  DCHECK(!thread_checker_ || thread_checker_->CalledOnValidThread());
+
   ClearContexts();
 }
 
-void DomainReliabilityMonitor::Init(
+void DomainReliabilityMonitor::InitReportingPref(
+    PrefService* local_state_pref_service,
+    const char* reporting_pref_name,
+    const scoped_refptr<base::SingleThreadTaskRunner>&
+        network_task_runner) {
+  DCHECK(!reporting_pref_initialized_);
+
+  reporting_pref_.Init(
+      reporting_pref_name,
+      local_state_pref_service,
+      base::Bind(&DomainReliabilityMonitor::OnReportingPrefChanged,
+                 base::Unretained(this)));
+  reporting_pref_initialized_ = true;
+  reporting_pref_.MoveToThread(network_task_runner);
+}
+
+void DomainReliabilityMonitor::InitURLRequestContext(
     net::URLRequestContext* url_request_context,
     const scoped_refptr<base::SingleThreadTaskRunner>& task_runner) {

[davidben, 2014/08/22 19:00:40]

I wonder if the thread_checker_ should be replaced with a
scoped_refptr<base::SingleThreadTaskRunner> that's saved once in
InitReportingPref. Then you don't need to pass it in twice. Three times, really,
if you include the service. Would something like this work with all the
plumbing?

- Move InitReportingPref parameters to Monitor ctor and Service::CreateMonitor
- Service::CreateMonitor passes parameters to Monitor ctor 
- ctor saves network_task_runner as a member and calls MoveToThread.
- InitURLRequestContext must be called on network_task_runner, add DCHECK.
- Instead of passing task_runner as parameter, used the saved one.

[davidben, 2014/08/22 19:02:10]

Er, that got lost in rewording. By 'add DCHECK', I mean add DCHECK for
RunsTasksOnCurrentThread() which I think does the same thing as the
ThreadChecker?

+  DCHECK(reporting_pref_initialized_);
   DCHECK(!thread_checker_);
 
   scoped_refptr<net::URLRequestContextGetter> url_request_context_getter =
       new net::TrivialURLRequestContextGetter(url_request_context,
                                               task_runner);
-  Init(url_request_context_getter);
+  InitURLRequestContext(url_request_context_getter);
 }
 
-void DomainReliabilityMonitor::Init(
+void DomainReliabilityMonitor::InitURLRequestContext(
     scoped_refptr<net::URLRequestContextGetter> url_request_context_getter) {
+  DCHECK(reporting_pref_initialized_);
   DCHECK(!thread_checker_);
 
   DCHECK(url_request_context_getter->GetNetworkTaskRunner()->
          RunsTasksOnCurrentThread());
 
+  thread_checker_.reset(new base::ThreadChecker());
   uploader_ = DomainReliabilityUploader::Create(url_request_context_getter);
-  thread_checker_.reset(new base::ThreadChecker());
+  OnReportingPrefChanged();
 }
 
 void DomainReliabilityMonitor::AddBakedInConfigs() {
   DCHECK(thread_checker_ && thread_checker_->CalledOnValidThread());
   base::Time now = base::Time::Now();
   for (size_t i = 0; kBakedInJsonConfigs[i]; ++i) {
     std::string json(kBakedInJsonConfigs[i]);
     scoped_ptr<const DomainReliabilityConfig> config =
         DomainReliabilityConfig::FromJSON(json);
     if (config && config->IsExpired(now)) {
@@ skipping 154 matching lines @@
     beacon.server_ip.clear();
   beacon.protocol = GetDomainReliabilityProtocol(
       request.response_info.connection_info,
       request.response_info.ssl_info.is_valid());
   beacon.http_response_code = response_code;
   beacon.start_time = request.load_timing_info.request_start;
   beacon.elapsed = time_->NowTicks() - beacon.start_time;
   context->OnBeacon(request.url, beacon);
 }
 
+void DomainReliabilityMonitor::OnReportingPrefChanged() {

[davidben, 2014/08/22 19:00:40]

Probably want to add a check for uploader_ being NULL here, otherwise you have
another subtle ordering constraint: between InitReportingPref and
InitURLRequestContext, the preference cannot change and fire an IO thread event.

[Deprecated (see juliatuttle), 2014/08/23 00:10:29]

Done.

+  // When metrics reporting is disabled, discard Domain Reliability uploads.
+  uploader_->set_discard_uploads(!*reporting_pref_);
+}
+
+void DomainReliabilityMonitor::DestroyReportingPref() {
+  reporting_pref_.Destroy();
+}
+
 // TODO(ttuttle): Keep a separate wildcard_contexts_ map to avoid having to
 // prepend '*.' to domains.
 DomainReliabilityContext* DomainReliabilityMonitor::GetContextForHost(
     const std::string& host) const {
   ContextMap::const_iterator context_it;
 
   context_it = contexts_.find(host);
   if (context_it != contexts_.end())
     return context_it->second;
 
@@ skipping 15 matching lines @@
 
   return NULL;
 }
 
 base::WeakPtr<DomainReliabilityMonitor>
 DomainReliabilityMonitor::MakeWeakPtr() {
   return weak_factory_.GetWeakPtr();
 }
 
 }  // namespace domain_reliability