Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23458016:
Added probe to determine if data reduction proxy can be used (Closed)
|
Created:
7 years, 3 months ago by bengr Modified:
7 years, 2 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, marq (ping after 24h) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdded probe to determine if data reduction proxy can be used
Chrome will now probe the URL /connect, which is hosted by the
data reduction proxy, before using the proxy. This CL also
upstreams some of the settings logic for the proxy.
BUG=270958
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=227410
Patch Set 1 #
Total comments: 24
Patch Set 2 : Addressed comments and added tests #Patch Set 3 : Added initializations #
Total comments: 2
Patch Set 4 : Added policy management #Patch Set 5 : Added OWNERS #
Total comments: 89
Patch Set 6 : Addressed comments from mmenke #
Total comments: 50
Patch Set 7 : Addressed additional comments #
Total comments: 35
Patch Set 8 : Added more unit tests #
Total comments: 46
Patch Set 9 : Addressed changes #
Total comments: 67
Patch Set 10 : Another round of comments addressed #
Total comments: 8
Patch Set 11 : Removed trials from tests #Patch Set 12 : Addressed comments from mef #
Total comments: 26
Patch Set 13 : Addressed comments from mef #
Total comments: 32
Patch Set 14 : Addressed comments #
Total comments: 14
Patch Set 15 : Renamed java and addressed other comments #Patch Set 16 : Fixed test #Messages
Total messages: 48 (0 generated)
Regarding tests: Maybe you could add some helper accessor-type methods (GetXXXForTest) available to Java, and write all these tests in Java-land so you ensure that it all works through JNI? They could then be run in the chromium testshell. https://codereview.chromium.org/23458016/diff/1/chrome/android/java/src/org/c... File chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettingsAndroid.java (right): https://codereview.chromium.org/23458016/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettingsAndroid.java:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. Nit: Nowadays, there should be no (c), but the year stays. https://codereview.chromium.org/23458016/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettingsAndroid.java:21: public ContentLengths(long original, long received) { private https://codereview.chromium.org/23458016/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettingsAndroid.java:35: private static DataReductionProxySettingsAndroid sConfig; sSettings? At least something that is part of the class name, or you could of course use something generic like sInstance. https://codereview.chromium.org/23458016/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettingsAndroid.java:122: public long[] getOriginalNetworkStatsHistory(int days) { |days| is unused. something that will be used soon? https://codereview.chromium.org/23458016/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettingsAndroid.java:132: public long[] getReceivedNetworkStatsHistory(int days) { |days| is unused. https://codereview.chromium.org/23458016/diff/1/chrome/browser/net/spdyproxy/... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/23458016/diff/1/chrome/browser/net/spdyproxy/... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:150: // BUG(169959): For some reason DataReductionProxy is breaking the omnibox TODO(bengr): ...blah... See: http://crbug.com/169959 https://codereview.chromium.org/23458016/diff/1/chrome/browser/net/spdyproxy/... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:161: AddHostPatternToBypass("*freezone.google.com"); Seems like this pattern might match too much. Could this be expressed as two rules? Something like: AddHostPatternToBypass("freezone.google.com"); AddHostPatternToBypass("*.freezone.google.com"); https://codereview.chromium.org/23458016/diff/1/chrome/browser/net/spdyproxy/... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:305: LOG(WARNING) << "SPDY proxy OFF at startup."; DLOG? https://codereview.chromium.org/23458016/diff/1/chrome/browser/net/spdyproxy/... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:347: // Keys duplicated from proxy_config_dictionary.cc Add TODO about moving them to proxy_config_dictionary.h and reuse them here, now that it all lives upstream? https://codereview.chromium.org/23458016/diff/1/chrome/browser/net/spdyproxy/... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h (right): https://codereview.chromium.org/23458016/diff/1/chrome/browser/net/spdyproxy/... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:47: void CheckDataReductionProxy(JNIEnv* env, jobject obj); Could you document what this method does? Or give it a better name that explains what it does. https://codereview.chromium.org/23458016/diff/1/chrome/browser/net/spdyproxy/... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:52: FRIEND_TEST_ALL_PREFIXES(DataReductionProxySettingsAndroidTest, Is this needed? https://codereview.chromium.org/23458016/diff/1/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/23458016/diff/1/chrome/chrome_browser.gypi#ne... chrome/chrome_browser.gypi:3459: 'android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettingsAndroid.java', Nit: alphabetical ordering
https://codereview.chromium.org/23458016/diff/1/chrome/android/java/src/org/c... File chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettingsAndroid.java (right): https://codereview.chromium.org/23458016/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettingsAndroid.java:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/09/04 02:20:15, nyquist wrote: > Nit: Nowadays, there should be no (c), but the year stays. Done. https://codereview.chromium.org/23458016/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettingsAndroid.java:21: public ContentLengths(long original, long received) { On 2013/09/04 02:20:15, nyquist wrote: > private Done. https://codereview.chromium.org/23458016/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettingsAndroid.java:35: private static DataReductionProxySettingsAndroid sConfig; On 2013/09/04 02:20:15, nyquist wrote: > sSettings? At least something that is part of the class name, or you could of > course use something generic like sInstance. Done. https://codereview.chromium.org/23458016/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettingsAndroid.java:122: public long[] getOriginalNetworkStatsHistory(int days) { On 2013/09/04 02:20:15, nyquist wrote: > |days| is unused. something that will be used soon? Done. https://codereview.chromium.org/23458016/diff/1/chrome/android/java/src/org/c... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettingsAndroid.java:132: public long[] getReceivedNetworkStatsHistory(int days) { On 2013/09/04 02:20:15, nyquist wrote: > |days| is unused. Done. https://codereview.chromium.org/23458016/diff/1/chrome/browser/net/spdyproxy/... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/23458016/diff/1/chrome/browser/net/spdyproxy/... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:150: // BUG(169959): For some reason DataReductionProxy is breaking the omnibox On 2013/09/04 02:20:15, nyquist wrote: > TODO(bengr): ...blah... See: http://crbug.com/169959 Done. https://codereview.chromium.org/23458016/diff/1/chrome/browser/net/spdyproxy/... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:161: AddHostPatternToBypass("*freezone.google.com"); I've added a TODO. On 2013/09/04 02:20:15, nyquist wrote: > Seems like this pattern might match too much. Could this be expressed as two > rules? Something like: > > AddHostPatternToBypass("freezone.google.com"); > AddHostPatternToBypass("*.freezone.google.com"); https://codereview.chromium.org/23458016/diff/1/chrome/browser/net/spdyproxy/... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:305: LOG(WARNING) << "SPDY proxy OFF at startup."; We need this to be a WARNING so that it is included with user feedback. On 2013/09/04 02:20:15, nyquist wrote: > DLOG? https://codereview.chromium.org/23458016/diff/1/chrome/browser/net/spdyproxy/... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:347: // Keys duplicated from proxy_config_dictionary.cc On 2013/09/04 02:20:15, nyquist wrote: > Add TODO about moving them to proxy_config_dictionary.h and reuse them here, now > that it all lives upstream? Done. https://codereview.chromium.org/23458016/diff/1/chrome/browser/net/spdyproxy/... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h (right): https://codereview.chromium.org/23458016/diff/1/chrome/browser/net/spdyproxy/... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:47: void CheckDataReductionProxy(JNIEnv* env, jobject obj); On 2013/09/04 02:20:15, nyquist wrote: > Could you document what this method does? Or give it a better name that explains > what it does. Done. https://codereview.chromium.org/23458016/diff/1/chrome/browser/net/spdyproxy/... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:52: FRIEND_TEST_ALL_PREFIXES(DataReductionProxySettingsAndroidTest, Now it is. On 2013/09/04 02:20:15, nyquist wrote: > Is this needed? https://codereview.chromium.org/23458016/diff/1/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/23458016/diff/1/chrome/chrome_browser.gypi#ne... chrome/chrome_browser.gypi:3459: 'android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettingsAndroid.java', On 2013/09/04 02:20:15, nyquist wrote: > Nit: alphabetical ordering Done.
lgtm https://codereview.chromium.org/23458016/diff/12001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc (right): https://codereview.chromium.org/23458016/diff/12001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:33: base::Time::Now().LocalMidnight() -base::TimeDelta::FromDays(60); Nit; space after -
mmenke: PTAL at chrome/browser/net/* https://codereview.chromium.org/23458016/diff/12001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc (right): https://codereview.chromium.org/23458016/diff/12001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:33: base::Time::Now().LocalMidnight() -base::TimeDelta::FromDays(60); On 2013/09/05 14:53:44, nyquist wrote: > Nit; space after - Done.
A couple quick comments (Wanted to take a break from the whole intern evaluation thing, still working on it) https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:43: // The c++ definition of enum SpdyProxyAuthState defined in nit: Capitalize C++? https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:44: // tools/histograms/histograms.xml. Think it's worth mentioning that new values should be added at the end. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:55: const char kSpdyProxyValueSwitch[] = "spdy-proxy-auth-value"; Switches should be defined in src/chrome/common/chrome_switches.h https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:63: JNIEnv* env, jobject obj) : The ":" should go in the next line, and all following lines should be indented by two more. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:86: DataReductionProxySettingsAndroid::GetDataReductionProxyOrigin( Confused about why this is different from GetDataReductionProxyOriginHostPort. Here we check the switch, there we check the pref...Why? https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:93: switches::kSpdyProxyAuthOrigin)); Think this indentation violates the style guide - all arguments should be lined up ("If the arguments do not all fit on one line, they should be broken up onto multiple lines, with each subsequent line aligned with the first argument. Do not add spaces after the open paren or before the close paren:"). https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:112: command_line.GetSwitchValueASCII(kSpdyProxyValueSwitch)); Again, this seems to violate the style guide. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:146: AddHostToBypass("localhost"); This rule seems to be a subset of the next one. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:148: AddHostToBypass("127.0.0.1"); What about 192.168.*? 10.*? 172.16.0.0 - 172.31.255.255? https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:169: AddURLPatternToBypass("http://g.co/freezone*"); Why URLPattern here, but URLSubstring above? Having both methods seems unnecessary. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:225: std::string pac = "function FindProxyForURL(url, host) {" I'm not an expert, but do we really need a PAC? Seems like we could just make our own ProxyResolver for the purpose, and bypass Javascript completely. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h (right): https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:19: class DataReductionProxySettingsAndroid Class should be documented, as should most of these functions. Also, I'm assuming this lives on the UI thread? A lot of browser/net stuff lives on the IO, so this should be documented. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:80: void set_local_state(PrefService* local_state); If this is only intended for testing, the name should be set_local_state_for_testing() https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:117: PrefService* local_state_; I don't think it's at all clear what the difference between these two is.
A couple more comments, mostly minor nits. I'll wait for you to update the CL before continuing to review. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/android/ch... File chrome/browser/android/chrome_jni_registrar.cc (right): https://codereview.chromium.org/23458016/diff/26001/chrome/browser/android/ch... chrome/browser/android/chrome_jni_registrar.cc:96: DataReductionProxySettingsAndroid::Register }, nit: This file is using a 2-space indent in this case. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:57: const unsigned int kNumDaysInHistory = 60; I don't think it's a good idea to have the same constant in 3 different files (here, tests, and the delegate) https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:98: return ConvertUTF8ToJavaString(env, std::string()); Suggest replacing the #endif with an #else, to emphasize that this isn't run if SPDY_PROXY_AUTH_ORIGIN is defined (And the same goes for the next line as well) https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:132: jobject obj) { nit: Fix indent. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:136: GetContentLengths(kNumDaysInHistorySummary, &original_content_length, Wonder about this constant being defined in this file. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:209: "DataCompressionProxyRollout") == "Enabled"); I think this return statement could be formatted to be more readable. I believe everything up to and include the "==" could fit on one line. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:214: "DataCompressionProxyPromoVisibility") == "Enabled"); Again, think this could be formatted a bit better. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:352: nit: Remove blank line. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:353: // Disable the proxy is it's not meant to be available. This comment doesn't seem to be accurate. We don't do anything if it's "not meant" to be available (Also think "not meant" is a bit ambiguous) https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:354: if (!IsDataReductionProxyAvailable()) return; return should be on its own line. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:378: bool at_startup) { nit: Fix indent https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:378: bool at_startup) { nit: Fix indent. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:433: std::string url) { This should be a const std::string&, no? https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:437: net::LOAD_IGNORE_ALL_CERT_ERRORS); Why are we ignoring cert errors? https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:479: return gurl.scheme() + "://enabled." + gurl.host() + gurl.path() + "connect"; There's a class call GURL::Replacements, or something like that, for this purpose. I think it's a lot clearer to use it than a roll-your-own solution. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h (right): https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:37: void SetDataReductionProxyEnabled(JNIEnv* env, jobject obj,jboolean enabled); nit: Missing space. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:105: // session. I'm not really follow...This is SPDY proxy auth specifically for the data reduction policy, right, and not in general? https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:117: PrefService* local_state_; Also, if these are only used for unit tests, they should be renamed to *_for_testing_. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc (right): https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:15: protected: nit: "testing::Test implementation:" (Comment is often skipped in test fixtures, but think it's more consistent to include it)
Have you considered putting the code to update the relevant prefs in, say, chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc instead of in the chrome_network_delegate, and just calling in from the network delegate? May make things a little cleaner. Think it's well beyond the scope of this CL, if the idea appeals to you. May also be able to share some code to read through the ListValues. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:58: const unsigned int kNumDaysInHistorySummary = 30; Should have a compile assert that this is less than the number of days in the history. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:485: jlongArray result = env->NewLongArray(kNumDaysInHistory); nit: Fix indent. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:490: const ListValue* list_value = local_state->GetList(pref_name); This guaranteed to exist and be a list? https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:503: } Suggest just making the body of this loop a function, and using it here and in GetContentLengths... https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:526: int64* last_update_time) { Should DCHECK that days is no more than kDaysInHsitory. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:543: received_list->GetSize() != kNumDaysInHistory) { Are we guaranteed these will exists and be lists? https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:557: std::string result("0"); Initialization to 0 doesn't seem to serve any purpose here, since you check return values. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:561: orig +=val; nit: Add space. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:563: DCHECK(false); These should be NOTREACHED(). https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:569: recv +=val; nit: Add space. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:581: static jint Init(JNIEnv* env, jobject obj) { This does seem to be used anywere.
The grand plan is indeed to move the data reduction proxy-related stuff from ChromeNetworkDelegate to here. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/android/ch... File chrome/browser/android/chrome_jni_registrar.cc (right): https://codereview.chromium.org/23458016/diff/26001/chrome/browser/android/ch... chrome/browser/android/chrome_jni_registrar.cc:96: DataReductionProxySettingsAndroid::Register }, On 2013/09/09 15:40:05, mmenke wrote: > nit: This file is using a 2-space indent in this case. Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:43: // The c++ definition of enum SpdyProxyAuthState defined in On 2013/09/05 21:34:33, mmenke wrote: > nit: Capitalize C++? Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:44: // tools/histograms/histograms.xml. On 2013/09/05 21:34:33, mmenke wrote: > Think it's worth mentioning that new values should be added at the end. Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:55: const char kSpdyProxyValueSwitch[] = "spdy-proxy-auth-value"; On 2013/09/05 21:34:33, mmenke wrote: > Switches should be defined in src/chrome/common/chrome_switches.h Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:57: const unsigned int kNumDaysInHistory = 60; I fixed for here and the tests. I'll fix for the delegate if needed in a later CL once I move the data reduction proxy code from there to here. On 2013/09/09 15:40:05, mmenke wrote: > I don't think it's a good idea to have the same constant in 3 different files > (here, tests, and the delegate) https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:58: const unsigned int kNumDaysInHistorySummary = 30; On 2013/09/09 16:01:07, mmenke wrote: > Should have a compile assert that this is less than the number of days in the > history. Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:63: JNIEnv* env, jobject obj) : On 2013/09/05 21:34:33, mmenke wrote: > The ":" should go in the next line, and all following lines should be indented > by two more. Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:86: DataReductionProxySettingsAndroid::GetDataReductionProxyOrigin( It actually doesn't matter because the switch is mapped to the pref. I've changed GetDataReductionProxyOriginHostPort to refer to the switch for consistency. On 2013/09/05 21:34:33, mmenke wrote: > Confused about why this is different from GetDataReductionProxyOriginHostPort. > Here we check the switch, there we check the pref...Why? https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:93: switches::kSpdyProxyAuthOrigin)); On 2013/09/05 21:34:33, mmenke wrote: > Think this indentation violates the style guide - all arguments should be lined > up ("If the arguments do not all fit on one line, they should be broken up onto > multiple lines, with each subsequent line aligned with the first argument. Do > not add spaces after the open paren or before the close paren:"). Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:98: return ConvertUTF8ToJavaString(env, std::string()); On 2013/09/09 15:40:05, mmenke wrote: > Suggest replacing the #endif with an #else, to emphasize that this isn't run if > SPDY_PROXY_AUTH_ORIGIN is defined (And the same goes for the next line as well) Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:112: command_line.GetSwitchValueASCII(kSpdyProxyValueSwitch)); On 2013/09/05 21:34:33, mmenke wrote: > Again, this seems to violate the style guide. Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:132: jobject obj) { On 2013/09/09 15:40:05, mmenke wrote: > nit: Fix indent. Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:136: GetContentLengths(kNumDaysInHistorySummary, &original_content_length, On 2013/09/09 15:40:05, mmenke wrote: > Wonder about this constant being defined in this file. Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:146: AddHostToBypass("localhost"); On 2013/09/05 21:34:33, mmenke wrote: > This rule seems to be a subset of the next one. The next one uses shExpMatch underneath, which afaik doesn't use standard regular expressions. I.e., the pattern will match localhost., but not localhost Please correct me if I'm wrong. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:148: AddHostToBypass("127.0.0.1"); On 2013/09/05 21:34:33, mmenke wrote: > What about 192.168.*? 10.*? 172.16.0.0 - 172.31.255.255? I think we didn't bypass these because we were worried about the processing time of adding too many rules. I've added a TODO to revisit. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:169: AddURLPatternToBypass("http://g.co/freezone*"); On 2013/09/05 21:34:33, mmenke wrote: > Why URLPattern here, but URLSubstring above? Having both methods seems > unnecessary. Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:209: "DataCompressionProxyRollout") == "Enabled"); On 2013/09/09 15:40:05, mmenke wrote: > I think this return statement could be formatted to be more readable. I believe > everything up to and include the "==" could fit on one line. Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:214: "DataCompressionProxyPromoVisibility") == "Enabled"); On 2013/09/09 15:40:05, mmenke wrote: > Again, think this could be formatted a bit better. Actually, I'm at a loss for how to format this one any better. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:225: std::string pac = "function FindProxyForURL(url, host) {" This code is what we use currently in stable and so I'm a little hesitant to replace it without significant testing. I added a TODO to re-evaluate our options. On 2013/09/05 21:34:33, mmenke wrote: > I'm not an expert, but do we really need a PAC? Seems like we could just make > our own ProxyResolver for the purpose, and bypass Javascript completely. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:352: On 2013/09/09 15:40:05, mmenke wrote: > nit: Remove blank line. Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:353: // Disable the proxy is it's not meant to be available. On 2013/09/09 15:40:05, mmenke wrote: > This comment doesn't seem to be accurate. We don't do anything if it's "not > meant" to be available (Also think "not meant" is a bit ambiguous) Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:354: if (!IsDataReductionProxyAvailable()) return; On 2013/09/09 15:40:05, mmenke wrote: > return should be on its own line. Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:378: bool at_startup) { On 2013/09/09 15:40:05, mmenke wrote: > nit: Fix indent Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:378: bool at_startup) { On 2013/09/09 15:40:05, mmenke wrote: > nit: Fix indent Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:433: std::string url) { On 2013/09/09 15:40:05, mmenke wrote: > This should be a const std::string&, no? Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:437: net::LOAD_IGNORE_ALL_CERT_ERRORS); On 2013/09/09 15:40:05, mmenke wrote: > Why are we ignoring cert errors? Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:479: return gurl.scheme() + "://enabled." + gurl.host() + gurl.path() + "connect"; On 2013/09/09 15:40:05, mmenke wrote: > There's a class call GURL::Replacements, or something like that, for this > purpose. I think it's a lot clearer to use it than a roll-your-own solution. Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:485: jlongArray result = env->NewLongArray(kNumDaysInHistory); On 2013/09/09 16:01:07, mmenke wrote: > nit: Fix indent. Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:490: const ListValue* list_value = local_state->GetList(pref_name); On 2013/09/09 16:01:07, mmenke wrote: > This guaranteed to exist and be a list? Yes. We'll hit a NOTREACHED if the pref isn't registered or the type isn't base::Value::TYPE_LIST. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:503: } On 2013/09/09 16:01:07, mmenke wrote: > Suggest just making the body of this loop a function, and using it here and in > GetContentLengths... Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:526: int64* last_update_time) { On 2013/09/09 16:01:07, mmenke wrote: > Should DCHECK that days is no more than kDaysInHsitory. Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:543: received_list->GetSize() != kNumDaysInHistory) { On 2013/09/09 16:01:07, mmenke wrote: > Are we guaranteed these will exists and be lists? Yes. We'll hit a NOTREACHED if the pref isn't registered or the type isn't base::Value::TYPE_LIST. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:557: std::string result("0"); On 2013/09/09 16:01:07, mmenke wrote: > Initialization to 0 doesn't seem to serve any purpose here, since you check > return values. Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:561: orig +=val; On 2013/09/09 16:01:07, mmenke wrote: > nit: Add space. Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:563: DCHECK(false); On 2013/09/09 16:01:07, mmenke wrote: > These should be NOTREACHED(). Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:569: recv +=val; On 2013/09/09 16:01:07, mmenke wrote: > nit: Add space. Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:581: static jint Init(JNIEnv* env, jobject obj) { On 2013/09/09 16:01:07, mmenke wrote: > This does seem to be used anywere. This is used by generated jni code. I've added a comment. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h (right): https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:19: class DataReductionProxySettingsAndroid On 2013/09/05 21:34:33, mmenke wrote: > Class should be documented, as should most of these functions. > > Also, I'm assuming this lives on the UI thread? A lot of browser/net stuff > lives on the IO, so this should be documented. Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:37: void SetDataReductionProxyEnabled(JNIEnv* env, jobject obj,jboolean enabled); On 2013/09/09 15:40:05, mmenke wrote: > nit: Missing space. Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:80: void set_local_state(PrefService* local_state); On 2013/09/05 21:34:33, mmenke wrote: > If this is only intended for testing, the name should be > set_local_state_for_testing() Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:105: // session. On 2013/09/09 15:40:05, mmenke wrote: > I'm not really follow...This is SPDY proxy auth specifically for the data > reduction policy, right, and not in general? Done. The data reduction proxy had been called "SPDY proxy auth" for various reasons. I'm slowly updating the code to refer to it as the data reduction proxy. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:117: PrefService* local_state_; On 2013/09/05 21:34:33, mmenke wrote: > I don't think it's at all clear what the difference between these two is. Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:117: PrefService* local_state_; On 2013/09/09 15:40:05, mmenke wrote: > Also, if these are only used for unit tests, they should be renamed to > *_for_testing_. Done. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc (right): https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:15: protected: On 2013/09/09 15:40:05, mmenke wrote: > nit: "testing::Test implementation:" (Comment is often skipped in test > fixtures, but think it's more consistent to include it) Done.
https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:505: // int64 val = 0; Remove this now that you have extracted the method https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:513: jval[i] =GetInt64PrefValue(*list_value, i); nit: space after =
I'll continue looking at this after lunch. https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/23458016/diff/26001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:146: AddHostToBypass("localhost"); On 2013/09/10 00:56:09, bengr1 wrote: > On 2013/09/05 21:34:33, mmenke wrote: > > This rule seems to be a subset of the next one. > > The next one uses shExpMatch underneath, which afaik doesn't use standard > regular expressions. I.e., the pattern will match localhost., but not localhost > > Please correct me if I'm wrong. You're right. The fact that shExpMatch is written in JS, but doesn't use JS regexps confused me. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:96: DataReductionProxySettingsAndroid::GetDataReductionProxyOrigin( On 2013/09/10 00:56:09, bengr1 wrote: > It actually doesn't matter because the switch is mapped to the pref. I've > changed GetDataReductionProxyOriginHostPort to refer to the switch for > consistency. > > On 2013/09/05 21:34:33, mmenke wrote: > > Confused about why this is different from GetDataReductionProxyOriginHostPort. > > > Here we check the switch, there we check the pref...Why? > 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. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:154: // time. Those question marks don't seem to be needed. Maybe commas instead? https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:154: // time. 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. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:175: AddURLPatternToBypass("http://g.co/freezone*"); 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. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:210: "Enabled"); Optional: Think this is a little easier to read if indented 4 beyond the open paren. If you disagree, fine to leave as is. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:215: "DataCompressionProxyPromoVisibility") == "Enabled"); On 2013/09/10 00:56:09, bengr1 wrote: > On 2013/09/09 15:40:05, mmenke wrote: > > Again, think this could be formatted a bit better. > > Actually, I'm at a loss for how to format this one any better. > 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. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:216: } 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. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:244: GetOriginalProfile()->GetPrefs(); Why are we using the default profile, and not the active one? https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:274: void DataReductionProxySettingsAndroid::RemoveSchemeAndTrailingSlash( 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. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:308: if (!IsDataReductionProxyAvailable()) return; nit: return should be on next line. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:314: CommandLine& command_line = *CommandLine::ForCurrentProcess(); This should be "const CommandLine&". Or keep it as a pointer. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:457: // it, configure it to the user's desires. 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. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:470: std::string DataReductionProxySettingsAndroid::GetProxyCheckURL() { Can go in an anonymous namespace https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:472: return DATA REDUCTION_PROXY_PROBE_URL; BUG: Looks like you're missing an underscore after DATA. Did this even compile? https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:478: int64 DataReductionProxySettingsAndroid::GetInt64PrefValue( 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. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:567: if (original_list->GetString(read_index, &result)) { GetInt64PrefValue? https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:575: if (received_list->GetString(read_index, &result)) { GetInt64PrefValue? https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:589: // Used by generated jni code. I'm confused. Code outside this file can't call a static global function. How does jni call this function? https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h (right): https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:49: jboolean IsDataReductionProxyAvailable(JNIEnv* env, jobject obj); Style guide comes out pretty strongly against overloading, and I think this case is a bit confusing, too. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:62: ScopedJavaLocalRef<jstring> GetDataReductionProxyValue(JNIEnv* env, Suggest renaming this "GetDataReductionProxyAuthString" or "GetDataReductionProxyAuth". "Value" seems very ambiguous to me. Same goes for the switch. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:126: void set_pref_service(PrefService* pref_service); This should also be set_pref_service_for_testing. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:165: PrefService* local_state_prefs_for_testing_; I'm still confused by these two. One is for the profile, and one is for...The default profile, so also effectively also global state?
https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:96: DataReductionProxySettingsAndroid::GetDataReductionProxyOrigin( On 2013/09/10 16:17:29, mmenke wrote: > On 2013/09/10 00:56:09, bengr1 wrote: > > It actually doesn't matter because the switch is mapped to the pref. I've > > changed GetDataReductionProxyOriginHostPort to refer to the switch for > > consistency. > > > > On 2013/09/05 21:34:33, mmenke wrote: > > > Confused about why this is different from > GetDataReductionProxyOriginHostPort. > > > > > Here we check the switch, there we check the pref...Why? > > > > 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. The plan is to get rid of the pref, once we verify that iOS does not depend on it. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:154: // time. On 2013/09/10 16:17:29, mmenke wrote: > Those question marks don't seem to be needed. Maybe commas instead? Done. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:154: // time. On 2013/09/10 16:17:29, mmenke wrote: > 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. Thanks. I think iOS moved away from using a PAC, which is probably the right way to go. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:175: AddURLPatternToBypass("http://g.co/freezone*"); On 2013/09/10 16:17:29, mmenke wrote: > 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. Done. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:210: "Enabled"); On 2013/09/10 16:17:29, mmenke wrote: > Optional: Think this is a little easier to read if indented 4 beyond the open > paren. If you disagree, fine to leave as is. Done. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:215: "DataCompressionProxyPromoVisibility") == "Enabled"); On 2013/09/10 16:17:29, mmenke wrote: > On 2013/09/10 00:56:09, bengr1 wrote: > > On 2013/09/09 15:40:05, mmenke wrote: > > > Again, think this could be formatted a bit better. > > > > Actually, I'm at a loss for how to format this one any better. > > > > 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. Done. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:216: } On 2013/09/10 16:17:29, mmenke wrote: > 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. Done. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:244: GetOriginalProfile()->GetPrefs(); On 2013/09/10 16:17:29, mmenke wrote: > Why are we using the default profile, and not the active one? Android only has one non-incognito profile, and this is the convention for accessing it. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:274: void DataReductionProxySettingsAndroid::RemoveSchemeAndTrailingSlash( On 2013/09/10 16:17:29, mmenke wrote: > 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. Done. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:308: if (!IsDataReductionProxyAvailable()) return; On 2013/09/10 16:17:29, mmenke wrote: > nit: return should be on next line. Done. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:314: CommandLine& command_line = *CommandLine::ForCurrentProcess(); On 2013/09/10 16:17:29, mmenke wrote: > This should be "const CommandLine&". Or keep it as a pointer. Done. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:457: // it, configure it to the user's desires. On 2013/09/10 16:17:29, mmenke wrote: > 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. Done. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:470: std::string DataReductionProxySettingsAndroid::GetProxyCheckURL() { On 2013/09/10 16:17:29, mmenke wrote: > Can go in an anonymous namespace Done. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:472: return DATA REDUCTION_PROXY_PROBE_URL; On 2013/09/10 16:17:29, mmenke wrote: > BUG: Looks like you're missing an underscore after DATA. Did this even > compile? 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. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:478: int64 DataReductionProxySettingsAndroid::GetInt64PrefValue( On 2013/09/10 16:17:29, mmenke wrote: > 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. Done. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:505: // int64 val = 0; On 2013/09/10 02:24:02, nyquist wrote: > Remove this now that you have extracted the method Done. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:513: jval[i] =GetInt64PrefValue(*list_value, i); On 2013/09/10 02:24:02, nyquist wrote: > nit: space after = Done. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:567: if (original_list->GetString(read_index, &result)) { On 2013/09/10 16:17:29, mmenke wrote: > GetInt64PrefValue? Done. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:575: if (received_list->GetString(read_index, &result)) { On 2013/09/10 16:17:29, mmenke wrote: > GetInt64PrefValue? Done. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:589: // Used by generated jni code. On 2013/09/10 16:17:29, mmenke wrote: > I'm confused. Code outside this file can't call a static global function. How > does jni call this function? 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) https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h (right): https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:49: jboolean IsDataReductionProxyAvailable(JNIEnv* env, jobject obj); You mean the Google C++ guide? I didn't see anything about overloading in the Chromium style guide. Regardless, I agreee it should be avoided. I'm at a loss for names. PTAL. On 2013/09/10 16:17:29, mmenke wrote: > Style guide comes out pretty strongly against overloading, and I think this case > is a bit confusing, too. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:62: ScopedJavaLocalRef<jstring> GetDataReductionProxyValue(JNIEnv* env, On 2013/09/10 16:17:29, mmenke wrote: > Suggest renaming this "GetDataReductionProxyAuthString" or > "GetDataReductionProxyAuth". "Value" seems very ambiguous to me. Same goes for > the switch. Done. I'm leaving the switch as is b/c iOS relies on it. We plan to write another CL that unifies a lot of the Android and iOS code, and when we do that, we'll rename the switch. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:126: void set_pref_service(PrefService* pref_service); On 2013/09/10 16:17:29, mmenke wrote: > This should also be set_pref_service_for_testing. Done. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:165: PrefService* local_state_prefs_for_testing_; On 2013/09/10 16:17:29, mmenke wrote: > I'm still confused by these two. One is for the profile, and one is for...The > default profile, so also effectively also global state? One of these is for the profile. The other is for profile-agnostic prefs, which are stored in "Local State."
Some more quick comments. I'll get you some more tomorrow morning or later tonight. Think we're really making progress. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:96: DataReductionProxySettingsAndroid::GetDataReductionProxyOrigin( On 2013/09/10 18:51:05, bengr1 wrote: > On 2013/09/10 16:17:29, mmenke wrote: > > On 2013/09/10 00:56:09, bengr1 wrote: > > > It actually doesn't matter because the switch is mapped to the pref. I've > > > changed GetDataReductionProxyOriginHostPort to refer to the switch for > > > consistency. > > > > > > On 2013/09/05 21:34:33, mmenke wrote: > > > > Confused about why this is different from > > GetDataReductionProxyOriginHostPort. > > > > > > > Here we check the switch, there we check the pref...Why? > > > > > > > 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. > > The plan is to get rid of the pref, once we verify that iOS does not depend on > it. > Ok, then using the command line switch seems reasonable to me. https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h (right): https://codereview.chromium.org/23458016/diff/37001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:49: jboolean IsDataReductionProxyAvailable(JNIEnv* env, jobject obj); Yes, the Google C++ style guide On 2013/09/10 18:51:05, bengr1 wrote: > You mean the Google C++ guide? I didn't see anything about overloading in the > Chromium style guide. Regardless, I agreee it should be avoided. I'm at a loss > for names. PTAL. > > On 2013/09/10 16:17:29, mmenke wrote: > > Style guide comes out pretty strongly against overloading, and I think this > case > > is a bit confusing, too. > https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:461: SetProxyPac(true, false); nit: Use braces on multi-line conditional bodies. https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:469: SetProxyPac(false, false); nit: Braces on multi-line conditional bodies. https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h (right): https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:56: jboolean IsDataReductionProxyAvailable(JNIEnv* env, jobject obj); How difficult is it to test these in C++ unit tests? Or in Java unit tests? https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:144: int64* last_update_time); 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. https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:144: int64* last_update_time); 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. https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc (right): https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:35: spdyproxy::kNumDaysInHistory); optional nit: Think this may be clearer (I really like having functions on the same line as arguments, when possible): last_update_time_ = base::Time::Now().LocalMidnight() - base::TimeDelta::FromDays(spdyproxy::kNumDaysInHistory); https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:62: DCHECK_EQ(expected[i++], *it); With multi-line for statements, should use braces. https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:62: DCHECK_EQ(expected[i++], *it); This should be EXPECT_EQ, not DCHECK_EQ. Or ASSERT_EQ if you really want the early exit. (Also, in general, DCHECKs as single-line bodies without braces seems a bad idea to me. One could image "#ifndef DEBUG / #define DCHECK() / #endif". Not what we do, but still seems like a bad idea to me).
https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h (right): https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:11: #include "base/basictypes.h" need "base/compiler_specific.h" as well, for OVERRIDE. https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:11: #include "base/basictypes.h" Need the scoped_ptr header, too (base/memory/scoped_ptr.h?) https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:13: #include "base/prefs/pref_service.h" Think this can be forward declared. https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:15: #include "net/url_request/url_fetcher.h" We can forward declare this. https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:45: // Add a pattern for a Host or URL to bypass, respectively. Wildcards should 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"? https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:47: // in proxy PAC resolution. These function should be called before the proxy nit: should -> must (Maybe I've been reading too many RFCs lately). https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:56: jboolean IsDataReductionProxyAvailable(JNIEnv* env, jobject obj); On 2013/09/10 19:54:06, mmenke wrote: > How difficult is it to test these in C++ unit tests? Or in Java unit tests? 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. https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:110: TestFormatURL); 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. https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:146: void CheckDataReductionProxyIsAvailable(const std::string& url); 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.
https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:461: SetProxyPac(true, false); On 2013/09/10 19:54:06, mmenke wrote: > nit: Use braces on multi-line conditional bodies. Done. https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:469: SetProxyPac(false, false); On 2013/09/10 19:54:06, mmenke wrote: > nit: Braces on multi-line conditional bodies. Done. https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h (right): https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:11: #include "base/basictypes.h" On 2013/09/11 16:08:59, mmenke wrote: > need "base/compiler_specific.h" as well, for OVERRIDE. Done. https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:11: #include "base/basictypes.h" On 2013/09/11 16:08:59, mmenke wrote: > Need the scoped_ptr header, too (base/memory/scoped_ptr.h?) Done. https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:13: #include "base/prefs/pref_service.h" On 2013/09/11 16:08:59, mmenke wrote: > Think this can be forward declared. Done. https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:15: #include "net/url_request/url_fetcher.h" On 2013/09/11 16:08:59, mmenke wrote: > We can forward declare this. Done. https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:45: // Add a pattern for a Host or URL to bypass, respectively. Wildcards should On 2013/09/11 16:08:59, mmenke wrote: > 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"? Done. https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:47: // in proxy PAC resolution. These function should be called before the proxy On 2013/09/11 16:08:59, mmenke wrote: > nit: should -> must (Maybe I've been reading too many RFCs lately). changed to "must only." I've read too many RFCs in my time. :) https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:56: jboolean IsDataReductionProxyAvailable(JNIEnv* env, jobject obj); On 2013/09/11 16:08:59, mmenke wrote: > On 2013/09/10 19:54:06, mmenke wrote: > > How difficult is it to test these in C++ unit tests? Or in Java unit tests? > > 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. I added a bunch of tests. Let me know if this is sufficient. https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:56: jboolean IsDataReductionProxyAvailable(JNIEnv* env, jobject obj); On 2013/09/10 19:54:06, mmenke wrote: > How difficult is it to test these in C++ unit tests? Or in Java unit tests? It's not too bad (see my updated patch). https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:110: TestFormatURL); On 2013/09/11 16:08:59, mmenke wrote: > 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. I agree, but will leave as is because it's simpler. https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:144: int64* last_update_time); On 2013/09/10 19:54:06, mmenke wrote: > 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. Done. https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:144: int64* last_update_time); On 2013/09/10 19:54:06, mmenke wrote: > 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. Sorry, which can be const? https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:146: void CheckDataReductionProxyIsAvailable(const std::string& url); On 2013/09/11 16:08:59, mmenke wrote: > 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. Done. https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc (right): https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:35: spdyproxy::kNumDaysInHistory); On 2013/09/10 19:54:06, mmenke wrote: > optional nit: Think this may be clearer (I really like having functions on the > same line as arguments, when possible): > > last_update_time_ = > base::Time::Now().LocalMidnight() - > base::TimeDelta::FromDays(spdyproxy::kNumDaysInHistory); Done. https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:62: DCHECK_EQ(expected[i++], *it); On 2013/09/10 19:54:06, mmenke wrote: > With multi-line for statements, should use braces. Done. https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:62: DCHECK_EQ(expected[i++], *it); On 2013/09/10 19:54:06, mmenke wrote: > This should be EXPECT_EQ, not DCHECK_EQ. Or ASSERT_EQ if you really want the > early exit. (Also, in general, DCHECKs as single-line bodies without braces > seems a bad idea to me. One could image "#ifndef DEBUG / #define DCHECK() / > #endif". Not what we do, but still seems like a bad idea to me). Got it. Done.
https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:57: bool IsProxyAvailable() { If a proxy is specified via the command line, shouldn't we be ignoring the field trial? Also, if it's enabled by the command line, shouldn't we be ignoring the field trial? https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:82: return DATA_REDUCTION_PROXY_PROBE_URL; Should be returning an empty string in the case that a proxy URL is specified by the command line, I assume? https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:124: DataReductionProxySettingsAndroid::GetDataReductionProxyOriginInternal() { nit: Definition order should match declaration order. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:153: if (command_line.HasSwitch(switches::kSpdyProxyAuthValue)) { What if a proxy origin is set via the command line, but not an auth value? We'd end up using the built-in auth value with an untrusted, third-party server. I assume the underlying protocol salts it in some way, and one can always just steal it from the binary, but still seems a bad idea to me. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:194: AddHostToBypass("127.0.0.1"); "::1" for IPv6 support? (Or should it be [::1]? - not sure if we get the IPv6 address or host part of the URL in PACs) https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:330: LOG(WARNING) << "SPDY proxy OFF at startup."; Do we really need the log statement? Fine with a DLOG. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:354: ProbeWhetherDataReductionProxyIsAvailable(GetProxyCheckURL()); While this works, it seems weird to me that this is first - the callback it triggers on complete depends on everything after this line being already done, so cognitively, seems like this should be at the end of this function. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:354: ProbeWhetherDataReductionProxyIsAvailable(GetProxyCheckURL()); Should we be doing the probe if the proxy is disabled? This seems like a privacy concern, particularly since we send cookies with the request. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:358: prefs->SetBoolean(prefs::kSpdyProxyAuthEnabled, enabled); This seems a little weird to me. Is there any reason we can't just monitor the pref, rather than managing it? This also seems to have the potential for bugs - managed prefs override user prefs (I assume), so if we somehow manage to have a call to SetDataReductionProxyEnabled when the managed pref is false, things break. Monitoring the pref instead avoids the issue. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:370: SetProxyPac(enabled_by_user_, !env); To avoid weird duplicate enabled cases, you should set disabled_by_carrier_ to false here. Or perhaps better, call "SetProxyPac(enabled_by_user_ && !disabled_by_carrier_, ...)" https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:387: LOG(WARNING) << "SPDY proxy ON " << (at_startup ? kAtStartup : kByUser); Not a big fan of LOGs compiled into production code. Do we need them? https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:430: const std::string& url) { Do we really need to take the URL as a parameter? Seems like we could just call GetProxyCheckURL(). https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h (right): https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:62: jboolean IsDataReductionProxyAvailable(JNIEnv* env, jobject obj); "Allowed" may be clearer than "Available" ("Available" sounds more like the probe passed). This goes for all "Available" functions, though the promo ones are debatable. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:84: // Enables or disables the data reduction proxy. Think it's worth adding something along the lines of "If a probe URL is available, and a probe request fails at some point, the proxy won't be used until a probe succeed." Or alternatively, maybe adding something about the probes to the class description. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:160: void ProbeWhetherDataReductionProxyIsAvailable(const std::string& url); Think it's worth a description...Along the lines of "Requests the proxy probe URL, if one is set. If unable to do so, disables the proxy, if enabled. Otherwise enables the proxy, if disabled". https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc (right): https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:26: nit: Blank line not needed. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:56: switches::kSpdyProxyAuthValue, "12345"); Suggest making both of these strings consts char[]s, and putting them at the top of the file. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:84: } This isn't used, and doesn't do anything. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:89: scoped_ptr<base::FieldTrialList> field_trial_list_; Think it's worth mentioning somewhere that this is a singleton that will clear all set field trials on destruction. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:92: TEST_F(DataReductionProxySettingsAndroidTest, TestGetDataReductionProxyOrigin) { Suggest also testing setting this via the command line (Same goes for the auth test). https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:134: EXPECT_EQ("foo:443", settings_->GetDataReductionProxyOriginHostPort()); Again, think we want to test with a value set by the command line. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:138: TestResetDataReductionStatistics) { Think this should go just after TestContentLengthsInternal. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:221: } I'd really like to see some some pretty extensive testing of SetDataReductionProxyEnabled / ProbeWhetherDataReductionProxyIsAvailable / OnURLFetchComplete, both in the command line case and in the non-command-line-base. There are mock URL fetcher classes for tests that shouldn't make it too bad. I'd like to see all "transitions" via probes (enabled -> enabled, enabled -> disabled, disabled -> disabled, disabled -> enabled), and maybe some playing with enabled state as well. To make sure it's disabled, you can check the prefs::kProxy pref. Some enabling/disabling via SetDataReductionProxyEnabled in various probe states would be nice.
https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:57: bool IsProxyAvailable() { On 2013/09/12 16:30:41, mmenke wrote: > If a proxy is specified via the command line, shouldn't we be ignoring the field > trial? Also, if it's enabled by the command line, shouldn't we be ignoring the > field trial? Done. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:82: return DATA_REDUCTION_PROXY_PROBE_URL; On 2013/09/12 16:30:41, mmenke wrote: > Should be returning an empty string in the case that a proxy URL is specified by > the command line, I assume? We didn't have a switch for this, but I've added one. Done. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:124: DataReductionProxySettingsAndroid::GetDataReductionProxyOriginInternal() { On 2013/09/12 16:30:41, mmenke wrote: > nit: Definition order should match declaration order. Done. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:153: if (command_line.HasSwitch(switches::kSpdyProxyAuthValue)) { On 2013/09/12 16:30:41, mmenke wrote: > What if a proxy origin is set via the command line, but not an auth value? We'd > end up using the built-in auth value with an untrusted, third-party server. I > assume the underlying protocol salts it in some way, and one can always just > steal it from the binary, but still seems a bad idea to me. Done. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:194: AddHostToBypass("127.0.0.1"); On 2013/09/12 16:30:41, mmenke wrote: > "::1" for IPv6 support? (Or should it be [::1]? - not sure if we get the IPv6 > address or host part of the URL in PACs) I'll add [::1]. Do you know how to add (and see) log statements from a PAC? alert() seems to execute, but I can't get the output to a console. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:330: LOG(WARNING) << "SPDY proxy OFF at startup."; On 2013/09/12 16:30:41, mmenke wrote: > Do we really need the log statement? Fine with a DLOG. Unfortunately, yes. See my reply below. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:354: ProbeWhetherDataReductionProxyIsAvailable(GetProxyCheckURL()); On 2013/09/12 16:30:41, mmenke wrote: > Should we be doing the probe if the proxy is disabled? This seems like a > privacy concern, particularly since we send cookies with the request. Done. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:354: ProbeWhetherDataReductionProxyIsAvailable(GetProxyCheckURL()); On 2013/09/12 16:30:41, mmenke wrote: > While this works, it seems weird to me that this is first - the callback it > triggers on complete depends on everything after this line being already done, > so cognitively, seems like this should be at the end of this function. Done. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:358: prefs->SetBoolean(prefs::kSpdyProxyAuthEnabled, enabled); On 2013/09/12 16:30:41, mmenke wrote: > This seems a little weird to me. Is there any reason we can't just monitor the > pref, rather than managing it? > > This also seems to have the potential for bugs - managed prefs override user > prefs (I assume), so if we somehow manage to have a call to > SetDataReductionProxyEnabled when the managed pref is false, things break. > Monitoring the pref instead avoids the issue. Done. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:370: SetProxyPac(enabled_by_user_, !env); On 2013/09/12 16:30:41, mmenke wrote: > To avoid weird duplicate enabled cases, you should set disabled_by_carrier_ to > false here. Or perhaps better, call "SetProxyPac(enabled_by_user_ && > !disabled_by_carrier_, ...)" Done. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:387: LOG(WARNING) << "SPDY proxy ON " << (at_startup ? kAtStartup : kByUser); On 2013/09/12 16:30:41, mmenke wrote: > Not a big fan of LOGs compiled into production code. Do we need them? Unfortunately we do. We have code that searches for this string in user submitted feedback to indicate the the proxy may be involved in the issue being filed. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:430: const std::string& url) { On 2013/09/12 16:30:41, mmenke wrote: > Do we really need to take the URL as a parameter? Seems like we could just call > GetProxyCheckURL(). Done. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h (right): https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:62: jboolean IsDataReductionProxyAvailable(JNIEnv* env, jobject obj); On 2013/09/12 16:30:41, mmenke wrote: > "Allowed" may be clearer than "Available" ("Available" sounds more like the > probe passed). This goes for all "Available" functions, though the promo ones > are debatable. Done. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:84: // Enables or disables the data reduction proxy. On 2013/09/12 16:30:41, mmenke wrote: > Think it's worth adding something along the lines of "If a probe URL is > available, and a probe request fails at some point, the proxy won't be used > until a probe succeed." Or alternatively, maybe adding something about the > probes to the class description. Done. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:160: void ProbeWhetherDataReductionProxyIsAvailable(const std::string& url); On 2013/09/12 16:30:41, mmenke wrote: > Think it's worth a description...Along the lines of "Requests the proxy probe > URL, if one is set. If unable to do so, disables the proxy, if enabled. > Otherwise enables the proxy, if disabled". Done. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc (right): https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:26: On 2013/09/12 16:30:41, mmenke wrote: > nit: Blank line not needed. Done. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:56: switches::kSpdyProxyAuthValue, "12345"); On 2013/09/12 16:30:41, mmenke wrote: > Suggest making both of these strings consts char[]s, and putting them at the top > of the file. Done. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:84: } On 2013/09/12 16:30:41, mmenke wrote: > This isn't used, and doesn't do anything. Done. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:89: scoped_ptr<base::FieldTrialList> field_trial_list_; On 2013/09/12 16:30:41, mmenke wrote: > Think it's worth mentioning somewhere that this is a singleton that will clear > all set field trials on destruction. Done. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:92: TEST_F(DataReductionProxySettingsAndroidTest, TestGetDataReductionProxyOrigin) { On 2013/09/12 16:30:41, mmenke wrote: > Suggest also testing setting this via the command line (Same goes for the auth > test). I do test the via the command line. See SetUp(). I added a comment to clarify. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:134: EXPECT_EQ("foo:443", settings_->GetDataReductionProxyOriginHostPort()); On 2013/09/12 16:30:41, mmenke wrote: > Again, think we want to test with a value set by the command line. Configured in SetUp(). https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:138: TestResetDataReductionStatistics) { On 2013/09/12 16:30:41, mmenke wrote: > Think this should go just after TestContentLengthsInternal. Done. https://codereview.chromium.org/23458016/diff/62001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:221: } On 2013/09/12 16:30:41, mmenke wrote: > I'd really like to see some some pretty extensive testing of > SetDataReductionProxyEnabled / ProbeWhetherDataReductionProxyIsAvailable / > OnURLFetchComplete, both in the command line case and in the > non-command-line-base. There are mock URL fetcher classes for tests that > shouldn't make it too bad. > > I'd like to see all "transitions" via probes (enabled -> enabled, enabled -> > disabled, disabled -> disabled, disabled -> enabled), and maybe some playing > with enabled state as well. To make sure it's disabled, you can check the > prefs::kProxy pref. > > Some enabling/disabling via SetDataReductionProxyEnabled in various probe states > would be nice. Done. I didn't test enabling/disabling while a probe is outstanding but did test all other states. I've added a TODO.
https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:49: // New values should be added at the end before |NUM_SPDY_PROXY_AUTH_STATE| nit: +period. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:97: return command_line.GetSwitchValueASCII(switches::kSpdyProxyAuthOrigin); switches::kDataReductionProxyProbeURL? https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:131: base::Unretained(this))); Rather than use both a registrar and a pref lookup, you should just use a BooleanPrefMember. It will basically get rid of every other use of kSpdyProxyAuthEnabled in this file. It reduces redundant code, and protects a bit against copying/pasting the wrong pref string. Note that calling BooleanPrefMember::SetValue won't call back into |this|, to prevent re-entrancy, so you'll need to call the onchange function directly, I believe. I think that's a little cleaner than relying on non-obvious re-entrancy, anyways. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:147: spdy_proxy_enabled) { nit: Checking spdy_proxy_enabled_ first is marginally faster. Performance doesn't really matter here, of course, but still...Seems like the more expensive check should go second. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:193: return ConvertUTF8ToJavaString(env, std::string()); Think this is worth a comment (Just something along the lines of "Don't expose SPDY_PROXY_AUTH_VALUE to a proxy passed in via the command line"). https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:213: void DataReductionProxySettingsAndroid::SetDataReductionProxyEnabled( Are there already Java functions to set preferences directly? Not strongly opposed to this, just wondering if there's a preferable way of doing it. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:300: if (enabled_by_user_) DCHECK(IsProxyAllowed())? We should maintain some sort of invariant if !IsProxyAllowed(), and the simplest seems to be that if !IsProxyAllowed(), then enabled_by_user_ is always false, and we don't run any probes. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:304: void DataReductionProxySettingsAndroid::OnProxyEnabledPrefChange() { Should we just exit of not Allowed? https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:313: AddHostToBypass("[::1]"); On 2013/09/17 01:06:19, bengr1 wrote: > On 2013/09/12 16:30:41, mmenke wrote: > > "::1" for IPv6 support? (Or should it be [::1]? - not sure if we get the IPv6 > > address or host part of the URL in PACs) > > I'll add [::1]. Do you know how to add (and see) log statements from a PAC? > alert() seems to execute, but I can't get the output to a console. I believe you can use about:net-internals#events. Calls to alert() in Javascript should show up in the HTTP_STREAM_REQUEST events that resulted in the call (Look for PAC_JAVASCRIPT_ALERT). That having been said, looking at the code, I think it should be "::1", rather than "[::1]". See https://code.google.com/p/chromium/codesearch#chromium/src/net/proxy/proxy_re.... Could also just ask eroman. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:364: DataReductionProxySettingsAndroid::GetDataReductionProxyOriginInternal() { Looks like this function and the next one can be put in the anonymous namespace. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:366: return std::string(); Is this default behavior needed? Doesn't seem like it matters in the not allowed case. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:516: local_state->GetInt64(prefs::kDailyHttpContentLengthLastUpdateDate); Seems like we don't get anything from having the temporary here - can just move the call down when setting *last_update_time. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:544: fetcher_.reset(net::URLFetcher::Create(0, GURL(url), Since you aren't using TestUrlFetcherFactory, you should just get rid of the first argument. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:558: net::URLFetcher* fetcher = GetURLFetcher(); optional: I think it would be much cleaner to change ownership semantics so this is fetcher_.reset(GetURLFetcher()). Could alternatively use TestUrlFetcherFactory instead, but since we're subclassing for tests anyways, I don't think that gets us all that much. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:559: if (fetcher) Since this can't happen, I think it's best not to be doing this check. My general opinion is if something logically can't happen, don't handle it. If something unexpected happens, it's better to crash and then debug it, than to silently continue on in an unexpected state. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:573: " alert(\"Hello World\");" I assume this was just for testing? https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:577: " if (url.substring(0, 5) == \"http:\") {" I thought media bypassed flywheel? Or is that done by using no PAC for the media URL request context, rather than at the PAC level? https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h (right): https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:143: nit: Remove linebreaks. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc (right): https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:44: nit: "// DataReductionProxySettingsAndroid implementation:" https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:59: void set_probe_for_testing(const std::string& test_url, Maybe "set_probe_result" Since this is in a unit test file, I don't think the "for_testing" is needed. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:61: bool success) { nit: Fix ident. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:75: int fake_fetcher_request_cnt() { nit: Spell out "count". https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:80: friend class DataReductionProxySettingsAndroidTest; I don't think this is needed, since the parent class friends the test. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:80: friend class DataReductionProxySettingsAndroidTest; optional: You could get rid of all the friends in both class, and make the fields/methods the tests need protected, and then make them public in TestDataReductionProxySettingsAndroid with using declarations (Or with wrapper methods). Then you could also put this entire file in an anonymous namespace. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:143: base::TimeDelta::FromDays(spdyproxy::kNumDaysInHistory); Any motivation for this? https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:171: bool expected_enabled) { Fix indent. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:185: } Fix indent. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:194: TEST_F(DataReductionProxySettingsAndroidTest, TestGetDataReductionProxyOrigin) { On 2013/09/17 01:06:19, bengr1 wrote: > On 2013/09/12 16:30:41, mmenke wrote: > > Suggest also testing setting this via the command line (Same goes for the auth > > test). > > I do test the via the command line. See SetUp(). I added a comment to clarify. Oh, right...then should have a test that checks the pref. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:232: it != settings_->bypass_rules_.end(); ++it) { nit: Seems much more common to align the next line with the inside of the "(" in for loops.
[+mef] Misha will be taking over the flywheel reviews. Misha: I'm pretty happy overall with this CL. My main concern is that the code's a little on the complicated side, so I want to be sure the unit tests are exhaustive. I recommend you look at the other CL first, since this one references some performance metrics stored as "preferences" in the other CL.
On 2013/09/17 21:33:14, mmenke wrote: > [+mef] Misha will be taking over the flywheel reviews. > > Misha: I'm pretty happy overall with this CL. My main concern is that the > code's a little on the complicated side, so I want to be sure the unit tests are > exhaustive. I recommend you look at the other CL first, since this one > references some performance metrics stored as "preferences" in the other CL. Oh, and Misha is not yet a browser/net OWNER, so you'll still need my signoff for the moment. Once he gets some reviews under his belt, we can change that.
One more quick comment. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:304: void DataReductionProxySettingsAndroid::OnProxyEnabledPrefChange() { On 2013/09/17 20:25:25, mmenke wrote: > Should we just exit of not Allowed? Err.... Or "kEnableSpdyProxyAuth" is set - either the pref or the command line flag. Separating out "enabled" and "allowed" seems really weird to me.
https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/OWNERS (right): https://codereview.chromium.org/23458016/diff/54001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/OWNERS:1: bengr@chromium.org Please add marq@chromium.org https://codereview.chromium.org/23458016/diff/69001/chrome/common/chrome_swit... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/23458016/diff/69001/chrome/common/chrome_swit... chrome/common/chrome_switches.cc:246: // If set, the data reduction proxy will only be enabled if a request for this OS_IOS https://codereview.chromium.org/23458016/diff/69001/chrome/common/chrome_swit... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/23458016/diff/69001/chrome/common/chrome_swit... chrome/common/chrome_switches.h:80: #if defined(OS_ANDROID) Please add OS_IOS any time you know we'll need the same code in both platforms.
https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:49: // New values should be added at the end before |NUM_SPDY_PROXY_AUTH_STATE| On 2013/09/17 20:25:25, mmenke wrote: > nit: +period. Done. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:97: return command_line.GetSwitchValueASCII(switches::kSpdyProxyAuthOrigin); On 2013/09/17 20:25:25, mmenke wrote: > switches::kDataReductionProxyProbeURL? Done. (Sorry, I caught this one after uploading and forgot to upload again.) https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:131: base::Unretained(this))); On 2013/09/17 20:25:25, mmenke wrote: > Rather than use both a registrar and a pref lookup, you should just use a > BooleanPrefMember. It will basically get rid of every other use of > kSpdyProxyAuthEnabled in this file. It reduces redundant code, and protects a > bit against copying/pasting the wrong pref string. > > Note that calling BooleanPrefMember::SetValue won't call back into |this|, to > prevent re-entrancy, so you'll need to call the onchange function directly, I > believe. I think that's a little cleaner than relying on non-obvious > re-entrancy, anyways. Done. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:147: spdy_proxy_enabled) { On 2013/09/17 20:25:25, mmenke wrote: > nit: Checking spdy_proxy_enabled_ first is marginally faster. Performance > doesn't really matter here, of course, but still...Seems like the more expensive > check should go second. Done. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:193: return ConvertUTF8ToJavaString(env, std::string()); On 2013/09/17 20:25:25, mmenke wrote: > Think this is worth a comment (Just something along the lines of "Don't expose > SPDY_PROXY_AUTH_VALUE to a proxy passed in via the command line"). Done. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:213: void DataReductionProxySettingsAndroid::SetDataReductionProxyEnabled( On 2013/09/17 20:25:25, mmenke wrote: > Are there already Java functions to set preferences directly? Not strongly > opposed to this, just wondering if there's a preferable way of doing it. This is how Chrome for Android does things. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:300: if (enabled_by_user_) On 2013/09/17 20:25:25, mmenke wrote: > DCHECK(IsProxyAllowed())? We should maintain some sort of invariant if > !IsProxyAllowed(), and the simplest seems to be that if !IsProxyAllowed(), then > enabled_by_user_ is always false, and we don't run any probes. Done. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:304: void DataReductionProxySettingsAndroid::OnProxyEnabledPrefChange() { On 2013/09/17 20:25:25, mmenke wrote: > Should we just exit of not Allowed? Done. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:304: void DataReductionProxySettingsAndroid::OnProxyEnabledPrefChange() { On 2013/09/17 21:51:51, mmenke wrote: > On 2013/09/17 20:25:25, mmenke wrote: > > Should we just exit of not Allowed? > > Err.... Or "kEnableSpdyProxyAuth" is set - either the pref or the command line > flag. Separating out "enabled" and "allowed" seems really weird to me. One reason to separate is for the UI. I.e., we shouldn't show the UI at all if not part of the field trial, irrespective of whether the pref is set to enabled. I thought a bit about creating an enum with states: DISALLOWED, DISABLED, ENABLED, DISABLED_VIA_PROBE_FAILURE, but I don't think it would simplify things. I'm ok with leaving the code as is, but would entertain suggestions. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:313: AddHostToBypass("[::1]"); On 2013/09/17 20:25:25, mmenke wrote: > On 2013/09/17 01:06:19, bengr1 wrote: > > On 2013/09/12 16:30:41, mmenke wrote: > > > "::1" for IPv6 support? (Or should it be [::1]? - not sure if we get the > IPv6 > > > address or host part of the URL in PACs) > > > > I'll add [::1]. Do you know how to add (and see) log statements from a PAC? > > alert() seems to execute, but I can't get the output to a console. > > I believe you can use about:net-internals#events. Calls to alert() in > Javascript should show up in the HTTP_STREAM_REQUEST events that resulted in the > call (Look for PAC_JAVASCRIPT_ALERT). > > That having been said, looking at the code, I think it should be "::1", rather > than "[::1]". See > https://code.google.com/p/chromium/codesearch#chromium/src/net/proxy/proxy_re.... > Could also just ask eroman. Done. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:364: DataReductionProxySettingsAndroid::GetDataReductionProxyOriginInternal() { On 2013/09/17 20:25:25, mmenke wrote: > Looks like this function and the next one can be put in the anonymous namespace. Done. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:366: return std::string(); On 2013/09/17 20:25:25, mmenke wrote: > Is this default behavior needed? Doesn't seem like it matters in the not > allowed case. Done. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:516: local_state->GetInt64(prefs::kDailyHttpContentLengthLastUpdateDate); On 2013/09/17 20:25:25, mmenke wrote: > Seems like we don't get anything from having the temporary here - can just move > the call down when setting *last_update_time. Done. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:544: fetcher_.reset(net::URLFetcher::Create(0, GURL(url), On 2013/09/17 20:25:25, mmenke wrote: > Since you aren't using TestUrlFetcherFactory, you should just get rid of the > first argument. Done. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:558: net::URLFetcher* fetcher = GetURLFetcher(); On 2013/09/17 20:25:25, mmenke wrote: > optional: I think it would be much cleaner to change ownership semantics so > this is fetcher_.reset(GetURLFetcher()). > > Could alternatively use TestUrlFetcherFactory instead, but since we're > subclassing for tests anyways, I don't think that gets us all that much. Done. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:559: if (fetcher) On 2013/09/17 20:25:25, mmenke wrote: > Since this can't happen, I think it's best not to be doing this check. My > general opinion is if something logically can't happen, don't handle it. If > something unexpected happens, it's better to crash and then debug it, than to > silently continue on in an unexpected state. This could happen if the proxy check url is empty. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:573: " alert(\"Hello World\");" On 2013/09/17 20:25:25, mmenke wrote: > I assume this was just for testing? Done. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:577: " if (url.substring(0, 5) == \"http:\") {" On 2013/09/17 20:25:25, mmenke wrote: > I thought media bypassed flywheel? Or is that done by using no PAC for the > media URL request context, rather than at the PAC level? Media is bypassed using a proxy-bypass signal from the proxy. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h (right): https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:143: On 2013/09/17 20:25:25, mmenke wrote: > nit: Remove linebreaks. Done. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc (right): https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:44: On 2013/09/17 20:25:25, mmenke wrote: > nit: "// DataReductionProxySettingsAndroid implementation:" Done. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:59: void set_probe_for_testing(const std::string& test_url, On 2013/09/17 20:25:25, mmenke wrote: > Maybe "set_probe_result" Since this is in a unit test file, I don't think the > "for_testing" is needed. Done. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:61: bool success) { On 2013/09/17 20:25:25, mmenke wrote: > nit: Fix ident. Done. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:75: int fake_fetcher_request_cnt() { On 2013/09/17 20:25:25, mmenke wrote: > nit: Spell out "count". Done. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:80: friend class DataReductionProxySettingsAndroidTest; On 2013/09/17 20:25:25, mmenke wrote: > I don't think this is needed, since the parent class friends the test. Done. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:80: friend class DataReductionProxySettingsAndroidTest; On 2013/09/17 20:25:25, mmenke wrote: > optional: You could get rid of all the friends in both class, and make the > fields/methods the tests need protected, and then make them public in > TestDataReductionProxySettingsAndroid with using declarations (Or with wrapper > methods). Then you could also put this entire file in an anonymous namespace. I'll leave things as is but will consider on a subsequent CL where we merge common code for Android and iOS. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:143: base::TimeDelta::FromDays(spdyproxy::kNumDaysInHistory); On 2013/09/17 20:25:25, mmenke wrote: > Any motivation for this? None that I recall. Done. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:171: bool expected_enabled) { On 2013/09/17 20:25:25, mmenke wrote: > Fix indent. Done. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:185: } On 2013/09/17 20:25:25, mmenke wrote: > Fix indent. Done. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:194: TEST_F(DataReductionProxySettingsAndroidTest, TestGetDataReductionProxyOrigin) { On 2013/09/17 20:25:25, mmenke wrote: > On 2013/09/17 01:06:19, bengr1 wrote: > > On 2013/09/12 16:30:41, mmenke wrote: > > > Suggest also testing setting this via the command line (Same goes for the > auth > > > test). > > > > I do test the via the command line. See SetUp(). I added a comment to clarify. > > Oh, right...then should have a test that checks the pref. I don't know what you mean here. https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:232: it != settings_->bypass_rules_.end(); ++it) { On 2013/09/17 20:25:25, mmenke wrote: > nit: Seems much more common to align the next line with the inside of the "(" > in for loops. Done. https://codereview.chromium.org/23458016/diff/69001/chrome/common/chrome_swit... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/23458016/diff/69001/chrome/common/chrome_swit... chrome/common/chrome_switches.cc:246: // If set, the data reduction proxy will only be enabled if a request for this On 2013/09/17 21:57:50, marq_ wrote: > OS_IOS Done. https://codereview.chromium.org/23458016/diff/69001/chrome/common/chrome_swit... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/23458016/diff/69001/chrome/common/chrome_swit... chrome/common/chrome_switches.h:80: #if defined(OS_ANDROID) On 2013/09/17 21:57:50, marq_ wrote: > Please add OS_IOS any time you know we'll need the same code in both platforms. Done.
https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc (right): https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:194: TEST_F(DataReductionProxySettingsAndroidTest, TestGetDataReductionProxyOrigin) { On 2013/09/18 22:15:26, bengr1 wrote: > On 2013/09/17 20:25:25, mmenke wrote: > > On 2013/09/17 01:06:19, bengr1 wrote: > > > On 2013/09/12 16:30:41, mmenke wrote: > > > > Suggest also testing setting this via the command line (Same goes for the > > auth > > > > test). > > > > > > I do test the via the command line. See SetUp(). I added a comment to > clarify. > > > > Oh, right...then should have a test that checks the pref. > > I don't know what you mean here. Sorry, field trial. Right now, you're testing the code with both the field trial and the command line enabling it, which seems wrong - could imagine a case where they're both broken individually, but things work when they're both enabled. Should have tests that do things separately. The field trial requires DATA_REDUCTION_PROXY_PROBE_URL and SPDY_PROXY_AUTH_ORIGIN be set, which isn't the case for Chromium (And probably everywhere else, except official Android/iOS builders), so I think it's simplest just to use #ifdefs around a test for that, and make the default tests not use the field trials, since it isn't getting us anything, since we're using the command line as well.
https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc (right): https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:194: TEST_F(DataReductionProxySettingsAndroidTest, TestGetDataReductionProxyOrigin) { On 2013/09/18 22:27:31, mmenke wrote: > On 2013/09/18 22:15:26, bengr1 wrote: > > On 2013/09/17 20:25:25, mmenke wrote: > > > On 2013/09/17 01:06:19, bengr1 wrote: > > > > On 2013/09/12 16:30:41, mmenke wrote: > > > > > Suggest also testing setting this via the command line (Same goes for > the > > > auth > > > > > test). > > > > > > > > I do test the via the command line. See SetUp(). I added a comment to > > clarify. > > > > > > Oh, right...then should have a test that checks the pref. > > > > I don't know what you mean here. > > Sorry, field trial. Right now, you're testing the code with both the field > trial and the command line enabling it, which seems wrong - could imagine a case > where they're both broken individually, but things work when they're both > enabled. > > Should have tests that do things separately. The field trial requires > DATA_REDUCTION_PROXY_PROBE_URL and SPDY_PROXY_AUTH_ORIGIN be set, which isn't > the case for Chromium (And probably everywhere else, except official Android/iOS > builders), so I think it's simplest just to use #ifdefs around a test for that, > and make the default tests not use the field trials, since it isn't getting us > anything, since we're using the command line as well. Alternatively, could just get rid of the field trial stuff from these tests - calls to it are being shortcircuited as-is, so it's not getting us anything.
https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc (right): https://codereview.chromium.org/23458016/diff/69001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:194: TEST_F(DataReductionProxySettingsAndroidTest, TestGetDataReductionProxyOrigin) { On 2013/09/18 22:30:02, mmenke wrote: > On 2013/09/18 22:27:31, mmenke wrote: > > On 2013/09/18 22:15:26, bengr1 wrote: > > > On 2013/09/17 20:25:25, mmenke wrote: > > > > On 2013/09/17 01:06:19, bengr1 wrote: > > > > > On 2013/09/12 16:30:41, mmenke wrote: > > > > > > Suggest also testing setting this via the command line (Same goes for > > the > > > > auth > > > > > > test). > > > > > > > > > > I do test the via the command line. See SetUp(). I added a comment to > > > clarify. > > > > > > > > Oh, right...then should have a test that checks the pref. > > > > > > I don't know what you mean here. > > > > Sorry, field trial. Right now, you're testing the code with both the field > > trial and the command line enabling it, which seems wrong - could imagine a > case > > where they're both broken individually, but things work when they're both > > enabled. > > > > Should have tests that do things separately. The field trial requires > > DATA_REDUCTION_PROXY_PROBE_URL and SPDY_PROXY_AUTH_ORIGIN be set, which isn't > > the case for Chromium (And probably everywhere else, except official > Android/iOS > > builders), so I think it's simplest just to use #ifdefs around a test for > that, > > and make the default tests not use the field trials, since it isn't getting us > > anything, since we're using the command line as well. > > Alternatively, could just get rid of the field trial stuff from these tests - > calls to it are being shortcircuited as-is, so it's not getting us anything. I removed the field trial stuff. All of these tests are only run on Android due to some gyp magic (scanning for the file name suffix 'android.[cc|h]'). I added a test, though to cover not returning auth set by the preprocessor for an origin set by a switch.
Here are my 2 cents. https://codereview.chromium.org/23458016/diff/88001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/23458016/diff/88001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:69: (FieldTrialList::FindFullName("DataCompressionProxyRollout") == nit: Probably should define those strings (especially "Enabled") as char[] constants. https://codereview.chromium.org/23458016/diff/88001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:347: // concern was that adding these and other rules would add to the processing I think those must be added, otherwise how would it work with local network servers? https://codereview.chromium.org/23458016/diff/88001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:424: spdy_proxy_auth_enabled_.GetValue() && spdy_proxy_origin != ""; !spdy_proxy_origin.empty() https://codereview.chromium.org/23458016/diff/88001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h (right): https://codereview.chromium.org/23458016/diff/88001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:106: // Returning an array containing the aggregate received HTTP content in nit: Returns
https://codereview.chromium.org/23458016/diff/88001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/23458016/diff/88001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:69: (FieldTrialList::FindFullName("DataCompressionProxyRollout") == On 2013/09/19 20:21:44, mef wrote: > nit: Probably should define those strings (especially "Enabled") as char[] > constants. Done for "Enabled". The others only appear once, so I don't see the point. https://codereview.chromium.org/23458016/diff/88001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:347: // concern was that adding these and other rules would add to the processing On 2013/09/19 20:21:44, mef wrote: > I think those must be added, otherwise how would it work with local network > servers? The data reduction proxy will issue a proxy-bypass message for request to local addresses. https://codereview.chromium.org/23458016/diff/88001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:424: spdy_proxy_auth_enabled_.GetValue() && spdy_proxy_origin != ""; On 2013/09/19 20:21:44, mef wrote: > !spdy_proxy_origin.empty() Done. https://codereview.chromium.org/23458016/diff/88001/chrome/browser/net/spdypr... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h (right): https://codereview.chromium.org/23458016/diff/88001/chrome/browser/net/spdypr... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:106: // Returning an array containing the aggregate received HTTP content in On 2013/09/19 20:21:44, mef wrote: > nit: Returns Done.
On 2013/09/19 20:31:24, bengr1 wrote: > https://codereview.chromium.org/23458016/diff/88001/chrome/browser/net/spdypr... > File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc > (right): > > https://codereview.chromium.org/23458016/diff/88001/chrome/browser/net/spdypr... > chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:69: > (FieldTrialList::FindFullName("DataCompressionProxyRollout") == > On 2013/09/19 20:21:44, mef wrote: > > nit: Probably should define those strings (especially "Enabled") as char[] > > constants. > > Done for "Enabled". The others only appear once, so I don't see the point. > > https://codereview.chromium.org/23458016/diff/88001/chrome/browser/net/spdypr... > chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:347: // > concern was that adding these and other rules would add to the processing > On 2013/09/19 20:21:44, mef wrote: > > I think those must be added, otherwise how would it work with local network > > servers? > > The data reduction proxy will issue a proxy-bypass message for request to local > addresses. > > https://codereview.chromium.org/23458016/diff/88001/chrome/browser/net/spdypr... > chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:424: > spdy_proxy_auth_enabled_.GetValue() && spdy_proxy_origin != ""; > On 2013/09/19 20:21:44, mef wrote: > > !spdy_proxy_origin.empty() > > Done. > > https://codereview.chromium.org/23458016/diff/88001/chrome/browser/net/spdypr... > File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h > (right): > > https://codereview.chromium.org/23458016/diff/88001/chrome/browser/net/spdypr... > chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:106: // > Returning an array containing the aggregate received HTTP content in > On 2013/09/19 20:21:44, mef wrote: > > nit: Returns > > Done. mef, mmenke, PTAL. Thanks.
Few more comments... https://codereview.chromium.org/23458016/diff/103001/chrome/browser/net/spdyp... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/23458016/diff/103001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:166: CommandLine* command_line = CommandLine::ForCurrentProcess(); const CommandLine& command_line = *CommandLine::ForCurrentProcess(); https://codereview.chromium.org/23458016/diff/103001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:192: JNIEnv* env, jobject obj) { Does JNI allow marking methods as 'const'? If yes, then this and others should probably be marked as such. https://codereview.chromium.org/23458016/diff/103001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:211: return ConvertUTF8ToJavaString(env, std::string()); Suggest simplifying: std::string spdy_proxy_auth_value; if (IsProxyAllowed()) { 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. // Don't expose SPDY_PROXY_AUTH_VALUE to a proxy passed in via the command // line. if (command_line.HasSwitch(switches::kSpdyProxyAuthValue)) spdy_proxy_auth_value = command_line.GetSwitchValueASCII(switches::kSpdyProxyAuthValue)); } else { #if defined(SPDY_PROXY_AUTH_VALUE) spdy_proxy_auth_value = SPDY_PROXY_AUTH_VALUE; #endif } } return ConvertUTF8ToJavaString(env, spdy_proxy_auth_value); https://codereview.chromium.org/23458016/diff/103001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:271: &received_content_length, &last_update_internal); nit: move &last_update_internal); to new line? https://codereview.chromium.org/23458016/diff/103001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:534: for (unsigned int i = 0; i < days; ++i) { BUG? What if days > kNumDaysInHistory? https://codereview.chromium.org/23458016/diff/103001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:535: int read_index = spdyproxy::kNumDaysInHistory - 1 - i; i is unsigned, read_index is signed... Maybe loop like for(i = kNumDaysInHistory - days; i < kNumDaysInHistory; ++i) and get rid of read_index? https://codereview.chromium.org/23458016/diff/103001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:536: std::string result; unused? https://codereview.chromium.org/23458016/diff/103001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:574: std::string bypass_clause = "(" + JoinString(bypass_rules_, ") || (") + ")"; Can bypass_rules be influenced by user? If so, then could this be dangerous from the script injection point of view?
few comments about tests... https://codereview.chromium.org/23458016/diff/103001/chrome/browser/net/spdyp... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc (right): https://codereview.chromium.org/23458016/diff/103001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:161: EXPECT_EQ(std::string(), pac_url); nit: EXPECT_TRUE(pac_url.empty()); https://codereview.chromium.org/23458016/diff/103001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:216: // Test that the auth value set by preprocessor directive is not returned nit: Put comment above test declaration. Same for other tests. https://codereview.chromium.org/23458016/diff/103001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:296: ASSERT_EQ(static_cast<long>(118 - (2 * i)), value); What's the magic of 118? https://codereview.chromium.org/23458016/diff/103001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:320: settings_->GetContentLengthsInternal(spdyproxy::kNumDaysInHistory, Add tests with days == 0, 1, spdyproxy::kNumDaysInHistory - 1, spdyproxy::kNumDaysInHistory + 1. https://codereview.chromium.org/23458016/diff/103001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:325: EXPECT_EQ(1770L, received_content_length); Where are these constants coming from?
https://codereview.chromium.org/23458016/diff/103001/chrome/browser/net/spdyp... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/23458016/diff/103001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:166: CommandLine* command_line = CommandLine::ForCurrentProcess(); On 2013/09/24 16:31:17, mef wrote: > const CommandLine& command_line = *CommandLine::ForCurrentProcess(); Done. https://codereview.chromium.org/23458016/diff/103001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:192: JNIEnv* env, jobject obj) { On 2013/09/24 16:31:17, mef wrote: > Does JNI allow marking methods as 'const'? > If yes, then this and others should probably be marked as such. No. (Or at least not the way chromium uses it.) https://codereview.chromium.org/23458016/diff/103001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:211: return ConvertUTF8ToJavaString(env, std::string()); On 2013/09/24 16:31:17, mef wrote: > Suggest simplifying: > > std::string spdy_proxy_auth_value; > if (IsProxyAllowed()) { > 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. > // Don't expose SPDY_PROXY_AUTH_VALUE to a proxy passed in via the command > // line. > if (command_line.HasSwitch(switches::kSpdyProxyAuthValue)) > spdy_proxy_auth_value = > command_line.GetSwitchValueASCII(switches::kSpdyProxyAuthValue)); > } else { > #if defined(SPDY_PROXY_AUTH_VALUE) > spdy_proxy_auth_value = SPDY_PROXY_AUTH_VALUE; > #endif > } > } > return ConvertUTF8ToJavaString(env, spdy_proxy_auth_value); I'm going to leave this one as is. In general, I like returning early when possible. I think it makes the code easier to read, if slightly more verbose in cases like this one. https://codereview.chromium.org/23458016/diff/103001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:271: &received_content_length, &last_update_internal); On 2013/09/24 16:31:17, mef wrote: > nit: move &last_update_internal); to new line? Done. https://codereview.chromium.org/23458016/diff/103001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:534: for (unsigned int i = 0; i < days; ++i) { On 2013/09/24 16:31:17, mef wrote: > BUG? What if days > kNumDaysInHistory? There's a DCHECK at the top of the function to catch this. I believe I used to have the check closer to it's use, but one of the reviewers suggested moving it up. https://codereview.chromium.org/23458016/diff/103001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:535: int read_index = spdyproxy::kNumDaysInHistory - 1 - i; On 2013/09/24 16:31:17, mef wrote: > i is unsigned, read_index is signed... > Maybe loop like > for(i = kNumDaysInHistory - days; i < kNumDaysInHistory; ++i) > and get rid of read_index? Done. https://codereview.chromium.org/23458016/diff/103001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:536: std::string result; On 2013/09/24 16:31:17, mef wrote: > unused? Done. https://codereview.chromium.org/23458016/diff/103001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:574: std::string bypass_clause = "(" + JoinString(bypass_rules_, ") || (") + ")"; On 2013/09/24 16:31:17, mef wrote: > Can bypass_rules be influenced by user? > If so, then could this be dangerous from the script injection point of view? No. I don't believe that the bypass rules can be influenced by the user. https://codereview.chromium.org/23458016/diff/103001/chrome/browser/net/spdyp... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc (right): https://codereview.chromium.org/23458016/diff/103001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:161: EXPECT_EQ(std::string(), pac_url); On 2013/09/24 16:44:15, mef wrote: > nit: EXPECT_TRUE(pac_url.empty()); Done. https://codereview.chromium.org/23458016/diff/103001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:216: // Test that the auth value set by preprocessor directive is not returned On 2013/09/24 16:44:15, mef wrote: > nit: Put comment above test declaration. Same for other tests. Done. https://codereview.chromium.org/23458016/diff/103001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:296: ASSERT_EQ(static_cast<long>(118 - (2 * i)), value); On 2013/09/24 16:44:15, mef wrote: > What's the magic of 118? Done. https://codereview.chromium.org/23458016/diff/103001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:320: settings_->GetContentLengthsInternal(spdyproxy::kNumDaysInHistory, On 2013/09/24 16:44:15, mef wrote: > Add tests with days == 0, 1, spdyproxy::kNumDaysInHistory - 1, > spdyproxy::kNumDaysInHistory + 1. Done, except for kNumDaysInHistory + 1, because GetContentLengthsInternal DCHECKs for days <= kNumDaysInHistory. https://codereview.chromium.org/23458016/diff/103001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:325: EXPECT_EQ(1770L, received_content_length); On 2013/09/24 16:44:15, mef wrote: > Where are these constants coming from? Done.
mef, PTAL.
On 2013/09/25 21:34:10, bengr1 wrote: > mef, PTAL. It seems good to me, but you'll need Matt's approval.
mmemke: PTAL.
https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:378: StringPrintf("shExpMatch(%s, \"%s\")", Rather than using \", why not just use single quotes in all Javascript strings? Google style guide also states to use single quotes in Javascript (To mix it with HTML happily, but the same reasonable applies with C++ as well). https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:534: // We include days from the end of the list going backwards. nit: Don't use "we" in comments. Can just remove the word and capitalize the next word. https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:557: // Configure to max_retries at most kMaxRetries times for 5xx errors. nit: Fix grammar. "Configure max_retries to be at most..." (Or maybe use "max retries" instead?) https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:577: // only process HTTP traffic, so we concoct a PAC configuration nit: Don't use "We" in comments, due to ambiguity of who it's referring to. https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc (right): https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:73: int fake_fetcher_request_count() { nit: const https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:83: PrefService* local_state_prefs_for_testing_; nit: Now that these are only in test code, the "for_testing_" isn't needed. https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:98: chrome_variations::testing::ClearAllVariationParams(); Since we enable at the command line, I don't any of this actually does anything. https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:108: } Again, doesn't seem to do anything. https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:149: dict->GetString("mode", &mode); Should probably assert on the result in these 4 GetString calls in this function and the next. https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:153: } nit: Line break https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:168: settings_->AddDefaultProxyBypassRules(); I don't think we should be making this call, since it affects setting's state, and we want to call use it as normally as possible. My suggestion is to move the next three lines to just below the expect_enabled check instead. https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:216: // when an origin is set via a switch. There is no preprocessor directive anyways in Chromium, right? May be worth mentioning this only does anything useful in Chrome builds. https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:222: DataReductionProxySettingsAndroid::Register(env); optional: May want to just move these two lines into the test fixture. https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:383: } We never test OnIPAddressChanged, which follows a slightly different path. I'd like to see it switch state from enabled -> enabled -> disabled -> disabled -> enabled, so we get all possible state transitions, just to be safe. https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:383: } Also, none of these tests call InitDataReductionProxySettings, which is normally called first. https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:383: } Also, none of the tests check that when prefs::kSpdyProxyAuthEnabled is changed, we get a notification and do everything we should
https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:378: StringPrintf("shExpMatch(%s, \"%s\")", On 2013/09/26 18:09:44, mmenke wrote: > Rather than using \", why not just use single quotes in all Javascript strings? > Google style guide also states to use single quotes in Javascript (To mix it > with HTML happily, but the same reasonable applies with C++ as well). Done. https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:534: // We include days from the end of the list going backwards. On 2013/09/26 18:09:44, mmenke wrote: > nit: Don't use "we" in comments. Can just remove the word and capitalize the > next word. Done. https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:557: // Configure to max_retries at most kMaxRetries times for 5xx errors. On 2013/09/26 18:09:44, mmenke wrote: > nit: Fix grammar. "Configure max_retries to be at most..." (Or maybe use "max > retries" instead?) Done. https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:577: // only process HTTP traffic, so we concoct a PAC configuration On 2013/09/26 18:09:44, mmenke wrote: > nit: Don't use "We" in comments, due to ambiguity of who it's referring to. Done. https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc (right): https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:73: int fake_fetcher_request_count() { On 2013/09/26 18:09:44, mmenke wrote: > nit: const Done. https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:83: PrefService* local_state_prefs_for_testing_; On 2013/09/26 18:09:44, mmenke wrote: > nit: Now that these are only in test code, the "for_testing_" isn't needed. Done. https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:98: chrome_variations::testing::ClearAllVariationParams(); On 2013/09/26 18:09:44, mmenke wrote: > Since we enable at the command line, I don't any of this actually does anything. Done. https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:108: } On 2013/09/26 18:09:44, mmenke wrote: > Again, doesn't seem to do anything. Done. https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:149: dict->GetString("mode", &mode); On 2013/09/26 18:09:44, mmenke wrote: > Should probably assert on the result in these 4 GetString calls in this function > and the next. Done. https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:153: } On 2013/09/26 18:09:44, mmenke wrote: > nit: Line break Done. https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:168: settings_->AddDefaultProxyBypassRules(); On 2013/09/26 18:09:44, mmenke wrote: > I don't think we should be making this call, since it affects setting's state, > and we want to call use it as normally as possible. My suggestion is to move > the next three lines to just below the expect_enabled check instead. Done. https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:216: // when an origin is set via a switch. On 2013/09/26 18:09:44, mmenke wrote: > There is no preprocessor directive anyways in Chromium, right? May be worth > mentioning this only does anything useful in Chrome builds. Done. https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:222: DataReductionProxySettingsAndroid::Register(env); On 2013/09/26 18:09:44, mmenke wrote: > optional: May want to just move these two lines into the test fixture. Done. https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:383: } On 2013/09/26 18:09:44, mmenke wrote: > We never test OnIPAddressChanged, which follows a slightly different path. I'd > like to see it switch state from enabled -> enabled -> disabled -> disabled -> > enabled, so we get all possible state transitions, just to be safe. Done. https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:383: } On 2013/09/26 18:09:44, mmenke wrote: > Also, none of the tests check that when prefs::kSpdyProxyAuthEnabled is changed, > we get a notification and do everything we should Done. https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:383: } On 2013/09/26 18:09:44, mmenke wrote: > Also, none of these tests call InitDataReductionProxySettings, which is normally > called first. Done.
You forgot to upload. On 2013/09/27 00:28:12, bengr1 wrote: > https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... > File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc > (right): > > https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... > chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:378: > StringPrintf("shExpMatch(%s, \"%s\")", > On 2013/09/26 18:09:44, mmenke wrote: > > Rather than using \", why not just use single quotes in all Javascript > strings? > > Google style guide also states to use single quotes in Javascript (To mix it > > with HTML happily, but the same reasonable applies with C++ as well). > > Done. > > https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... > chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:534: // We > include days from the end of the list going backwards. > On 2013/09/26 18:09:44, mmenke wrote: > > nit: Don't use "we" in comments. Can just remove the word and capitalize the > > next word. > > Done. > > https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... > chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:557: // > Configure to max_retries at most kMaxRetries times for 5xx errors. > On 2013/09/26 18:09:44, mmenke wrote: > > nit: Fix grammar. "Configure max_retries to be at most..." (Or maybe use > "max > > retries" instead?) > > Done. > > https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... > chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:577: // > only process HTTP traffic, so we concoct a PAC configuration > On 2013/09/26 18:09:44, mmenke wrote: > > nit: Don't use "We" in comments, due to ambiguity of who it's referring to. > > Done. > > https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... > File > chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc > (right): > > https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... > chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:73: > int fake_fetcher_request_count() { > On 2013/09/26 18:09:44, mmenke wrote: > > nit: const > > Done. > > https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... > chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:83: > PrefService* local_state_prefs_for_testing_; > On 2013/09/26 18:09:44, mmenke wrote: > > nit: Now that these are only in test code, the "for_testing_" isn't needed. > > Done. > > https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... > chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:98: > chrome_variations::testing::ClearAllVariationParams(); > On 2013/09/26 18:09:44, mmenke wrote: > > Since we enable at the command line, I don't any of this actually does > anything. > > Done. > > https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... > chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:108: > } > On 2013/09/26 18:09:44, mmenke wrote: > > Again, doesn't seem to do anything. > > Done. > > https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... > chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:149: > dict->GetString("mode", &mode); > On 2013/09/26 18:09:44, mmenke wrote: > > Should probably assert on the result in these 4 GetString calls in this > function > > and the next. > > Done. > > https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... > chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:153: > } > On 2013/09/26 18:09:44, mmenke wrote: > > nit: Line break > > Done. > > https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... > chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:168: > settings_->AddDefaultProxyBypassRules(); > On 2013/09/26 18:09:44, mmenke wrote: > > I don't think we should be making this call, since it affects setting's state, > > and we want to call use it as normally as possible. My suggestion is to move > > the next three lines to just below the expect_enabled check instead. > > Done. > > https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... > chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:216: > // when an origin is set via a switch. > On 2013/09/26 18:09:44, mmenke wrote: > > There is no preprocessor directive anyways in Chromium, right? May be worth > > mentioning this only does anything useful in Chrome builds. > > Done. > > https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... > chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:222: > DataReductionProxySettingsAndroid::Register(env); > On 2013/09/26 18:09:44, mmenke wrote: > > optional: May want to just move these two lines into the test fixture. > > Done. > > https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... > chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:383: > } > On 2013/09/26 18:09:44, mmenke wrote: > > We never test OnIPAddressChanged, which follows a slightly different path. > I'd > > like to see it switch state from enabled -> enabled -> disabled -> disabled -> > > enabled, so we get all possible state transitions, just to be safe. > > Done. > > https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... > chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:383: > } > On 2013/09/26 18:09:44, mmenke wrote: > > Also, none of the tests check that when prefs::kSpdyProxyAuthEnabled is > changed, > > we get a notification and do everything we should > > Done. > > https://codereview.chromium.org/23458016/diff/109001/chrome/browser/net/spdyp... > chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:383: > } > On 2013/09/26 18:09:44, mmenke wrote: > > Also, none of these tests call InitDataReductionProxySettings, which is > normally > > called first. > > Done.
Uggh, I ran out before verifying that the upload completed. I got this: <h1>500 Server Error</h1> Sorry about that.
No problem. LGTM https://codereview.chromium.org/23458016/diff/134001/chrome/browser/net/spdyp... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/23458016/diff/134001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:330: LOG(WARNING) << "In OnIPAddressChanged"; Do we need all these LOGs? Prefer to remove them or have DLOGs, if we can get away with it. https://codereview.chromium.org/23458016/diff/134001/chrome/browser/net/spdyp... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc (right): https://codereview.chromium.org/23458016/diff/134001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:141: ProxyModeToString(ProxyPrefs::MODE_PAC_SCRIPT)); nit: -4 indent? https://codereview.chromium.org/23458016/diff/134001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:281: ProxyModeToString(ProxyPrefs::MODE_PAC_SCRIPT)); nit: -4 indent https://codereview.chromium.org/23458016/diff/134001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:402: CheckProbeOnIPChange(kProbeURLWithBadResponse, "Bad", true, false); Suggest a bad-> good transition, just to be complete.
https://codereview.chromium.org/23458016/diff/134001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettingsAndroid.java (right): https://codereview.chromium.org/23458016/diff/134001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettingsAndroid.java:10: public class DataReductionProxySettingsAndroid { Could you remove "Android" from the class name? It is redundant since this is Java and it is in the chrome/android. https://codereview.chromium.org/23458016/diff/134001/chrome/browser/net/spdyp... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/23458016/diff/134001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:63: if (command_line.HasSwitch(switches::kSpdyProxyAuthOrigin)) { Nit: return command_line.HasSwitch(switches::kSpdyProxyAuthOrigin); ?
https://chromiumcodereview.appspot.com/23458016/diff/134001/chrome/android/ja... File chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettingsAndroid.java (right): https://chromiumcodereview.appspot.com/23458016/diff/134001/chrome/android/ja... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettingsAndroid.java:100: /* Missing a second star /**
https://codereview.chromium.org/23458016/diff/134001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettingsAndroid.java (right): https://codereview.chromium.org/23458016/diff/134001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettingsAndroid.java:10: public class DataReductionProxySettingsAndroid { On 2013/10/02 16:36:22, nyquist wrote: > Could you remove "Android" from the class name? It is redundant since this is > Java and it is in the chrome/android. Done. https://codereview.chromium.org/23458016/diff/134001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettingsAndroid.java:100: /* On 2013/10/03 00:45:49, aurimas wrote: > Missing a second star /** Done. https://codereview.chromium.org/23458016/diff/134001/chrome/browser/net/spdyp... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/23458016/diff/134001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:63: if (command_line.HasSwitch(switches::kSpdyProxyAuthOrigin)) { On 2013/10/02 16:36:22, nyquist wrote: > Nit: return command_line.HasSwitch(switches::kSpdyProxyAuthOrigin); ? Done. https://codereview.chromium.org/23458016/diff/134001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:330: LOG(WARNING) << "In OnIPAddressChanged"; On 2013/09/27 17:10:14, mmenke wrote: > Do we need all these LOGs? Prefer to remove them or have DLOGs, if we can get > away with it. Done. Leaving those in was a mistake. https://codereview.chromium.org/23458016/diff/134001/chrome/browser/net/spdyp... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc (right): https://codereview.chromium.org/23458016/diff/134001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:141: ProxyModeToString(ProxyPrefs::MODE_PAC_SCRIPT)); On 2013/09/27 17:10:14, mmenke wrote: > nit: -4 indent? Done. https://codereview.chromium.org/23458016/diff/134001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:281: ProxyModeToString(ProxyPrefs::MODE_PAC_SCRIPT)); On 2013/09/27 17:10:14, mmenke wrote: > nit: -4 indent Done. https://codereview.chromium.org/23458016/diff/134001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:402: CheckProbeOnIPChange(kProbeURLWithBadResponse, "Bad", true, false); On 2013/09/27 17:10:14, mmenke wrote: > Suggest a bad-> good transition, just to be complete. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bengr@chromium.org/23458016/146001
Sorry for I got bad news for ya. Compile failed with a clobber build on win_x64_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bengr@chromium.org/23458016/146001
Sorry for I got bad news for ya. Compile failed with a clobber build on win. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bengr@chromium.org/23458016/176001
Message was sent while issue was closed.
Change committed as 227410 |
