Installer refactoring: using string16 instead of wstring for GoogleUpdateSettings.

Installer refactoring: using string16 instead of wstring for GoogleUpdateSettings.

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

[huangs, 2014-07-07 17:20:02]

Ping gab@.

[gab, 2014-07-07 18:46:43]

On 2014/07/07 17:20:02, huangs1 wrote:
> Ping gab@.

ping as an initial request?! :-)

Also update google_update_settings_posix.cc (I know it's weird, someone put an
impl to this outside of installer/util...

And also make sure all users of the GoogleUpdateSettings::* API are adapted
(this is generally safer and specifically for POSIX users of the API wstring !=
string16 which may imply implicit conversion :( ).

Cheers,
Gab

[gab, 2014-07-07 18:47:49]

https://codereview.chromium.org/371753002/diff/1/chrome/installer/util/google...
File chrome/installer/util/google_update_settings.cc (right):

https://codereview.chromium.org/371753002/diff/1/chrome/installer/util/google...
chrome/installer/util/google_update_settings.cc:8: #include <string>
rm include already in header.

[huangs, 2014-07-07 19:29:24]

Oops forgot to Mail Comments again...

Updated; used code search to look at every uses of GoogleUpdateSettings. Found
and fixed only 2 files that interfaced using wstring.

PTAL.

https://codereview.chromium.org/371753002/diff/1/chrome/installer/util/google...
File chrome/installer/util/google_update_settings.cc (right):

https://codereview.chromium.org/371753002/diff/1/chrome/installer/util/google...
chrome/installer/util/google_update_settings.cc:8: #include <string>
On 2014/07/07 18:47:49, gab wrote:
> rm include already in header.

Done.

https://codereview.chromium.org/371753002/diff/60001/chrome/installer/gcapi/g...
File chrome/installer/gcapi/gcapi.cc (right):

https://codereview.chromium.org/371753002/diff/60001/chrome/installer/gcapi/g...
chrome/installer/gcapi/gcapi.cc:608: &last_run) == ERROR_SUCCESS &&
ReadValue() takes wstring, but I found other places in code where string16 gets
passed.

https://codereview.chromium.org/371753002/diff/60001/chrome/installer/gcapi/g...
chrome/installer/gcapi/gcapi.cc:722: if (!_wcsicmp(installed_brandcode.c_str(),
partner_brandcode_list[i])) {
Found other instances in code where _wcsicmp() is used on string16.

https://codereview.chromium.org/371753002/diff/60001/chrome/installer/util/la...
File chrome/installer/util/language_selector.cc (right):

https://codereview.chromium.org/371753002/diff/60001/chrome/installer/util/la...
chrome/installer/util/language_selector.cc:194: base::string16 language;
Keeping code changes small here and not changing other uses of wstring.

[gab, 2014-07-07 20:43:58]

lgtm, thanks!

[huangs, 2014-07-07 20:49:23]

Thanks! Committing.

[huangs, 2014-07-07 20:49:28]

The CQ bit was checked by huangs@chromium.org

[commit-bot: I haz the power, 2014-07-07 20:50:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/huangs@chromium.org/371753002/60001

[gab, 2014-07-07 21:41:14]

Another thing I spotted while playing in this code (unchecking CQ, sorry about
not seeing it earlier).

https://codereview.chromium.org/371753002/diff/60001/chrome/installer/util/go...
File chrome/installer/util/google_update_settings.cc (left):

https://codereview.chromium.org/371753002/diff/60001/chrome/installer/util/go...
chrome/installer/util/google_update_settings.cc:309: std::wstring metrics_id_w =
base::UTF8ToWide(metrics_id);
Please also update all UT8ToWide and WideToUTF8 macro to UTF16ToUTF8 and
UTF8ToUTF16.

Thanks!

[gab, 2014-07-07 21:41:17]

The CQ bit was unchecked by gab@chromium.org

[grt (UTC plus 2), 2014-07-07 22:48:39]

two nits. otherwise, lgtm.

https://codereview.chromium.org/371753002/diff/60001/chrome/installer/util/go...
File chrome/installer/util/google_update_settings.cc (right):

https://codereview.chromium.org/371753002/diff/60001/chrome/installer/util/go...
chrome/installer/util/google_update_settings.cc:300: base::string16
metrics_id_w;
_w -> 16

https://codereview.chromium.org/371753002/diff/60001/chrome/installer/util/go...
chrome/installer/util/google_update_settings.cc:307: base::string16 metrics_id_w
= base::UTF8ToWide(metrics_id);
_w -> 16

[grt (UTC plus 2), 2014-07-07 22:49:09]

On 2014/07/07 22:48:39, grt wrote:
> two nits. otherwise, lgtm.

modulo Gab's comments, of course.

[huangs, 2014-07-08 00:02:19]

Updated. Attempting submit again!

https://codereview.chromium.org/371753002/diff/60001/chrome/installer/util/go...
File chrome/installer/util/google_update_settings.cc (left):

https://codereview.chromium.org/371753002/diff/60001/chrome/installer/util/go...
chrome/installer/util/google_update_settings.cc:309: std::wstring metrics_id_w =
base::UTF8ToWide(metrics_id);
On 2014/07/07 21:41:14, gab wrote:
> Please also update all UT8ToWide and WideToUTF8 macro to UTF16ToUTF8 and
> UTF8ToUTF16.
> 
> Thanks!

Ah good catch. Done!

https://codereview.chromium.org/371753002/diff/60001/chrome/installer/util/go...
File chrome/installer/util/google_update_settings.cc (right):

https://codereview.chromium.org/371753002/diff/60001/chrome/installer/util/go...
chrome/installer/util/google_update_settings.cc:300: base::string16
metrics_id_w;
On 2014/07/07 22:48:39, grt wrote:
> _w -> 16

Done.

https://codereview.chromium.org/371753002/diff/60001/chrome/installer/util/go...
chrome/installer/util/google_update_settings.cc:307: base::string16 metrics_id_w
= base::UTF8ToWide(metrics_id);
On 2014/07/07 22:48:39, grt wrote:
> _w -> 16

Done.

[huangs, 2014-07-08 00:02:37]

The CQ bit was checked by huangs@chromium.org

[commit-bot: I haz the power, 2014-07-08 00:04:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/huangs@chromium.org/371753002/80001

[commit-bot: I haz the power, 2014-07-08 02:53:17]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-08 03:17:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-08 03:17:25]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)

[huangs, 2014-07-08 13:09:55]

The CQ bit was checked by huangs@chromium.org

[commit-bot: I haz the power, 2014-07-08 13:10:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/huangs@chromium.org/371753002/80001

[commit-bot: I haz the power, 2014-07-08 13:59:48]

Change committed as 281740