Cleaned up usage of std::wstring in src/remoting. Added presubmit warning supressions for the reman…

Cleaned up usage of std::wstring in src/remoting. Added presubmit warning supressions for the remaning instances because they depend on hard-to-change public APIs.

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

[alexeypa (please no reviews), 2012-08-03 17:52:19]

The last CL for 133003. Please take a look.

[Wez, 2012-08-03 21:26:15]

https://chromiumcodereview.appspot.com/10824166/diff/6001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://chromiumcodereview.appspot.com/10824166/diff/6001/PRESUBMIT.py#newcod...
PRESUBMIT.py:295: supressed = False
typo: suppressed

I'd suggest calling this allowWString rather than suppressed.

https://chromiumcodereview.appspot.com/10824166/diff/6001/remoting/host/host_...
File remoting/host/host_event_logger_win.cc (right):

https://chromiumcodereview.appspot.com/10824166/diff/6001/remoting/host/host_...
remoting/host/host_event_logger_win.cc:118: raw_strings[i] =
utf16_strings[i].c_str();
nit: Clarify in the comment above that this is OK only because wide-characters
are 16-bit on Windows?
nit: It seems weird to be making that assumption here, where in other
Windows-specific code in this CL we use UTF8ToWide()...

https://chromiumcodereview.appspot.com/10824166/diff/6001/remoting/host/win/l...
File remoting/host/win/launch_process_with_token.cc (right):

https://chromiumcodereview.appspot.com/10824166/diff/6001/remoting/host/win/l...
remoting/host/win/launch_process_with_token.cc:80: pipe_name = UTF8ToUTF16(
nit: Why not have pipe_name be std::string and UTF8ToWide() it when you pass it
to the CreateFile() API?

https://chromiumcodereview.appspot.com/10824166/diff/6001/remoting/host/win/l...
remoting/host/win/launch_process_with_token.cc:115: string16
desktop_name(WideToUTF16(kDefaultDesktopName));
nit: Why does kDefaultDesktopName need to be wide?

https://chromiumcodereview.appspot.com/10824166/diff/6001/remoting/host/win/w...
File remoting/host/win/wts_session_process_launcher.cc (right):

https://chromiumcodereview.appspot.com/10824166/diff/6001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:191: HANDLE pipe =
CreateNamedPipeW(UTF8ToUTF16(pipe_name).c_str(),
nit: UTF8ToWide()?

[alexeypa (please no reviews), 2012-08-03 22:00:18]

http://codereview.chromium.org/10824166/diff/6001/PRESUBMIT.py
File PRESUBMIT.py (right):

http://codereview.chromium.org/10824166/diff/6001/PRESUBMIT.py#newcode295
PRESUBMIT.py:295: supressed = False
On 2012/08/03 21:26:15, Wez wrote:
> typo: suppressed
> 
> I'd suggest calling this allowWString rather than suppressed.

Done.

http://codereview.chromium.org/10824166/diff/6001/remoting/host/host_event_lo...
File remoting/host/host_event_logger_win.cc (right):

http://codereview.chromium.org/10824166/diff/6001/remoting/host/host_event_lo...
remoting/host/host_event_logger_win.cc:118: raw_strings[i] =
utf16_strings[i].c_str();
On 2012/08/03 21:26:15, Wez wrote:
> nit: Clarify in the comment above that this is OK only because wide-characters
> are 16-bit on Windows?

I wound need to write such a comment next to every Win32 API call then. I don't
think that this is a good stop to document this... mess.

> nit: It seems weird to be making that assumption here, where in other
> Windows-specific code in this CL we use UTF8ToWide()...

There is a bit of logic:

WCHAR, string16 == UTF-16
wchar_t, wstring == wide

In other places whene we do XxxToWide wither wchar_t or wstring is used.

http://codereview.chromium.org/10824166/diff/6001/remoting/host/win/launch_pr...
File remoting/host/win/launch_process_with_token.cc (right):

http://codereview.chromium.org/10824166/diff/6001/remoting/host/win/launch_pr...
remoting/host/win/launch_process_with_token.cc:80: pipe_name = UTF8ToUTF16(
On 2012/08/03 21:26:15, Wez wrote:
> nit: Why not have pipe_name be std::string and UTF8ToWide() it when you pass
it
> to the CreateFile() API?

To avoid UTF8 <-> UTF16 conversion in three places.

http://codereview.chromium.org/10824166/diff/6001/remoting/host/win/launch_pr...
remoting/host/win/launch_process_with_token.cc:115: string16
desktop_name(WideToUTF16(kDefaultDesktopName));
On 2012/08/03 21:26:15, Wez wrote:
> nit: Why does kDefaultDesktopName need to be wide?

Done.

http://codereview.chromium.org/10824166/diff/6001/remoting/host/win/wts_sessi...
File remoting/host/win/wts_session_process_launcher.cc (right):

http://codereview.chromium.org/10824166/diff/6001/remoting/host/win/wts_sessi...
remoting/host/win/wts_session_process_launcher.cc:191: HANDLE pipe =
CreateNamedPipeW(UTF8ToUTF16(pipe_name).c_str(),
On 2012/08/03 21:26:15, Wez wrote:
> nit: UTF8ToWide()?

WCHAR == UTF-16

[Wez, 2012-08-06 17:29:38]

LGTM but I think we should avoid using the "Wide" string conversion routines,
and types, in any Windows-specific code, if possible, to avoid confusion.

http://codereview.chromium.org/10824166/diff/6001/remoting/host/host_event_lo...
File remoting/host/host_event_logger_win.cc (right):

http://codereview.chromium.org/10824166/diff/6001/remoting/host/host_event_lo...
remoting/host/host_event_logger_win.cc:118: raw_strings[i] =
utf16_strings[i].c_str();
On 2012/08/03 22:00:18, alexeypa wrote:
> On 2012/08/03 21:26:15, Wez wrote:
> > nit: Clarify in the comment above that this is OK only because
wide-characters
> > are 16-bit on Windows?
> 
> I wound need to write such a comment next to every Win32 API call then. I
don't
> think that this is a good stop to document this... mess.

Yeah, we definitely don't want that, but at the same time it's really confusing
to have swathes of code using string16/UTF8ToUTF16 while other parts use
wstring/UTF8ToWide, with no clear distinction.

> > nit: It seems weird to be making that assumption here, where in other
> > Windows-specific code in this CL we use UTF8ToWide()...
> 
> There is a bit of logic:
> 
> WCHAR, string16 == UTF-16
> wchar_t, wstring == wide
> 
> In other places whene we do XxxToWide wither wchar_t or wstring is used.

Right; but there seems to be code you're touching elsewhere in this CL that uses
wstring and/or UTF8ToWide for no good reason; i.e. where UTF16 would do.

http://codereview.chromium.org/10824166/diff/6001/remoting/host/win/launch_pr...
File remoting/host/win/launch_process_with_token.cc (right):

http://codereview.chromium.org/10824166/diff/6001/remoting/host/win/launch_pr...
remoting/host/win/launch_process_with_token.cc:80: pipe_name = UTF8ToUTF16(
On 2012/08/03 22:00:18, alexeypa wrote:
> On 2012/08/03 21:26:15, Wez wrote:
> > nit: Why not have pipe_name be std::string and UTF8ToWide() it when you pass
> it
> > to the CreateFile() API?
> 
> To avoid UTF8 <-> UTF16 conversion in three places.

Makes sense; I'd recommend working with std::string for pipe_name, though, and
just creating a string16 from it immediately outside the for() loop, similar to
what you've done to keep DefaultDesktopName a const char[].

http://codereview.chromium.org/10824166/diff/6001/remoting/host/win/wts_sessi...
File remoting/host/win/wts_session_process_launcher.cc (right):

http://codereview.chromium.org/10824166/diff/6001/remoting/host/win/wts_sessi...
remoting/host/win/wts_session_process_launcher.cc:191: HANDLE pipe =
CreateNamedPipeW(UTF8ToUTF16(pipe_name).c_str(),
On 2012/08/03 22:00:18, alexeypa wrote:
> On 2012/08/03 21:26:15, Wez wrote:
> > nit: UTF8ToWide()?
> 
> WCHAR == UTF-16

Yes; but the intent is to match UTF8ToWide/UTF8ToUTF16 with wstring/string16
usage then don't you want to use UTF8ToWide() here, to match the type the
function takes?

[alexeypa (please no reviews), 2012-08-06 17:56:18]

http://codereview.chromium.org/10824166/diff/6001/remoting/host/host_event_lo...
File remoting/host/host_event_logger_win.cc (right):

http://codereview.chromium.org/10824166/diff/6001/remoting/host/host_event_lo...
remoting/host/host_event_logger_win.cc:118: raw_strings[i] =
utf16_strings[i].c_str();
> Right; but there seems to be code you're touching elsewhere in this CL that
uses
> wstring and/or UTF8ToWide for no good reason; i.e. where UTF16 would do.

I'm sorry for being blind but I don't see that. :-) What exact file and line are
you talking about?

http://codereview.chromium.org/10824166/diff/6001/remoting/host/win/launch_pr...
File remoting/host/win/launch_process_with_token.cc (right):

http://codereview.chromium.org/10824166/diff/6001/remoting/host/win/launch_pr...
remoting/host/win/launch_process_with_token.cc:80: pipe_name = UTF8ToUTF16(
On 2012/08/06 17:29:38, Wez wrote:
> Makes sense; I'd recommend working with std::string for pipe_name, though, and
> just creating a string16 from it immediately outside the for() loop, similar
to
> what you've done to keep DefaultDesktopName a const char[].

This will reduce number of conversions to two, not to one. |pipe_name| is
assigned above, next to the win_station_query_information call.

http://codereview.chromium.org/10824166/diff/6001/remoting/host/win/wts_sessi...
File remoting/host/win/wts_session_process_launcher.cc (right):

http://codereview.chromium.org/10824166/diff/6001/remoting/host/win/wts_sessi...
remoting/host/win/wts_session_process_launcher.cc:191: HANDLE pipe =
CreateNamedPipeW(UTF8ToUTF16(pipe_name).c_str(),
On 2012/08/06 17:29:38, Wez wrote:
> On 2012/08/03 22:00:18, alexeypa wrote:
> > On 2012/08/03 21:26:15, Wez wrote:
> > > nit: UTF8ToWide()?
> > 
> > WCHAR == UTF-16
> 
> Yes; but the intent is to match UTF8ToWide/UTF8ToUTF16 with wstring/string16
> usage then don't you want to use UTF8ToWide() here, to match the type the
> function takes?

The function takes WCHAR*, which is assumed to be UTF-16. I'd use UTF8ToWide()
if it took wchar_t*.

[Wez, 2012-08-06 18:33:18]

I've highlighted the bits where we're still using "wide" conversions; I think it
should be straightforward to switch to string16 for most of these except for
fetching values from the registry, where the APIs are currently defined with
wstring?

http://codereview.chromium.org/10824166/diff/12002/remoting/base/breakpad_win.cc
File remoting/base/breakpad_win.cc (right):

http://codereview.chromium.org/10824166/diff/12002/remoting/base/breakpad_win...
remoting/base/breakpad_win.cc:159: version =
UTF16ToWide(version_info->product_version());
Here.

http://codereview.chromium.org/10824166/diff/12002/remoting/host/plugin/daemo...
File remoting/host/plugin/daemon_installer_win.cc (right):

http://codereview.chromium.org/10824166/diff/12002/remoting/host/plugin/daemo...
remoting/host/plugin/daemon_installer_win.cc:297: string16 command_line =
WideToUTF16(
We only need to printf wide and convert because google_update may be
internationalized, I think, so why not convert that to string16 and printf to
string16?

http://codereview.chromium.org/10824166/diff/12002/remoting/host/policy_hack/...
File remoting/host/policy_hack/policy_watcher_win.cc (right):

http://codereview.chromium.org/10824166/diff/12002/remoting/host/policy_hack/...
remoting/host/policy_hack/policy_watcher_win.cc:124: *result =
WideToUTF8(value);
Here.

http://codereview.chromium.org/10824166/diff/12002/remoting/host/policy_hack/...
remoting/host/policy_hack/policy_watcher_win.cc:132: *result =
WideToUTF8(value);
Here.

http://codereview.chromium.org/10824166/diff/12002/remoting/host/policy_hack/...
remoting/host/policy_hack/policy_watcher_win.cc:142: std::wstring
value_name_wide = UTF8ToWide(value_name);
Here.

http://codereview.chromium.org/10824166/diff/12002/remoting/host/usage_stats_...
File remoting/host/usage_stats_consent_win.cc (right):

http://codereview.chromium.org/10824166/diff/12002/remoting/host/usage_stats_...
remoting/host/usage_stats_consent_win.cc:30: remoting::kHostOmahaAppid);
Here.

http://codereview.chromium.org/10824166/diff/12002/remoting/host/usage_stats_...
remoting/host/usage_stats_consent_win.cc:78: kHostOmahaAppid);
Here.

[Wez, 2012-08-06 18:41:20]

LGTM

http://codereview.chromium.org/10824166/diff/6001/remoting/host/win/launch_pr...
File remoting/host/win/launch_process_with_token.cc (right):

http://codereview.chromium.org/10824166/diff/6001/remoting/host/win/launch_pr...
remoting/host/win/launch_process_with_token.cc:80: pipe_name = UTF8ToUTF16(
On 2012/08/06 17:56:18, alexeypa wrote:
> On 2012/08/06 17:29:38, Wez wrote:
> > Makes sense; I'd recommend working with std::string for pipe_name, though,
and
> > just creating a string16 from it immediately outside the for() loop, similar
> to
> > what you've done to keep DefaultDesktopName a const char[].
> 
> This will reduce number of conversions to two, not to one. |pipe_name| is
> assigned above, next to the win_station_query_information call.

Of course; good point.

http://codereview.chromium.org/10824166/diff/6001/remoting/host/win/wts_sessi...
File remoting/host/win/wts_session_process_launcher.cc (right):

http://codereview.chromium.org/10824166/diff/6001/remoting/host/win/wts_sessi...
remoting/host/win/wts_session_process_launcher.cc:191: HANDLE pipe =
CreateNamedPipeW(UTF8ToUTF16(pipe_name).c_str(),
On 2012/08/06 17:56:18, alexeypa wrote:
> On 2012/08/06 17:29:38, Wez wrote:
> > On 2012/08/03 22:00:18, alexeypa wrote:
> > > On 2012/08/03 21:26:15, Wez wrote:
> > > > nit: UTF8ToWide()?
> > > 
> > > WCHAR == UTF-16
> > 
> > Yes; but the intent is to match UTF8ToWide/UTF8ToUTF16 with wstring/string16
> > usage then don't you want to use UTF8ToWide() here, to match the type the
> > function takes?
> 
> The function takes WCHAR*, which is assumed to be UTF-16. I'd use UTF8ToWide()
> if it took wchar_t*.

Of course; my mistake.

[alexeypa (please no reviews), 2012-08-06 19:31:54]

> I've highlighted the bits where we're still using "wide" conversions; I think
it
should be straightforward to switch to string16 for most of these except for
fetching values from the registry, where the APIs are currently defined with
wstring?

All those wstrings are imposed by an external API. That is the only reason for
their existence. (I thought you were talking about Win32 API. This is why I was
confused).

http://codereview.chromium.org/10824166/diff/12002/remoting/base/breakpad_win.cc
File remoting/base/breakpad_win.cc (right):

http://codereview.chromium.org/10824166/diff/12002/remoting/base/breakpad_win...
remoting/base/breakpad_win.cc:159: version =
UTF16ToWide(version_info->product_version());
On 2012/08/06 18:33:18, Wez wrote:
> Here.

UTF16ToWide is used here because CustomInfoEntry requires a wide pointer while
product_version gives me a UTF-16 pointer.

But I see a different problem here: the returned |custon_info| should not point
to a buffer, allocated by wstring because it will be deleted once we leave the
scope.

http://codereview.chromium.org/10824166/diff/12002/remoting/host/plugin/daemo...
File remoting/host/plugin/daemon_installer_win.cc (right):

http://codereview.chromium.org/10824166/diff/12002/remoting/host/plugin/daemo...
remoting/host/plugin/daemon_installer_win.cc:297: string16 command_line =
WideToUTF16(
On 2012/08/06 18:33:18, Wez wrote:
> We only need to printf wide and convert because google_update may be
> internationalized, I think, so why not convert that to string16 and printf to
> string16?

There is no string16 version of printf, only acsii and wide.

http://codereview.chromium.org/10824166/diff/12002/remoting/host/policy_hack/...
File remoting/host/policy_hack/policy_watcher_win.cc (right):

http://codereview.chromium.org/10824166/diff/12002/remoting/host/policy_hack/...
remoting/host/policy_hack/policy_watcher_win.cc:124: *result =
WideToUTF8(value);
On 2012/08/06 18:33:18, Wez wrote:
> Here.

RegKey's using wchar_t*.

http://codereview.chromium.org/10824166/diff/12002/remoting/host/policy_hack/...
remoting/host/policy_hack/policy_watcher_win.cc:132: *result =
WideToUTF8(value);
On 2012/08/06 18:33:18, Wez wrote:
> Here.

RegKey's using wchar_t*.

http://codereview.chromium.org/10824166/diff/12002/remoting/host/policy_hack/...
remoting/host/policy_hack/policy_watcher_win.cc:142: std::wstring
value_name_wide = UTF8ToWide(value_name);
On 2012/08/06 18:33:18, Wez wrote:
> Here.

RegKey's using wchar_t*.

http://codereview.chromium.org/10824166/diff/12002/remoting/host/usage_stats_...
File remoting/host/usage_stats_consent_win.cc (right):

http://codereview.chromium.org/10824166/diff/12002/remoting/host/usage_stats_...
remoting/host/usage_stats_consent_win.cc:30: remoting::kHostOmahaAppid);
On 2012/08/06 18:33:18, Wez wrote:
> Here.

RegKey's using wchar_t.

http://codereview.chromium.org/10824166/diff/12002/remoting/host/usage_stats_...
remoting/host/usage_stats_consent_win.cc:78: kHostOmahaAppid);
On 2012/08/06 18:33:18, Wez wrote:
> Here.

RegKey's using wchar_t

[commit-bot: I haz the power, 2012-08-06 20:41:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/10824166/12007

[commit-bot: I haz the power, 2012-08-06 22:36:02]

Try job failure for 10824166-12007 (retry) on linux_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-08-06 22:47:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/10824166/12007

[commit-bot: I haz the power, 2012-08-06 23:19:03]

Try job failure for 10824166-12007 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-08-06 23:23:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/10824166/12007

[commit-bot: I haz the power, 2012-08-07 01:33:00]

Change committed as 150224