Added probe to determine if data reduction proxy can be used

chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc

Index: chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc
diff --git a/chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc b/chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc
new file mode 100644
index 0000000000000000000000000000000000000000..4a15b0f3d1d9b556566400b94ddd1ea4d769ae85
--- /dev/null
+++ b/chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc
@@ -0,0 +1,600 @@
+// Copyright 2013 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/net/spdyproxy/data_reduction_proxy_settings_android.h"
+
+#include "base/android/build_info.h"
+#include "base/android/jni_android.h"
+#include "base/android/jni_string.h"
+#include "base/base64.h"
+#include "base/command_line.h"
+#include "base/metrics/field_trial.h"
+#include "base/metrics/histogram.h"
+#include "base/prefs/pref_service.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_util.h"
+#include "base/strings/stringprintf.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/prefs/proxy_prefs.h"
+#include "chrome/browser/prefs/scoped_user_pref_update.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/common/pref_names.h"
+#include "jni/DataReductionProxySettingsAndroid_jni.h"
+#include "net/base/load_flags.h"
+#include "net/base/net_errors.h"
+#include "net/url_request/url_fetcher.h"
+#include "net/url_request/url_fetcher_delegate.h"
+#include "net/url_request/url_request_status.h"
+#include "url/gurl.h"
+
+using base::android::CheckException;
+using base::android::ConvertJavaStringToUTF8;
+using base::android::ConvertUTF8ToJavaString;
+using base::android::ScopedJavaLocalRef;
+using base::StringPrintf;
+
+
+namespace {
+
+// The C++ definition of enum SpdyProxyAuthState defined in
+// tools/histograms/histograms.xml.
+// New values should be added at the end before |NUM_SPDY_PROXY_AUTH_STATE|
+enum {
+  CHROME_STARTUP,
+  SPDY_PROXY_AUTH_ON_AT_STARTUP,
+  SPDY_PROXY_AUTH_ON_BY_USER,
+  SPDY_PROXY_AUTH_OFF_BY_USER,
+  // Used by UMA histograms and should always be the last value.
+  NUM_SPDY_PROXY_AUTH_STATE
+};
+
+}  // namespace
+
+
+DataReductionProxySettingsAndroid::DataReductionProxySettingsAndroid(
+    JNIEnv* env, jobject obj)
+    : has_turned_on_(false),
+      has_turned_off_(false),
+      disabled_by_carrier_(false),
+      enabled_by_user_(false),
+      original_profile_prefs_for_testing_(NULL),
+      local_state_prefs_for_testing_(NULL) {
+}
+
+DataReductionProxySettingsAndroid::~DataReductionProxySettingsAndroid() {
+}
+
+jboolean DataReductionProxySettingsAndroid::IsDataReductionProxyAvailable(
+    JNIEnv* env, jobject obj) {
+  return IsDataReductionProxyAvailable();
+}
+
+jboolean DataReductionProxySettingsAndroid::IsDataReductionProxyPromoAvailable(
+    JNIEnv* env, jobject obj) {
+  return IsDataReductionProxyPromoAvailable();
+}
+
+std::string DataReductionProxySettingsAndroid::GetDataReductionProxyOrigin() {
+  if (!IsDataReductionProxyAvailable())
+    return std::string();
+  const CommandLine& command_line = *CommandLine::ForCurrentProcess();
+  if (command_line.HasSwitch(switches::kSpdyProxyAuthOrigin)) {
+    return command_line.GetSwitchValueASCII(switches::kSpdyProxyAuthOrigin);
+  }
+#if defined(SPDY_PROXY_AUTH_ORIGIN)
+  return SPDY_PROXY_AUTH_ORIGIN;
+#else
+  return std::string();
+#endif
+}
+
+ScopedJavaLocalRef<jstring>
+DataReductionProxySettingsAndroid::GetDataReductionProxyOrigin(

[mmenke, 2013/09/10 16:17:29]

If the switch is mapped to the pref, seems like we should actually be using the
pref, not the switch, since the pref can theoretically be set other ways. 
Alternatively, we could get rid of the pref and just use the switch.

[bengr, 2013/09/10 18:51:05]

The plan is to get rid of the pref, once we verify that iOS does not depend on
it.

[mmenke, 2013/09/10 19:54:06]

Ok, then using the command line switch seems reasonable to me.

+    JNIEnv* env, jobject obj) {
+  return ConvertUTF8ToJavaString(env, GetDataReductionProxyOrigin());
+}
+
+ScopedJavaLocalRef<jstring>
+DataReductionProxySettingsAndroid::GetDataReductionProxyValue(
+    JNIEnv* env, jobject obj) {
+  if (!IsDataReductionProxyAvailable())
+    return ConvertUTF8ToJavaString(env, std::string());
+  const CommandLine& command_line = *CommandLine::ForCurrentProcess();
+  if (command_line.HasSwitch(switches::kSpdyProxyAuthOrigin)) {
+    // If an origin is provided via a switch, then only consider the value
+    // that is provided by a switch. Do not use the preprocessor constant.
+    if (command_line.HasSwitch(switches::kSpdyProxyAuthValue)) {
+      return ConvertUTF8ToJavaString(
+          env,
+          command_line.GetSwitchValueASCII(switches::kSpdyProxyAuthValue));
+    }
+  }
+#if defined(SPDY_PROXY_AUTH_VALUE)
+  return ConvertUTF8ToJavaString(env, SPDY_PROXY_AUTH_VALUE);
+#else
+  return ConvertUTF8ToJavaString(env, std::string());
+#endif
+}
+
+jlong DataReductionProxySettingsAndroid::GetDataReductionLastUpdateTime(
+    JNIEnv* env, jobject obj) {
+  PrefService* local_state = GetLocalStatePrefs();
+  int64 last_update_internal =
+      local_state->GetInt64(prefs::kDailyHttpContentLengthLastUpdateDate);
+  base::Time last_update = base::Time::FromInternalValue(last_update_internal);
+  return static_cast<int64>(last_update.ToJsTime());
+}
+
+base::android::ScopedJavaLocalRef<jobject>
+DataReductionProxySettingsAndroid::GetContentLengths(JNIEnv* env,
+                                                     jobject obj) {
+  int64 original_content_length;
+  int64 received_content_length;
+  int64 last_update_internal;
+  GetContentLengths(spdyproxy::kNumDaysInHistorySummary,
+                    &original_content_length,
+                    &received_content_length, &last_update_internal);
+
+  return Java_ContentLengths_create(env,
+                                    original_content_length,
+                                    received_content_length);
+}
+
+void DataReductionProxySettingsAndroid::AddDefaultProxyBypassRules() {
+  // localhost
+  AddHostToBypass("localhost");
+  AddHostPatternToBypass("localhost.*");
+  AddHostToBypass("127.0.0.1");
+  // TODO(bengr): revisit 192.168.*?  10.*? 172.16.0.0 - 172.31.255.255. The
+  // concern was that adding these and other rules would add to the processing
+  // time.

[mmenke, 2013/09/10 16:17:29]

Those question marks don't seem to be needed.  Maybe commas instead?

[mmenke, 2013/09/10 16:17:29]

If we're concerned about processing time, we could do much better.

1)  We could precompute regular expressions, instead of using shExpMatch.  I
believe it would get rid of much of the computational cost.  We'd have to switch
to using JS regexps directly instead of using shExpMatch to convert them, but
it's a simple switch.
2)  See other suggestion about using C++ instead of JS.  Obviously it's a more
significant change.

Neither belongs in this CL, just thought I'd throw the ideas out there.

[bengr, 2013/09/10 18:51:05]

Done.

[bengr, 2013/09/10 18:51:05]

Thanks. I think iOS moved away from using a PAC, which is probably the right way
to go.

+
+  // TODO(bengr): See http://crbug.com/169959. For some reason the data
+  // reduction proxy is breaking the omnibox SearchProvider.  Remove this rule
+  // when this is fixed.
+  AddURLPatternToBypass("http://www.google.com/complete/search*");
+
+  // http://freezone.google.com/*
+  // http://www.freezone.google.com/*
+  // http://accounts.freezone.google.com/*
+  // http://mail.freezone.google.com/*
+  // http://plus.freezone.google.com/*
+  // http://search.freezone.google.com/*
+  // TODO(bengr): Consider being more restrictive and match
+  // "freezone.google.com" and "*.freezone.google.com" instead.
+  AddHostPatternToBypass("*freezone.google.com");
+  // http://freezone.googleusercontent.com/*
+  AddHostPatternToBypass("freezone.googleusercontent.com");
+  // http://g.co/gms/*
+  AddURLPatternToBypass("http://g.co/gms/*");
+  // http://g.co/freezone*
+  AddURLPatternToBypass("http://g.co/freezone*");

[mmenke, 2013/09/10 16:17:29]

I think all these these exceptions need a bit more explanation.  If the
explanation is non-public information (And not destined to become public
information), they should probably not go in the public repository.

[bengr, 2013/09/10 18:51:05]

Done.

+
+  // Check for proxy availability
+  std::string proxy_check_url = GetProxyCheckURL();
+  if (!proxy_check_url.empty()) {
+    AddURLPatternToBypass(GetProxyCheckURL());
+  }
+}
+
+void DataReductionProxySettingsAndroid::AddURLPatternToBypass(
+    const std::string& pattern) {
+  AddPatternToBypass("url", pattern);
+}
+
+void DataReductionProxySettingsAndroid::AddHostPatternToBypass(
+    const std::string& pattern) {
+  AddPatternToBypass("host", pattern);
+}
+
+void DataReductionProxySettingsAndroid::AddPatternToBypass(
+    const std::string& url_or_host,
+    const std::string& pattern) {
+  bypass_rules_.push_back(
+      StringPrintf("shExpMatch(%s, \"%s\")",
+                   url_or_host.c_str(), pattern.c_str()));
+}
+
+void DataReductionProxySettingsAndroid::AddHostToBypass(
+    const std::string& host) {
+  bypass_rules_.push_back(
+      StringPrintf("host == \"%s\"", host.c_str()));
+}
+
+bool DataReductionProxySettingsAndroid::IsDataReductionProxyAvailable() {
+  return (base::FieldTrialList::FindFullName("DataCompressionProxyRollout") ==
+      "Enabled");

[mmenke, 2013/09/10 16:17:29]

Optional:  Think this is a little easier to read if indented 4 beyond the open
paren.  If you disagree, fine to leave as is.

[bengr, 2013/09/10 18:51:05]

Done.

+}
+
+bool DataReductionProxySettingsAndroid::IsDataReductionProxyPromoAvailable() {
+  return (IsDataReductionProxyAvailable() && base::FieldTrialList::FindFullName(
+      "DataCompressionProxyPromoVisibility") == "Enabled");

[mmenke, 2013/09/10 16:17:29]

I think just a "using base::FieldTrialList;" up with the other using directives
would be simplest.

Could also go with a subroutine or storing the field trial name in a temporary.

If you aren't a big fan of any of these, that's fine.

[bengr, 2013/09/10 18:51:05]

Done.

+}

[mmenke, 2013/09/10 16:17:29]

Since these two functions aren't used outside this file, and only depend on
globals, suggest removing both functions from this class, and putting both them
in an anonymous namespace in this file.

[bengr, 2013/09/10 18:51:05]

Done.

+
+// TODO(bengr): Replace with our own ProxyResolver.
+std::string DataReductionProxySettingsAndroid::GetProxyPacScript() {
+  std::string bypass_clause = "(" + JoinString(bypass_rules_, ") || (") + ")";
+
+  // We want to fall back to direct loading when the proxy is unavailable and
+  // only process HTTP traffic, so we concoct a PAC configuration
+  // accordingly. (With a statically configured proxy, proxy failures will
+  // simply result in a connection error presented to users.)
+
+  std::string pac = "function FindProxyForURL(url, host) {"
+      "  if (" + bypass_clause + ") {"
+      "    return \"DIRECT\";"
+      "  } "
+      "  if (url.substring(0, 5) == \"http:\") {"
+      "    return \"HTTPS " + GetDataReductionProxyOriginHostPort() +
+      "; DIRECT\";"
+      "  }"
+      "  return \"DIRECT\";"
+      "}";
+  return pac;
+}
+
+PrefService* DataReductionProxySettingsAndroid::OriginalProfilePrefs() {
+  if (original_profile_prefs_for_testing_)
+    return original_profile_prefs_for_testing_;
+  return g_browser_process->profile_manager()->GetDefaultProfile()->
+      GetOriginalProfile()->GetPrefs();

[mmenke, 2013/09/10 16:17:29]

Why are we using the default profile, and not the active one?

[bengr, 2013/09/10 18:51:05]

Android only has one non-incognito profile, and this is the convention for
accessing it.

+}
+
+PrefService* DataReductionProxySettingsAndroid::GetLocalStatePrefs() {
+  if (local_state_prefs_for_testing_)
+    return local_state_prefs_for_testing_;
+  return g_browser_process->local_state();
+}
+
+void DataReductionProxySettingsAndroid::set_pref_service(
+    PrefService* pref_service) {
+  original_profile_prefs_for_testing_ = pref_service;
+}
+
+void DataReductionProxySettingsAndroid::set_local_state_for_testing(
+    PrefService* local_state) {
+  local_state_prefs_for_testing_ = local_state;
+}
+
+std::string
+DataReductionProxySettingsAndroid::GetDataReductionProxyOriginHostPort() {
+  std::string spdy_proxy = GetDataReductionProxyOrigin();
+  if (spdy_proxy.empty()) {
+    DLOG(ERROR) << "A SPDY proxy has not been set.";
+    return spdy_proxy;
+  }
+  RemoveSchemeAndTrailingSlash(&spdy_proxy);
+  return spdy_proxy;
+}
+
+void DataReductionProxySettingsAndroid::RemoveSchemeAndTrailingSlash(

[mmenke, 2013/09/10 16:17:29]

I think this kind of roll your own URL manipulation thing tends to lead to bugs.
 I'd suggest using a GURL and then extracting its host and port.  Could also use
net::HostPorPair(GURL(url)).ToString(), though that may be overkill.

It may be a bit slower, but it's much less susceptible to bugs, and doesn't look
like speed matters here.

[bengr, 2013/09/10 18:51:05]

Done.

+    std::string* url) {
+  // Remove a trailing slash from the proxy string if one exists as well as
+  // leading HTTPS scheme.
+  DCHECK(url);
+  unsigned len = url->size();
+  if (len > 0 && (*url)[len - 1] == '/') {
+    url->erase(len - 1, 1);
+  }
+  std::string https_scheme("https://");
+  if (len > https_scheme.length() &&
+      url->compare(0, https_scheme.length(), https_scheme) == 0) {
+    url->erase(0, https_scheme.length());
+  }
+}
+
+void DataReductionProxySettingsAndroid::ResetDataReductionStatistics() {
+  PrefService* prefs = GetLocalStatePrefs();
+  if (!prefs)
+    return;
+  ListPrefUpdate original_update(prefs, prefs::kDailyHttpOriginalContentLength);
+  ListPrefUpdate received_update(prefs, prefs::kDailyHttpReceivedContentLength);
+  original_update->Clear();
+  received_update->Clear();
+  for (size_t i = 0; i < spdyproxy::kNumDaysInHistory; ++i) {
+    original_update->AppendString(base::Int64ToString(0));
+    received_update->AppendString(base::Int64ToString(0));
+  }
+}
+
+void DataReductionProxySettingsAndroid::InitDataReductionProxySettings(
+    JNIEnv* env,
+    jobject obj) {
+  // Disable the proxy is it's not meant to be available.
+  if (!IsDataReductionProxyAvailable()) return;

[mmenke, 2013/09/10 16:17:29]

nit:  return should be on next line.

[bengr, 2013/09/10 18:51:05]

Done.

+
+  AddDefaultProxyBypassRules();
+  net::NetworkChangeNotifier::AddIPAddressObserver(this);
+
+  PrefService* prefs = OriginalProfilePrefs();
+  CommandLine& command_line = *CommandLine::ForCurrentProcess();

[mmenke, 2013/09/10 16:17:29]

This should be "const CommandLine&".  Or keep it as a pointer.

[bengr, 2013/09/10 18:51:05]

Done.

+  bool spdy_proxy_enabled = prefs->GetBoolean(prefs::kSpdyProxyAuthEnabled);
+
+  // Setting the kEnableSpdyProxyAuth switch has the same effect as enabling
+  // the feature via settings, in that once set, the preference will be sticky
+  // across instances of Chrome. Disabling the feature can only be done through
+  // the settings menu.
+  UMA_HISTOGRAM_ENUMERATION("SpdyProxyAuth.State", CHROME_STARTUP,
+                            NUM_SPDY_PROXY_AUTH_STATE);
+  if (command_line.HasSwitch(switches::kEnableSpdyProxyAuth) ||
+      spdy_proxy_enabled) {
+    SetDataReductionProxyEnabled(NULL, NULL, true);
+  } else {
+    LOG(WARNING) << "SPDY proxy OFF at startup.";
+  }
+}
+
+jboolean DataReductionProxySettingsAndroid::IsDataReductionProxyEnabled(
+    JNIEnv* env, jobject obj) {
+  return OriginalProfilePrefs()->GetBoolean(prefs::kSpdyProxyAuthEnabled);
+}
+
+jboolean DataReductionProxySettingsAndroid::IsDataReductionProxyManaged(
+    JNIEnv* env, jobject obj) {
+  return OriginalProfilePrefs()->IsManagedPreference(
+      prefs::kSpdyProxyAuthEnabled);
+}
+
+void DataReductionProxySettingsAndroid::SetDataReductionProxyEnabled(
+    JNIEnv* env,
+    jobject obj,
+    jboolean enabled) {
+  // Prevent configuring the proxy when it is unavailable.
+  if (!IsDataReductionProxyAvailable())
+    return;
+
+  // Check if the proxy has been disabled explicitly by the carrier.
+  CheckDataReductionProxyIsAvailable(GetProxyCheckURL());
+
+  PrefService* prefs = OriginalProfilePrefs();
+
+  prefs->SetBoolean(prefs::kSpdyProxyAuthEnabled, enabled);
+
+  if (enabled && !prefs->GetBoolean(prefs::kSpdyProxyAuthWasEnabledBefore)) {
+    prefs->SetBoolean(prefs::kSpdyProxyAuthWasEnabledBefore, true);
+    ResetDataReductionStatistics();
+  }
+
+  std::string spdy_proxy_origin = GetDataReductionProxyOriginHostPort();
+
+  // Configure use of the data reduction proxy if it is enabled and the proxy
+  // origin is non-empty.
+  enabled_by_user_= enabled && spdy_proxy_origin != "";
+  SetProxyPac(enabled_by_user_, !env);
+}
+
+void DataReductionProxySettingsAndroid::SetProxyPac(bool enable_spdy_proxy,
+                                                    bool at_startup) {
+  PrefService* prefs = OriginalProfilePrefs();
+  DCHECK(prefs);
+  // Keys duplicated from proxy_config_dictionary.cc
+  // TODO(bengr): Move these to proxy_config_dictionary.h and reuse them here.
+  const char kProxyMode[] = "mode";
+  const char kProxyPacURL[] = "pac_url";
+  const char kAtStartup[] = "at startup";
+  const char kByUser[] = "by user action";
+
+  DictionaryPrefUpdate update(prefs, prefs::kProxy);
+  DictionaryValue* dict = update.Get();
+  if (enable_spdy_proxy) {
+    LOG(WARNING) << "SPDY proxy ON " << (at_startup ? kAtStartup : kByUser);
+    // Convert to a data URI and update the PAC settings.
+    std::string base64_pac;
+    base::Base64Encode(GetProxyPacScript(), &base64_pac);
+
+    dict->SetString(kProxyPacURL,
+                    "data:application/x-ns-proxy-autoconfig;base64," +
+                    base64_pac);
+    dict->SetString(kProxyMode,
+                    ProxyModeToString(ProxyPrefs::MODE_PAC_SCRIPT));
+
+    if (at_startup) {
+      UMA_HISTOGRAM_ENUMERATION("SpdyProxyAuth.State",
+                                SPDY_PROXY_AUTH_ON_AT_STARTUP,
+                                NUM_SPDY_PROXY_AUTH_STATE);
+    } else if (!has_turned_on_) {
+      // SPDY proxy auth is turned on by user action for the first time in
+      // this session.
+      UMA_HISTOGRAM_ENUMERATION("SpdyProxyAuth.State",
+                                SPDY_PROXY_AUTH_ON_BY_USER,
+                                NUM_SPDY_PROXY_AUTH_STATE);
+      has_turned_on_ = true;
+    }
+  } else {
+    LOG(WARNING) << "SPDY proxy OFF " << (at_startup ? kAtStartup : kByUser);
+    dict->SetString(kProxyMode, ProxyModeToString(ProxyPrefs::MODE_SYSTEM));
+    dict->SetString(kProxyPacURL, "");
+
+    if (!at_startup && !has_turned_off_) {
+      UMA_HISTOGRAM_ENUMERATION("SpdyProxyAuth.State",
+                                SPDY_PROXY_AUTH_OFF_BY_USER,
+                                NUM_SPDY_PROXY_AUTH_STATE);
+      has_turned_off_ = true;
+    }
+  }
+}
+
+void DataReductionProxySettingsAndroid::OnIPAddressChanged() {
+  CheckDataReductionProxyIsAvailable(GetProxyCheckURL());
+}
+
+void DataReductionProxySettingsAndroid::CheckDataReductionProxyIsAvailable(
+    const std::string& url) {
+  if (url.empty())
+    return;
+  fetcher_.reset(net::URLFetcher::Create(0, GURL(url),
+                                         net::URLFetcher::GET, this));
+  fetcher_->SetLoadFlags(net::LOAD_DISABLE_CACHE);
+  Profile* profile = g_browser_process->profile_manager()->
+      GetDefaultProfile();
+  fetcher_->SetRequestContext(profile->GetRequestContext());
+  // Configure to max_retries at most kMaxRetries times for 5xx errors.
+  static const int kMaxRetries = 5;
+  fetcher_->SetMaxRetriesOn5xx(kMaxRetries);
+  fetcher_->Start();
+}
+
+void DataReductionProxySettingsAndroid::OnURLFetchComplete(
+    const net::URLFetcher* source) {
+  net::URLRequestStatus status = source->GetStatus();
+  if (status.status() == net::URLRequestStatus::FAILED &&
+      status.error() == net::ERR_INTERNET_DISCONNECTED) {
+    return;
+  }
+
+  std::string response;
+  source->GetResponseAsString(&response);
+
+  if ("OK" == response.substr(0, 2)) {
+    DVLOG(1) << "The data reduction proxy is not blocked.";
+
+    // The user enabled the proxy, but sometime previously in the session,
+    // the carrier had disabled the proxy. Now that the carrier is enabling
+    // it, configure it to the user's desires.

[mmenke, 2013/09/10 16:17:29]

I suggest putting this in the body of the if statement, to make clear it's
referring to the if, and not this entire code block.  Alternatively, could
rephrase the comment, to make it clear it's conditional.  ("Check if...  If that
was the case, now the carrier...")

Also not really clear it was the carrier - if the runs in the wifi case, could
be a captive portal or something.

[bengr, 2013/09/10 18:51:05]

Done.

+    if (enabled_by_user_ && disabled_by_carrier_)
+      SetProxyPac(true, false);
+    disabled_by_carrier_ = false;
+    return;
+  }
+  DVLOG(1) << "The data reduction proxy is blocked.";
+  // Disable the proxy.
+  if (enabled_by_user_ && !disabled_by_carrier_)
+    SetProxyPac(false, false);
+  disabled_by_carrier_ = true;
+}
+
+std::string DataReductionProxySettingsAndroid::GetProxyCheckURL() {

[mmenke, 2013/09/10 16:17:29]

Can go in an anonymous namespace

[bengr, 2013/09/10 18:51:05]

Done.

+#if defined(DATA_REDUCTION_PROXY_PROBE_URL)
+  return DATA REDUCTION_PROXY_PROBE_URL;

[mmenke, 2013/09/10 16:17:29]

BUG:  Looks like you're missing an underscore after DATA.  Did this even
compile?

[bengr, 2013/09/10 18:51:05]

Yes, it compiled because I didn't define DATA_REDUCTION_PROXY_PROBE_URL. I'm
waiting for the CL to be in better shape before retesting this logic again.

+#else
+  return std::string();
+#endif
+}
+
+int64 DataReductionProxySettingsAndroid::GetInt64PrefValue(

[mmenke, 2013/09/10 16:17:29]

Don't think this needs to be a member of DataReductionProxySettingsAndroid - can
just put it in an anonymous namespace.  No reason for the header to require
base::Value to be declared.

[bengr, 2013/09/10 18:51:05]

Done.

+    const ListValue& list_value, size_t index) {
+  int64 val = 0;
+  std::string pref_value;
+  bool rv = list_value.GetString(index, &pref_value);
+  DCHECK(rv);
+  if (rv) {
+    rv = base::StringToInt64(pref_value, &val);
+    DCHECK(rv);
+  }
+  return val;
+}
+
+ScopedJavaLocalRef<jlongArray>
+DataReductionProxySettingsAndroid::GetDailyContentLengths(
+    JNIEnv* env, const char* pref_name) {
+  jlongArray result = env->NewLongArray(spdyproxy::kNumDaysInHistory);
+  PrefService* local_state = GetLocalStatePrefs();
+  if (!local_state)
+    return ScopedJavaLocalRef<jlongArray>(env, result);
+
+  const ListValue* list_value = local_state->GetList(pref_name);
+  if (list_value->GetSize() != spdyproxy::kNumDaysInHistory)
+    return ScopedJavaLocalRef<jlongArray>(env, result);
+
+  jlong jval[spdyproxy::kNumDaysInHistory];
+  for (size_t i = 0; i < spdyproxy::kNumDaysInHistory; ++i) {
+//    int64 val = 0;

[nyquist, 2013/09/10 02:24:02]

Remove this now that you have extracted the method

[bengr, 2013/09/10 18:51:05]

Done.

+//    std::string pref_value;
+//    bool rv = list_value->GetString(i, &pref_value);
+//    DCHECK(rv);
+//    if (rv) {
+//      rv = base::StringToInt64(pref_value, &val);
+//      DCHECK(rv);
+//    }
+    jval[i] =GetInt64PrefValue(*list_value, i);

[nyquist, 2013/09/10 02:24:02]

nit: space after =

[bengr, 2013/09/10 18:51:05]

Done.

+  }
+  env->SetLongArrayRegion(result, 0, spdyproxy::kNumDaysInHistory, jval);
+  return ScopedJavaLocalRef<jlongArray>(env, result);
+}
+
+ScopedJavaLocalRef<jlongArray>
+DataReductionProxySettingsAndroid::GetDailyOriginalContentLengths(
+    JNIEnv* env, jobject obj) {
+  return GetDailyContentLengths(env, prefs::kDailyHttpOriginalContentLength);
+}
+
+ScopedJavaLocalRef<jlongArray>
+DataReductionProxySettingsAndroid::GetDailyReceivedContentLengths(
+    JNIEnv* env, jobject obj) {
+  return GetDailyContentLengths(env, prefs::kDailyHttpReceivedContentLength);
+}
+
+void DataReductionProxySettingsAndroid::GetContentLengths(
+    unsigned int days,
+    int64* original_content_length,
+    int64* received_content_length,
+    int64* last_update_time) {
+  DCHECK_LE(days, spdyproxy::kNumDaysInHistory);
+  PrefService* local_state = GetLocalStatePrefs();
+  if (!local_state) {
+    *original_content_length = 0L;
+    *received_content_length = 0L;
+    *last_update_time = 0L;
+    return;
+  }
+
+  const ListValue* original_list =
+      local_state->GetList(prefs::kDailyHttpOriginalContentLength);
+  const ListValue* received_list =
+      local_state->GetList(prefs::kDailyHttpReceivedContentLength);
+  int64 last_update_internal =
+      local_state->GetInt64(prefs::kDailyHttpContentLengthLastUpdateDate);
+
+  if (original_list->GetSize() != spdyproxy::kNumDaysInHistory ||
+      received_list->GetSize() != spdyproxy::kNumDaysInHistory) {
+    *original_content_length = 0L;
+    *received_content_length = 0L;
+    *last_update_time = 0L;
+    return;
+  }
+
+  int64 orig = 0L;
+  int64 recv = 0L;
+  // We include days from the end of the list going backwards.
+  for (unsigned int i = 0; i < days; ++i) {
+    int read_index = spdyproxy::kNumDaysInHistory - 1 - i;
+    std::string result;
+    int64 val;
+    if (original_list->GetString(read_index, &result)) {

[mmenke, 2013/09/10 16:17:29]

GetInt64PrefValue?

[bengr, 2013/09/10 18:51:05]

Done.

+      if (base::StringToInt64(result, &val))
+        orig += val;
+      else
+        DCHECK(false);
+    } else {
+      DCHECK(false);
+    }
+    if (received_list->GetString(read_index, &result)) {

[mmenke, 2013/09/10 16:17:29]

GetInt64PrefValue?

[bengr, 2013/09/10 18:51:05]

Done.

+      if (base::StringToInt64(result, &val))
+        recv += val;
+      else
+        NOTREACHED();
+    } else {
+      NOTREACHED();
+    }
+  }
+  *original_content_length = orig;
+  *received_content_length = recv;
+  *last_update_time = last_update_internal;
+}
+
+// Used by generated jni code.

[mmenke, 2013/09/10 16:17:29]

I'm confused.  Code outside this file can't call a static global function.  How
does jni call this function?

[bengr, 2013/09/10 18:51:05]

The binding is included. jni/DataReductionProxySettingsAndroid_jni.h is
generated and included here. It adds:

static jint Init(JNIEnv* env, jobject obj);

and binds the java call "nativeInit" to reinterpret_cast<void*>(Init)

+static jint Init(JNIEnv* env, jobject obj) {
+  DataReductionProxySettingsAndroid* settings =
+      new DataReductionProxySettingsAndroid(env, obj);
+  return reinterpret_cast<jint>(settings);
+}
+
+// static
+bool DataReductionProxySettingsAndroid::Register(JNIEnv* env) {
+  bool register_natives_impl_result = RegisterNativesImpl(env);
+  return register_natives_impl_result;
+}