Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(67)

Issue 43113: Merge NTLMAuthModule into HttpAuthHandlerNTLM. The... (Closed)

Created:
11 years, 9 months ago by wtc
Modified:
9 years, 7 months ago
Reviewers:
eroman
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

Make GetHostNameProc return a std::string. Add the ScopedProcSetter helper class so that unit tests restore the original GenerateRandom and GetHostName functions on completion. Merge NTLMAuthModule into HttpAuthHandlerNTLM. The data members domain_, username_, and password_ are moved. The Init method is inlined at its only call site. The GetNextToken method is moved. Make generate_random_proc_ and get_host_name_proc_ static members of the HttpAuthHandlerNTLM class. R=eroman BUG=6567, 6824 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=12290

Patch Set 1 #

Total comments: 13

Patch Set 2 : Address review comments #

Patch Set 3 : Upload before checkin #

Patch Set 4 : Upload before checkin #

Unified diffs Side-by-side diffs Delta from patch set Stats (+113 lines, -123 lines) Patch
M net/http/http_auth_handler_ntlm.h View 1 2 3 4 chunks +42 lines, -18 lines 0 comments Download
M net/http/http_auth_handler_ntlm.cc View 1 2 3 12 chunks +63 lines, -92 lines 0 comments Download
M net/http/http_network_transaction_unittest.cc View 1 2 3 5 chunks +8 lines, -13 lines 0 comments Download

Messages

Total messages: 4 (0 generated)
wtc
This CL implements the rest of your suggested changes from your review of http://codereview.chromium.org/42052. With ...
11 years, 9 months ago (2009-03-12 00:39:39 UTC) #1
eroman
lgtm, good improvements http://codereview.chromium.org/43113/diff/1/3 File net/http/http_auth_handler_ntlm.cc (right): http://codereview.chromium.org/43113/diff/1/3#newcode442 Line 442: static std::string GetHostName() { You ...
11 years, 9 months ago (2009-03-19 18:32:33 UTC) #2
wtc
Thanks a lot for your comments and suggested changes. I made all of your suggested ...
11 years, 9 months ago (2009-03-21 01:07:10 UTC) #3
eroman
11 years, 9 months ago (2009-03-21 01:33:43 UTC) #4
LGTM

> Do you mind if I rename your GetMyHostName function
> GetHostName (in a follow-up CL)? 

sounds fine to me, go right ahead.

http://codereview.chromium.org/43113/diff/1/3
File net/http/http_auth_handler_ntlm.cc (right):

http://codereview.chromium.org/43113/diff/1/3#newcode529
Line 529: ucs_host_buf.assign(hostname.begin(), hostname.end());
On 2009/03/21 01:07:10, wtc wrote:
> On 2009/03/19 18:32:33, eroman wrote:
> > how about:
> > ucs_host_buf = hostname;
> 
> Does that work?  ucs_host_buf is string16, but hostname
> is std::string.
> 
> 

ah, i forgot it was string16 :)

Powered by Google App Engine
This is Rietveld 408576698