Kerberos authentication backoff cleanup.

Kerberos authentication backoff cleanup.

BUG=33033
TEST=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=53207

[ahendrickson, 2010-07-20 18:24:57]

These minor changes are per Eric's comments in
http://codereview.chromium.org/3010010/show.

[eroman, 2010-07-20 20:57:26]

LGTM, thanks!

Also please add a unittest :)

[cbentzel, 2010-07-20 21:12:16]

Ditto.

On Tue, Jul 20, 2010 at 4:57 PM, <eroman@chromium.org> wrote:

> LGTM, thanks!
>
> Also please add a unittest :)
>
>
> http://codereview.chromium.org/2883031/show
>

[ahendrickson, 2010-07-21 15:15:01]

Unit tests are in.

[cbentzel, 2010-07-21 15:41:57]

Mostly nits, otherwise LGTM

http://codereview.chromium.org/2883031/diff/11001/12001
File net/http/http_auth_controller.cc (right):

http://codereview.chromium.org/2883031/diff/11001/12001#newcode91
net/http/http_auth_controller.cc:91: else
Nit: most of the other cases of if/else in the code base use braces. 

Personally, I wish we required braces on just if statements, and think it's a
mistake that we don't.

http://codereview.chromium.org/2883031/diff/11001/12002
File net/http/http_auth_handler_negotiate_unittest.cc (right):

http://codereview.chromium.org/2883031/diff/11001/12002#newcode44
net/http/http_auth_handler_negotiate_unittest.cc:44: void
SetupMocks(MockAuthLibrary* mock_library, size_t start, size_t length) {
It's very confusing that you are setting up expectations externally by indexing
into a table contained inside this method.

I recommend just adding two methods for setting up expectations on the mock
libraries, and both can call a helper function.

http://codereview.chromium.org/2883031/diff/11001/12002#newcode144
net/http/http_auth_handler_negotiate_unittest.cc:144: 0x96C73A07,               
     // Minor response code (no server)
At one point I thought you had symbols for the minor codes instead of explicit
values.

http://codereview.chromium.org/2883031/diff/11001/12002#newcode317
net/http/http_auth_handler_negotiate_unittest.cc:317: #if defined(OS_POSIX)
Please comment why these are OS_POSIX only (not just due to GSSAPI, but due to
lack of ability to use explicit credentials in GSSAPI).

http://codereview.chromium.org/2883031/diff/11001/12002#newcode331
net/http/http_auth_handler_negotiate_unittest.cc:331: EXPECT_EQ(ERR_IO_PENDING,
auth_handler->GenerateAuthToken(
Could you just pass in NULL username/password here?

http://codereview.chromium.org/2883031/diff/11001/12002#newcode334
net/http/http_auth_handler_negotiate_unittest.cc:334: #if defined(OS_WIN)
This isn't needed as the tests are wrapped inside #if defined(OS_POSIX)

Actually, the SPN expectations are a bit superfluous here.

[ahendrickson, 2010-07-21 16:08:46]

http://codereview.chromium.org/2883031/diff/11001/12001
File net/http/http_auth_controller.cc (right):

http://codereview.chromium.org/2883031/diff/11001/12001#newcode91
net/http/http_auth_controller.cc:91: else
I think I'll get style complaints if I put braces here.

On 2010/07/21 15:41:57, cbentzel wrote:
> Nit: most of the other cases of if/else in the code base use braces. 
> 
> Personally, I wish we required braces on just if statements, and think it's a
> mistake that we don't.

http://codereview.chromium.org/2883031/diff/11001/12002
File net/http/http_auth_handler_negotiate_unittest.cc (right):

http://codereview.chromium.org/2883031/diff/11001/12002#newcode44
net/http/http_auth_handler_negotiate_unittest.cc:44: void
SetupMocks(MockAuthLibrary* mock_library, size_t start, size_t length) {
On 2010/07/21 15:41:57, cbentzel wrote:
> It's very confusing that you are setting up expectations externally by
indexing
> into a table contained inside this method.
> 
> I recommend just adding two methods for setting up expectations on the mock
> libraries, and both can call a helper function.

Done.

http://codereview.chromium.org/2883031/diff/11001/12002#newcode144
net/http/http_auth_handler_negotiate_unittest.cc:144: 0x96C73A07,               
     // Minor response code (no server)
No, I can't find them in GSSAPI, and I believe they are implementation-specific.

On 2010/07/21 15:41:57, cbentzel wrote:
> At one point I thought you had symbols for the minor codes instead of explicit
> values.

http://codereview.chromium.org/2883031/diff/11001/12002#newcode317
net/http/http_auth_handler_negotiate_unittest.cc:317: #if defined(OS_POSIX)
On 2010/07/21 15:41:57, cbentzel wrote:
> Please comment why these are OS_POSIX only (not just due to GSSAPI, but due to
> lack of ability to use explicit credentials in GSSAPI).

Done.

http://codereview.chromium.org/2883031/diff/11001/12002#newcode331
net/http/http_auth_handler_negotiate_unittest.cc:331: EXPECT_EQ(ERR_IO_PENDING,
auth_handler->GenerateAuthToken(
On 2010/07/21 15:41:57, cbentzel wrote:
> Could you just pass in NULL username/password here?

Done.

http://codereview.chromium.org/2883031/diff/11001/12002#newcode334
net/http/http_auth_handler_negotiate_unittest.cc:334: #if defined(OS_WIN)
On 2010/07/21 15:41:57, cbentzel wrote:
> This isn't needed as the tests are wrapped inside #if defined(OS_POSIX)
> 
> Actually, the SPN expectations are a bit superfluous here.

Done.

[cbentzel, 2010-07-21 17:17:59]

LGTM

Thanks for adding unit tests at this level. I'd also like to see unit tests at
the HttpNetworkTransaction level which test that Negotiate is not retried after
a failure to generate an auth token.

http://codereview.chromium.org/2883031/diff/11001/12001
File net/http/http_auth_controller.cc (right):

http://codereview.chromium.org/2883031/diff/11001/12001#newcode91
net/http/http_auth_controller.cc:91: else
On 2010/07/21 16:08:46, ahendrickson wrote:
> I think I'll get style complaints if I put braces here.

Eric: Do you have a preference? I know if-without-else are braceless, but I
think most of the if/else code I've seen in chrome uses braces.

[eroman, 2010-07-21 18:29:57]

LGTM.

You can also expand the CL description to mention addition of unittest.

http://codereview.chromium.org/2883031/diff/11001/12001
File net/http/http_auth_controller.cc (right):

http://codereview.chromium.org/2883031/diff/11001/12001#newcode91
net/http/http_auth_controller.cc:91: else
On 2010/07/21 17:18:00, cbentzel wrote:
> On 2010/07/21 16:08:46, ahendrickson wrote:
> > I think I'll get style complaints if I put braces here.
> 
> Eric: Do you have a preference? I know if-without-else are braceless, but I
> think most of the if/else code I've seen in chrome uses braces.

The style guide doesn't have a hard rule on this, so either way is acceptable.

I personally like putting curly braces around it, but I have had reviewers ask
me to change this at times. Whereas chrome is consistent in omitting curlies on
single-line ifs, there isn't an obvious style for if-else so you can get away
with whatever your reviewer like likes :)