Finch Blacklist is now added to the Hardcoded blacklist.

Finch Blacklist is now added to the Hardcoded blacklist.

The finch blacklist is collected from finch in chrome_elf_init_win.cc,
stored in a registry, and added to the blacklist in blacklist.cc.



BUG=

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

[csharp, 2014-05-29 20:51:51]

https://codereview.chromium.org/300933002/diff/120001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_unittest_win.cc (right):

https://codereview.chromium.org/300933002/diff/120001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_unittest_win.cc:183: kBrowserBlacklistTrialName,
kBrowserBlacklistTrialEnabledGroupName));
Since this is only used in this file (I think), just make it a locally constant

https://codereview.chromium.org/300933002/diff/120001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_unittest_win.cc:199:
blacklist::kRegistryFinchListPath,
nit: These lines need to line up H, or all the arguments need to be on a new
line.

https://codereview.chromium.org/300933002/diff/120001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_unittest_win.cc:207: const char* name_str =
(it->first).c_str();
Same comment here about using wstring

https://codereview.chromium.org/300933002/diff/120001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/300933002/diff/120001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:21: const char
kBrowserBlacklistTrialEnabledGroupName[] = "Enabled";
Seems to be unused, remove?

https://codereview.chromium.org/300933002/diff/120001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:113: base::win::RegKey
finch_blacklist_registry_key(HKEY_CURRENT_USER,
Incorrect indent (same for the following two lines). "git cl format" might fix
this for you.

https://codereview.chromium.org/300933002/diff/120001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:117: // No point in trying to continue if
the registry key isn't valid.
Should be indented 2 more spaces

https://codereview.chromium.org/300933002/diff/120001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:123: const char* name_str =
(it->first).c_str();
The converting can be done with:
std::wstring wide_string = base::UTF8ToWide(string);
wide_string.c_str()

less code and no manually frees/deletes :)

https://codereview.chromium.org/300933002/diff/120001/chrome_elf/blacklist/bl...
File chrome_elf/blacklist/blacklist.cc (right):

https://codereview.chromium.org/300933002/diff/120001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:352: void AddDllsFromRegistryToBlacklist() {
Should this return a boolean to indicate failure or success?

https://codereview.chromium.org/300933002/diff/120001/chrome_elf/chrome_elf.def
File chrome_elf/chrome_elf.def (right):

https://codereview.chromium.org/300933002/diff/120001/chrome_elf/chrome_elf.d...
chrome_elf/chrome_elf.def:12: SuccessfullyBlocked
Try to fix as discussed offline

[krstnmnlsn, 2014-05-30 14:09:12]

https://codereview.chromium.org/300933002/diff/120001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_unittest_win.cc (right):

https://codereview.chromium.org/300933002/diff/120001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_unittest_win.cc:183: kBrowserBlacklistTrialName,
kBrowserBlacklistTrialEnabledGroupName));
On 2014/05/29 20:51:52, csharp wrote:
> Since this is only used in this file (I think), just make it a locally
constant

Done.

https://codereview.chromium.org/300933002/diff/120001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_unittest_win.cc:199:
blacklist::kRegistryFinchListPath,
On 2014/05/29 20:51:52, csharp wrote:
> nit: These lines need to line up H, or all the arguments need to be on a new
> line.

Done.

https://codereview.chromium.org/300933002/diff/120001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_unittest_win.cc:207: const char* name_str =
(it->first).c_str();
On 2014/05/29 20:51:52, csharp wrote:
> Same comment here about using wstring

Done.

https://codereview.chromium.org/300933002/diff/120001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/300933002/diff/120001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:21: const char
kBrowserBlacklistTrialEnabledGroupName[] = "Enabled";
On 2014/05/29 20:51:52, csharp wrote:
> Seems to be unused, remove?

Done.

https://codereview.chromium.org/300933002/diff/120001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:113: base::win::RegKey
finch_blacklist_registry_key(HKEY_CURRENT_USER,
On 2014/05/29 20:51:52, csharp wrote:
> Incorrect indent (same for the following two lines). "git cl format" might fix
> this for you.

Done.

https://codereview.chromium.org/300933002/diff/120001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:117: // No point in trying to continue if
the registry key isn't valid.
On 2014/05/29 20:51:52, csharp wrote:
> Should be indented 2 more spaces

Done.

https://codereview.chromium.org/300933002/diff/120001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:123: const char* name_str =
(it->first).c_str();
On 2014/05/29 20:51:52, csharp wrote:
> The converting can be done with:
> std::wstring wide_string = base::UTF8ToWide(string);
> wide_string.c_str()
> 
> less code and no manually frees/deletes :)

this is great!

https://codereview.chromium.org/300933002/diff/120001/chrome_elf/blacklist/bl...
File chrome_elf/blacklist/blacklist.cc (right):

https://codereview.chromium.org/300933002/diff/120001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:352: void AddDllsFromRegistryToBlacklist() {
On 2014/05/29 20:51:52, csharp wrote:
> Should this return a boolean to indicate failure or success? 

Done.

https://codereview.chromium.org/300933002/diff/120001/chrome_elf/chrome_elf.def
File chrome_elf/chrome_elf.def (right):

https://codereview.chromium.org/300933002/diff/120001/chrome_elf/chrome_elf.d...
chrome_elf/chrome_elf.def:12: SuccessfullyBlocked
On 2014/05/29 20:51:52, csharp wrote:
> Try to fix as discussed offline

Done.

[robertshield, 2014-05-30 21:07:08]

Awesome! comments and such below:

https://codereview.chromium.org/300933002/diff/200001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_unittest_win.cc (right):

https://codereview.chromium.org/300933002/diff/200001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_unittest_win.cc:198: // This should add the dll's
in those parameters to the registry.
s/dll's/dlls

https://codereview.chromium.org/300933002/diff/200001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_unittest_win.cc:212: while (it !=
desired_params.end()) {
minor nit, this form:

for (; it != desired_params.end(); ++it) {
...
}

keeps all the loop-related stuff in one place and is slightly more conventional.

https://codereview.chromium.org/300933002/diff/200001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_unittest_win.cc:214: const wchar_t* name_cstr =
name.c_str();
why create the name_cstr temporary?

adding |name| increases readability, but this second one doesn't seem to.

https://codereview.chromium.org/300933002/diff/200001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/300933002/diff/200001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:115: KEY_QUERY_VALUE | KEY_SET_VALUE);
Do we need KEY_QUERY_VALUE?

https://codereview.chromium.org/300933002/diff/200001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:126: const wchar_t* val_cstr =
val.c_str();
same comment about the _cstr temporaries here, can do away with those.

https://codereview.chromium.org/300933002/diff/200001/chrome_elf/blacklist/bl...
File chrome_elf/blacklist/blacklist.cc (right):

https://codereview.chromium.org/300933002/diff/200001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:363: int num_dlls;
= 0

https://codereview.chromium.org/300933002/diff/200001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:364: int longest_name;
= 0

https://codereview.chromium.org/300933002/diff/200001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:372: std::vector<wchar_t*>
dll_names(num_dlls);
using a std::vector<std::wstring> would save a bunch of the manual memory
management below.

https://codereview.chromium.org/300933002/diff/200001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:375: DWORD name_len(longest_name + 1);
prefer = syntax for POD types

https://codereview.chromium.org/300933002/diff/200001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:376: DWORD value_len;
= 0

https://codereview.chromium.org/300933002/diff/200001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:377: wchar_t* name_buffer = new
wchar_t[name_len];
it looks like name_buffer is leaked on every iteration? consider using a
vector<wchar_t> and reserve()ing it first, that way you don't need to worry
about cleanup.

https://codereview.chromium.org/300933002/diff/200001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:390: dll_names[i] = name_buffer;
use push_back() instead

https://codereview.chromium.org/300933002/diff/200001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:396: ::RegDeleteValue(key, dll_names[i]);
Why do we delete the values from the registry here?

https://codereview.chromium.org/300933002/diff/200001/chrome_elf/blacklist/te...
File chrome_elf/blacklist/test/blacklist_test.cc (right):

https://codereview.chromium.org/300933002/diff/200001/chrome_elf/blacklist/te...
chrome_elf/blacklist/test/blacklist_test.cc:235: dlls[i] = value;
indent (try running git cl format)

https://codereview.chromium.org/300933002/diff/200001/chrome_elf/blacklist/te...
chrome_elf/blacklist/test/blacklist_test.cc:253: test_data[i].dll_name,
test_data[i].dll_name);
indent +2

[krstnmnlsn, 2014-06-02 14:00:37]

https://codereview.chromium.org/300933002/diff/200001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_unittest_win.cc (right):

https://codereview.chromium.org/300933002/diff/200001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_unittest_win.cc:198: // This should add the dll's
in those parameters to the registry.
On 2014/05/30 21:07:08, robertshield wrote:
> s/dll's/dlls

Done.

https://codereview.chromium.org/300933002/diff/200001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_unittest_win.cc:212: while (it !=
desired_params.end()) {
On 2014/05/30 21:07:08, robertshield wrote:
> minor nit, this form:
> 
> for (; it != desired_params.end(); ++it) {
> ...
> }
> 
> keeps all the loop-related stuff in one place and is slightly more
conventional.

Done.

https://codereview.chromium.org/300933002/diff/200001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_unittest_win.cc:214: const wchar_t* name_cstr =
name.c_str();
On 2014/05/30 21:07:08, robertshield wrote:
> why create the name_cstr temporary?
> 
> adding |name| increases readability, but this second one doesn't seem to.

Done.

https://codereview.chromium.org/300933002/diff/200001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/300933002/diff/200001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:115: KEY_QUERY_VALUE | KEY_SET_VALUE);
On 2014/05/30 21:07:08, robertshield wrote:
> Do we need KEY_QUERY_VALUE?

Apparently not :)

https://codereview.chromium.org/300933002/diff/200001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:126: const wchar_t* val_cstr =
val.c_str();
On 2014/05/30 21:07:08, robertshield wrote:
> same comment about the _cstr temporaries here, can do away with those.

Done.

https://codereview.chromium.org/300933002/diff/200001/chrome_elf/blacklist/bl...
File chrome_elf/blacklist/blacklist.cc (right):

https://codereview.chromium.org/300933002/diff/200001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:363: int num_dlls;
On 2014/05/30 21:07:08, robertshield wrote:
> = 0

Done.

https://codereview.chromium.org/300933002/diff/200001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:364: int longest_name;
On 2014/05/30 21:07:08, robertshield wrote:
> = 0

Done.

https://codereview.chromium.org/300933002/diff/200001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:372: std::vector<wchar_t*>
dll_names(num_dlls);
On 2014/05/30 21:07:08, robertshield wrote:
> using a std::vector<std::wstring> would save a bunch of the manual memory
> management below.
Done.

https://codereview.chromium.org/300933002/diff/200001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:375: DWORD name_len(longest_name + 1);
On 2014/05/30 21:07:08, robertshield wrote:
> prefer = syntax for POD types

Done.

https://codereview.chromium.org/300933002/diff/200001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:376: DWORD value_len;
On 2014/05/30 21:07:08, robertshield wrote:
> = 0

Done.

https://codereview.chromium.org/300933002/diff/200001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:377: wchar_t* name_buffer = new
wchar_t[name_len];
On 2014/05/30 21:07:08, robertshield wrote:
> it looks like name_buffer is leaked on every iteration? consider using a
> vector<wchar_t> and reserve()ing it first, that way you don't need to worry
> about cleanup. 

In my defense, I don't actually think this was leaking, I was collecting the
names and then deleting them in the second loop after using them.  But even if
I'm right about that it's probably not good to have your code looking like a
memory leak so switched to vectors now.

https://codereview.chromium.org/300933002/diff/200001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:390: dll_names[i] = name_buffer;
On 2014/05/30 21:07:08, robertshield wrote:
> use push_back() instead

Done.

https://codereview.chromium.org/300933002/diff/200001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:396: ::RegDeleteValue(key, dll_names[i]);
On 2014/05/30 21:07:08, robertshield wrote:
> Why do we delete the values from the registry here?

I thought that deleting values from the registry while looping over values in
the registry would cause problems.  Is there a simpler way of getting around
that?

https://codereview.chromium.org/300933002/diff/200001/chrome_elf/blacklist/te...
File chrome_elf/blacklist/test/blacklist_test.cc (right):

https://codereview.chromium.org/300933002/diff/200001/chrome_elf/blacklist/te...
chrome_elf/blacklist/test/blacklist_test.cc:235: dlls[i] = value;
On 2014/05/30 21:07:08, robertshield wrote:
> indent (try running git cl format)

Done.

https://codereview.chromium.org/300933002/diff/200001/chrome_elf/blacklist/te...
chrome_elf/blacklist/test/blacklist_test.cc:253: test_data[i].dll_name,
test_data[i].dll_name);
On 2014/05/30 21:07:08, robertshield wrote:
> indent +2

Done.

[robertshield, 2014-06-03 03:02:59]

Sorry for the delay, looks really good, just a few more questions/comments.

https://codereview.chromium.org/300933002/diff/200001/chrome_elf/blacklist/bl...
File chrome_elf/blacklist/blacklist.cc (right):

https://codereview.chromium.org/300933002/diff/200001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:377: wchar_t* name_buffer = new
wchar_t[name_len];
On 2014/06/02 14:00:37, krstnmnlsn wrote:
> On 2014/05/30 21:07:08, robertshield wrote:
> > it looks like name_buffer is leaked on every iteration? consider using a
> > vector<wchar_t> and reserve()ing it first, that way you don't need to worry
> > about cleanup. 
> 
> In my defense, I don't actually think this was leaking, I was collecting the
> names and then deleting them in the second loop after using them.  But even if
> I'm right about that it's probably not good to have your code looking like a
> memory leak so switched to vectors now.

Indeed, not leaking! Sorry for misreading :( still vectors much better :)

https://codereview.chromium.org/300933002/diff/200001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:396: ::RegDeleteValue(key, dll_names[i]);
On 2014/06/02 14:00:37, krstnmnlsn wrote:
> On 2014/05/30 21:07:08, robertshield wrote:
> > Why do we delete the values from the registry here?
> 
> I thought that deleting values from the registry while looping over values in
> the registry would cause problems.  Is there a simpler way of getting around
> that?

I actually meant, why do we delete them at all? Does deleting them mean that
these dlls are not blacklisted on next startup? (This is more of an "I don't
know how this works" comment :-))

https://codereview.chromium.org/300933002/diff/220001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/300933002/diff/220001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:120: while (it != params.end()) {
prefer for (; it != params.end(); ++it) { ... } form

https://codereview.chromium.org/300933002/diff/220001/chrome_elf/blacklist/bl...
File chrome_elf/blacklist/blacklist.cc (right):

https://codereview.chromium.org/300933002/diff/220001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:383: result = ::RegEnumValue(key, i,
&name_buffer[0], &name_len, NULL, NULL,
name_len was increased by 1 on line 379, but I don't see name_buffer's size
having been increased. Do you need to increment it at all (it is already set to
longest_name + 1)?

https://codereview.chromium.org/300933002/diff/220001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:386: value_buffer[value_len] = L'\0';
if value_buffer.size() is value_len, then the last element of value_buffer is
value_buffer[value_len - 1];

https://codereview.chromium.org/300933002/diff/220001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:393: dll_names.push_back(name_str);
nit:
dll_names.push_back(std::wstring(name_buffer.begin(), name_buffer.end()));

The temporary doesn't increase readability here.

https://codereview.chromium.org/300933002/diff/220001/chrome_elf/blacklist/te...
File chrome_elf/blacklist/test/blacklist_test.cc (right):

https://codereview.chromium.org/300933002/diff/220001/chrome_elf/blacklist/te...
chrome_elf/blacklist/test/blacklist_test.cc:235: dlls[i] = value;
prefer dlls.push_back(value);

[krstnmnlsn, 2014-06-03 19:30:16]

https://codereview.chromium.org/300933002/diff/220001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/300933002/diff/220001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:120: while (it != params.end()) {
On 2014/06/03 03:03:00, robertshield wrote:
> prefer for (; it != params.end(); ++it) { ... } form

Done.

https://codereview.chromium.org/300933002/diff/220001/chrome_elf/blacklist/bl...
File chrome_elf/blacklist/blacklist.cc (right):

https://codereview.chromium.org/300933002/diff/220001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:386: value_buffer[value_len] = L'\0';
On 2014/06/03 03:03:00, robertshield wrote:
> if value_buffer.size() is value_len, then the last element of value_buffer is
> value_buffer[value_len - 1];

Done.

https://codereview.chromium.org/300933002/diff/220001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:393: dll_names.push_back(name_str);
On 2014/06/03 03:03:00, robertshield wrote:
> nit:
> dll_names.push_back(std::wstring(name_buffer.begin(), name_buffer.end()));
> 
> The temporary doesn't increase readability here.

Done.

https://codereview.chromium.org/300933002/diff/220001/chrome_elf/blacklist/te...
File chrome_elf/blacklist/test/blacklist_test.cc (right):

https://codereview.chromium.org/300933002/diff/220001/chrome_elf/blacklist/te...
chrome_elf/blacklist/test/blacklist_test.cc:235: dlls[i] = value;
On 2014/06/03 03:03:00, robertshield wrote:
> prefer dlls.push_back(value);

Done.

[csharp, 2014-06-03 21:11:36]

https://codereview.chromium.org/300933002/diff/270001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_unittest_win.cc (right):

https://codereview.chromium.org/300933002/diff/270001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_unittest_win.cc:201: // Check that the set of
values in the registry is the same
The comment might be clearer as "Check that all the values in desired_params
were added to the registry"

https://codereview.chromium.org/300933002/diff/270001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/300933002/diff/270001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:110: if
(chrome_variations::GetVariationParams(kBrowserBlacklistTrialName,
I don't think you need this if statement here. If it would return false, it
won't change params (so it would be empty), so the for loop on line 119 will
handle not iterating at all.

https://codereview.chromium.org/300933002/diff/270001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:112: base::win::RegKey
finch_blacklist_registry_key(
I'd move lines 112-117 to the top of the function and check them first

https://codereview.chromium.org/300933002/diff/270001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.h (right):

https://codereview.chromium.org/300933002/diff/270001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.h:16: // Store the blacklist from the finch
configs in the registry.
Nit: Store -> Add (to be consistent with the function name)

https://codereview.chromium.org/300933002/diff/270001/chrome_elf/blacklist/bl...
File chrome_elf/blacklist/blacklist.cc (right):

https://codereview.chromium.org/300933002/diff/270001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:369: assert(name_len > 0);
This assert doesn't seem to useful, so you can probably remove it

https://codereview.chromium.org/300933002/diff/270001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:370: std::vector<wchar_t>
name_buffer(name_len);
Since you are always initializing it to the same size, you can pull it out of
the loop

https://codereview.chromium.org/300933002/diff/270001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:375: assert(value_len > 0);
I'm not sure if this assert is useful, I don't think value_len could ever be
less than 1 here.

https://codereview.chromium.org/300933002/diff/270001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:388: for (size_t i = 0; i < dll_names.size();
++i) {
Can RegDeleteKey replace this loop?

https://codereview.chromium.org/300933002/diff/270001/chrome_elf/blacklist/te...
File chrome_elf/blacklist/test/blacklist_test.cc (right):

https://codereview.chromium.org/300933002/diff/270001/chrome_elf/blacklist/te...
chrome_elf/blacklist/test/blacklist_test.cc:213: base::FilePath current_dir;
Move lines 213/214 to 241 since it isn't used before then

https://codereview.chromium.org/300933002/diff/270001/chrome_elf/blacklist/te...
chrome_elf/blacklist/test/blacklist_test.cc:226: // Collect any dll names
currently stored in the regisry and delete them.
nit: regisry -> registry

https://codereview.chromium.org/300933002/diff/270001/chrome_elf/blacklist/te...
chrome_elf/blacklist/test/blacklist_test.cc:227: std::vector<std::wstring> dlls;
I think you could use DeleteKey to delete eveything at once instead of lines
227->240

https://codereview.chromium.org/300933002/diff/270001/chrome_elf/blacklist/te...
chrome_elf/blacklist/test/blacklist_test.cc:242: struct TestData {
The code from here to the bottom of this test looks very similar to the code in
the test above, could you move similar sections into a helper function.

[robertshield, 2014-06-04 13:24:37]

LGTM once Chris' comments are addressed

[krstnmnlsn, 2014-06-04 16:55:44]

https://codereview.chromium.org/300933002/diff/270001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_unittest_win.cc (right):

https://codereview.chromium.org/300933002/diff/270001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_unittest_win.cc:201: // Check that the set of
values in the registry is the same
On 2014/06/03 21:11:36, csharp wrote:
> The comment might be clearer as "Check that all the values in desired_params
> were added to the registry"

Done.

https://codereview.chromium.org/300933002/diff/270001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/300933002/diff/270001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:110: if
(chrome_variations::GetVariationParams(kBrowserBlacklistTrialName,
On 2014/06/03 21:11:36, csharp wrote:
> I don't think you need this if statement here. If it would return false, it
> won't change params (so it would be empty), so the for loop on line 119 will
> handle not iterating at all.

Done.

https://codereview.chromium.org/300933002/diff/270001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:112: base::win::RegKey
finch_blacklist_registry_key(
On 2014/06/03 21:11:36, csharp wrote:
> I'd move lines 112-117 to the top of the function and check them first

Done.

https://codereview.chromium.org/300933002/diff/270001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.h (right):

https://codereview.chromium.org/300933002/diff/270001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.h:16: // Store the blacklist from the finch
configs in the registry.
On 2014/06/03 21:11:36, csharp wrote:
> Nit: Store -> Add (to be consistent with the function name)

Done.

https://codereview.chromium.org/300933002/diff/270001/chrome_elf/blacklist/bl...
File chrome_elf/blacklist/blacklist.cc (right):

https://codereview.chromium.org/300933002/diff/270001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:369: assert(name_len > 0);
On 2014/06/03 21:11:36, csharp wrote:
> This assert doesn't seem to useful, so you can probably remove it

Done.

https://codereview.chromium.org/300933002/diff/270001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:370: std::vector<wchar_t>
name_buffer(name_len);
On 2014/06/03 21:11:36, csharp wrote:
> Since you are always initializing it to the same size, you can pull it out of
> the loop

Done.

https://codereview.chromium.org/300933002/diff/270001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:375: assert(value_len > 0);
On 2014/06/03 21:11:36, csharp wrote:
> I'm not sure if this assert is useful, I don't think value_len could ever be
> less than 1 here.

Yeah the only case I could think of involved someone coming along and changing
my DWORDS to signed ints ...

https://codereview.chromium.org/300933002/diff/270001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:388: for (size_t i = 0; i < dll_names.size();
++i) {
On 2014/06/03 21:11:36, csharp wrote:
> Can RegDeleteKey replace this loop?

It seems to work yes :)

https://codereview.chromium.org/300933002/diff/270001/chrome_elf/blacklist/te...
File chrome_elf/blacklist/test/blacklist_test.cc (right):

https://codereview.chromium.org/300933002/diff/270001/chrome_elf/blacklist/te...
chrome_elf/blacklist/test/blacklist_test.cc:213: base::FilePath current_dir;
On 2014/06/03 21:11:36, csharp wrote:
> Move lines 213/214 to 241 since it isn't used before then

Done.

https://codereview.chromium.org/300933002/diff/270001/chrome_elf/blacklist/te...
chrome_elf/blacklist/test/blacklist_test.cc:226: // Collect any dll names
currently stored in the regisry and delete them.
On 2014/06/03 21:11:36, csharp wrote:
> nit: regisry -> registry

Done.

https://codereview.chromium.org/300933002/diff/270001/chrome_elf/blacklist/te...
chrome_elf/blacklist/test/blacklist_test.cc:227: std::vector<std::wstring> dlls;
On 2014/06/03 21:11:36, csharp wrote:
> I think you could use DeleteKey to delete eveything at once instead of lines
> 227->240

Done.

https://codereview.chromium.org/300933002/diff/270001/chrome_elf/blacklist/te...
chrome_elf/blacklist/test/blacklist_test.cc:242: struct TestData {
On 2014/06/03 21:11:36, csharp wrote:
> The code from here to the bottom of this test looks very similar to the code
in
> the test above, could you move similar sections into a helper function.

Done.

[csharp, 2014-06-04 17:12:55]

LGTM with two small changes left

https://codereview.chromium.org/300933002/diff/330001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/300933002/diff/330001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:119: std::map<std::string,
std::string>::iterator it = params.begin();
Move this into the for loop (line 120)

https://codereview.chromium.org/300933002/diff/330001/chrome_elf/blacklist/bl...
File chrome_elf/blacklist/blacklist.cc (right):

https://codereview.chromium.org/300933002/diff/330001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:387: if (result != ERROR_SUCCESS) {
Remove this code (lines 387-389) and replace line 391 with "return result ==
ERROR_SUCCESS" (this way the key is closed even if we fail to delete it)

[krstnmnlsn, 2014-06-04 17:52:35]

The CQ bit was checked by krstnmnlsn@chromium.org

[commit-bot: I haz the power, 2014-06-04 17:54:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/krstnmnlsn@chromium.org/300933002/370001

[krstnmnlsn, 2014-06-04 18:11:59]

https://codereview.chromium.org/300933002/diff/330001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/300933002/diff/330001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:119: std::map<std::string,
std::string>::iterator it = params.begin();
On 2014/06/04 17:12:56, csharp wrote:
> Move this into the for loop (line 120)

Done.

https://codereview.chromium.org/300933002/diff/330001/chrome_elf/blacklist/bl...
File chrome_elf/blacklist/blacklist.cc (right):

https://codereview.chromium.org/300933002/diff/330001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:387: if (result != ERROR_SUCCESS) {
On 2014/06/04 17:12:56, csharp wrote:
> Remove this code (lines 387-389) and replace line 391 with "return result ==
> ERROR_SUCCESS" (this way the key is closed even if we fail to delete it)

Done.

[commit-bot: I haz the power, 2014-06-04 21:30:30]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[commit-bot: I haz the power, 2014-06-04 21:37:29]

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

[commit-bot: I haz the power, 2014-06-04 21:37:30]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)

[krstnmnlsn, 2014-06-05 12:59:49]

The CQ bit was checked by krstnmnlsn@chromium.org

[krstnmnlsn, 2014-06-05 13:00:04]

The CQ bit was checked by krstnmnlsn@chromium.org

[commit-bot: I haz the power, 2014-06-05 13:00:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/krstnmnlsn@chromium.org/300933002/370001

[commit-bot: I haz the power, 2014-06-05 14:09:09]

Change committed as 275086