Added probe to determine if data reduction proxy can be used

chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h

+// 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.
+
+#ifndef CHROME_BROWSER_NET_SPDYPROXY_DATA_REDUCTION_PROXY_SETTINGS_ANDROID_H_
+#define CHROME_BROWSER_NET_SPDYPROXY_DATA_REDUCTION_PROXY_SETTINGS_ANDROID_H_
+
+#include "base/android/jni_helper.h"
+#include "base/android/jni_string.h"
+#include "base/android/scoped_java_ref.h"
+#include "base/basictypes.h"

[mmenke, 2013/09/11 16:08:59]

need "base/compiler_specific.h" as well, for OVERRIDE.

[mmenke, 2013/09/11 16:08:59]

Need the scoped_ptr header, too (base/memory/scoped_ptr.h?)

[bengr, 2013/09/11 22:01:02]

Done.

[bengr, 2013/09/11 22:01:02]

Done.

+#include "base/gtest_prod_util.h"
+#include "base/prefs/pref_service.h"

[mmenke, 2013/09/11 16:08:59]

Think this can be forward declared.

[bengr, 2013/09/11 22:01:02]

Done.

+#include "net/base/network_change_notifier.h"
+#include "net/url_request/url_fetcher.h"

[mmenke, 2013/09/11 16:08:59]

We can forward declare this.

[bengr, 2013/09/11 22:01:02]

Done.

+#include "net/url_request/url_fetcher_delegate.h"
+
+using base::android::ScopedJavaLocalRef;
+
+namespace spdyproxy {
+
+// The number of days of bandwidth usage statistics that are tracked.
+const unsigned int kNumDaysInHistory = 60;
+
+// The number of days of bandwidth usage statistics that are presented.
+const unsigned int kNumDaysInHistorySummary = 30;
+
+COMPILE_ASSERT(kNumDaysInHistorySummary <= kNumDaysInHistory,
+               DataReductionProxySettings_summary_too_long);
+
+}  // namespace spdyproxy
+
+// Central point for configuring the data reduction proxy on Android.
+// This object lives on the UI thread and all of its methods are expected to
+// be called from there.
+class DataReductionProxySettingsAndroid
+    : public net::URLFetcherDelegate,
+      public net::NetworkChangeNotifier::IPAddressObserver {
+ public:
+  DataReductionProxySettingsAndroid(JNIEnv* env, jobject obj);
+  virtual ~DataReductionProxySettingsAndroid();
+
+  void InitDataReductionProxySettings(JNIEnv* env, jobject obj);
+
+  // Add a pattern for a Host or URL to bypass, respectively. Wildcards should

[mmenke, 2013/09/11 16:08:59]

Think "Add a pattern for a Host or URL to bypass" is confusing.  Maybe "Add a
host or URL pattern to bypass the proxy, respectively"?

[bengr, 2013/09/11 22:01:02]

Done.

+  // be compatible with the JavaScript function shExpMatch, which can be used
+  // in proxy PAC resolution. These function should be called before the proxy

[mmenke, 2013/09/11 16:08:59]

nit:  should -> must (Maybe I've been reading too many RFCs lately).

[bengr, 2013/09/11 22:01:02]

changed to "must only." I've read too many RFCs in my time. :)

+  // is used.
+  void BypassHostPattern(JNIEnv* env, jobject obj, jstring pattern);
+  void BypassURLPattern(JNIEnv* env, jobject obj, jstring pattern);
+
+  // Returns true if the data reduction proxy is available for use on this
+  // instance of Chrome. This could return false, for example, if this instance
+  // is not part of the field trial, or if the proxy name is not configured
+  // via gyp.
+  jboolean IsDataReductionProxyAvailable(JNIEnv* env, jobject obj);

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

How difficult is it to test these in C++ unit tests?  Or in Java unit tests?

[mmenke, 2013/09/11 16:08:59]

If it's difficult, could make them all lightweight wrappers around C++
functions, and just test the inner C++ functions.  Really think we should have
pretty thorough testing of all the logic, and both the class and testing code
should be simple enough that it's not too difficult to do so.

[bengr, 2013/09/11 22:01:02]

I added a bunch of tests. Let me know if this is sufficient.

[bengr, 2013/09/11 22:01:02]

It's not too bad (see my updated patch).

+
+  // Returns true if a screen promoting the data reduction proxy is available
+  // for use. Logic that decides when to show the promo should check its
+  // availability. This would return false if not part of a separate field
+  // trial that governs the use of the promotion.
+  jboolean IsDataReductionProxyPromoAvailable(JNIEnv* env, jobject obj);
+
+  // Returns the origin of the data reduction proxy.
+  ScopedJavaLocalRef<jstring> GetDataReductionProxyOrigin(JNIEnv* env,
+                                                          jobject obj);
+
+  // Returns a configuration string for the proxy.
+  ScopedJavaLocalRef<jstring> GetDataReductionProxyAuth(JNIEnv* env,
+                                                        jobject obj);
+
+  // Returns true if the proxy is enabled.
+  jboolean IsDataReductionProxyEnabled(JNIEnv* env, jobject obj);
+
+  // Returns true if the proxy is managed by an adminstrator's policy.
+  jboolean IsDataReductionProxyManaged(JNIEnv* env, jobject obj);
+
+  // Enables or disables the data reduction proxy.
+  void SetDataReductionProxyEnabled(JNIEnv* env, jobject obj, jboolean enabled);
+
+  // Returns the time in microseconds that the last update was made to the
+  // daily original and received content lengths.
+  jlong GetDataReductionLastUpdateTime(JNIEnv* env, jobject obj);
+
+  // Return a Java |ContentLengths| object containing the total number of bytes
+  // of all received content, before and after compression by the data
+  // reduction proxy.
+  base::android::ScopedJavaLocalRef<jobject> GetContentLengths(JNIEnv* env,
+                                                               jobject obj);
+
+  // Returns an array containing the total size of all HTTP content that was
+  // received over the last |kNumDaysInHistory| before any compression by the
+  // data reduction proxy. Each element in the array contains one day of data.
+  ScopedJavaLocalRef<jlongArray> GetDailyOriginalContentLengths(JNIEnv* env,
+                                                                jobject obj);
+
+  // Returning an array containing the aggregate received HTTP content in
+  // the last |kNumDaysInHistory| days
+  ScopedJavaLocalRef<jlongArray> GetDailyReceivedContentLengths(JNIEnv* env,
+                                                                jobject obj);
+
+  // Registers the native methods to be call from Java.
+  static bool Register(JNIEnv* env);
+
+ private:
+  friend class DataReductionProxySettingsAndroidTest;
+  FRIEND_TEST_ALL_PREFIXES(DataReductionProxySettingsAndroidTest,
+                           TestBypassRules);
+  FRIEND_TEST_ALL_PREFIXES(DataReductionProxySettingsAndroidTest,
+                           TestFormatURL);

[mmenke, 2013/09/11 16:08:59]

There's no such test.

My general feeling is that FRIEND_TEST_ALL_PREFIXES is kinda ugly both because
of this, and because you have to list all the tests in production code.  My own
preference is to just friend the test fixture, and give it access to what it
needs.  Or could use protected functions, and use a subclass in the test to make
things public, though that's much less common.  If you prefer this approach (Or
just think it's simpler to stick with it) then this is completely fine, just
throwing out some suggestions.

[bengr, 2013/09/11 22:01:02]

I agree, but will leave as is because it's simpler.

+  FRIEND_TEST_ALL_PREFIXES(DataReductionProxySettingsAndroidTest,
+                           TestStatistics);
+
+  // net::URLFetcherDelegate:
+  virtual void OnURLFetchComplete(const net::URLFetcher* source) OVERRIDE;
+
+
+  // NetworkChangeNotifier::IPAddressObserver:
+  virtual void OnIPAddressChanged() OVERRIDE;
+
+  void AddDefaultProxyBypassRules();
+  void AddURLPatternToBypass(const std::string& pattern);
+  void AddHostPatternToBypass(const std::string& pattern);
+  void AddPatternToBypass(const std::string& url_or_host,
+                          const std::string& pattern);
+  void AddHostToBypass(const std::string& host);
+
+  PrefService* OriginalProfilePrefs();
+  PrefService* GetLocalStatePrefs();
+  void set_profile_prefs_for_testing(PrefService* pref_service);
+  void set_local_state_prefs_for_testing(PrefService* local_state);
+
+  std::string GetDataReductionProxyOrigin();
+  std::string GetDataReductionProxyOriginHostPort();
+  void ResetDataReductionStatistics();
+
+  void SetProxyPac(bool enable_spdy_proxy, bool at_startup);
+
+  ScopedJavaLocalRef<jlongArray> GetDailyContentLengths(JNIEnv* env,
+                                                        const char* pref_name);
+  void GetContentLengths(unsigned int days,
+                         int64* original_content_length,
+                         int64* received_content_length,
+                         int64* last_update_time);

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

I think the simplest thing to do is to just to add an "Internal" (or,
alternatively, "Impl") to the end of private functions that share the name of
their public counterparts.

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

nit:  Think some more of these can probably be const (GetContentLengths,
GetDailyContentLengths, and their Java equivalents).  If the prefservice stuff
makes it a pain, don't bother with it.

[bengr, 2013/09/11 22:01:02]

Done.

[bengr, 2013/09/11 22:01:02]

Sorry, which can be const?

+
+  void CheckDataReductionProxyIsAvailable(const std::string& url);

[mmenke, 2013/09/11 16:08:59]

nit:  I don't think this function name screams "Doing an async URL request",
particularly since "IsProxyAvailable" just checks the command line flags.

One suggestion is ProbeWhetherDataReductionProxyIsAvailable...Feel free to come
up with something better.

[bengr, 2013/09/11 22:01:02]

Done.

+
+  std::string GetProxyPacScript();
+
+  std::vector<std::string> bypass_rules_;
+
+  // Indicate whether a user has turned on the data reduction proxy previously
+  // in this session.
+  bool has_turned_on_;
+
+  // Indicate whether a user has turned off the data reduction proxy previously
+  // in this session.
+  bool has_turned_off_;
+
+  bool disabled_by_carrier_;
+  bool enabled_by_user_;
+
+  scoped_ptr<net::URLFetcher> fetcher_;
+  PrefService* profile_prefs_for_testing_;
+  PrefService* local_state_prefs_for_testing_;
+
+  DISALLOW_COPY_AND_ASSIGN(DataReductionProxySettingsAndroid);
+};
+
+#endif  // CHROME_BROWSER_NET_SPDYPROXY_DATA_REDUCTION_PROXY_SETTINGS_ANDROID_H_