Cleanup our Registry API.

Cleanup our Registry API.

- Use wchar_t instead of TCHAR.
- Use DCHECK instead of assert.
- Remove this keyword (we don't use it on chromium).
- Add DISALLOW_COPY_AND_ASSIGN to the classes.
- Make it more compliant with chromium code style.
- Remove ununsed methods.
- Use arraysize macro for array size calculation instead of doing it manually.

BUG=44644
TEST=trybots

TODO: Write unittests for this API.
TODO: Remove all the default arguments from the methods in this API. They aren't allowed by our style guide.

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

[M-A Ruel, 2010-08-06 14:12:48]

Thanks, this file is by far the ugliest in our code base. I'd like to take a
look again after you made the requested changes.

http://codereview.chromium.org/3007037/diff/5002/7002
File base/registry.h (right):

http://codereview.chromium.org/3007037/diff/5002/7002#newcode10
base/registry.h:10: #include <tchar.h>
tchar.h shouldn't be needed anymore.

http://codereview.chromium.org/3007037/diff/5002/7002#newcode11
base/registry.h:11: #include <windows.h>
The #include <windows.h> first was intentional, it's the normal way to include
it. It's fine to reorder if it still works, it simply means that windows.h was
included by shlwapi.h so it's not necessary anymore.

http://codereview.chromium.org/3007037/diff/5002/7002#newcode20
base/registry.h:20: #define tchar TCHAR
please get rid of all these defines and use wchar_t. In practice we'd want to
use string16 but I'm not sure it'd compile. TCHAR is defined to wchar_t anyway
so it's a no-op for now.

http://codereview.chromium.org/3007037/diff/5002/7002#newcode39
base/registry.h:39: // Create a subkey (or open if exists).
while at it, changes the verbs to be imperative, e.g. Creates, Opens, etc.

http://codereview.chromium.org/3007037/diff/5002/7002#newcode181
base/registry.h:181: // undo the local types defined above
remove these too

[tfarina, 2010-08-07 03:47:44]

Ready for another look.

http://codereview.chromium.org/3007037/diff/5002/7002
File base/registry.h (right):

http://codereview.chromium.org/3007037/diff/5002/7002#newcode10
base/registry.h:10: #include <tchar.h>
On 2010/08/06 14:12:48, Marc-Antoine Ruel wrote:
> tchar.h shouldn't be needed anymore.

Done.

http://codereview.chromium.org/3007037/diff/5002/7002#newcode11
base/registry.h:11: #include <windows.h>
On 2010/08/06 14:12:48, Marc-Antoine Ruel wrote:
> The #include <windows.h> first was intentional, it's the normal way to include
> it. It's fine to reorder if it still works, it simply means that windows.h was
> included by shlwapi.h so it's not necessary anymore.
Removed.

http://codereview.chromium.org/3007037/diff/5002/7002#newcode20
base/registry.h:20: #define tchar TCHAR
On 2010/08/06 14:12:48, Marc-Antoine Ruel wrote:
> please get rid of all these defines and use wchar_t. In practice we'd want to
> use string16 but I'm not sure it'd compile. TCHAR is defined to wchar_t anyway
> so it's a no-op for now.

Done.

http://codereview.chromium.org/3007037/diff/5002/7002#newcode39
base/registry.h:39: // Create a subkey (or open if exists).
On 2010/08/06 14:12:48, Marc-Antoine Ruel wrote:
> while at it, changes the verbs to be imperative, e.g. Creates, Opens, etc.

Done.

http://codereview.chromium.org/3007037/diff/5002/7002#newcode181
base/registry.h:181: // undo the local types defined above
On 2010/08/06 14:12:48, Marc-Antoine Ruel wrote:
> remove these too

Done.

[M-A Ruel, 2010-08-07 11:28:55]

http://codereview.chromium.org/3007037/diff/22001/23001
File base/registry.cc (right):

http://codereview.chromium.org/3007037/diff/22001/23001#newcode12
base/registry.cc:12: LPCTSTR folder_key) {
const wchar_t* instead

http://codereview.chromium.org/3007037/diff/22001/23001#newcode74
base/registry.cc:74: LPCTSTR folder_key) {
same

http://codereview.chromium.org/3007037/diff/22001/23001#newcode164
base/registry.cc:164: const LONG result = RegCreateKeyEx(rootkey,
remove const here, we rarely use it for locals, in fact, it's frowned upon.

http://codereview.chromium.org/3007037/diff/22001/23001#newcode185
base/registry.cc:185: const LONG result = RegOpenKeyEx(rootkey, subkey, 0,
access, &key_ );
same (and a few times below too)

http://codereview.chromium.org/3007037/diff/22001/23001#newcode268
base/registry.cc:268: // Success: returns the number of TCHARs copied
wchar_t

http://codereview.chromium.org/3007037/diff/22001/23002
File base/registry.h (right):

http://codereview.chromium.org/3007037/diff/22001/23002#newcode9
base/registry.h:9: #include <shlwapi.h>
I'm not sure you need shlwapi.h in the header file, I think it's only needed in
the source file. windows.h would probably be sufficient.

http://codereview.chromium.org/3007037/diff/22001/23002#newcode138
base/registry.h:138: RegistryKeyIterator(HKEY root_key, LPCTSTR folder_key);
s/LPCTSTR/const wchar_t*/

http://codereview.chromium.org/3007037/diff/22001/23003
File chrome/browser/importer/firefox_importer_utils_win.cc (right):

http://codereview.chromium.org/3007037/diff/22001/23003#newcode46
chrome/browser/importer/firefox_importer_utils_win.cc:46: TCHAR
buffer[MAX_PATH];
can you s/TCHAR/wchar_t/ here?

This function is one of the least efficient one I've seen in a long time but
that's outside the scope of this change. i.e. it should only construct one
std::wstring on the stack and always return it.

[tfarina, 2010-08-07 15:35:39]

http://codereview.chromium.org/3007037/diff/22001/23001
File base/registry.cc (right):

http://codereview.chromium.org/3007037/diff/22001/23001#newcode12
base/registry.cc:12: LPCTSTR folder_key) {
On 2010/08/07 11:28:55, Marc-Antoine Ruel wrote:
> const wchar_t* instead

Done.

http://codereview.chromium.org/3007037/diff/22001/23001#newcode74
base/registry.cc:74: LPCTSTR folder_key) {
On 2010/08/07 11:28:55, Marc-Antoine Ruel wrote:
> same

Done.

http://codereview.chromium.org/3007037/diff/22001/23001#newcode164
base/registry.cc:164: const LONG result = RegCreateKeyEx(rootkey,
On 2010/08/07 11:28:55, Marc-Antoine Ruel wrote:
> remove const here, we rarely use it for locals, in fact, it's frowned upon.

Done.

http://codereview.chromium.org/3007037/diff/22001/23001#newcode185
base/registry.cc:185: const LONG result = RegOpenKeyEx(rootkey, subkey, 0,
access, &key_ );
On 2010/08/07 11:28:55, Marc-Antoine Ruel wrote:
> same (and a few times below too)

Done.

http://codereview.chromium.org/3007037/diff/22001/23001#newcode268
base/registry.cc:268: // Success: returns the number of TCHARs copied
On 2010/08/07 11:28:55, Marc-Antoine Ruel wrote:
> wchar_t

Done.

http://codereview.chromium.org/3007037/diff/22001/23002
File base/registry.h (right):

http://codereview.chromium.org/3007037/diff/22001/23002#newcode9
base/registry.h:9: #include <shlwapi.h>
On 2010/08/07 11:28:55, Marc-Antoine Ruel wrote:
> I'm not sure you need shlwapi.h in the header file, I think it's only needed
in
> the source file. windows.h would probably be sufficient.
Removed, and included windows.h in the .cc file.

http://codereview.chromium.org/3007037/diff/22001/23002#newcode138
base/registry.h:138: RegistryKeyIterator(HKEY root_key, LPCTSTR folder_key);
On 2010/08/07 11:28:55, Marc-Antoine Ruel wrote:
> s/LPCTSTR/const wchar_t*/

Done.

http://codereview.chromium.org/3007037/diff/22001/23003
File chrome/browser/importer/firefox_importer_utils_win.cc (right):

http://codereview.chromium.org/3007037/diff/22001/23003#newcode46
chrome/browser/importer/firefox_importer_utils_win.cc:46: TCHAR
buffer[MAX_PATH];
On 2010/08/07 11:28:55, Marc-Antoine Ruel wrote:
> can you s/TCHAR/wchar_t/ here?
> 
> This function is one of the least efficient one I've seen in a long time but
> that's outside the scope of this change. i.e. it should only construct one
> std::wstring on the stack and always return it.

Done.

[M-A Ruel, 2010-08-08 22:34:59]

lgtm!

The code is still full of awful limitations like arbitrary limits on name length
but this refactor is fine as-is.

I've added 2 nits but you can commit anyway. Thanks.

http://codereview.chromium.org/3007037/diff/3003/34001
File base/registry.cc (right):

http://codereview.chromium.org/3007037/diff/3003/34001#newcode256
base/registry.cc:256: static const size_t kMaxStringLength = 1024;  // This is
after expansion.
remove static here

http://codereview.chromium.org/3007037/diff/3003/34005
File chrome/installer/util/google_update_settings_unittest.cc (right):

http://codereview.chromium.org/3007037/diff/3003/34005#newcode18
chrome/installer/util/google_update_settings_unittest.cc:18: const wchar_t
kHKCUReplacement[] =
In fact I prefer const wchar_t* const (const pointer to const data) than an
array. I agree that a non-const global pointer was a bad idea!

[tfarina, 2010-08-08 23:52:28]

On 2010/08/08 22:34:59, Marc-Antoine Ruel wrote:
> lgtm!
> 
> The code is still full of awful limitations like arbitrary limits on name
length
> but this refactor is fine as-is.
> 
> I've added 2 nits but you can commit anyway. Thanks.
>  
> http://codereview.chromium.org/3007037/diff/3003/34005
> File chrome/installer/util/google_update_settings_unittest.cc (right):
> 
> http://codereview.chromium.org/3007037/diff/3003/34005#newcode18
> chrome/installer/util/google_update_settings_unittest.cc:18: const wchar_t
> kHKCUReplacement[] =
> In fact I prefer const wchar_t* const (const pointer to const data) than an
> array. I agree that a non-const global pointer was a bad idea!

The best answer I know for this is that const char[] = "foo"; just creates a
single variable in assembly.

Quoting from http://kernelnewbies.org/KernelJanitors:

"
From: Jeff Garzik

The string form

        [const] char *foo = "blah";
creates two variables in the final assembly output, a static string,
and a char pointer to the static string. The alternate string form

        [const] char foo[] = "blah";
is better because it declares a single variable.
"

Logically there is a size difference between these two forms, the latter saves
some memory too.

http://codereview.chromium.org/3007037/diff/3003/34001
File base/registry.cc (right):

http://codereview.chromium.org/3007037/diff/3003/34001#newcode256
base/registry.cc:256: static const size_t kMaxStringLength = 1024;  // This is
after expansion.
On 2010/08/08 22:34:59, Marc-Antoine Ruel wrote:
> remove static here

Done.

[M-A Ruel, 2010-08-09 00:12:35]

On 2010/08/08 23:52:28, tfarina wrote:
> On 2010/08/08 22:34:59, Marc-Antoine Ruel wrote:
> > lgtm!
> > 
> > The code is still full of awful limitations like arbitrary limits on name
> length
> > but this refactor is fine as-is.
> > 
> > I've added 2 nits but you can commit anyway. Thanks.
> >  
> > http://codereview.chromium.org/3007037/diff/3003/34005
> > File chrome/installer/util/google_update_settings_unittest.cc (right):
> > 
> > http://codereview.chromium.org/3007037/diff/3003/34005#newcode18
> > chrome/installer/util/google_update_settings_unittest.cc:18: const wchar_t
> > kHKCUReplacement[] =
> > In fact I prefer const wchar_t* const (const pointer to const data) than an
> > array. I agree that a non-const global pointer was a bad idea!
> 
> The best answer I know for this is that const char[] = "foo"; just creates a
> single variable in assembly.
> 
> Quoting from http://kernelnewbies.org/KernelJanitors:
> 
> "
> From: Jeff Garzik
> 
> The string form
> 
>         [const] char *foo = "blah";
> creates two variables in the final assembly output, a static string,
> and a char pointer to the static string. The alternate string form
> 
>         [const] char foo[] = "blah";
> is better because it declares a single variable.
> "
> 
> Logically there is a size difference between these two forms, the latter saves
> some memory too.

And that's exactly not what I said; I said to use the form:
  const char* const foo = "blah";

As a shortcut:
1) const char* foo = "blah";
2) const char foo[] = "blah";
3) const char* const foo = "blah";

Form 1) is just plain bad, it declares a non const global variable. It should
never be used.
Form 2) only creates one symbol, the data.
Form 3) causes the compiler to create 2 symbols: the pointer and the data. The
nice thing is that the compiler can inline the actual pointer since it's a const
value, but only if there is link-time optimization or if the pointer is only
used through the source file it is defined in. So when it's optimized in, only
the actual data remains.

Now things become interesting when people define their const strings into an
empty namespace or declare it a static variable.
For form 2), the data isn't shared since the data is using two different symbol
names.
For form 3), the data is shared since the actual data is nameless.

Sadly, http://msdn.microsoft.com/en-us/library/s0s0asdt.aspx is quite thin on
details of the inner working.

It always depends on the optimization options, etc. Anyway, no big deal.

M-A