Use char version functions in base/stringprintf.h.

Use char version functions in base/stringprintf.h.

A chromium change in base/ is going to remove wchar/wstring version of StringPrintf. So change to use char version of functions in win/lib/rlz_value_store_registry.cc .

http://codereview.chromium.org/10449042/

[thakis, 2012-05-29 14:12:48]

http://codereview.appspot.com/6247057/diff/1/win/lib/rlz_value_store_registry.cc
File win/lib/rlz_value_store_registry.cc (right):

http://codereview.appspot.com/6247057/diff/1/win/lib/rlz_value_store_registry...
win/lib/rlz_value_store_registry.cc:20: const char kLibKeyNameChar[]            
 = "Software\\Google\\Common\\Rlz";
Please find a way that doesn't require having this literal twice. It's probably
possible to use the char[] version everywhere?

http://codereview.appspot.com/6247057/diff/1/win/lib/rlz_value_store_registry...
win/lib/rlz_value_store_registry.cc:68: ret = key->Create(HKEY_CURRENT_USER,
ASCIIToWide(key_location).c_str(), access);
80 cols

http://codereview.appspot.com/6247057/diff/1/win/lib/rlz_value_store_registry...
win/lib/rlz_value_store_registry.cc:70: ret = key->Open(HKEY_CURRENT_USER,
ASCIIToWide(key_location).c_str(), access);
80 cols

http://codereview.appspot.com/6247057/diff/1/win/lib/rlz_value_store_registry...
win/lib/rlz_value_store_registry.cc:353: base::StringAppendF(&subkey_name,
"%s\\%s", rlz_lib::kLibKeyNameChar, subkeys[i]);
80 cols

http://codereview.appspot.com/6247057/diff/1/win/lib/rlz_value_store_registry...
win/lib/rlz_value_store_registry.cc:356:
VERIFY(DeleteKeyIfEmpty(HKEY_CURRENT_USER, ASCIIToWide(subkey_name).c_str()));
80 cols

[Hao Zheng, 2012-05-30 06:28:04]

Thanks for review. I think I cannot submit code, so if this looks good, could
you help submit it please?

http://codereview.appspot.com/6247057/diff/1/win/lib/rlz_value_store_registry.cc
File win/lib/rlz_value_store_registry.cc (right):

http://codereview.appspot.com/6247057/diff/1/win/lib/rlz_value_store_registry...
win/lib/rlz_value_store_registry.cc:20: const char kLibKeyNameChar[]            
 = "Software\\Google\\Common\\Rlz";
On 2012/05/29 14:12:48, thakis wrote:
> Please find a way that doesn't require having this literal twice. It's
probably
> possible to use the char[] version everywhere?
Extracted a static method. Done.

http://codereview.appspot.com/6247057/diff/1/win/lib/rlz_value_store_registry...
win/lib/rlz_value_store_registry.cc:68: ret = key->Create(HKEY_CURRENT_USER,
ASCIIToWide(key_location).c_str(), access);
On 2012/05/29 14:12:48, thakis wrote:
> 80 cols

Done.

http://codereview.appspot.com/6247057/diff/1/win/lib/rlz_value_store_registry...
win/lib/rlz_value_store_registry.cc:70: ret = key->Open(HKEY_CURRENT_USER,
ASCIIToWide(key_location).c_str(), access);
On 2012/05/29 14:12:48, thakis wrote:
> 80 cols

Done.

http://codereview.appspot.com/6247057/diff/1/win/lib/rlz_value_store_registry...
win/lib/rlz_value_store_registry.cc:353: base::StringAppendF(&subkey_name,
"%s\\%s", rlz_lib::kLibKeyNameChar, subkeys[i]);
On 2012/05/29 14:12:48, thakis wrote:
> 80 cols

Done.

http://codereview.appspot.com/6247057/diff/1/win/lib/rlz_value_store_registry...
win/lib/rlz_value_store_registry.cc:356:
VERIFY(DeleteKeyIfEmpty(HKEY_CURRENT_USER, ASCIIToWide(subkey_name).c_str()));
On 2012/05/29 14:12:48, thakis wrote:
> 80 cols

Done.

[thakis, 2012-05-31 05:01:34]

This looks fine. One question below.

http://codereview.appspot.com/6247057/diff/4005/win/lib/machine_deal.cc
File win/lib/machine_deal.cc (right):

http://codereview.appspot.com/6247057/diff/4005/win/lib/machine_deal.cc#newco...
win/lib/machine_deal.cc:139: RlzValueStoreRegistry::GetWideLibKeyName().c_str(),
I could imagine that you're doing this change in many places when doing the
chromium side. Do you think it'd make sense to change RegKey to have an
additional constructor that takes char* instead (and which does this conversion
internally)?

http://codereview.appspot.com/6247057/diff/4005/win/lib/rlz_value_store_regis...
File win/lib/rlz_value_store_registry.h (right):

http://codereview.appspot.com/6247057/diff/4005/win/lib/rlz_value_store_regis...
win/lib/rlz_value_store_registry.h:16: static std::wstring GetWideLibKeyName();
nit: Leave this global. (I can do this when landing.)

[Hao Zheng, 2012-05-31 05:09:13]

http://codereview.appspot.com/6247057/diff/4005/win/lib/machine_deal.cc
File win/lib/machine_deal.cc (right):

http://codereview.appspot.com/6247057/diff/4005/win/lib/machine_deal.cc#newco...
win/lib/machine_deal.cc:139: RlzValueStoreRegistry::GetWideLibKeyName().c_str(),
On 2012/05/31 05:01:34, thakis wrote:
> I could imagine that you're doing this change in many places when doing the
> chromium side. Do you think it'd make sense to change RegKey to have an
> additional constructor that takes char* instead (and which does this
conversion
> internally)?

Not only constructor, but also almost all other methods, e.g. CreateKey().
Actually, we can count how many places actually need to be wchar_t*. I don't see
any places up to my knowledge. We can do the conversion when calling Windows
API, then the api of RegKey would be easier to use.

But my change doesn't affect RegKey much, see the chromium CL
https://chromiumcodereview.appspot.com/10449042/.

http://codereview.appspot.com/6247057/diff/4005/win/lib/rlz_value_store_regis...
File win/lib/rlz_value_store_registry.h (right):

http://codereview.appspot.com/6247057/diff/4005/win/lib/rlz_value_store_regis...
win/lib/rlz_value_store_registry.h:16: static std::wstring GetWideLibKeyName();
On 2012/05/31 05:01:34, thakis wrote:
> nit: Leave this global. (I can do this when landing.)

Thanks!

[thakis, 2012-05-31 05:10:28]

Ok, LGTM.

I'll land this tomorrow morning.

[Hao Zheng, 2012-05-31 05:45:04]

On 2012/05/31 05:10:28, thakis wrote:
> Ok, LGTM.
> 
> I'll land this tomorrow morning.

Thanks!

[thakis, 2012-05-31 23:46:08]

Landed in rlz 129/130. You will have to update chromium's DEPS file to pull that
new revision into chromium.

[Hao Zheng, 2012-06-01 03:38:03]

On 2012/05/31 23:46:08, thakis wrote:
> Landed in rlz 129/130. You will have to update chromium's DEPS file to pull
that
> new revision into chromium.

OK, Thanks!