Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
275103012:
Add WOW64 support and DeleteEmptyKey to base::win::registry. (Closed)
|
Created:
6 years, 7 months ago by Will Harris Modified:
6 years, 7 months ago CC:
chromium-reviews, erikwright+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd WOW64 support to DeleteKey. Add DeleteEmptyKey to mimic SHDeleteEmptyKey. Add lots of WOW64 tests. Add a DeleteKey and a CreateKey test.
These additions are needed to support Win64 installer logic.
BUG=338706, 348626
TEST=base_unittests --gtest_filter=Registry* on XP (x86), Vista (x86/x64), Win7 (x86/x64).
TEST=installer_util_unittests
R=cpu@chromium.org, grt@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272213
Patch Set 1 #Patch Set 2 : de-hungarian the variable names #Patch Set 3 : separate the wow64 code from the bug fixes #
Total comments: 52
Patch Set 4 : code review changes #
Total comments: 65
Patch Set 5 : more code review changes #Patch Set 6 : add DeleteEmptyKey #
Total comments: 24
Patch Set 7 : moar code review changes #
Total comments: 6
Patch Set 8 : formatting nits. move static declaration. add todo. #Patch Set 9 : allow DeleteKey to work on empty keys. Remove Wow64ConflictingViews test #
Total comments: 2
Patch Set 10 : Vista 64 doesn't allow opening WOW6432 node when KEY_WOW64_64KEY is specified. #Patch Set 11 : remove Wow6432NodeFromRedirected test #Patch Set 12 : rebase #
Messages
Total messages: 32 (0 generated)
wfh, thanks for doing this, I think you just showed in time to do some cleanup here. Lets split this CL into two, cleanup and the bug you found and the 64-bit support in a second CL. The cleanup part of the first CL is removing the MAX_PATH stuff from the stack, this should be heap allocated possibly via std::string --> WriteInto wrapper.
On 2014/05/14 17:29:48, cpu wrote: > wfh, thanks for doing this, I think you just showed in time to do some cleanup > here. > > Lets split this CL into two, cleanup and the bug you found and the 64-bit > support in a second CL. > > The cleanup part of the first CL is removing the MAX_PATH stuff from the stack, > this should be heap allocated possibly via std::string --> WriteInto wrapper. PTAL - this CL now just contains the WOW64 code changes. Also moved all allocations to the heap.
+grt
thanks for taking this on! the volume of comments is a sign of affection. :-) https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc File base/win/registry.cc (left): https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#old... base/win/registry.cc:14: #pragma comment(lib, "shlwapi.lib") // for SHDeleteKey w00t! https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc File base/win/registry.cc (right): https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:78: wow64access_ = access & KEY_WOW64_RES; looks like this is an undocumented constant in winnt.h. for safety's sake, wdyt of making a constant in the unnamed namespace for this: const REGSAM kWow64AccessMask = KEY_WOW64_32KEY | KEY_WOW64_64KEY; https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:177: LONG result = RegOpenKeyEx(key_, name, 0, KEY_READ | wow64access_, &subkey); if this is just an existence test, i think READ_CONTROL is the minimal access needed. https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:178: if (result == ERROR_SUCCESS) { suggestion: if (result != ERROR_SUCCESS) return result; ... https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:180: std::wstring keyname(name); new code should use string16. would you mind switching registry.{h,cc,_unittest.cc} wholesale in a separate change? https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:182: return RegKey::RegDelRecurse(key_, keyname, wow64access_); remove RegKey:: https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:359: LONG RegKey::RegDeleteKeyExWrapper(HKEY hKey, use chromium naming style rather than hungarian https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:366: if (base::win::GetVersion() >= VERSION_VISTA) { some versions prior to vista have RegDeleteKeyEx, so i think it's better to unconditionally try GetProcAddress and fallback to RegDeleteKey any time it isn't found. https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:381: // static this isn't a static method according to the header. https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:382: LONG RegKey::RegDelRecurse(HKEY root_key, std::wstring name, REGSAM access) { pass name by const ref https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:386: HKEY hKey; no hungarians allowed. i suggest target_key or something since this is a handle to the key to be deleted. https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:387: FILETIME ft; is this needed? msdn says that the last param to EnumKeyEx can be NULL. https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:395: result = RegOpenKeyEx(root_key, name.c_str(), 0, KEY_READ | access, &hKey); request the least required access: KEY_READ -> KEY_ENUMERATE_SUB_KEYS https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:397: if (result != ERROR_SUCCESS) { if (result == ERROR_FILE_NOT_FOUND) return ERROR_SUCCESS if (result != ERROR_SUCCESS) return result; https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:406: if (name[name.length() - 1] != L'\\') what if |name| is empty? perhaps the top of the function should have: if (name.empty()) { NOTREACHED(); return ERROR_INVALID_PARAMETER; } https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:411: result = RegEnumKeyEx(hKey, 0, key_name, &key_size, NULL, NULL, NULL, &ft); i think cpu already mentioned this, but do something like this to have the key name on the heap instead of the stack: const DWORD kMaxKeyNameLength = MAX_PATH; string16 key_name; DWORD key_name_length = kMaxKeyNameLength; result = RegEnumKeyEx(hKey, 0, WriteInto(&key_name, kMaxKeyNameLength), &key_name_length, NULL, NULL, NULL, NULL); if (result == ERROR_SUCCESS) key_name.resize(key_name_length); https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:413: if (result == ERROR_SUCCESS) { can you get rid of if { do { with: for (; result == ERROR_SUCCESS; ) { https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:415: std::wstring full_name(name); pull full_name's definition out of the loop https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:424: NULL, NULL, &ft); shouldn't this be aligned with hKey above? did "git cl format" format this? https://codereview.chromium.org/275103012/diff/40001/base/win/registry.h File base/win/registry.h (right): https://codereview.chromium.org/275103012/diff/40001/base/win/registry.h#newc... base/win/registry.h:135: static LONG RegDeleteKeyExWrapper(HKEY hKey, const wchar_t* lpSubKey, add doc string https://codereview.chromium.org/275103012/diff/40001/base/win/registry.h#newc... base/win/registry.h:137: LONG RegDelRecurse(HKEY root_key, std::wstring name, REGSAM access); add doc string https://codereview.chromium.org/275103012/diff/40001/base/win/registry_unitte... File base/win/registry_unittest.cc (right): https://codereview.chromium.org/275103012/diff/40001/base/win/registry_unitte... base/win/registry_unittest.cc:12: #include "base/win/windows_version.h" unused https://codereview.chromium.org/275103012/diff/40001/base/win/registry_unitte... base/win/registry_unittest.cc:43: KEY_SET_VALUE | KEY_WOW64_32KEY)); indentation. please try "git cl format". it wraps the conditional operator wrong, though. :-( https://codereview.chromium.org/275103012/diff/40001/base/win/registry_unitte... base/win/registry_unittest.cc:172: TEST_F(RegistryTest, RecursiveDelete) { DeleteKey was untested before?!??!? https://codereview.chromium.org/275103012/diff/40001/base/win/registry_unitte... base/win/registry_unittest.cc:207: // registry writes to HKLM\Software can the Administrator requirement be removed by making the test operate in HKEY_CURRENT_USER\Software\Classes or one of its subkeys that is redirected/reflected? https://codereview.chromium.org/275103012/diff/40001/base/win/registry_unitte... base/win/registry_unittest.cc:219: // Test 32-bit key access from 64-bit. it looks like a lot of the _WIN64 part and the non-_WIN64 part are the same except for some constants. could you split this into a few small TEST functions that test individual aspects of this? for example, you could do something like: class RegistryTest { protected: // Returns true if the current process is influenced by the registry redirector. static bool IsRedirectorPresent() { #if defined(_WIN64) return true; #else return os_info->wow64_status() == OSInfo::WOW64_ENABLED; #endif } static REGASM GetOppositeViewMask() { DCHECK(IsRedirectoryPresent()); #if defined(_WIN64) return KEY_WOW64_32KEY; #else return KEY_WOW64_64KEY; #endif } }; TEST_F(RegistryTest, AccessOpposite) { // early exit if (!IsRedirectorPresent()) return; // test accessing either 32-bit from 64-bit, or 64-bit from 32-bit // using GetOppositeViewMask() } TEST_F(RegistryTest, DeleteOpposite) { // early exit if (!IsRedirectorPresent()) return; // test deleting either 32-bit from 64-bit, or 64-bit from 32-bit // using GetOppositeViewMask() } ...
https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc File base/win/registry.cc (left): https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#old... base/win/registry.cc:14: #pragma comment(lib, "shlwapi.lib") // for SHDeleteKey On 2014/05/16 15:44:43, grt wrote: > w00t! :D https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc File base/win/registry.cc (right): https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:78: wow64access_ = access & KEY_WOW64_RES; On 2014/05/16 15:44:43, grt wrote: > looks like this is an undocumented constant in winnt.h. for safety's sake, wdyt > of making a constant in the unnamed namespace for this: > > const REGSAM kWow64AccessMask = KEY_WOW64_32KEY | KEY_WOW64_64KEY; Done. https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:177: LONG result = RegOpenKeyEx(key_, name, 0, KEY_READ | wow64access_, &subkey); On 2014/05/16 15:44:43, grt wrote: > if this is just an existence test, i think READ_CONTROL is the minimal access > needed. Done. https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:178: if (result == ERROR_SUCCESS) { On 2014/05/16 15:44:43, grt wrote: > suggestion: > if (result != ERROR_SUCCESS) > return result; > ... Done. https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:180: std::wstring keyname(name); On 2014/05/16 15:44:43, grt wrote: > new code should use string16. would you mind switching > registry.{h,cc,_unittest.cc} wholesale in a separate change? will do in another CL https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:182: return RegKey::RegDelRecurse(key_, keyname, wow64access_); On 2014/05/16 15:44:43, grt wrote: > remove RegKey:: Done. https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:359: LONG RegKey::RegDeleteKeyExWrapper(HKEY hKey, On 2014/05/16 15:44:43, grt wrote: > use chromium naming style rather than hungarian Was intentionally using these since it's a wrapper to an MS function. https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:366: if (base::win::GetVersion() >= VERSION_VISTA) { On 2014/05/16 15:44:43, grt wrote: > some versions prior to vista have RegDeleteKeyEx, so i think it's better to > unconditionally try GetProcAddress and fallback to RegDeleteKey any time it > isn't found. Done. We need to support these Windows XP 64-bit users! :) https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:381: // static On 2014/05/16 15:44:43, grt wrote: > this isn't a static method according to the header. Good spot - the static keyword was accidently dropped when I changed the function prototype. https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:382: LONG RegKey::RegDelRecurse(HKEY root_key, std::wstring name, REGSAM access) { On 2014/05/16 15:44:43, grt wrote: > pass name by const ref Done. https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:386: HKEY hKey; On 2014/05/16 15:44:43, grt wrote: > no hungarians allowed. i suggest target_key or something since this is a handle > to the key to be deleted. Done. https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:387: FILETIME ft; On 2014/05/16 15:44:43, grt wrote: > is this needed? msdn says that the last param to EnumKeyEx can be NULL. Done. https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:395: result = RegOpenKeyEx(root_key, name.c_str(), 0, KEY_READ | access, &hKey); On 2014/05/16 15:44:43, grt wrote: > request the least required access: > KEY_READ -> KEY_ENUMERATE_SUB_KEYS Done. https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:397: if (result != ERROR_SUCCESS) { On 2014/05/16 15:44:43, grt wrote: > if (result == ERROR_FILE_NOT_FOUND) > return ERROR_SUCCESS > if (result != ERROR_SUCCESS) > return result; Done. https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:406: if (name[name.length() - 1] != L'\\') On 2014/05/16 15:44:43, grt wrote: > what if |name| is empty? perhaps the top of the function should have: > if (name.empty()) { > NOTREACHED(); > return ERROR_INVALID_PARAMETER; > } Done. https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:411: result = RegEnumKeyEx(hKey, 0, key_name, &key_size, NULL, NULL, NULL, &ft); On 2014/05/16 15:44:43, grt wrote: > i think cpu already mentioned this, but do something like this to have the key > name on the heap instead of the stack: > const DWORD kMaxKeyNameLength = MAX_PATH; > string16 key_name; > DWORD key_name_length = kMaxKeyNameLength; > result = RegEnumKeyEx(hKey, 0, WriteInto(&key_name, kMaxKeyNameLength), > &key_name_length, NULL, NULL, NULL, NULL); > if (result == ERROR_SUCCESS) > key_name.resize(key_name_length); Done. https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:413: if (result == ERROR_SUCCESS) { On 2014/05/16 15:44:43, grt wrote: > can you get rid of if { do { with: > for (; result == ERROR_SUCCESS; ) { Done. https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:415: std::wstring full_name(name); On 2014/05/16 15:44:43, grt wrote: > pull full_name's definition out of the loop Done. https://codereview.chromium.org/275103012/diff/40001/base/win/registry.cc#new... base/win/registry.cc:424: NULL, NULL, &ft); On 2014/05/16 15:44:43, grt wrote: > shouldn't this be aligned with hKey above? did "git cl format" format this? Done. https://codereview.chromium.org/275103012/diff/40001/base/win/registry.h File base/win/registry.h (right): https://codereview.chromium.org/275103012/diff/40001/base/win/registry.h#newc... base/win/registry.h:135: static LONG RegDeleteKeyExWrapper(HKEY hKey, const wchar_t* lpSubKey, On 2014/05/16 15:44:43, grt wrote: > add doc string Done. https://codereview.chromium.org/275103012/diff/40001/base/win/registry.h#newc... base/win/registry.h:137: LONG RegDelRecurse(HKEY root_key, std::wstring name, REGSAM access); On 2014/05/16 15:44:43, grt wrote: > add doc string Done. https://codereview.chromium.org/275103012/diff/40001/base/win/registry_unitte... File base/win/registry_unittest.cc (right): https://codereview.chromium.org/275103012/diff/40001/base/win/registry_unitte... base/win/registry_unittest.cc:12: #include "base/win/windows_version.h" On 2014/05/16 15:44:43, grt wrote: > unused used for OSInfo below https://codereview.chromium.org/275103012/diff/40001/base/win/registry_unitte... base/win/registry_unittest.cc:43: KEY_SET_VALUE | KEY_WOW64_32KEY)); On 2014/05/16 15:44:43, grt wrote: > indentation. please try "git cl format". it wraps the conditional operator > wrong, though. :-( Done. https://codereview.chromium.org/275103012/diff/40001/base/win/registry_unitte... base/win/registry_unittest.cc:172: TEST_F(RegistryTest, RecursiveDelete) { On 2014/05/16 15:44:43, grt wrote: > DeleteKey was untested before?!??!? Yes - scary. I tested the old SHDeleteKey code with this new test to verify the new behavior was identical. https://codereview.chromium.org/275103012/diff/40001/base/win/registry_unitte... base/win/registry_unittest.cc:207: // registry writes to HKLM\Software On 2014/05/16 15:44:43, grt wrote: > can the Administrator requirement be removed by making the test operate in > HKEY_CURRENT_USER\Software\Classes or one of its subkeys that is > redirected/reflected? HKLM\Software is the only key that is universally redirected on all versions of Windows. HKCU\Software\Classes is reflected below Win7, and so would have required a load of windows version specific test code. https://codereview.chromium.org/275103012/diff/40001/base/win/registry_unitte... base/win/registry_unittest.cc:219: // Test 32-bit key access from 64-bit. On 2014/05/16 15:44:43, grt wrote: > it looks like a lot of the _WIN64 part and the non-_WIN64 part are the same > except for some constants. could you split this into a few small TEST functions > that test individual aspects of this? for example, you could do something like: > > class RegistryTest { > protected: > // Returns true if the current process is influenced by the registry > redirector. > static bool IsRedirectorPresent() { > #if defined(_WIN64) > return true; > #else > return os_info->wow64_status() == OSInfo::WOW64_ENABLED; > #endif > } > static REGASM GetOppositeViewMask() { > DCHECK(IsRedirectoryPresent()); > #if defined(_WIN64) > return KEY_WOW64_32KEY; > #else > return KEY_WOW64_64KEY; > #endif > } > }; > > TEST_F(RegistryTest, AccessOpposite) { > // early exit > if (!IsRedirectorPresent()) > return; > // test accessing either 32-bit from 64-bit, or 64-bit from 32-bit > // using GetOppositeViewMask() > } > > TEST_F(RegistryTest, DeleteOpposite) { > // early exit > if (!IsRedirectorPresent()) > return; > // test deleting either 32-bit from 64-bit, or 64-bit from 32-bit > // using GetOppositeViewMask() > } > > ... I went ahead and split this test up a bit.
i haven't looked at the test yet, but here are more comments for the implementation. sorry i missed some things on the first pass. https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc File base/win/registry.cc (right): https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:41: watch_event_(0) { wow64access_(0) https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:46: watch_event_(0) { this ctor needs to take the wow64 bits of |key| as a parameter, right? if that's the way to go, and those are the only two bits that should ever be set, i would also suggest a: DCHECK_EQ(0, (access & ~kWow64AccessMask)); to make sure no other bits are set https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:49: RegKey::RegKey(HKEY rootkey, const wchar_t* subkey, REGSAM access) wow64access_(0) what does it mean for rootkey and subkey to both be 0/NULL? does anyone call this that way? maybe this should be just after the DCHECK on line 58: wow64access_ = access & kWow64AccessMask; https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:87: LONG RegKey::CreateKey(const wchar_t* name, REGSAM access) { msdn says "After the application has accessed an alternate registry view using one of the flags, all subsequent operations (create, delete, or open) on child registry keys must explicitly use the same flag." it seems that this function should enforce that. one way would be to have something like: if ((access & wow64access_) != wow64access_) return ERROR_INVALID_PARAMETER; https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:115: LONG RegKey::OpenKey(const wchar_t* relative_key_name, REGSAM access) { same comment here about enforcing that |access| contains the same bits as wow64access_ https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:134: key_ = NULL; wow64access_ = 0; https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:138: void RegKey::Set(HKEY key) { this needs to take a REGSAM for the wow64 bits, no? (with a DCHECK_EQ(0, (access & ~kWow64AccessMask));)? https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:148: key_ = NULL; wow64access_ = 0; https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:185: std::wstring keyname(name); nit: put this inline with the recursive call: return RegDelRecurse(key_, std::wstring(name), wow64access_); https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:365: typedef LSTATUS(WINAPI * RegDeleteKeyExPtr)( WINAPI* https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:366: HKEY hKey, LPCWSTR lpSubKey, REGSAM samDesired, DWORD Reserved); nit: remove the parameter names https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:381: std::wstring const& name, const std::wstring& https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:386: size_t name_length = name.length(); remove this and change line 388 to if (name.empty()) https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:394: result = RegDeleteKeyExWrapper(root_key, name.c_str(), access, 0); the style guide says to define local vars "as close to the first use as possible" (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Local_Variables), so move the definition of result here: LONG result = ... https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:399: result = RegOpenKeyEx( put the definition of target_key just above this line: HKEY target_key = NULL; https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:418: DWORD kMaxKeyNameLength = MAX_PATH; const DWORD https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:419: define this here: const size_t base_key_lenght = subkey_name.length(); https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:421: std::wstring key_name; move the definition of key_name out of the loop. https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:436: subkey_name.resize(name_length); name_length -> base_key_lenght https://codereview.chromium.org/275103012/diff/60001/base/win/registry.h File base/win/registry.h (right): https://codereview.chromium.org/275103012/diff/60001/base/win/registry.h#newc... base/win/registry.h:135: // Wrapper function for RegDeleteKeyEx which is not supported on all Please make the comments descriptive as per the style guide (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Comments). For example: // Calls RegDeleteKeyEx on supported platforms. Otherwise, attempts to delete the key via RegDeleteKey. https://codereview.chromium.org/275103012/diff/60001/base/win/registry.h#newc... base/win/registry.h:142: // Internal function to perform a recursive registry key delete. suggestion: // Recursively deletes a key and all of its subkeys. https://codereview.chromium.org/275103012/diff/60001/base/win/registry.h#newc... base/win/registry.h:144: std::wstring const& name, const std::wstring&
some test comments https://codereview.chromium.org/275103012/diff/60001/base/win/registry_unitte... File base/win/registry_unittest.cc (right): https://codereview.chromium.org/275103012/diff/60001/base/win/registry_unitte... base/win/registry_unittest.cc:23: const REGSAM kNativeViewMask = KEY_WOW64_64KEY; make these static const protected members of the test fixture. may as well do the same to kRootKey. i can't think of any advantage to having it hanging out in namespace scope like that. https://codereview.chromium.org/275103012/diff/60001/base/win/registry_unitte... base/win/registry_unittest.cc:32: RegistryTest() { while you're here, please move this ctor into the protected: section. https://codereview.chromium.org/275103012/diff/60001/base/win/registry_unitte... base/win/registry_unittest.cc:33: foo_software_key_ = L"Software\\"; may as well do this work in SetUp so that the fixture initialization isn't divided between the ctor and SetUp methods. https://codereview.chromium.org/275103012/diff/60001/base/win/registry_unitte... base/win/registry_unittest.cc:61: key.DeleteKey(kRootKey); ? EXPECT_EQ(ERROR_SUCCESS, key.DeleteKey(kRootKey));? below, too? https://codereview.chromium.org/275103012/diff/60001/base/win/registry_unitte... base/win/registry_unittest.cc:70: std::wstring foo_software_key_; move these down to the end of the protected: section (non-const data members come last; http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order). https://codereview.chromium.org/275103012/diff/60001/base/win/registry_unitte... base/win/registry_unittest.cc:79: return os_info->wow64_status() == OSInfo::WOW64_ENABLED; return OSInfo::GetInstance()->wow64_status() == OSInfo::WOW64_ENABLED; https://codereview.chromium.org/275103012/diff/60001/base/win/registry_unitte... base/win/registry_unittest.cc:242: if (IsRedirectorPresent()) { please have these early-exit in the case where the test doesn't apply. it avoids having the whole body of the test indented an extra two spaces and makes it more clear that the whole test is skipped when there's no redirection. https://codereview.chromium.org/275103012/diff/60001/base/win/registry_unitte... base/win/registry_unittest.cc:271: // Open the redirected view and delete the key created above. https://codereview.chromium.org/275103012/diff/60001/base/win/registry_unitte... base/win/registry_unittest.cc:317: // Test access to 32-bit values on 64-bit via the Wow6432Node key. is this worth testing? i thought Wow6432Node was an implementation detail and that apps aren't supposed to try to use it directly. https://codereview.chromium.org/275103012/diff/60001/base/win/registry_unitte... base/win/registry_unittest.cc:345: // 32-bit on 32-bit shouldn't even see a Wow6432Node key. this also seems to be testing an implementation detail if windows rather than correct behavior of RegKey
https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc File base/win/registry.cc (right): https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:87: LONG RegKey::CreateKey(const wchar_t* name, REGSAM access) { On 2014/05/17 12:54:33, grt wrote: > msdn says "After the application has accessed an alternate registry view using > one of the flags, all subsequent operations (create, delete, or open) on child > registry keys must explicitly use the same flag." it seems that this function > should enforce that. one way would be to have something like: > > if ((access & wow64access_) != wow64access_) > return ERROR_INVALID_PARAMETER; very good catch.
Hopefully this is looking good - still getting used to Chromium standards after my oppressive C89 upbringing. Take() and Set() semantics seem like the only contentious part. I prefer leaving the function API the same and just failing if the key was previously opened with WOW64 semantics. https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc File base/win/registry.cc (right): https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:41: watch_event_(0) { On 2014/05/17 12:54:33, grt wrote: > wow64access_(0) Done. https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:46: watch_event_(0) { On 2014/05/17 12:54:33, grt wrote: > this ctor needs to take the wow64 bits of |key| as a parameter, right? if that's > the way to go, and those are the only two bits that should ever be set, i would > also suggest a: > DCHECK_EQ(0, (access & ~kWow64AccessMask)); > to make sure no other bits are set HEKY is not RegKey - no way to change/modify access here. https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:49: RegKey::RegKey(HKEY rootkey, const wchar_t* subkey, REGSAM access) On 2014/05/17 12:54:33, grt wrote: > wow64access_(0) > > what does it mean for rootkey and subkey to both be 0/NULL? does anyone call > this that way? maybe this should be just after the DCHECK on line 58: > wow64access_ = access & kWow64AccessMask; yes, good spot, I looked through the code and can't see any callers to this path, but it's best to be safe here. https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:87: LONG RegKey::CreateKey(const wchar_t* name, REGSAM access) { On 2014/05/17 12:54:33, grt wrote: > msdn says "After the application has accessed an alternate registry view using > one of the flags, all subsequent operations (create, delete, or open) on child > registry keys must explicitly use the same flag." it seems that this function > should enforce that. one way would be to have something like: > > if ((access & wow64access_) != wow64access_) > return ERROR_INVALID_PARAMETER; Done. https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:115: LONG RegKey::OpenKey(const wchar_t* relative_key_name, REGSAM access) { On 2014/05/17 12:54:33, grt wrote: > same comment here about enforcing that |access| contains the same bits as > wow64access_ Done. https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:134: key_ = NULL; On 2014/05/17 12:54:33, grt wrote: > wow64access_ = 0; Done. https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:138: void RegKey::Set(HKEY key) { On 2014/05/17 12:54:33, grt wrote: > this needs to take a REGSAM for the wow64 bits, no? (with a DCHECK_EQ(0, (access > & ~kWow64AccessMask));)? it's not possible to check that key was opened with a particular access mask, and thus check that the access mask being passed here is correct. Either we have to change Take() to return both key_ and wow64access_ so it can be passed into Set(), or we just deny a Take() if the wow64access_ is specified. There's still nothing to stop someone from passing a key opened with a WOW64 flag into Set() here. https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:148: key_ = NULL; On 2014/05/17 12:54:33, grt wrote: > wow64access_ = 0; Done. https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:185: std::wstring keyname(name); On 2014/05/17 12:54:33, grt wrote: > nit: put this inline with the recursive call: > return RegDelRecurse(key_, std::wstring(name), wow64access_); Done. https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:365: typedef LSTATUS(WINAPI * RegDeleteKeyExPtr)( On 2014/05/17 12:54:33, grt wrote: > WINAPI* this was git gl format :) https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:366: HKEY hKey, LPCWSTR lpSubKey, REGSAM samDesired, DWORD Reserved); On 2014/05/17 12:54:33, grt wrote: > nit: remove the parameter names Done. https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:381: std::wstring const& name, On 2014/05/17 12:54:33, grt wrote: > const std::wstring& Done. https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:386: size_t name_length = name.length(); On 2014/05/17 12:54:33, grt wrote: > remove this and change line 388 to if (name.empty()) Done. https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:394: result = RegDeleteKeyExWrapper(root_key, name.c_str(), access, 0); On 2014/05/17 12:54:33, grt wrote: > the style guide says to define local vars "as close to the first use as > possible" > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Local_Variables), > so move the definition of result here: > LONG result = ... Done. https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:399: result = RegOpenKeyEx( On 2014/05/17 12:54:33, grt wrote: > put the definition of target_key just above this line: > HKEY target_key = NULL; Done. https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:418: DWORD kMaxKeyNameLength = MAX_PATH; On 2014/05/17 12:54:33, grt wrote: > const DWORD Done. https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:419: On 2014/05/17 12:54:33, grt wrote: > define this here: > const size_t base_key_lenght = subkey_name.length(); Done. https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:421: std::wstring key_name; On 2014/05/17 12:54:33, grt wrote: > move the definition of key_name out of the loop. Done. https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:436: subkey_name.resize(name_length); On 2014/05/17 12:54:33, grt wrote: > name_length -> base_key_lenght Done. https://codereview.chromium.org/275103012/diff/60001/base/win/registry.h File base/win/registry.h (right): https://codereview.chromium.org/275103012/diff/60001/base/win/registry.h#newc... base/win/registry.h:135: // Wrapper function for RegDeleteKeyEx which is not supported on all On 2014/05/17 12:54:33, grt wrote: > Please make the comments descriptive as per the style guide > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Comments). > For example: > > // Calls RegDeleteKeyEx on supported platforms. Otherwise, attempts to delete > the key via RegDeleteKey. Done. https://codereview.chromium.org/275103012/diff/60001/base/win/registry.h#newc... base/win/registry.h:142: // Internal function to perform a recursive registry key delete. On 2014/05/17 12:54:33, grt wrote: > suggestion: > // Recursively deletes a key and all of its subkeys. Done. https://codereview.chromium.org/275103012/diff/60001/base/win/registry.h#newc... base/win/registry.h:144: std::wstring const& name, On 2014/05/17 12:54:33, grt wrote: > const std::wstring& Done. https://codereview.chromium.org/275103012/diff/60001/base/win/registry_unitte... File base/win/registry_unittest.cc (right): https://codereview.chromium.org/275103012/diff/60001/base/win/registry_unitte... base/win/registry_unittest.cc:23: const REGSAM kNativeViewMask = KEY_WOW64_64KEY; On 2014/05/17 19:31:53, grt wrote: > make these static const protected members of the test fixture. > may as well do the same to kRootKey. i can't think of any advantage to having it > hanging out in namespace scope like that. Done. https://codereview.chromium.org/275103012/diff/60001/base/win/registry_unitte... base/win/registry_unittest.cc:32: RegistryTest() { On 2014/05/17 19:31:53, grt wrote: > while you're here, please move this ctor into the protected: section. Done. https://codereview.chromium.org/275103012/diff/60001/base/win/registry_unitte... base/win/registry_unittest.cc:33: foo_software_key_ = L"Software\\"; On 2014/05/17 19:31:53, grt wrote: > may as well do this work in SetUp so that the fixture initialization isn't > divided between the ctor and SetUp methods. Done. https://codereview.chromium.org/275103012/diff/60001/base/win/registry_unitte... base/win/registry_unittest.cc:70: std::wstring foo_software_key_; On 2014/05/17 19:31:53, grt wrote: > move these down to the end of the protected: section (non-const data members > come last; > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order). Done. https://codereview.chromium.org/275103012/diff/60001/base/win/registry_unitte... base/win/registry_unittest.cc:79: return os_info->wow64_status() == OSInfo::WOW64_ENABLED; On 2014/05/17 19:31:53, grt wrote: > return OSInfo::GetInstance()->wow64_status() == OSInfo::WOW64_ENABLED; Done. https://codereview.chromium.org/275103012/diff/60001/base/win/registry_unitte... base/win/registry_unittest.cc:242: if (IsRedirectorPresent()) { On 2014/05/17 19:31:53, grt wrote: > please have these early-exit in the case where the test doesn't apply. it avoids > having the whole body of the test indented an extra two spaces and makes it more > clear that the whole test is skipped when there's no redirection. Done. https://codereview.chromium.org/275103012/diff/60001/base/win/registry_unitte... base/win/registry_unittest.cc:271: On 2014/05/17 19:31:53, grt wrote: > // Open the redirected view and delete the key created above. Done. https://codereview.chromium.org/275103012/diff/60001/base/win/registry_unitte... base/win/registry_unittest.cc:317: // Test access to 32-bit values on 64-bit via the Wow6432Node key. On 2014/05/17 19:31:53, grt wrote: > is this worth testing? i thought Wow6432Node was an implementation detail and > that apps aren't supposed to try to use it directly. earlier tests just confirm that when KEY_WOW64_32KEY is passed, the keys go into a different place in the registry, but don't actually test that they go into the right place in the registry... I think this test is still worth having. https://codereview.chromium.org/275103012/diff/60001/base/win/registry_unitte... base/win/registry_unittest.cc:345: // 32-bit on 32-bit shouldn't even see a Wow6432Node key. On 2014/05/17 19:31:53, grt wrote: > this also seems to be testing an implementation detail if windows rather than > correct behavior of RegKey Happy for this to be removed.
Added DeleteEmptyKey. https://codereview.chromium.org/275103012/diff/100001/base/win/registry.cc File base/win/registry.cc (right): https://codereview.chromium.org/275103012/diff/100001/base/win/registry.cc#ne... base/win/registry.cc:208: LONG RegKey::DeleteEmptyKey(const wchar_t* name) { added DeleteEmptyKey as a WOW64 compliant replacement for SHDeleteEmptyKey, which is used in the installer code.
looking good. mostly down to nits and the three methods that don't fit in (Set, Take, and the RegKey(HEKY) ctor). i think the best thing to do is to remove these unsafe methods from the API altogether. any code currently using them can be made to use the Win32 API directly if there's no other way. i think the benefits of having a clean, consistent API are worth it. it looks like a very simple job to change chrome\app\chrome_breakpad_client.cc to use Win32 directly and chrome\browser\chrome_elf_init_win.cc to use RegKey slightly differently. https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc File base/win/registry.cc (right): https://codereview.chromium.org/275103012/diff/60001/base/win/registry.cc#new... base/win/registry.cc:365: typedef LSTATUS(WINAPI * RegDeleteKeyExPtr)( On 2014/05/19 21:49:03, Will Harris wrote: > On 2014/05/17 12:54:33, grt wrote: > > WINAPI* > > this was git gl format :) Oh. How 'bout that. May as well do as it says, although it looks out of place to me given the normal formatting. Feel free to go with git cl format and ignore me. https://codereview.chromium.org/275103012/diff/100001/base/win/registry.cc File base/win/registry.cc (right): https://codereview.chromium.org/275103012/diff/100001/base/win/registry.cc#ne... base/win/registry.cc:99: return ERROR_INVALID_PARAMETER; i had a thought about this yesterday after sending the review comment. hitting this statement means that the RegKey API is being mis-used. whaddya think about NOTREACHED(); just before the return so that debug builds blow up fast? this will make the mis-use much easier to diagnose. https://codereview.chromium.org/275103012/diff/100001/base/win/registry.cc#ne... base/win/registry.cc:133: return ERROR_INVALID_PARAMETER; NOTREACHED(); here as well? https://codereview.chromium.org/275103012/diff/100001/base/win/registry.cc#ne... base/win/registry.cc:195: HKEY subkey; nit: initialize this to NULL. https://codereview.chromium.org/275103012/diff/100001/base/win/registry.cc#ne... base/win/registry.cc:208: LONG RegKey::DeleteEmptyKey(const wchar_t* name) { On 2014/05/19 23:47:25, Will Harris wrote: > added DeleteEmptyKey as a WOW64 compliant replacement for SHDeleteEmptyKey, > which is used in the installer code. According to MSDN, SHDeleteEmptyKey won't delete the key if it has values. If RegKey::DeleteEmptyKey is a replacement for it, shouldn't this function behave the same? https://codereview.chromium.org/275103012/diff/100001/base/win/registry.cc#ne... base/win/registry.cc:414: nit: remove empty line https://codereview.chromium.org/275103012/diff/100001/base/win/registry.cc#ne... base/win/registry.cc:418: HKEY target_key; nit: initialize to NULL https://codereview.chromium.org/275103012/diff/100001/base/win/registry.cc#ne... base/win/registry.cc:427: // Copy string before modifying it. remove unneeded comment (name is const, so it can't be modified). https://codereview.chromium.org/275103012/diff/100001/base/win/registry.cc#ne... base/win/registry.cc:431: if (subkey_name[name.length() - 1] != L'\\') { nit: no braces for single-liner (did "git cl format" add these?) https://codereview.chromium.org/275103012/diff/100001/base/win/registry.cc#ne... base/win/registry.cc:439: nit: remove empty line https://codereview.chromium.org/275103012/diff/100001/base/win/registry.h File base/win/registry.h (right): https://codereview.chromium.org/275103012/diff/100001/base/win/registry.h#new... base/win/registry.h:74: // Delete an empty subkey. If the subkey has subkeys then this will fail. nit: Delete -> Deletes https://codereview.chromium.org/275103012/diff/100001/base/win/registry_unitt... File base/win/registry_unittest.cc (right): https://codereview.chromium.org/275103012/diff/100001/base/win/registry_unitt... base/win/registry_unittest.cc:23: static const REGSAM kNativeViewMask = KEY_WOW64_64KEY; i think you still need definitions of these at namespace scope as per section 9.4.2 of C++-03. i suggest adding the following just after line 67 (before the TEST functions): // static const REGSAM RegistryTest::kNativeViewMask; const REGSAM RegistryTest::kRedirectedViewMask;
All done. PTAL. https://codereview.chromium.org/275103012/diff/100001/base/win/registry.cc File base/win/registry.cc (right): https://codereview.chromium.org/275103012/diff/100001/base/win/registry.cc#ne... base/win/registry.cc:99: return ERROR_INVALID_PARAMETER; On 2014/05/20 15:28:52, grt wrote: > i had a thought about this yesterday after sending the review comment. hitting > this statement means that the RegKey API is being mis-used. whaddya think about > NOTREACHED(); just before the return so that debug builds blow up fast? this > will make the mis-use much easier to diagnose. Done. https://codereview.chromium.org/275103012/diff/100001/base/win/registry.cc#ne... base/win/registry.cc:133: return ERROR_INVALID_PARAMETER; On 2014/05/20 15:28:52, grt wrote: > NOTREACHED(); here as well? Done. https://codereview.chromium.org/275103012/diff/100001/base/win/registry.cc#ne... base/win/registry.cc:195: HKEY subkey; On 2014/05/20 15:28:52, grt wrote: > nit: initialize this to NULL. Done. https://codereview.chromium.org/275103012/diff/100001/base/win/registry.cc#ne... base/win/registry.cc:208: LONG RegKey::DeleteEmptyKey(const wchar_t* name) { On 2014/05/20 15:28:52, grt wrote: > On 2014/05/19 23:47:25, Will Harris wrote: > > added DeleteEmptyKey as a WOW64 compliant replacement for SHDeleteEmptyKey, > > which is used in the installer code. > > According to MSDN, SHDeleteEmptyKey won't delete the key if it has values. If > RegKey::DeleteEmptyKey is a replacement for it, shouldn't this function behave > the same? Done. https://codereview.chromium.org/275103012/diff/100001/base/win/registry.cc#ne... base/win/registry.cc:414: On 2014/05/20 15:28:52, grt wrote: > nit: remove empty line Done. https://codereview.chromium.org/275103012/diff/100001/base/win/registry.cc#ne... base/win/registry.cc:418: HKEY target_key; On 2014/05/20 15:28:52, grt wrote: > nit: initialize to NULL Done. https://codereview.chromium.org/275103012/diff/100001/base/win/registry.cc#ne... base/win/registry.cc:427: // Copy string before modifying it. On 2014/05/20 15:28:52, grt wrote: > remove unneeded comment (name is const, so it can't be modified). Done. https://codereview.chromium.org/275103012/diff/100001/base/win/registry.cc#ne... base/win/registry.cc:431: if (subkey_name[name.length() - 1] != L'\\') { On 2014/05/20 15:28:52, grt wrote: > nit: no braces for single-liner (did "git cl format" add these?) I left those in by accident and git cl format didn't catch it https://codereview.chromium.org/275103012/diff/100001/base/win/registry.cc#ne... base/win/registry.cc:439: On 2014/05/20 15:28:52, grt wrote: > nit: remove empty line Done. https://codereview.chromium.org/275103012/diff/100001/base/win/registry.h File base/win/registry.h (right): https://codereview.chromium.org/275103012/diff/100001/base/win/registry.h#new... base/win/registry.h:74: // Delete an empty subkey. If the subkey has subkeys then this will fail. On 2014/05/20 15:28:52, grt wrote: > nit: Delete -> Deletes Done. https://codereview.chromium.org/275103012/diff/100001/base/win/registry_unitt... File base/win/registry_unittest.cc (right): https://codereview.chromium.org/275103012/diff/100001/base/win/registry_unitt... base/win/registry_unittest.cc:23: static const REGSAM kNativeViewMask = KEY_WOW64_64KEY; On 2014/05/20 15:28:52, grt wrote: > i think you still need definitions of these at namespace scope as per section > 9.4.2 of C++-03. i suggest adding the following just after line 67 (before the > TEST functions): > > // static > const REGSAM RegistryTest::kNativeViewMask; > const REGSAM RegistryTest::kRedirectedViewMask; Okay I read the standard and I think I am now compliant. I think just leaving these definitions in the anonymous namespace as per previous patchset might have also been compliant with the style guide?
almost there! https://codereview.chromium.org/275103012/diff/100001/base/win/registry_unitt... File base/win/registry_unittest.cc (right): https://codereview.chromium.org/275103012/diff/100001/base/win/registry_unitt... base/win/registry_unittest.cc:23: static const REGSAM kNativeViewMask = KEY_WOW64_64KEY; On 2014/05/20 17:03:34, Will Harris wrote: > On 2014/05/20 15:28:52, grt wrote: > > i think you still need definitions of these at namespace scope as per section > > 9.4.2 of C++-03. i suggest adding the following just after line 67 (before the > > TEST functions): > > > > // static > > const REGSAM RegistryTest::kNativeViewMask; > > const REGSAM RegistryTest::kRedirectedViewMask; > > Okay I read the standard and I think I am now compliant. I think just leaving > these definitions in the anonymous namespace as per previous patchset might have > also been compliant with the style guide? You could keep the initializers up here in the class. The bit I was quoting simply says that you still need definitions for the variables. I'm surprised that it compiled without them. It must be a MS-specific "extension" to the language (unless I'm mis-reading the language spec). Yeah, they could be in the unnamed namespace. In my opinion, they're specific to the test fixture so they belong in it. For what it's worth, this fixture should not be in the unnamed namespace, but I didn't want to harass you too much. :-) https://codereview.chromium.org/275103012/diff/120001/base/win/registry.cc File base/win/registry.cc (right): https://codereview.chromium.org/275103012/diff/120001/base/win/registry.cc#ne... base/win/registry.cc:218: &target_key); shouldn't this be aligned with the open paren above? https://codereview.chromium.org/275103012/diff/120001/base/win/registry.cc#ne... base/win/registry.cc:225: NULL, NULL, NULL, NULL); indentation here, too? https://codereview.chromium.org/275103012/diff/120001/base/win/registry.h File base/win/registry.h (right): https://codereview.chromium.org/275103012/diff/120001/base/win/registry.h#new... base/win/registry.h:29: explicit RegKey(HKEY key); please remove this constructor, the Set() method, and the Take method as per my previous CR comment.
please update the CL description since it now does more than just fix DeleteKey. thanks!
almost there! https://codereview.chromium.org/275103012/diff/120001/base/win/registry.cc File base/win/registry.cc (right): https://codereview.chromium.org/275103012/diff/120001/base/win/registry.cc#ne... base/win/registry.cc:218: &target_key); On 2014/05/20 17:46:56, grt wrote: > shouldn't this be aligned with the open paren above? Done. https://codereview.chromium.org/275103012/diff/120001/base/win/registry.cc#ne... base/win/registry.cc:225: NULL, NULL, NULL, NULL); On 2014/05/20 17:46:56, grt wrote: > indentation here, too? Done. https://codereview.chromium.org/275103012/diff/120001/base/win/registry.h File base/win/registry.h (right): https://codereview.chromium.org/275103012/diff/120001/base/win/registry.h#new... base/win/registry.h:29: explicit RegKey(HKEY key); On 2014/05/20 17:46:56, grt wrote: > please remove this constructor, the Set() method, and the Take method as per my > previous CR comment. http://crbug.com/375400 raised, will be done in a subsequent CL as this changes some caller behavior.
lgtm. i've fired off the win and win_rel trybots for you.
On 2014/05/20 19:43:11, grt wrote: > lgtm. i've fired off the win and win_rel trybots for you. bots are blowing up because it turns out that calling SHDeleteKey(subkey, L"") or RegDeleteKey(subkey, L"") deletes subkey. This isn't documented on MSDN, but it seems some of our code relies on this. This is triggering the DCHECK.
https://codereview.chromium.org/275103012/diff/160001/base/win/registry_unitt... File base/win/registry_unittest.cc (right): https://codereview.chromium.org/275103012/diff/160001/base/win/registry_unitt... base/win/registry_unittest.cc:335: foo_software_wow64_key_.c_str(), This reinforces my previous comment that you're testing implementation details of the OS here. MSDN says not to operate on Wow6432Node, so I think you're better off removing foo_software_wow64_key_ altogether.
https://codereview.chromium.org/275103012/diff/160001/base/win/registry_unitt... File base/win/registry_unittest.cc (right): https://codereview.chromium.org/275103012/diff/160001/base/win/registry_unitt... base/win/registry_unittest.cc:335: foo_software_wow64_key_.c_str(), On 2014/05/21 13:52:19, grt wrote: > This reinforces my previous comment that you're testing implementation details > of the OS here. MSDN says not to operate on Wow6432Node, so I think you're > better off removing foo_software_wow64_key_ altogether. Where by "this" I mean the fact that you had to change this test to work on Vista64.
lgtm. I am too somewhat puzzled about the wow32_64, do you plan to use it? I can only see it being useful in the sandbox but I seriously doubt that this base class is up to the tasks of the sandbox.
On 2014/05/21 15:36:56, cpu wrote: > lgtm. > > I am too somewhat puzzled about the wow32_64, do you plan to use it? I can only > see it being useful in the sandbox but I seriously doubt that this base class is > up to the tasks of the sandbox. Agree with both of you. I'll remove the redundant WoW6432 tests. Grt, can you confirm you're happy with the fix to support empty path name in ps9?
On 2014/05/21 15:43:21, Will Harris wrote: > On 2014/05/21 15:36:56, cpu wrote: > > lgtm. > > > > I am too somewhat puzzled about the wow32_64, do you plan to use it? I can > only > > see it being useful in the sandbox but I seriously doubt that this base class > is > > up to the tasks of the sandbox. > > Agree with both of you. I'll remove the redundant WoW6432 tests. > > Grt, can you confirm you're happy with the fix to support empty path name in > ps9? Yes, looks good. Looks like you've added test coverage for it, too, right?
On 2014/05/21 16:45:24, grt wrote: > On 2014/05/21 15:43:21, Will Harris wrote: > > On 2014/05/21 15:36:56, cpu wrote: > > > lgtm. > > > > > > I am too somewhat puzzled about the wow32_64, do you plan to use it? I can > > only > > > see it being useful in the sandbox but I seriously doubt that this base > class > > is > > > up to the tasks of the sandbox. > > > > Agree with both of you. I'll remove the redundant WoW6432 tests. > > > > Grt, can you confirm you're happy with the fix to support empty path name in > > ps9? > > Yes, looks good. Looks like you've added test coverage for it, too, right? Yes, lots of new tests for DeleteKey were added as part of this CL. Thanks for the reviews!
lgtm!
The CQ bit was checked by wfh@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wfh@chromium.org/275103012/190001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/10893)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
On 2014/05/22 08:14:47, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium > (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) Looks like you either need to make the std::wstring -> base::string16 conversion now, or dcommit.
Message was sent while issue was closed.
Committed patchset #12 manually as r272213 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
