Introduce IEImporterTestRegistryOverrider to be able to override the registry for a test and flag t…

Introduce IEImporterTestRegistryOverrider to be able to override the registry for a test and flag the environment for the child process to know it should also override the registry.

Prerequisite to make IEImporterBrowserTest pass when using the ExternalProcessImporterHost in https://codereview.chromium.org/12670013/. Keeping this as a separate CL given that one is already very complex and has been lgtm'ed as is by 4 reviewers already.

Precursor CL to https://codereview.chromium.org/12670013/.

BUG=219419, 22412
TEST=Make sure tests run without affecting the real registry. 
Make sure (via logging) that the override doesn't happen when running import directly from Chrome. 
Make sure the IEImporterBrowserTest pass both now (in-process) and on top of https://codereview.chromium.org/12670013/ (out-of-process).

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

[gab, 2013-04-26 17:30:10]

Robert/Miranda PTAL, this is the cleanest way me and Robert could think of to
flag test child processes that they should also override the registry.

Cheers!
Gab

[robertshield, 2013-04-26 19:20:53]

https://codereview.chromium.org/14143010/diff/3001/chrome/browser/importer/ie...
File chrome/browser/importer/ie_importer.cc (right):

https://codereview.chromium.org/14143010/diff/3001/chrome/browser/importer/ie...
chrome/browser/importer/ie_importer.cc:425: if
(!test_registry_overrider_.StartRegistryOverrideIfNeeded()) {
This doesn't disambiguate between the case where there is an unspecified error
reading the environment var vs. the case where the env var is successfully read,
but HKCU could not be overridden. 

In the latter case, it makes sense to abort, not sure that it makes sense to
abort in the former case too.

https://codereview.chromium.org/14143010/diff/3001/chrome/browser/importer/ie...
File chrome/browser/importer/ie_importer_test_registry_overrider_win.cc (right):

https://codereview.chromium.org/14143010/diff/3001/chrome/browser/importer/ie...
chrome/browser/importer/ie_importer_test_registry_overrider_win.cc:20: const
char16 kTestHKCUOverrideEnvironmentVariableValue[] =
nit: don't wrap

https://codereview.chromium.org/14143010/diff/3001/chrome/browser/importer/ie...
chrome/browser/importer/ie_importer_test_registry_overrider_win.cc:45:
kTestHKCUOverrideEnvironmentVariableValue) != 0;
use the stuff in base\environment.h here

https://codereview.chromium.org/14143010/diff/3001/chrome/browser/importer/ie...
chrome/browser/importer/ie_importer_test_registry_overrider_win.cc:52:
arraysize(buffer));
use the stuff in base\environment.h here

https://codereview.chromium.org/14143010/diff/3001/chrome/browser/importer/ie...
File chrome/browser/importer/ie_importer_test_registry_overrider_win.h (right):

https://codereview.chromium.org/14143010/diff/3001/chrome/browser/importer/ie...
chrome/browser/importer/ie_importer_test_registry_overrider_win.h:26: bool
SetRegistryOverride();
If I am reading this correctly, a caller should never call both
SetRegistryOverride and StartRegistryOverrideIfNeeded, right? If true, could
that be DCHECKed against somehow (possibly DCHECK(!override_active_))?

https://codereview.chromium.org/14143010/diff/3001/chrome/browser/importer/ie...
chrome/browser/importer/ie_importer_test_registry_overrider_win.h:39: bool
override_active_;
should this be static?

https://codereview.chromium.org/14143010/diff/3001/chrome/browser/importer/ie...
chrome/browser/importer/ie_importer_test_registry_overrider_win.h:41: // Wheter
this process is the one that set the override (and thus should be
nit: Whether

[gab, 2013-04-29 02:00:43]

Sir, PTAL, I changed the interface and the logic quite a bit.

The previous patchset didn't work for the current in-process importer as it
would set the override twice in the same process (i.e. once in the test, once in
the importer). I fixed that by changing the interface + adding some static state
+ synchronization.

It now works great whether the importer runs in or out of process :).

Cheers!
Gab

https://codereview.chromium.org/14143010/diff/3001/chrome/browser/importer/ie...
File chrome/browser/importer/ie_importer.cc (right):

https://codereview.chromium.org/14143010/diff/3001/chrome/browser/importer/ie...
chrome/browser/importer/ie_importer.cc:425: if
(!test_registry_overrider_.StartRegistryOverrideIfNeeded()) {
On 2013/04/26 19:20:53, robertshield wrote:
> This doesn't disambiguate between the case where there is an unspecified error
> reading the environment var vs. the case where the env var is successfully
read,
> but HKCU could not be overridden. 
> 
> In the latter case, it makes sense to abort, not sure that it makes sense to
> abort in the former case too.

Agreed, changed the method to always return true on failed read, I wanted to
DCHECK that the failure was ERROR_ENVVAR_NOT_FOUND, but it doesn't seem
base::Environment does it, nor do I like relying on how it uses
::GetEnvironmentVariable() to call ::GetLastError() in my code if GetVar()
returns false.

FWIW, I find base::Environment is a bit weak-sauce in its error-checking, but
I'm not going to change that :).

https://codereview.chromium.org/14143010/diff/3001/chrome/browser/importer/ie...
File chrome/browser/importer/ie_importer_test_registry_overrider_win.cc (right):

https://codereview.chromium.org/14143010/diff/3001/chrome/browser/importer/ie...
chrome/browser/importer/ie_importer_test_registry_overrider_win.cc:20: const
char16 kTestHKCUOverrideEnvironmentVariableValue[] =
On 2013/04/26 19:20:53, robertshield wrote:
> nit: don't wrap

Done.

https://codereview.chromium.org/14143010/diff/3001/chrome/browser/importer/ie...
chrome/browser/importer/ie_importer_test_registry_overrider_win.cc:45:
kTestHKCUOverrideEnvironmentVariableValue) != 0;
On 2013/04/26 19:20:53, robertshield wrote:
> use the stuff in base\environment.h here

Done.

https://codereview.chromium.org/14143010/diff/3001/chrome/browser/importer/ie...
chrome/browser/importer/ie_importer_test_registry_overrider_win.cc:52:
arraysize(buffer));
On 2013/04/26 19:20:53, robertshield wrote:
> use the stuff in base\environment.h here

Done.

https://codereview.chromium.org/14143010/diff/3001/chrome/browser/importer/ie...
File chrome/browser/importer/ie_importer_test_registry_overrider_win.h (right):

https://codereview.chromium.org/14143010/diff/3001/chrome/browser/importer/ie...
chrome/browser/importer/ie_importer_test_registry_overrider_win.h:26: bool
SetRegistryOverride();
On 2013/04/26 19:20:53, robertshield wrote:
> If I am reading this correctly, a caller should never call both
> SetRegistryOverride and StartRegistryOverrideIfNeeded, right? If true, could
> that be DCHECKed against somehow (possibly DCHECK(!override_active_))?

Changed the interface quite a bit, let me know if it makes sense now?

Basically, it's ok for the same process to call both (although it doesn't make
much sense to call StartRegistryOverrideIfNeeded() if you know you called
SetRegistryOverride(), but if you're not sure it doesn't hurt (i.e. the
in-process importer will do this until the OOP import CL is checked in)).

https://codereview.chromium.org/14143010/diff/3001/chrome/browser/importer/ie...
chrome/browser/importer/ie_importer_test_registry_overrider_win.h:39: bool
override_active_;
On 2013/04/26 19:20:53, robertshield wrote:
> should this be static?

See main comment.

https://codereview.chromium.org/14143010/diff/3001/chrome/browser/importer/ie...
chrome/browser/importer/ie_importer_test_registry_overrider_win.h:41: // Wheter
this process is the one that set the override (and thus should be
On 2013/04/26 19:20:53, robertshield wrote:
> nit: Whether

Done.

https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
File chrome/browser/importer/ie_importer_browsertest_win.cc (left):

https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
chrome/browser/importer/ie_importer_browsertest_win.cc:522: //
IUrlHistoryStg2::AddUrl seems to reset the override. Ensure it here.
This didn't seem to be true (i.e. it works fine without it).

In fact, in my new design, calling this again was causing trouble as it was
creating a fake entry inside the fake entry (the previous code was offsetting
this by always unsetting the fake before resetting it (i.e.
RegOverridePredefKey(HKEY_CURRENT_USER, NULL)), but this seems to be overkill).

[robertshield, 2013-04-30 03:15:42]

A few comments, also could you explain again why the overrider class needs to be
used from different threads?

https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
File chrome/browser/importer/ie_importer_test_registry_overrider_win.cc (right):

https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
chrome/browser/importer/ie_importer_test_registry_overrider_win.cc:35: if
(in_process_override_started_by_this_) {
shouldn't this set override_active_in_process_ to false (also holding the lock)?

https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
chrome/browser/importer/ie_importer_test_registry_overrider_win.cc:55:
env->SetVar(kTestHKCUOverrideEnvironmentVariable,
should the setvar / unsetvar calls also be protected?

https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
File chrome/browser/importer/ie_importer_test_registry_overrider_win.h (right):

https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
chrome/browser/importer/ie_importer_test_registry_overrider_win.h:26: // result
in an overriden registry. This method should only be called once
nit: registry -> HKEY_CURRENT_USER hive.

https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
chrome/browser/importer/ie_importer_test_registry_overrider_win.h:47: bool
in_process_override_started_by_this_;
nit: rename to override_performed_.

https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
chrome/browser/importer/ie_importer_test_registry_overrider_win.h:51: bool
override_set_by_this_;
maybe override_initiated_?

[Miranda Callahan, 2013-04-30 18:46:39]

Hmm -- unfortunately, my experience with the importer code was all on the Mac
side; I don't have enough Windows-fu to be a good reviewer of this CL, I'm
afraid. If you need another reviewer, cpu would perhaps fit the bill? Otherwise,
I think it would be fine to just have Robert review this patchset.

On 2013/04/30 03:15:42, robertshield wrote:
> A few comments, also could you explain again why the overrider class needs to
be
> used from different threads?
> 
>
https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
> File chrome/browser/importer/ie_importer_test_registry_overrider_win.cc
(right):
> 
>
https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
> chrome/browser/importer/ie_importer_test_registry_overrider_win.cc:35: if
> (in_process_override_started_by_this_) {
> shouldn't this set override_active_in_process_ to false (also holding the
lock)?
> 
>
https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
> chrome/browser/importer/ie_importer_test_registry_overrider_win.cc:55:
> env->SetVar(kTestHKCUOverrideEnvironmentVariable,
> should the setvar / unsetvar calls also be protected?
> 
>
https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
> File chrome/browser/importer/ie_importer_test_registry_overrider_win.h
(right):
> 
>
https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
> chrome/browser/importer/ie_importer_test_registry_overrider_win.h:26: //
result
> in an overriden registry. This method should only be called once
> nit: registry -> HKEY_CURRENT_USER hive.
> 
>
https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
> chrome/browser/importer/ie_importer_test_registry_overrider_win.h:47: bool
> in_process_override_started_by_this_;
> nit: rename to override_performed_.
> 
>
https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
> chrome/browser/importer/ie_importer_test_registry_overrider_win.h:51: bool
> override_set_by_this_;
> maybe override_initiated_?

[gab, 2013-04-30 19:22:14]

On 2013/04/30 18:46:39, Miranda Callahan wrote:
> Hmm -- unfortunately, my experience with the importer code was all on the Mac
> side; I don't have enough Windows-fu to be a good reviewer of this CL, I'm
> afraid. If you need another reviewer, cpu would perhaps fit the bill?
Otherwise,
> I think it would be fine to just have Robert review this patchset.

Right, that works, it was mostly for OWNERS, but now that I'm an owner I guess
Robert will suffice :).

Thanks!
Gab


> 
> On 2013/04/30 03:15:42, robertshield wrote:
> > A few comments, also could you explain again why the overrider class needs
to
> be
> > used from different threads?
> > 
> >
>
https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
> > File chrome/browser/importer/ie_importer_test_registry_overrider_win.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
> > chrome/browser/importer/ie_importer_test_registry_overrider_win.cc:35: if
> > (in_process_override_started_by_this_) {
> > shouldn't this set override_active_in_process_ to false (also holding the
> lock)?
> > 
> >
>
https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
> > chrome/browser/importer/ie_importer_test_registry_overrider_win.cc:55:
> > env->SetVar(kTestHKCUOverrideEnvironmentVariable,
> > should the setvar / unsetvar calls also be protected?
> > 
> >
>
https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
> > File chrome/browser/importer/ie_importer_test_registry_overrider_win.h
> (right):
> > 
> >
>
https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
> > chrome/browser/importer/ie_importer_test_registry_overrider_win.h:26: //
> result
> > in an overriden registry. This method should only be called once
> > nit: registry -> HKEY_CURRENT_USER hive.
> > 
> >
>
https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
> > chrome/browser/importer/ie_importer_test_registry_overrider_win.h:47: bool
> > in_process_override_started_by_this_;
> > nit: rename to override_performed_.
> > 
> >
>
https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
> > chrome/browser/importer/ie_importer_test_registry_overrider_win.h:51: bool
> > override_set_by_this_;
> > maybe override_initiated_?

[gab, 2013-04-30 21:44:52]

PTAL, the reason this needs to be thread-safe with the current code is that the
in-process importer will call StartRegistryOverrideIfNeeded() from the FILE
thread in the same process, the environment will be safe and thus
StartRegistryOverride() will be called, at which point it should realize that
override_active_in_process_ is true (this variable thus needs to be synchronized
to make sure that setting it on the original test thread (UI thread I think)
results in it being set when this is called on the FILE thread).

Cheers!
Gab

https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
File chrome/browser/importer/ie_importer_test_registry_overrider_win.cc (right):

https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
chrome/browser/importer/ie_importer_test_registry_overrider_win.cc:35: if
(in_process_override_started_by_this_) {
On 2013/04/30 03:15:42, robertshield wrote:
> shouldn't this set override_active_in_process_ to false (also holding the
lock)?
> 

This is done via static initialization at the end of this file (I debated
putting static initializers at the top, but decided to follow declaration order
in the class, WDYT?).

https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
chrome/browser/importer/ie_importer_test_registry_overrider_win.cc:55:
env->SetVar(kTestHKCUOverrideEnvironmentVariable,
On 2013/04/30 03:15:42, robertshield wrote:
> should the setvar / unsetvar calls also be protected?

They are protected by contract (i.e. this method should only be called once
across all processes).

The DCHECK also verifies the contract was honored (i.e. no one else set the
environment yet); although a race could technically bypass the DCHECK, it is not
really intended as thread-safe check since there should really only be one
caller of this method...

https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
File chrome/browser/importer/ie_importer_test_registry_overrider_win.h (right):

https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
chrome/browser/importer/ie_importer_test_registry_overrider_win.h:26: // result
in an overriden registry. This method should only be called once
On 2013/04/30 03:15:42, robertshield wrote:
> nit: registry -> HKEY_CURRENT_USER hive.

Done.

https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
chrome/browser/importer/ie_importer_test_registry_overrider_win.h:47: bool
in_process_override_started_by_this_;
On 2013/04/30 03:15:42, robertshield wrote:
> nit: rename to override_performed_.

Done.

https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
chrome/browser/importer/ie_importer_test_registry_overrider_win.h:51: bool
override_set_by_this_;
On 2013/04/30 03:15:42, robertshield wrote:
> maybe override_initiated_?

Done.

[robertshield, 2013-04-30 22:04:04]

ugh, my train is pulling in but I want to think on the thread safety some more,
I'm not sure I'm happy with a class that is only thread safe if it's methods are
called in a certain way.

https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
File chrome/browser/importer/ie_importer_test_registry_overrider_win.cc (right):

https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
chrome/browser/importer/ie_importer_test_registry_overrider_win.cc:35: if
(in_process_override_started_by_this_) {
On 2013/04/30 21:44:52, gab wrote:
> On 2013/04/30 03:15:42, robertshield wrote:
> > shouldn't this set override_active_in_process_ to false (also holding the
> lock)?
> > 
> 
> This is done via static initialization at the end of this file (I debated
> putting static initializers at the top, but decided to follow declaration
order
> in the class, WDYT?).

I think that this is the destructor and that after the object is destroyed that
override_active_in_process_ should be reset from true back to false. How does
static initialization help with this?

https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
chrome/browser/importer/ie_importer_test_registry_overrider_win.cc:55:
env->SetVar(kTestHKCUOverrideEnvironmentVariable,
On 2013/04/30 21:44:52, gab wrote:
> On 2013/04/30 03:15:42, robertshield wrote:
> > should the setvar / unsetvar calls also be protected?
> 
> They are protected by contract (i.e. this method should only be called once
> across all processes).
> 
> The DCHECK also verifies the contract was honored (i.e. no one else set the
> environment yet); although a race could technically bypass the DCHECK, it is
not
> really intended as thread-safe check since there should really only be one
> caller of this method...

I'm not sure that protected-by-contract is a feature :-) Generally, either a
class is thread safe, or it isn't. Having parts of it be makes callers lives
more difficult.

It feels like some of the state changes in this class are protected by a lock
and some aren't and it isn't clear to me yet why not.

[gab, 2013-04-30 22:56:47]

https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
File chrome/browser/importer/ie_importer_test_registry_overrider_win.cc (right):

https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
chrome/browser/importer/ie_importer_test_registry_overrider_win.cc:35: if
(in_process_override_started_by_this_) {
On 2013/04/30 22:04:05, robertshield wrote:
> On 2013/04/30 21:44:52, gab wrote:
> > On 2013/04/30 03:15:42, robertshield wrote:
> > > shouldn't this set override_active_in_process_ to false (also holding the
> > lock)?
> > > 
> > 
> > This is done via static initialization at the end of this file (I debated
> > putting static initializers at the top, but decided to follow declaration
> order
> > in the class, WDYT?).
> 
> I think that this is the destructor and that after the object is destroyed
that
> override_active_in_process_ should be reset from true back to false. How does
> static initialization help with this?

Err my bad, I somehow read this comment and saw the constructor initializer list
above and thought this comment referred to initialization....

Yes, you're right, will do (in fact I'm crossing my fingers and hoping this is
why tests were failing on the try bots... i.e., if this variable still said the
override was there it would not set it in the next test)!

https://codereview.chromium.org/14143010/diff/14001/chrome/browser/importer/i...
chrome/browser/importer/ie_importer_test_registry_overrider_win.cc:55:
env->SetVar(kTestHKCUOverrideEnvironmentVariable,
On 2013/04/30 22:04:05, robertshield wrote:
> On 2013/04/30 21:44:52, gab wrote:
> > On 2013/04/30 03:15:42, robertshield wrote:
> > > should the setvar / unsetvar calls also be protected?
> > 
> > They are protected by contract (i.e. this method should only be called once
> > across all processes).
> > 
> > The DCHECK also verifies the contract was honored (i.e. no one else set the
> > environment yet); although a race could technically bypass the DCHECK, it is
> not
> > really intended as thread-safe check since there should really only be one
> > caller of this method...
> 
> I'm not sure that protected-by-contract is a feature :-) Generally, either a
> class is thread safe, or it isn't. Having parts of it be makes callers lives
> more difficult.
> 
> It feels like some of the state changes in this class are protected by a lock
> and some aren't and it isn't clear to me yet why not. 

I disagree, callers shouldn't be confused, they should only call this method if
the environment isn't set (the contract is actually even stricter and says this
should only be called once across all processes; in theory it would be ok to
call it again after the initial setter has been destructed, but this is
unnecessary for our use-case and making the interface simpler/stricter is fine
imo).
A lock wouldn't help either as it wouldn't prevent a child process from racing
to set the environment (if it tried, but it shouldn't!).
By contract only one of these objects should be responsible to set the
environment and other objects should be "readers" (i.e. check the environment
and react accordingly).
The DCHECK also helps enforce the contract if a developer was to make the
mistake to call SetRegistryOverride from 2 different places.

[gab, 2013-05-03 13:20:55]

Sir, PTAL. Since last replies (from patch set 6), I:

- Made this class fully thread-safe (i.e., all static data and member access are
bound to holding |lock_|).
- Changed it to use a GUID as the last part of the temporary key (so that
multiple tests can run in parallel without conflicting).
- Made it so that calling StartRegistryOverride() when
|override_active_in_process_| results in re-applying the override instead of
ignoring the call (this is what the previous code did in
IEImporterBrowserTest::StartRegistryOverride()) -- it's not clear to me why this
is needed (a comment in the previous code hints that some Windows API may be
resetting the override, that's not the case on my local Windows 8 machine, but
adding this "re-apply override logic" seems to be fixing the Win7 bots...).

Cheers!
Gab

[robertshield, 2013-05-03 13:52:37]

Looking good, just a few more questions about how RegOverridePredefKey could be
getting reset by other Win32 calls. I wasn't aware that was possible and I'd
like to understand how it happens.

https://codereview.chromium.org/14143010/diff/80001/chrome/browser/importer/i...
File chrome/browser/importer/ie_importer_test_registry_overrider_win.cc (right):

https://codereview.chromium.org/14143010/diff/80001/chrome/browser/importer/i...
chrome/browser/importer/ie_importer_test_registry_overrider_win.cc:91: if
(!GetSubKeyFromEnvironment(&subkey))
why is it safe to not have this line protected by the lock?

https://codereview.chromium.org/14143010/diff/80001/chrome/browser/importer/i...
chrome/browser/importer/ie_importer_test_registry_overrider_win.cc:104: //
Override the registry whether |override_active_in_process_| or not as some
It seems like it would be better to allow only a single instance of this class
to be performing process-wide registry overrides at once.

https://codereview.chromium.org/14143010/diff/80001/chrome/browser/importer/i...
chrome/browser/importer/ie_importer_test_registry_overrider_win.cc:105: //
Windows API calls seem to reset the override. However, make sure to undo
which API calls reset the override? If the override can be spuriously reset,
then this won't help.

[gab, 2013-05-03 14:49:10]

PTAL, thanks!

https://codereview.chromium.org/14143010/diff/80001/chrome/browser/importer/i...
File chrome/browser/importer/ie_importer_test_registry_overrider_win.cc (right):

https://codereview.chromium.org/14143010/diff/80001/chrome/browser/importer/i...
chrome/browser/importer/ie_importer_test_registry_overrider_win.cc:91: if
(!GetSubKeyFromEnvironment(&subkey))
On 2013/05/03 13:52:37, robertshield wrote:
> why is it safe to not have this line protected by the lock?

Done.

https://codereview.chromium.org/14143010/diff/80001/chrome/browser/importer/i...
chrome/browser/importer/ie_importer_test_registry_overrider_win.cc:104: //
Override the registry whether |override_active_in_process_| or not as some
On 2013/05/03 13:52:37, robertshield wrote:
> It seems like it would be better to allow only a single instance of this class
> to be performing process-wide registry overrides at once.

Not doing this as per F2F discussion.

https://codereview.chromium.org/14143010/diff/80001/chrome/browser/importer/i...
chrome/browser/importer/ie_importer_test_registry_overrider_win.cc:105: //
Windows API calls seem to reset the override. However, make sure to undo
On 2013/05/03 13:52:37, robertshield wrote:
> which API calls reset the override? If the override can be spuriously reset,
> then this won't help. 

Seems to be IUrlHistoryStg2::AddUrl (as per the comment on a call removed in
this CL in ie_importer_browsertest_win.cc).

[robertshield, 2013-05-03 17:27:27]

lgtm

[commit-bot: I haz the power, 2013-05-03 19:46:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/14143010/96001

[commit-bot: I haz the power, 2013-05-04 00:06:08]

Change committed as 198256