Fetch a warmup URL if data reduction proxy is enabled

components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc

Index: components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc b/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
index 4a2b45e2be487e2f1adcfbbb74216213ee45a433..b7b37136331befdcd3cdab72529b5e4fc84a25f7 100644
--- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
+++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
@@ -282,6 +282,64 @@ class SecureProxyChecker : public net::URLFetcherDelegate {
   DISALLOW_COPY_AND_ASSIGN(SecureProxyChecker);
 };
 
+// Fetcher for fetching the warm up URL.

[RyanSturm, 2016/12/08 20:59:17]

"Fetcher for fetching" is redundant maybe "Class for fetching"

[tbansal1, 2016/12/10 00:48:49]

Done.

+class WarmupURLFetcher : public net::URLFetcherDelegate {
+ public:
+  explicit WarmupURLFetcher(const scoped_refptr<net::URLRequestContextGetter>&
+                                url_request_context_getter)
+      : url_request_context_getter_(url_request_context_getter),
+        fetcher_callback_(nullptr) {

[RyanSturm, 2016/12/08 20:59:17]

I don't think you need to explicitly initialize this with nullptr. The default
initialization of callbacks should set up a is_null callback.

[tbansal1, 2016/12/10 00:48:49]

Done.

+    DCHECK(url_request_context_getter_);
+  }
+
+  ~WarmupURLFetcher() override {}
+
+  // Creates and starts a URLFetcher that fetches the warm up URL.
+  void FetchWarmupURL() {
+    UMA_HISTOGRAM_EXACT_LINEAR("DataReductionProxy.WarmupURL.FetchInitiated", 1,
+                               2);
+
+    std::unique_ptr<net::URLFetcher> fetcher(net::URLFetcher::Create(

[RyanSturm, 2016/12/08 20:59:17]

If fetcher is always std::move'd to fetcher_ can you just do:
fetcher_ = ...

and stop using fetcher at all?

[tbansal1, 2016/12/10 00:48:49]

Done.

+        params::GetWarmupURL(), net::URLFetcher::GET, this));
+    data_use_measurement::DataUseUserData::AttachToFetcher(
+        fetcher.get(),
+        data_use_measurement::DataUseUserData::DATA_REDUCTION_PROXY);
+    fetcher->SetLoadFlags(net::LOAD_BYPASS_CACHE);

[RyanSturm, 2016/12/08 20:59:17]

consider LOAD_DISABLE_CACHE (I don't know which is better)

[tbansal1, 2016/12/10 00:48:49]

LOAD_DISABLE_CACHE disables writing to the local cache which seems unnecessary
in this case.

+    fetcher->SetRequestContext(url_request_context_getter_.get());
+    // |fetcher| should not retry on 5xx errors.
+    fetcher->SetAutomaticallyRetryOn5xx(false);
+    fetcher->SetAutomaticallyRetryOnNetworkChanges(0);
+    fetcher_ = std::move(fetcher);
+    fetcher_->Start();
+  }
+
+  void SetWarmupURLFetcherCallbackForTesting(

[RyanSturm, 2016/12/08 20:59:17]

Pretty duplicative naming
WarmupURLFetcher::SetWarmupURLFetcherCallbackForTesting.

Maybe just SetFetchCompletionCallback.

[tbansal1, 2016/12/10 00:48:48]

I think ForTesting suffix is helpful since a warning is generated
if a method with ForTesting suffix is called in non-test code.

Also, I would prefer to keep WarmipURL in the name since there
are 2 fetchers in this file, and it makes it easier to distinguish.

+      base::Callback<void()> warmup_url_fetched_callback) {
+    fetcher_callback_ = warmup_url_fetched_callback;
+  }
+
+ private:
+  void OnURLFetchComplete(const net::URLFetcher* source) override {
+    DCHECK_EQ(source, fetcher_.get());
+    UMA_HISTOGRAM_BOOLEAN(
+        "DataReductionProxy.WarmupURL.FetchSuccessful",
+        source->GetStatus().status() == net::URLRequestStatus::SUCCESS);
+
+    if (fetcher_callback_)
+      fetcher_callback_.Run();
+  }
+
+  scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;
+
+  // The URLFetcher being used for fetching the warm up URL.
+  std::unique_ptr<net::URLFetcher> fetcher_;
+
+  // Called when the warm up URL has been fetched. May be null.

[RyanSturm, 2016/12/08 20:59:17]

Can you mention that this will be called even if the fetch has failed?

[tbansal1, 2016/12/10 00:48:49]

Done.

+  base::Callback<void()> fetcher_callback_;

[RyanSturm, 2016/12/08 20:59:17]

nit:s/fetcher_callback_/fetch_completion_callback_/?

[tbansal1, 2016/12/10 00:48:49]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(WarmupURLFetcher);
+};
+
 DataReductionProxyConfig::DataReductionProxyConfig(
     scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
     net::NetLog* net_log,
@@ -320,10 +378,15 @@ DataReductionProxyConfig::~DataReductionProxyConfig() {
   net::NetworkChangeNotifier::RemoveIPAddressObserver(this);
 }
 
-void DataReductionProxyConfig::InitializeOnIOThread(const scoped_refptr<
-    net::URLRequestContextGetter>& url_request_context_getter) {
+void DataReductionProxyConfig::InitializeOnIOThread(
+    const scoped_refptr<net::URLRequestContextGetter>&
+        basic_url_request_context_getter,
+    net::URLRequestContextGetter* url_request_context_getter) {

[RyanSturm, 2016/12/08 20:59:17]

How come some places pass const& for the getters and some pass raw pointers?

[tbansal1, 2016/12/10 00:48:48]

Done.

+  DCHECK(thread_checker_.CalledOnValidThread());
+
   secure_proxy_checker_.reset(
-      new SecureProxyChecker(url_request_context_getter));
+      new SecureProxyChecker(basic_url_request_context_getter));
+  warmup_url_fetcher_.reset(new WarmupURLFetcher(url_request_context_getter));
 
   if (!config_values_->allowed())
     return;
@@ -647,6 +710,7 @@ void DataReductionProxyConfig::SetProxyConfig(bool enabled, bool at_startup) {
 
   if (enabled) {
     HandleCaptivePortal();
+    FetchWarmupURL();
 
     // Check if the proxy has been restricted explicitly by the carrier.
     // It is safe to use base::Unretained here, since it gets executed
@@ -732,6 +796,8 @@ void DataReductionProxyConfig::HandleSecureProxyCheckResponse(
 }
 
 void DataReductionProxyConfig::OnIPAddressChanged() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
   if (enabled_by_user_) {
     DCHECK(config_values_->allowed());
     RecordNetworkChangeEvent(IP_CHANGED);
@@ -741,6 +807,7 @@ void DataReductionProxyConfig::OnIPAddressChanged() {
     network_quality_at_last_query_ = NETWORK_QUALITY_AT_LAST_QUERY_UNKNOWN;
 
     HandleCaptivePortal();
+    FetchWarmupURL();
     // It is safe to use base::Unretained here, since it gets executed
     // synchronously on the IO thread, and |this| outlives
     // |secure_proxy_checker_|.
@@ -791,6 +858,23 @@ void DataReductionProxyConfig::SecureProxyCheck(
                                                      fetcher_callback);
 }
 
+void DataReductionProxyConfig::FetchWarmupURL() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  if (!enabled_by_user_ || !params::FetchWarmupURLEnabled())
+    return;
+
+  warmup_url_fetcher_->FetchWarmupURL();
+}
+
+void DataReductionProxyConfig::SetWarmupURLFetcherCallbackForTesting(
+    base::Callback<void()> warmup_url_fetched_callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  warmup_url_fetcher_->SetWarmupURLFetcherCallbackForTesting(
+      warmup_url_fetched_callback);
+}
+
 void DataReductionProxyConfig::SetLoFiModeOff() {
   DCHECK(thread_checker_.CalledOnValidThread());
   lofi_off_ = true;