Net: Convert username and password to string16.

Net: Convert username and password to string16.

BUG=none
TEST=none

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

[Lei Zhang, 2010-07-23 07:29:15]

Mostly mechanical.

[cbentzel, 2010-07-23 17:07:58]

I'll take a longer look at this - I'm a little concerned about there being bad
interaction with the GSSAPI/SSPI libraries.

Also, is there any chance this could hold off landing for a little bit? There's
a few issues with authentication and tunneling in recent dev builds and I'd like
to minimize changes in that area until we squash all the bugs.

[Lei Zhang, 2010-07-23 20:36:30]

On 2010/07/23 17:07:58, cbentzel wrote:
> I'll take a longer look at this - I'm a little concerned about there being bad
> interaction with the GSSAPI/SSPI libraries.
> 
> Also, is there any chance this could hold off landing for a little bit?
There's
> a few issues with authentication and tunneling in recent dev builds and I'd
like
> to minimize changes in that area until we squash all the bugs.

Sure, anytime.

[eroman, 2010-07-26 17:42:25]

lgtm

http://codereview.chromium.org/3040016/diff/50001/19010
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/3040016/diff/50001/19010#newcode703
net/ftp/ftp_network_transaction.cc:703: std::string command = "USER " +
UTF16ToUTF8(username_);
Hmm, FTP is only supposed to support ASCII not UTF8. Although doesn't look like
this is a behavior change from before so I guess its ok.

http://codereview.chromium.org/3040016/diff/50001/19038
File net/http/http_auth_sspi_win.cc (right):

http://codereview.chromium.org/3040016/diff/50001/19038#newcode172
net/http/http_auth_sspi_win.cc:172: if (username && password) {
Can you instead make the other functions take string16?

The translation should be easy, since string16 == wstring on windows anyway (and
this is windows only code).

Moreover, it looks like some of the the consumers are assuming the data
representation is utf16 anyway.

[Lei Zhang, 2010-07-27 18:12:14]

I'll wait for cbentzel's LGTM before committing. No rush.

http://codereview.chromium.org/3040016/diff/50001/19038
File net/http/http_auth_sspi_win.cc (right):

http://codereview.chromium.org/3040016/diff/50001/19038#newcode172
net/http/http_auth_sspi_win.cc:172: if (username && password) {
On 2010/07/26 17:42:26, eroman wrote:
> Can you instead make the other functions take string16?
> 
> The translation should be easy, since string16 == wstring on windows anyway
(and
> this is windows only code).
> 
> Moreover, it looks like some of the the consumers are assuming the data
> representation is utf16 anyway.

Done.

[eroman, 2010-07-28 00:09:12]

lgtm

http://codereview.chromium.org/3040016/diff/5004/74038
File net/http/http_auth_sspi_win.cc (right):

http://codereview.chromium.org/3040016/diff/5004/74038#newcode172
net/http/http_auth_sspi_win.cc:172: if (username && password) {
Can you remove these changes? They are no longer necessary since OnFirstRound
takes string16*

[cbentzel, 2010-07-28 19:06:14]

Sorry for the delay. I'll patch this in and test right now.

On Tue, Jul 27, 2010 at 8:09 PM, <eroman@chromium.org> wrote:

> lgtm
>
>
> http://codereview.chromium.org/3040016/diff/5004/74038
>
> File net/http/http_auth_sspi_win.cc (right):
>
> http://codereview.chromium.org/3040016/diff/5004/74038#newcode172
>
> net/http/http_auth_sspi_win.cc:172: if (username && password) {
> Can you remove these changes? They are no longer necessary since
> OnFirstRound takes string16*
>
>
> http://codereview.chromium.org/3040016/show
>

[cbentzel, 2010-07-28 19:33:55]

LGTM

http://codereview.chromium.org/3040016/diff/5004/74009
File net/ftp/ftp_auth_cache_unittest.cc (right):

http://codereview.chromium.org/3040016/diff/5004/74009#newcode15
net/ftp/ftp_auth_cache_unittest.cc:15: const string16
kBogus(ASCIIToUTF16("password"));
This should be "bogus" to match the previous behavior.

http://codereview.chromium.org/3040016/diff/5004/74015
File net/http/http_auth_cache.cc (right):

http://codereview.chromium.org/3040016/diff/5004/74015#newcode51
net/http/http_auth_cache.cc:51: explicit IsEnclosedBy(const std::string& path) :
path(path) { }
thanks for fixing the lint issues along the way.

http://codereview.chromium.org/3040016/diff/5004/74038
File net/http/http_auth_sspi_win.cc (right):

http://codereview.chromium.org/3040016/diff/5004/74038#newcode54
net/http/http_auth_sspi_win.cc:54: reinterpret_cast<unsigned
short*>(const_cast<wchar_t*>(user.c_str()));
Can you do this wchar_t* conversion since you know string16 is wstring on
Windows platforms? I suppose compiler will complain if that other changes since
this is just a const_cast here.

http://codereview.chromium.org/3040016/diff/5004/74038#newcode172
net/http/http_auth_sspi_win.cc:172: if (username && password) {
On 2010/07/28 00:09:13, eroman wrote:
> Can you remove these changes? They are no longer necessary since OnFirstRound
> takes string16*

Also - see the DCHECK at the top of the function which makes sure
username/password are either both NULL or both non-NULL.

http://codereview.chromium.org/3040016/diff/5004/74044
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/3040016/diff/5004/74044#newcode49
net/http/http_network_transaction_unittest.cc:49: const string16
kBar(ASCIIToUTF16("bar"));
Are string16 const's allowed since non-POD?

[eroman, 2010-07-28 19:58:33]

http://codereview.chromium.org/3040016/diff/5004/74044
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/3040016/diff/5004/74044#newcode49
net/http/http_network_transaction_unittest.cc:49: const string16
kBar(ASCIIToUTF16("bar"));
On 2010/07/28 19:33:55, cbentzel wrote:
> Are string16 const's allowed since non-POD?

In ordinary code definitely not.

However for unit-test code, I think this is acceptable.

(we have other places in unit-tests where this is done).


/me wishes upon a star there were a way to express literal UTF16 strings in C++
so we didnt need this asciitoutf16 garbage.

[Lei Zhang, 2010-07-28 23:07:25]

I too, wish there was a way to specify a string16 directly. :-\

http://codereview.chromium.org/3040016/diff/5004/74009
File net/ftp/ftp_auth_cache_unittest.cc (right):

http://codereview.chromium.org/3040016/diff/5004/74009#newcode15
net/ftp/ftp_auth_cache_unittest.cc:15: const string16
kBogus(ASCIIToUTF16("password"));
On 2010/07/28 19:33:55, cbentzel wrote:
> This should be "bogus" to match the previous behavior. 

Thanks for catching this copy+paste error.

http://codereview.chromium.org/3040016/diff/5004/74038
File net/http/http_auth_sspi_win.cc (right):

http://codereview.chromium.org/3040016/diff/5004/74038#newcode54
net/http/http_auth_sspi_win.cc:54: reinterpret_cast<unsigned
short*>(const_cast<wchar_t*>(user.c_str()));
On 2010/07/28 19:33:55, cbentzel wrote:
> Can you do this wchar_t* conversion since you know string16 is wstring on
> Windows platforms? I suppose compiler will complain if that other changes
since
> this is just a const_cast here. 

Ya, this only works because it's win32 only.

http://codereview.chromium.org/3040016/diff/5004/74038#newcode172
net/http/http_auth_sspi_win.cc:172: if (username && password) {
On 2010/07/28 00:09:13, eroman wrote:
> Can you remove these changes? They are no longer necessary since OnFirstRound
> takes string16*

Done.

[cbentzel, 2010-07-29 01:00:58]

LGTM