Add Data Saver support to Cronet

components/cronet/android/cronet_data_reduction_proxy.cc

+// Copyright 2015 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/cronet/android/cronet_data_reduction_proxy.h"
+
+#include "base/command_line.h"
+#include "base/prefs/pref_registry_simple.h"
+#include "base/prefs/pref_service.h"
+#include "base/prefs/pref_service_factory.h"
+#include "base/single_thread_task_runner.h"
+#include "components/cronet/android/cronet_in_memory_pref_store.h"
+#include "components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h"
+#include "components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h"
+#include "components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h"
+#include "components/data_reduction_proxy/core/browser/data_reduction_proxy_prefs.h"
+#include "components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h"
+#include "components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h"
+#include "components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h"
+#include "components/data_reduction_proxy/core/common/data_reduction_proxy_params.h"
+#include "components/data_reduction_proxy/core/common/data_reduction_proxy_switches.h"
+#include "net/url_request/url_request_context_getter.h"
+#include "net/url_request/url_request_interceptor.h"
+
+namespace net {
+class NetLog;

[mmenke, 2015/04/24 19:10:28]

This is also forward declared in the header, so not needed here.

[bengr, 2015/04/24 21:13:50]

Done.

+}
+
+namespace {
+// Shows notifications which correspond to PersistentPrefStore's reading errors.
+void HandleReadError(PersistentPrefStore::PrefReadError error) {
+}
+}  // namespace

[mmenke, 2015/04/24 19:10:27]

This block can be removed, per later comment.

[bengr, 2015/04/24 21:13:50]

Done.

+
+namespace cronet {
+namespace {

[mmenke, 2015/04/24 19:10:27]

int break after start of namespace (Can include one or not for nested
namespaces, but should be one after the inner namespace)

[bengr, 2015/04/24 21:13:50]

Done.

+scoped_ptr<PrefService> CreatePrefService() {
+  PrefRegistrySimple* pref_registry = new PrefRegistrySimple();

[mmenke, 2015/04/24 19:27:23]

using a scoped_refptr here makes it clearer this isn't leaked (Admittedly,
pref_service_factory.Create takes it as a raw pointer, which is weird, but still
think it's an improvement)

[bengr, 2015/04/24 21:13:50]

Done.

+  data_reduction_proxy::RegisterSimpleProfilePrefs(pref_registry);
+  base::PrefServiceFactory pref_service_factory;
+  pref_service_factory.set_user_prefs(
+      make_scoped_refptr(new CronetInMemoryPrefStore()));
+  pref_service_factory.set_read_error_callback(base::Bind(&HandleReadError));

[mmenke, 2015/04/24 19:10:28]

This isn't needed.  PrefServiceFactory does this exact same thing in its
constructor.

[bengr, 2015/04/24 21:13:50]

Done.

+  return pref_service_factory.Create(pref_registry).Pass();
+}
+
+void AddOptionsToCommandLine(const std::string& primary_proxy,
+                             const std::string& fallback_proxy,
+                             const std::string& secure_proxy_check_url,
+                             base::CommandLine* command_line) {
+  DCHECK((primary_proxy.empty() && fallback_proxy.empty() &&

[mmenke, 2015/04/24 19:10:27]

include base/logging.h for DCHECK

[bengr, 2015/04/24 21:13:50]

Done.

+      secure_proxy_check_url.empty()) ||

[mmenke, 2015/04/24 19:10:27]

If these are all empty, can we just not create this class at all?  (And so get
rid of the first part of this DCHECK)

[bengr, 2015/04/24 21:13:50]

These override statically configured defaults, so it's fine (and the default
behavior) to leave these blank. These are only used for tests.

+         (!primary_proxy.empty() && !fallback_proxy.empty() &&
+             !secure_proxy_check_url.empty()));

[mmenke, 2015/04/24 19:27:23]

You're using 3 space indent here for both of the above lines, rather than 4.

[bengr, 2015/04/24 21:13:50]

Done.

+  if (primary_proxy.empty())
+    return;
+  command_line->AppendSwitchASCII(
+      data_reduction_proxy::switches::kDataReductionProxy, primary_proxy);
+  command_line->AppendSwitchASCII(
+      data_reduction_proxy::switches::kDataReductionProxyFallback,
+      fallback_proxy);
+  command_line->AppendSwitchASCII(
+      data_reduction_proxy::switches::kDataReductionProxySecureProxyCheckURL,
+      secure_proxy_check_url);
+}
+}  // namespace

[mmenke, 2015/04/24 19:10:27]

line break before end of namespace.

[bengr, 2015/04/24 21:13:50]

Done.

+
+CronetDataReductionProxy::CronetDataReductionProxy(
+    const std::string& key,
+    const std::string& primary_proxy,
+    const std::string& fallback_proxy,
+    const std::string& secure_proxy_check_url,
+    const std::string& user_agent,
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+    net::NetLog* net_log)
+    : task_runner_(task_runner) {
+  AddOptionsToCommandLine(primary_proxy, fallback_proxy, secure_proxy_check_url,
+                          base::CommandLine::ForCurrentProcess());

[mmenke, 2015/04/24 19:10:27]

Hrm...  This isn't threadsafe.  If we were creating two of these at once on
different threads, even with the same parameters, bad things could happen.

Also, if the DRP code relies on reading command line options as it needs access
to the parameters, we could also get into serious trouble.

[bengr, 2015/04/24 21:13:50]

This is here only for tests. I've added a TODO to not use the command line. This
is planned anyway.

+  prefs_ = CreatePrefService();
+  settings_.reset(
+      new data_reduction_proxy::DataReductionProxySettings());
+  io_data_.reset(
+      new data_reduction_proxy::DataReductionProxyIOData(
+          data_reduction_proxy::Client::CRONET_ANDROID,
+          data_reduction_proxy::DataReductionProxyParams::kAllowed |
+              data_reduction_proxy::DataReductionProxyParams::kFallbackAllowed,
+          net_log, task_runner, task_runner, false, user_agent));
+  io_data_->request_options()->SetKeyOnIO(key);
+  io_data_->InitOnUIThread(prefs_.get());
+}
+
+CronetDataReductionProxy::~CronetDataReductionProxy() {
+  io_data_->ShutdownOnUIThread();
+}
+
+scoped_ptr<net::NetworkDelegate>
+CronetDataReductionProxy::CreateNetworkDelegate(
+    scoped_ptr<net::NetworkDelegate> wrapped_network_delegate) {
+  return io_data_->CreateNetworkDelegate(wrapped_network_delegate.Pass(),
+                                         false /* No bypass UMA */ );
+}
+
+scoped_ptr<net::URLRequestInterceptor>
+CronetDataReductionProxy::CreateInterceptor() {
+  return io_data_->CreateInterceptor();
+}
+
+void CronetDataReductionProxy::Init(bool enable,
+                                    net::URLRequestContext* context) {
+  url_request_context_getter_ =
+      new net::TrivialURLRequestContextGetter(
+          context, task_runner_);
+  scoped_ptr<data_reduction_proxy::DataReductionProxyCompressionStats>
+      compression_stats(
+          new data_reduction_proxy::DataReductionProxyCompressionStats(
+              prefs_.get(), task_runner_, base::TimeDelta()));
+  scoped_ptr<data_reduction_proxy::DataReductionProxyService>
+      data_reduction_proxy_service(
+          new data_reduction_proxy::DataReductionProxyService(
+              compression_stats.Pass(), settings_.get(),
+              url_request_context_getter_.get(), task_runner_));
+  io_data_->SetDataReductionProxyService(
+      data_reduction_proxy_service->GetWeakPtr());
+  settings_->InitDataReductionProxySettings(
+      prefs_.get(), io_data_.get(), data_reduction_proxy_service.Pass());
+  settings_->SetDataReductionProxyEnabled(enable);
+  settings_->MaybeActivateDataReductionProxy(true);
+}
+
+}  // namespace cronet