Simple fallback implementation.

Simple http fallback for the data reduction proxy.

This CL adds a new command line option and preprocessor macro for a second proxy that the data reduction proxy feature will add to its configs as a fallback from the primary proxy. If the fallback proxy isn't defined, only the primary proxy is part of the configurations, and vice-versa. 

(Also fixes a bug where a http proxy specified on the command line would still be configured as HTTPS in the generated PAC file). 

BUG=306634
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=230728

[bengr, 2013-10-20 18:19:31]

https://codereview.chromium.org/30883003/diff/1/chrome/browser/net/spdyproxy/...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc (right):

https://codereview.chromium.org/30883003/diff/1/chrome/browser/net/spdyproxy/...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:153: std::string
DataReductionProxySettings::GetDataReductionProxyFallback() {
I think we should enforce that a fallback only has a value if the origin has a
value. If we don't do that, then you'll also have to go through the entire
codebase and change every 'defined(SPDY_PROXY_AUTH_ORIGIN)' to
'defined(SPDY_PROXY_AUTH_ORIGIN) || 'defined(DATA_REDUCTION_FALLBACK_HOST)'

https://codereview.chromium.org/30883003/diff/1/chrome/browser/net/spdyproxy/...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:157: #if
defined(DATA_REDUCTION_FALLBACK_HOST)
I hate names. If we are going to move over to using names that include "data
reduction," then I think we should do it everywhere. E.g., instead of
kSpdyProxyAuthFallback, we should instead use
"kDataReductionProxyServerFallback". Also, the switch for setting a proxy is
--proxy-server, so I'd love to have some consistency here.

Finally, I believe SPDY_PROXY_AUTH_ORIGIN requires a trailing slash whereas
--proxy-server might not. Again, we should be consistent, but if I recall
correctly, removing the slash will break auth.

https://codereview.chromium.org/30883003/diff/1/chrome/browser/net/spdyproxy/...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:163: 
We should have a different SPDY_PROXY_AUTH_VALUE for the fallback. This,
unfortunately, will require some changes to the auth logic.

https://codereview.chromium.org/30883003/diff/1/chrome/browser/net/spdyproxy/...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:163: 
We should have a different SPDY_PROXY_AUTH_VALUE for the fallback. This,
unfortunately, will require some changes to the auth logic.

https://codereview.chromium.org/30883003/diff/1/chrome/browser/net/spdyproxy/...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:332: && (
!main_proxy.empty() || !fallback_proxy.empty() );
Requiring both will be easier reason about.

https://codereview.chromium.org/30883003/diff/1/chrome/browser/net/spdyproxy/...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc
(right):

https://codereview.chromium.org/30883003/diff/1/chrome/browser/net/spdyproxy/...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:58:
std::string ProtocolAndHost(std::string origin) {
Please add a comment that describes the format of the return. It's a little
strange that it includes a semicolon.

[marq (ping after 24h), 2013-10-20 20:43:52]

Thanks, PTAL.

https://codereview.chromium.org/30883003/diff/1/chrome/browser/net/spdyproxy/...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc (right):

https://codereview.chromium.org/30883003/diff/1/chrome/browser/net/spdyproxy/...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:153: std::string
DataReductionProxySettings::GetDataReductionProxyFallback() {
On 2013/10/20 18:19:31, bengr1 wrote:
> I think we should enforce that a fallback only has a value if the origin has a
> value. If we don't do that, then you'll also have to go through the entire
> codebase and change every 'defined(SPDY_PROXY_AUTH_ORIGIN)' to
> 'defined(SPDY_PROXY_AUTH_ORIGIN) || 'defined(DATA_REDUCTION_FALLBACK_HOST)' 

Yeah, good point. When I do the Great Renaming CL I'll refactor things to
distinguish features (proxy, proxy authentication) from parameters (hosts,
keys), which might simplify details like this. For now I'll enforce that this
only returns a value if GetDataReductionProxy() does.

https://codereview.chromium.org/30883003/diff/1/chrome/browser/net/spdyproxy/...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:157: #if
defined(DATA_REDUCTION_FALLBACK_HOST)
On 2013/10/20 18:19:31, bengr1 wrote:
> I hate names. If we are going to move over to using names that include "data
> reduction," then I think we should do it everywhere. E.g., instead of
> kSpdyProxyAuthFallback, we should instead use
> "kDataReductionProxyServerFallback". Also, the switch for setting a proxy is
> --proxy-server, so I'd love to have some consistency here.
> 
> Finally, I believe SPDY_PROXY_AUTH_ORIGIN requires a trailing slash whereas
> --proxy-server might not. Again, we should be consistent, but if I recall
> correctly, removing the slash will break auth. 

This implementation mirrors the one for getDataReductionProxyOrigin(), so no new
brokenness should be introduced here (we might need to adjust the private clank
auth code in another CL).

https://codereview.chromium.org/30883003/diff/1/chrome/browser/net/spdyproxy/...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:163: 
On 2013/10/20 18:19:31, bengr1 wrote:
> We should have a different SPDY_PROXY_AUTH_VALUE for the fallback. This,
> unfortunately, will require some changes to the auth logic.

Ack.

https://codereview.chromium.org/30883003/diff/1/chrome/browser/net/spdyproxy/...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:332: && (
!main_proxy.empty() || !fallback_proxy.empty() );
On 2013/10/20 18:19:31, bengr1 wrote:
> Requiring both will be easier reason about.

Done.

[bengr, 2013-10-20 21:51:48]

lgtm, with nit. Please do not submit this code until you are sure that
authentication works correctly on both iOS and Android.

https://codereview.chromium.org/30883003/diff/1/chrome/browser/net/spdyproxy/...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc (right):

https://codereview.chromium.org/30883003/diff/1/chrome/browser/net/spdyproxy/...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:157: #if
defined(DATA_REDUCTION_FALLBACK_HOST)
On 2013/10/20 20:43:52, marq_ wrote:
> On 2013/10/20 18:19:31, bengr1 wrote:
> > I hate names. If we are going to move over to using names that include "data
> > reduction," then I think we should do it everywhere. E.g., instead of
> > kSpdyProxyAuthFallback, we should instead use
> > "kDataReductionProxyServerFallback". Also, the switch for setting a proxy is
> > --proxy-server, so I'd love to have some consistency here.
> > 
> > Finally, I believe SPDY_PROXY_AUTH_ORIGIN requires a trailing slash whereas
> > --proxy-server might not. Again, we should be consistent, but if I recall
> > correctly, removing the slash will break auth. 
> 
> This implementation mirrors the one for getDataReductionProxyOrigin(), so no
new
> brokenness should be introduced here (we might need to adjust the private
clank
> auth code in another CL). 

Let's verify that we don't also need to adjust chromium auth code.

https://codereview.chromium.org/30883003/diff/60001/chrome/common/chrome_swit...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/30883003/diff/60001/chrome/common/chrome_swit...
chrome/common/chrome_switches.cc:1345: // Second origin that can be used for the
proxy.
for what proxy?

[marq (ping after 24h), 2013-10-22 00:41:18]

Okay. This is the meat of how I'd like to handle the authentication change.
Here's the overview; right now some of the unit tests aren't passing and I need
to add some more, but the overall structure of the change is reviewable.

- GetDataReductionProxyAuth() is gone. No code outside of the base settings
class needs to know about that value.

- There's now GetDataReductionProxies() which returns a vector of all the
configured proxies. The methods that configure proxies don't use this yet, but
they probably should.

- There's now an InitDataReductionProxySession, which can replace
ChromeDataReductionProxyAndroid::Init(). The change to profile_io_data to call
this new method still needs to be made. InitDataReductionProxySession adds the
auth key for each configured proxy.

- The private method AuthHashForSalt() handles the hashing. If
SPDY_PROXY_AUTH_VALUE isn't defined, it returns an empty string. That's as far
as the auth key is exposed in the API.

Two methods handle http auth challenges. Both accept net::AuthChallengeInfo
objects, which is what the network stack generates. The Java API instead just
uses host and realm strings, so the Android settings class recreates
AuthChallengeInfo object to pass in.

IsAcceptableAuthChallenge() determines if the challenge has the right realm and
has a host matching any of the configured servers. If not, calling code should
attempt other authentication methods.

GetTokenForAuthChallenge() generates the hashed token response for the
challenge; if that can't be done (for example, if the salt portion of the realm
isn't an int), it returns an empty string. Calling code should cancel
authentication if this happens.

- DataReductionProxySettings.java was updated.

[bengr, 2013-10-22 01:02:08]

I think what you've done makes sense. Here are some nits. I'll do a more serious
review once the tests work.

https://codereview.chromium.org/30883003/diff/190001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc (right):

https://codereview.chromium.org/30883003/diff/190001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:216:
kAuthenticationRealmName)
I'd move from '0, strlen' down to new line. indent +4 from if.

https://codereview.chromium.org/30883003/diff/190001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:222: it !=
proxies.end(); ++it) {
indent +1

https://codereview.chromium.org/30883003/diff/190001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:244: //
TODO(marq): Can we just deprecate unsalted auth? If so, remove this.
Yes.

https://codereview.chromium.org/30883003/diff/190001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h (right):

https://codereview.chromium.org/30883003/diff/190001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h:23: class
HttpNetworkSession;
alphabetize.

https://codereview.chromium.org/30883003/diff/190001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h:55: void
InitDataReductionProxySession(net::HttpNetworkSession* session);
Add a comment.

https://codereview.chromium.org/30883003/diff/190001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h:87:
DataReductionProxyList GetDataReductionProxies();
Comment.

https://codereview.chromium.org/30883003/diff/190001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h:89: bool
IsAcceptableAuthChallenge(net::AuthChallengeInfo* auth_info);
Comment.

https://codereview.chromium.org/30883003/diff/190001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h:90: base::string16
GetTokenForAuthChallenge(net::AuthChallengeInfo* auth_info);
Comment.

https://codereview.chromium.org/30883003/diff/190001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h:201: base::string16
AuthHashForSalt(int64 salt);
Comment.

https://codereview.chromium.org/30883003/diff/190001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h
(right):

https://codereview.chromium.org/30883003/diff/190001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:66:
ScopedJavaLocalRef<jstring> GetTokenForAuthChallenge(JNIEnv* env,
add blank line.

[marq (ping after 24h), 2013-10-22 14:44:20]

Tests passing now, more to come. PTAL.

https://codereview.chromium.org/30883003/diff/190001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc (right):

https://codereview.chromium.org/30883003/diff/190001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:216:
kAuthenticationRealmName)
On 2013/10/22 01:02:09, bengr1 wrote:
> I'd move from '0, strlen' down to new line. indent +4 from if.

Rewrote for clarity.

https://codereview.chromium.org/30883003/diff/190001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:222: it !=
proxies.end(); ++it) {
On 2013/10/22 01:02:09, bengr1 wrote:
> indent +1

Done.

https://codereview.chromium.org/30883003/diff/190001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:244: //
TODO(marq): Can we just deprecate unsalted auth? If so, remove this.
On 2013/10/22 01:02:09, bengr1 wrote:
> Yes.

Done.

https://codereview.chromium.org/30883003/diff/190001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h (right):

https://codereview.chromium.org/30883003/diff/190001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h:23: class
HttpNetworkSession;
On 2013/10/22 01:02:09, bengr1 wrote:
> alphabetize.

? Au < Ho < Ht, right?

https://codereview.chromium.org/30883003/diff/190001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h:55: void
InitDataReductionProxySession(net::HttpNetworkSession* session);
On 2013/10/22 01:02:09, bengr1 wrote:
> Add a comment.

Done.

https://codereview.chromium.org/30883003/diff/190001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h:87:
DataReductionProxyList GetDataReductionProxies();
On 2013/10/22 01:02:09, bengr1 wrote:
> Comment.

Done.

https://codereview.chromium.org/30883003/diff/190001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h:89: bool
IsAcceptableAuthChallenge(net::AuthChallengeInfo* auth_info);
On 2013/10/22 01:02:09, bengr1 wrote:
> Comment.

Done.

https://codereview.chromium.org/30883003/diff/190001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h:90: base::string16
GetTokenForAuthChallenge(net::AuthChallengeInfo* auth_info);
On 2013/10/22 01:02:09, bengr1 wrote:
> Comment.

Done.

https://codereview.chromium.org/30883003/diff/190001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h:201: base::string16
AuthHashForSalt(int64 salt);
On 2013/10/22 01:02:09, bengr1 wrote:
> Comment.

Done.

https://codereview.chromium.org/30883003/diff/190001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h
(right):

https://codereview.chromium.org/30883003/diff/190001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:66:
ScopedJavaLocalRef<jstring> GetTokenForAuthChallenge(JNIEnv* env,
On 2013/10/22 01:02:09, bengr1 wrote:
> add blank line.

Done.

[bengr, 2013-10-22 17:49:30]

https://codereview.chromium.org/30883003/diff/190001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h (right):

https://codereview.chromium.org/30883003/diff/190001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h:23: class
HttpNetworkSession;
On 2013/10/22 14:44:20, marq_ wrote:
> On 2013/10/22 01:02:09, bengr1 wrote:
> > alphabetize.
> 
> ? Au < Ho < Ht, right?

Right. I don't know what I was looking at.

https://codereview.chromium.org/30883003/diff/270001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java
(right):

https://codereview.chromium.org/30883003/diff/270001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:135:
* Returns true if the host and realm describe an appropriate authentication
challenge
I don't know what this comment means. Please clarify. How does a host and realm
describe anything?

https://codereview.chromium.org/30883003/diff/270001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:138:
public boolean isAcceptableAuthChallenge(String host, String realm) {
What makes it an appropriate challenge?

https://codereview.chromium.org/30883003/diff/270001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:143:
* Returns a string suitable for sending back to the data reduction proxy server
to act as an
Returns an authentication token for the data reduction proxy.

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc (right):

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:54: // TODO(marq):
factor this string out into a constant here and in
Factor

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:119:
net::HttpNetworkSession* session) {
indent +2

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:120: // This is a
no-op unless the authentication params are compiled in.
parameters

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:121: // (even
though values for them may be specified on the command line).
It's probably worth saying that when specified in a switch, authentication will
still work, but will be less performant, because it will require a
challenge/response round trip.

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:125:
(base::Time::Now() - base::Time::UnixEpoch()).InMilliseconds() / 1000;
indent -1

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:130:
net::HostPortPair auth_origin = *it;
const ref?

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:362: std::string
DataReductionProxySettings::GetDefaultProxyHost() {
Why isn't the handling of command line switch values in here and below?

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:372: return
DATA_REDUCTION_FALLBACK_HOST;
why not check in this function if GetDefaultProxyHost() returns anything?

Better yet, why not just have one function that returns both?

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h (right):

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h:89: // Returns a
vector of HostPortPairs for all configured proxies.
It seems a little strange to return a vector when we have at most a pair.

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc
(right):

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:182: //
token being empty and return a java null.
I don't understand this comment.

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc
(right):

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc:58:
virtual PrefService* GetOriginalProfilePrefs() OVERRIDE {
Can you use MOCK_METHOD0 for all of these overrides? Then you could test whether
they are called as expected.

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc:264:
std::string salt = "8675309";  // Jenny's number to test the hash generator.
:)

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc:311: {"",
"SpdyProxy1234567", false},        // 7. No challenger.
align

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc:327: //
More tests:
Why is this comment here?

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc:494:
AddProxyToCommandLine();
?

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
File
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc
(right):

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:58:
virtual PrefService* GetOriginalProfilePrefs() OVERRIDE {
Use MOCKs? Also this seems awfully similar to the test class for the base class.
Is it possible to combine?

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:66:
virtual std::string GetDefaultProxyHost() OVERRIDE { return std::string(); }
move return to new line.

[marq (ping after 24h), 2013-10-22 21:18:01]

Thanks for the feedback.

One notable change in this patch: GetDataReductionProxies() returns a vector of
GURLs instead of HostPortPairs. 

The two major changes you suggested that I'll get to later are using a pair
instead of a vector, and using proper MOCKs.

https://codereview.chromium.org/30883003/diff/270001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java
(right):

https://codereview.chromium.org/30883003/diff/270001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:135:
* Returns true if the host and realm describe an appropriate authentication
challenge
On 2013/10/22 17:49:30, bengr1 wrote:
> I don't know what this comment means. Please clarify. How does a host and
realm
> describe anything?

Done.

https://codereview.chromium.org/30883003/diff/270001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:138:
public boolean isAcceptableAuthChallenge(String host, String realm) {
On 2013/10/22 17:49:30, bengr1 wrote:
> What makes it an appropriate challenge? 

Done.

https://codereview.chromium.org/30883003/diff/270001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:143:
* Returns a string suitable for sending back to the data reduction proxy server
to act as an
On 2013/10/22 17:49:30, bengr1 wrote:
> Returns an authentication token for the data reduction proxy.

Done.

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc (right):

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:54: // TODO(marq):
factor this string out into a constant here and in
On 2013/10/22 17:49:30, bengr1 wrote:
> Factor

Done.

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:119:
net::HttpNetworkSession* session) {
On 2013/10/22 17:49:30, bengr1 wrote:
> indent +2

Done.

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:120: // This is a
no-op unless the authentication params are compiled in.
On 2013/10/22 17:49:30, bengr1 wrote:
> parameters

Done.

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:121: // (even
though values for them may be specified on the command line).
On 2013/10/22 17:49:30, bengr1 wrote:
> It's probably worth saying that when specified in a switch, authentication
will
> still work, but will be less performant, because it will require a
> challenge/response round trip.

Done.

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:125:
(base::Time::Now() - base::Time::UnixEpoch()).InMilliseconds() / 1000;
On 2013/10/22 17:49:30, bengr1 wrote:
> indent -1

Done.

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:130:
net::HostPortPair auth_origin = *it;
On 2013/10/22 17:49:30, bengr1 wrote:
> const ref?

Yes, but the code's changed slightly.

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:362: std::string
DataReductionProxySettings::GetDefaultProxyHost() {
On 2013/10/22 17:49:30, bengr1 wrote:
> Why isn't the handling of command line switch values in here and below?

These two methods are used so that unit tests can override them and bypass the
preprocessor flags. Otherwise there's no way to test cases where there are no
proxies configured.

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:372: return
DATA_REDUCTION_FALLBACK_HOST;
On 2013/10/22 17:49:30, bengr1 wrote:
> why not check in this function if GetDefaultProxyHost() returns anything?

I don't think the default value here is of necessity dependent on that.

> Better yet, why not just have one function that returns both?

Ah, maybe. I'll roll that into changing to use a pair instead of a vector.

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h (right):

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.h:89: // Returns a
vector of HostPortPairs for all configured proxies.
On 2013/10/22 17:49:30, bengr1 wrote:
> It seems a little strange to return a vector when we have at most a pair.

It's somewhat cleaner (to my mind) to write generic code that operates on one or
two objects using a vector than to repeatedly check if there's a second object
or not. I'll see if the pair looks less awkward to me.

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc
(right):

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:182: //
token being empty and return a java null.
On 2013/10/22 17:49:30, bengr1 wrote:
> I don't understand this comment.

The superclass method returns base::string16() to indicate a token couldn't be
generated.

The calling java code tests for token == null for the same condition.

If ConvertUTF16ToJavaString(env, base::string16()) returns a java null, then all
is well. If not, this needs to change.

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc
(right):

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc:58:
virtual PrefService* GetOriginalProfilePrefs() OVERRIDE {
On 2013/10/22 17:49:30, bengr1 wrote:
> Can you use MOCK_METHOD0 for all of these overrides? Then you could test
whether
> they are called as expected.

I was not previously aware of that, and that will actually get rid of some of
the uglier duplication between the test classes. In the interests of our
short-term goals, I'll restructure the tests to use mocks in a later patch.

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc:264:
std::string salt = "8675309";  // Jenny's number to test the hash generator.
On 2013/10/22 17:49:30, bengr1 wrote:
> :)

Ack :-)

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc:311: {"",
"SpdyProxy1234567", false},        // 7. No challenger.
On 2013/10/22 17:49:30, bengr1 wrote:
> align

Done.

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc:327: //
More tests:
On 2013/10/22 17:49:30, bengr1 wrote:
> Why is this comment here?

To remind me to write more tests. It's gone now and instead there are more
tests.

I realize that reviewing code that's still in progress isn't ideal.

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc:494:
AddProxyToCommandLine();
On 2013/10/22 17:49:30, bengr1 wrote:
> ?

Now gone.

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
File
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc
(right):

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:58:
virtual PrefService* GetOriginalProfilePrefs() OVERRIDE {
On 2013/10/22 17:49:30, bengr1 wrote:
> Use MOCKs? Also this seems awfully similar to the test class for the base
class.
> Is it possible to combine?

MOCKS: See previous comment.

I tried. The obvious way would seem to be to have this class inherit from both
DataReductionProxySettingsAndroid and TestDataReductionProxySettings, but I
wasn't able to get that to work.

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:66:
virtual std::string GetDefaultProxyHost() OVERRIDE { return std::string(); }
On 2013/10/22 17:49:30, bengr1 wrote:
> move return to new line.

Done.

[bengr, 2013-10-22 21:44:21]

lgtm, with nits, and of course don't submit until Android and iOS function as
expected.

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc
(right):

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc:58:
virtual PrefService* GetOriginalProfilePrefs() OVERRIDE {
On 2013/10/22 21:18:01, marq_ wrote:
> On 2013/10/22 17:49:30, bengr1 wrote:
> > Can you use MOCK_METHOD0 for all of these overrides? Then you could test
> whether
> > they are called as expected.
> 
> I was not previously aware of that, and that will actually get rid of some of
> the uglier duplication between the test classes. In the interests of our
> short-term goals, I'll restructure the tests to use mocks in a later patch.

Please add a TODO here and in all locations where you have planned changes.

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
File
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc
(right):

https://codereview.chromium.org/30883003/diff/270001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:58:
virtual PrefService* GetOriginalProfilePrefs() OVERRIDE {
On 2013/10/22 21:18:01, marq_ wrote:
> On 2013/10/22 17:49:30, bengr1 wrote:
> > Use MOCKs? Also this seems awfully similar to the test class for the base
> class.
> > Is it possible to combine?
> 
> MOCKS: See previous comment.
> 
> I tried. The obvious way would seem to be to have this class inherit from both
> DataReductionProxySettingsAndroid and TestDataReductionProxySettings, but I
> wasn't able to get that to work.

Hmm. Add a TODO to revisit.

https://codereview.chromium.org/30883003/diff/450001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc
(right):

https://codereview.chromium.org/30883003/diff/450001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc:240:
proxies.begin();
indent +3, i think.

https://codereview.chromium.org/30883003/diff/450001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc:253: 
Remove blank line.

https://codereview.chromium.org/30883003/diff/450001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc:279:
EXPECT_EQ("foo.com",proxies[0].host());
Here and in the three lines below, add a space after the comma.

https://codereview.chromium.org/30883003/diff/450001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc:341: for
(int i = 0; i <= 11; i++) {
++i

https://codereview.chromium.org/30883003/diff/450001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc:362:
{"SpdyProxy1234567", false},// 4. OK
Shift the comments all +1

https://codereview.chromium.org/30883003/diff/450001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc:365: for
(int i = 0; i <= 4; i++) {
++i

[marq (ping after 24h), 2013-10-23 00:32:02]

Last set of changes; this includes everything to make the auth cache
prepopulation work.

[bengr, 2013-10-23 01:02:44]

still lgtm, still outstanding nits from #6

[bengr, 2013-10-23 01:03:00]

On 2013/10/23 01:02:44, bengr1 wrote:
> still lgtm, still outstanding nits from #6

I mean #5

[bengr, 2013-10-23 01:38:15]

https://codereview.chromium.org/30883003/diff/330001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc (right):

https://codereview.chromium.org/30883003/diff/330001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:152:
crypto::RandBytes(&rand3, sizeof(rand3));
would it be possible to have something like: int32 rand[3];
crypto::RandBytes(rand, arraysize(rand)); then just use rand[0], rand[1],
rand[2] directly?

[marq (ping after 24h), 2013-10-23 12:13:53]

On 2013/10/23 01:38:15, bengr1 wrote:
>
https://codereview.chromium.org/30883003/diff/330001/chrome/browser/net/spdyp...
> File chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc (right):
> 
>
https://codereview.chromium.org/30883003/diff/330001/chrome/browser/net/spdyp...
> chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:152:
> crypto::RandBytes(&rand3, sizeof(rand3));
> would it be possible to have something like: int32 rand[3];
> crypto::RandBytes(rand, arraysize(rand)); then just use rand[0], rand[1],
> rand[2] directly?

That didn't work (the array entries after the first weren't randomized), and
it's not enough of a win to fiddle with.

[marq (ping after 24h), 2013-10-23 12:23:20]

Final tweaks, thanks.

https://codereview.chromium.org/30883003/diff/450001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc
(right):

https://codereview.chromium.org/30883003/diff/450001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc:240:
proxies.begin();
On 2013/10/22 21:44:22, bengr1 wrote:
> indent +3, i think.

Done.

https://codereview.chromium.org/30883003/diff/450001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc:253: 
On 2013/10/22 21:44:22, bengr1 wrote:
> Remove blank line.

Done.

https://codereview.chromium.org/30883003/diff/450001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc:279:
EXPECT_EQ("foo.com",proxies[0].host());
On 2013/10/22 21:44:22, bengr1 wrote:
> Here and in the three lines below, add a space after the comma.

Done.

https://codereview.chromium.org/30883003/diff/450001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc:341: for
(int i = 0; i <= 11; i++) {
On 2013/10/22 21:44:22, bengr1 wrote:
> ++i

Done.

https://codereview.chromium.org/30883003/diff/450001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc:362:
{"SpdyProxy1234567", false},// 4. OK
On 2013/10/22 21:44:22, bengr1 wrote:
> Shift the comments all +1

Done.

https://codereview.chromium.org/30883003/diff/450001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest.cc:365: for
(int i = 0; i <= 4; i++) {
On 2013/10/22 21:44:22, bengr1 wrote:
> ++i

Done.

https://codereview.chromium.org/30883003/diff/330001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc (right):

https://codereview.chromium.org/30883003/diff/330001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:152:
crypto::RandBytes(&rand3, sizeof(rand3));
On 2013/10/23 01:38:16, bengr1 wrote:
> would it be possible to have something like: int32 rand[3];
> crypto::RandBytes(rand, arraysize(rand)); then just use rand[0], rand[1],
> rand[2] directly?

That doesn't randomize the last two elements of the array, but RandBytes(rand, 3
* sizeof(rand[0])) does.

[marq (ping after 24h), 2013-10-23 12:36:57]

Adding OWNERS:

+willchan for profile_impl_io_data.cc
+dtrainor@chromium.org for chrome/app/android/... and chrome/android/..

We're pushing to have this through the CQ ASAP, so your prompt attention is very
much appreciated; the changes requiring your approval are a small part of this
CL. Please contact me (marq@) if you have any questions or concerns.

Summary of changes to these files:

profile_impl_io_data.cc: Exchange a deprecated call to ChromeDataReductionProxy
for a call to a static method on DataReductionProxySettings, and enable this for
iOS.

chrome_data_reduction_proxy_android.cc/h,
chrome_data_reduction_proxy_testshell_android.cc, and associated .gyp files: The
ChromeDataReductionProxy class and associated test shell are removed.

DataReductionProxySettings.java: Update interfaces to match changes to
underlying C++ class.

[David Trainor- moved to gerrit, 2013-10-23 16:12:37]

chrome/android and chrome/app/android lgtm

https://codereview.chromium.org/30883003/diff/930001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java
(right):

https://codereview.chromium.org/30883003/diff/930001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:135:
* Returns true if the host and realm (as passed in to
Tab.onReceivedHttpAuthRequest()) are such
Update javadoc: @return.  Add @param for the parameters.

https://codereview.chromium.org/30883003/diff/930001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:149:
public String getTokenForAuthChallenge(String host, String realm) {
Add @param for parameters.

[marq (ping after 24h), 2013-10-23 16:28:49]

dtrainor: Thanks for the quick review!

https://codereview.chromium.org/30883003/diff/930001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java
(right):

https://codereview.chromium.org/30883003/diff/930001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:135:
* Returns true if the host and realm (as passed in to
Tab.onReceivedHttpAuthRequest()) are such
On 2013/10/23 16:12:38, David Trainor wrote:
> Update javadoc: @return.  Add @param for the parameters.

Done.

https://codereview.chromium.org/30883003/diff/930001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:149:
public String getTokenForAuthChallenge(String host, String realm) {
On 2013/10/23 16:12:38, David Trainor wrote:
> Add @param for parameters.

Done.

[marq (ping after 24h), 2013-10-23 16:30:54]

-willchan, +mmenke as OWNER for profile_impl_io_data.cc

Everything else in this CL is reviewed and ready to commit, we just need an LGTM
ASAP on profile_impl_io_data.cc.

The extend of the change is just to exchange a deprecated call to
ChromeDataReductionProxy for a call to a static method on
DataReductionProxySettings, and enable this for iOS.

[mmenke, 2013-10-23 17:46:10]

On 2013/10/23 16:30:54, marq_ wrote:
> -willchan, +mmenke as OWNER for profile_impl_io_data.cc
> 
> Everything else in this CL is reviewed and ready to commit, we just need an
LGTM
> ASAP on profile_impl_io_data.cc.
> 
> The extend of the change is just to exchange a deprecated call to
> ChromeDataReductionProxy for a call to a static method on
> DataReductionProxySettings, and enable this for iOS.

LGTM, assuming we do actually compile ProfileIOData on iOS.

[marq (ping after 24h), 2013-10-23 17:51:08]

On 2013/10/23 17:46:10, mmenke wrote:
> On 2013/10/23 16:30:54, marq_ wrote:
> > -willchan, +mmenke as OWNER for profile_impl_io_data.cc
> > 
> > Everything else in this CL is reviewed and ready to commit, we just need an
> LGTM
> > ASAP on profile_impl_io_data.cc.
> > 
> > The extend of the change is just to exchange a deprecated call to
> > ChromeDataReductionProxy for a call to a static method on
> > DataReductionProxySettings, and enable this for iOS.
> 
> LGTM, assuming we do actually compile ProfileIOData on iOS.

We do. Thanks for the fast reply!

[commit-bot: I haz the power, 2013-10-23 18:28:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marq@chromium.org/30883003/990001

[commit-bot: I haz the power, 2013-10-24 01:02:38]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-10-24 14:30:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marq@chromium.org/30883003/990001

[commit-bot: I haz the power, 2013-10-24 15:54:41]

Change committed as 230728