Remove round trip from data reduction proxy authentication

Remove round trip from data reduction proxy authentication

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

[bengr (incorrect), 2013-05-20 21:25:11]

yfriedman: chrome/android/testshell/*, chrome/app/android/*,
chrome_android.gypi.

mmenke: profile_impl_io_data.cc

cbentzel: http_auth_controller.cc

[Yaron, 2013-05-21 18:04:29]

https://codereview.chromium.org/14994003/diff/6001/chrome/android/testshell/c...
File chrome/android/testshell/chrome_data_reduction_proxy_testshell_android.h
(right):

https://codereview.chromium.org/14994003/diff/6001/chrome/android/testshell/c...
chrome/android/testshell/chrome_data_reduction_proxy_testshell_android.h:10:
class ChromeDataReductionProxyTestShellAndroid
TestShell and Android seems kinda redundant but I see that another file has done
something similar.

https://codereview.chromium.org/14994003/diff/6001/chrome/app/android/chrome_...
File chrome/app/android/chrome_data_reduction_proxy_android.cc (right):

https://codereview.chromium.org/14994003/diff/6001/chrome/app/android/chrome_...
chrome/app/android/chrome_data_reduction_proxy_android.cc:9: //
ChromeMainDelegateAndroid is created when the library is loaded. It is always
?

https://codereview.chromium.org/14994003/diff/6001/chrome/app/android/chrome_...
File chrome/app/android/chrome_data_reduction_proxy_android.h (right):

https://codereview.chromium.org/14994003/diff/6001/chrome/app/android/chrome_...
chrome/app/android/chrome_data_reduction_proxy_android.h:1: // Copyright (c)
2013 The Chromium Authors. All rights reserved.
I believe no "(c)" is the new hawtness

https://codereview.chromium.org/14994003/diff/6001/chrome/app/android/chrome_...
chrome/app/android/chrome_data_reduction_proxy_android.h:10: class
ChromeDataReductionProxyAndroid {
Brief comment please

https://codereview.chromium.org/14994003/diff/6001/chrome/app/android/chrome_...
chrome/app/android/chrome_data_reduction_proxy_android.h:14: virtual void
Init(net::HttpNetworkSession* session) const;
I think you can just make this pure virtual (i.e. = 0) and avoid the definition.

https://codereview.chromium.org/14994003/diff/6001/chrome/app/android/chrome_...
chrome/app/android/chrome_data_reduction_proxy_android.h:20: 
nit: extra ws line

https://codereview.chromium.org/14994003/diff/6001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/14994003/diff/6001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl_io_data.cc:18: #if defined(OS_ANDROID)
Conditional if-defs go at the bottom

[Yaron, 2013-05-21 18:04:30]

https://codereview.chromium.org/14994003/diff/6001/chrome/android/testshell/c...
File chrome/android/testshell/chrome_data_reduction_proxy_testshell_android.h
(right):

https://codereview.chromium.org/14994003/diff/6001/chrome/android/testshell/c...
chrome/android/testshell/chrome_data_reduction_proxy_testshell_android.h:10:
class ChromeDataReductionProxyTestShellAndroid
TestShell and Android seems kinda redundant but I see that another file has done
something similar.

https://codereview.chromium.org/14994003/diff/6001/chrome/app/android/chrome_...
File chrome/app/android/chrome_data_reduction_proxy_android.cc (right):

https://codereview.chromium.org/14994003/diff/6001/chrome/app/android/chrome_...
chrome/app/android/chrome_data_reduction_proxy_android.cc:9: //
ChromeMainDelegateAndroid is created when the library is loaded. It is always
?

https://codereview.chromium.org/14994003/diff/6001/chrome/app/android/chrome_...
File chrome/app/android/chrome_data_reduction_proxy_android.h (right):

https://codereview.chromium.org/14994003/diff/6001/chrome/app/android/chrome_...
chrome/app/android/chrome_data_reduction_proxy_android.h:1: // Copyright (c)
2013 The Chromium Authors. All rights reserved.
I believe no "(c)" is the new hawtness

https://codereview.chromium.org/14994003/diff/6001/chrome/app/android/chrome_...
chrome/app/android/chrome_data_reduction_proxy_android.h:10: class
ChromeDataReductionProxyAndroid {
Brief comment please

https://codereview.chromium.org/14994003/diff/6001/chrome/app/android/chrome_...
chrome/app/android/chrome_data_reduction_proxy_android.h:14: virtual void
Init(net::HttpNetworkSession* session) const;
I think you can just make this pure virtual (i.e. = 0) and avoid the definition.

https://codereview.chromium.org/14994003/diff/6001/chrome/app/android/chrome_...
chrome/app/android/chrome_data_reduction_proxy_android.h:20: 
nit: extra ws line

https://codereview.chromium.org/14994003/diff/6001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/14994003/diff/6001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl_io_data.cc:18: #if defined(OS_ANDROID)
Conditional if-defs go at the bottom

[mmenke, 2013-05-22 15:55:50]

https://codereview.chromium.org/14994003/diff/6001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/14994003/diff/6001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl_io_data.cc:427: #endif
Is there a reason not to just do this all in a static function?

[mmenke, 2013-05-22 16:02:54]

Oh, and Chris is off for the rest of the week.  Probably best to have asanka
review the auth controller changes.

[bengr, 2013-05-22 19:03:15]

asanka: could you take a look at http_auth_controller.cc

https://codereview.chromium.org/14994003/diff/6001/chrome/android/testshell/c...
File chrome/android/testshell/chrome_data_reduction_proxy_testshell_android.h
(right):

https://codereview.chromium.org/14994003/diff/6001/chrome/android/testshell/c...
chrome/android/testshell/chrome_data_reduction_proxy_testshell_android.h:10:
class ChromeDataReductionProxyTestShellAndroid
Yes, I was following the existing naming scheme.

On 2013/05/21 18:04:30, Yaron wrote:
> TestShell and Android seems kinda redundant but I see that another file has
done
> something similar.

https://codereview.chromium.org/14994003/diff/6001/chrome/app/android/chrome_...
File chrome/app/android/chrome_data_reduction_proxy_android.cc (right):

https://codereview.chromium.org/14994003/diff/6001/chrome/app/android/chrome_...
chrome/app/android/chrome_data_reduction_proxy_android.cc:9: //
ChromeMainDelegateAndroid is created when the library is loaded. It is always
On 2013/05/21 18:04:30, Yaron wrote:
> ?

D'oh. Done.

https://codereview.chromium.org/14994003/diff/6001/chrome/app/android/chrome_...
File chrome/app/android/chrome_data_reduction_proxy_android.h (right):

https://codereview.chromium.org/14994003/diff/6001/chrome/app/android/chrome_...
chrome/app/android/chrome_data_reduction_proxy_android.h:1: // Copyright (c)
2013 The Chromium Authors. All rights reserved.
On 2013/05/21 18:04:30, Yaron wrote:
> I believe no "(c)" is the new hawtness

Done.

https://codereview.chromium.org/14994003/diff/6001/chrome/app/android/chrome_...
chrome/app/android/chrome_data_reduction_proxy_android.h:10: class
ChromeDataReductionProxyAndroid {
On 2013/05/21 18:04:30, Yaron wrote:
> Brief comment please

Done.

https://codereview.chromium.org/14994003/diff/6001/chrome/app/android/chrome_...
chrome/app/android/chrome_data_reduction_proxy_android.h:14: virtual void
Init(net::HttpNetworkSession* session) const;
On 2013/05/21 18:04:30, Yaron wrote:
> I think you can just make this pure virtual (i.e. = 0) and avoid the
definition.

Done.

https://codereview.chromium.org/14994003/diff/6001/chrome/app/android/chrome_...
chrome/app/android/chrome_data_reduction_proxy_android.h:20: 
On 2013/05/21 18:04:30, Yaron wrote:
> nit: extra ws line

Done.

https://codereview.chromium.org/14994003/diff/6001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/14994003/diff/6001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl_io_data.cc:18: #if defined(OS_ANDROID)
On 2013/05/21 18:04:30, Yaron wrote:
> Conditional if-defs go at the bottom

Done.

https://codereview.chromium.org/14994003/diff/6001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl_io_data.cc:427: #endif
On 2013/05/22 15:55:50, mmenke wrote:
> Is there a reason not to just do this all in a static function?

I was imagining that ChromeDataReductionProxy would eventually construct
credentials for challenges as well, and would cache the results. This CL doesn't
require the objects, so I'm happy to modify the code if you think that's best.
Let me know.

[mmenke, 2013-05-22 19:20:12]

https://codereview.chromium.org/14994003/diff/6001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/14994003/diff/6001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl_io_data.cc:427: #endif
On 2013/05/22 19:03:15, bengr1 wrote:
> On 2013/05/22 15:55:50, mmenke wrote:
> > Is there a reason not to just do this all in a static function?
> 
> I was imagining that ChromeDataReductionProxy would eventually construct
> credentials for challenges as well, and would cache the results. This CL
doesn't
> require the objects, so I'm happy to modify the code if you think that's best.
> Let me know.

Since this is an "eventually", and it currently seems a little weird as-is,
let's just switch to a static function for now.

[bengr (incorrect), 2013-05-22 21:25:58]

PTAL.

On 2013/05/22 19:20:12, mmenke wrote:
>
https://codereview.chromium.org/14994003/diff/6001/chrome/browser/profiles/pr...
> File chrome/browser/profiles/profile_impl_io_data.cc (right):
> 
>
https://codereview.chromium.org/14994003/diff/6001/chrome/browser/profiles/pr...
> chrome/browser/profiles/profile_impl_io_data.cc:427: #endif
> On 2013/05/22 19:03:15, bengr1 wrote:
> > On 2013/05/22 15:55:50, mmenke wrote:
> > > Is there a reason not to just do this all in a static function?
> > 
> > I was imagining that ChromeDataReductionProxy would eventually construct
> > credentials for challenges as well, and would cache the results. This CL
> doesn't
> > require the objects, so I'm happy to modify the code if you think that's
best.
> > Let me know.
> 
> Since this is an "eventually", and it currently seems a little weird as-is,
> let's just switch to a static function for now.

[asanka, 2013-05-22 21:47:43]

https://codereview.chromium.org/14994003/diff/22001/net/http/http_auth_contro...
File net/http/http_auth_controller.cc (right):

https://codereview.chromium.org/14994003/diff/22001/net/http/http_auth_contro...
net/http/http_auth_controller.cc:193: entry->realm().data(),
entry->auth_challenge().data());
entry->auth_challenge() is supposed to be the value of the challenge header. Why
isn't it already formatted to look like 'SpdyProxy realm="foo", ps="bar"'?

The SelectPreemptiveAuth() call should be able to construct the correct
AuthHandler from the cache entry based on a path lookup.

[mmenke, 2013-05-23 14:49:55]

chrome/browser/profiles/ LGTM

[Yaron, 2013-05-23 17:18:44]

android bits lgtm

https://codereview.chromium.org/14994003/diff/22001/chrome/chrome_android.gypi
File chrome/chrome_android.gypi (right):

https://codereview.chromium.org/14994003/diff/22001/chrome/chrome_android.gyp...
chrome/chrome_android.gypi:28:
'android/testshell/chrome_data_reduction_proxy_testshell_android.h',
You don't have a .h file anymore

[bengr, 2013-05-24 01:01:47]

https://codereview.chromium.org/14994003/diff/22001/chrome/chrome_android.gypi
File chrome/chrome_android.gypi (right):

https://codereview.chromium.org/14994003/diff/22001/chrome/chrome_android.gyp...
chrome/chrome_android.gypi:28:
'android/testshell/chrome_data_reduction_proxy_testshell_android.h',
On 2013/05/23 17:18:44, Yaron wrote:
> You don't have a .h file anymore

Done.

https://codereview.chromium.org/14994003/diff/22001/net/http/http_auth_contro...
File net/http/http_auth_controller.cc (right):

https://codereview.chromium.org/14994003/diff/22001/net/http/http_auth_contro...
net/http/http_auth_controller.cc:193: entry->realm().data(),
entry->auth_challenge().data());
On 2013/05/22 21:47:43, asanka wrote:
> entry->auth_challenge() is supposed to be the value of the challenge header.
Why
> isn't it already formatted to look like 'SpdyProxy realm="foo", ps="bar"'?
> 
> The SelectPreemptiveAuth() call should be able to construct the correct
> AuthHandler from the cache entry based on a path lookup.

Done.

[commit-bot: I haz the power, 2013-05-24 01:04:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bengr@chromium.org/14994003/32001

[commit-bot: I haz the power, 2013-05-24 02:14:40]

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

[commit-bot: I haz the power, 2013-05-24 16:33:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bengr@chromium.org/14994003/32001

[commit-bot: I haz the power, 2013-05-24 17:54:46]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
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.

[commit-bot: I haz the power, 2013-05-25 03:06:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bengr@chromium.org/14994003/71001

[commit-bot: I haz the power, 2013-05-25 04:46:47]

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

[commit-bot: I haz the power, 2013-05-25 04:49:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bengr@chromium.org/14994003/71001

[commit-bot: I haz the power, 2013-05-27 23:18:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bengr@chromium.org/14994003/71001

[commit-bot: I haz the power, 2013-05-27 23:18:26]

Change committed as 202472