Tommi: I need an owner review for the chrome frame changes.

Tommi: I need an owner review for the chrome frame changes.

Moving the test helper class TempRegKeyOverride since I need to do something
similar in some new RLZ tests, and I don't want to duplicate the code.  Please
suggest a better namespace name if needed, I was just following the same
pattern found in test_file_util in the same directory.

BUG=None
TEST=No new tests, just refactoring test helpers

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

[tommi, 2011-08-18 20:42:04]

lgtm.

[tommi (sloooow) - chröme, 2011-08-18 20:43:48]

lgtm.
(from the right account this time)

[Paweł Hajdan Jr., 2011-08-18 22:42:47]

http://codereview.chromium.org/7669061/diff/1/base/test/test_reg_util_win.cc
File base/test/test_reg_util_win.cc (right):

http://codereview.chromium.org/7669061/diff/1/base/test/test_reg_util_win.cc#...
base/test/test_reg_util_win.cc:15: : override_(override), temp_name_(temp_name)
{
nit: Each one on its own line please.

http://codereview.chromium.org/7669061/diff/1/base/test/test_reg_util_win.cc#...
base/test/test_reg_util_win.cc:16: DCHECK(temp_name && lstrlenW(temp_name));
Split that into two, so it's easier to see what failed.

http://codereview.chromium.org/7669061/diff/1/base/test/test_reg_util_win.cc#...
base/test/test_reg_util_win.cc:27: // The temp key will be deleted via a call to
DeleteAllTempKeys().
This relies on the caller calling DeleteAllTempKeys explicitly. Can we add some
safety feature, like an AtExit callback or yet another scoper? Alternatively,
call that in base::TestSuite, just make sure we will clean up.

http://codereview.chromium.org/7669061/diff/1/base/test/test_reg_util_win.h
File base/test/test_reg_util_win.h (right):

http://codereview.chromium.org/7669061/diff/1/base/test/test_reg_util_win.h#n...
base/test/test_reg_util_win.h:13: namespace reg_util {
nit: I don't like abbreviations. Why not registry_util?

http://codereview.chromium.org/7669061/diff/1/base/test/test_reg_util_win.h#n...
base/test/test_reg_util_win.h:18: class TempRegKeyOverride {
nit: Why not ScopedRegistryKeyOverride?

http://codereview.chromium.org/7669061/diff/1/base/test/test_reg_util_win.h#n...
base/test/test_reg_util_win.h:22: TempRegKeyOverride(HKEY override, const
wchar_t* temp_name);
nit: Why not const std::wstring& ?

http://codereview.chromium.org/7669061/diff/1/base/test/test_reg_util_win.h#n...
base/test/test_reg_util_win.h:31: };
nit: DISALLOW_COPY_AND_ASSIGN

[Roger Tawa OOO till Jul 10th, 2011-08-22 19:24:42]

Thanks Pawel, please take another look.

http://codereview.chromium.org/7669061/diff/1/base/test/test_reg_util_win.cc
File base/test/test_reg_util_win.cc (right):

http://codereview.chromium.org/7669061/diff/1/base/test/test_reg_util_win.cc#...
base/test/test_reg_util_win.cc:15: : override_(override), temp_name_(temp_name)
{
On 2011/08/18 22:42:47, Paweł Hajdan Jr. wrote:
> nit: Each one on its own line please.

Done.

http://codereview.chromium.org/7669061/diff/1/base/test/test_reg_util_win.cc#...
base/test/test_reg_util_win.cc:16: DCHECK(temp_name && lstrlenW(temp_name));
On 2011/08/18 22:42:47, Paweł Hajdan Jr. wrote:
> Split that into two, so it's easier to see what failed.

Done.

http://codereview.chromium.org/7669061/diff/1/base/test/test_reg_util_win.cc#...
base/test/test_reg_util_win.cc:27: // The temp key will be deleted via a call to
DeleteAllTempKeys().
On 2011/08/18 22:42:47, Paweł Hajdan Jr. wrote:
> This relies on the caller calling DeleteAllTempKeys explicitly. Can we add
some
> safety feature, like an AtExit callback or yet another scoper? Alternatively,
> call that in base::TestSuite, just make sure we will clean up.

added a scoper.

http://codereview.chromium.org/7669061/diff/1/base/test/test_reg_util_win.h
File base/test/test_reg_util_win.h (right):

http://codereview.chromium.org/7669061/diff/1/base/test/test_reg_util_win.h#n...
base/test/test_reg_util_win.h:13: namespace reg_util {
On 2011/08/18 22:42:47, Paweł Hajdan Jr. wrote:
> nit: I don't like abbreviations. Why not registry_util?

Done.

http://codereview.chromium.org/7669061/diff/1/base/test/test_reg_util_win.h#n...
base/test/test_reg_util_win.h:18: class TempRegKeyOverride {
On 2011/08/18 22:42:47, Paweł Hajdan Jr. wrote:
> nit: Why not ScopedRegistryKeyOverride?

Done.

http://codereview.chromium.org/7669061/diff/1/base/test/test_reg_util_win.h#n...
base/test/test_reg_util_win.h:22: TempRegKeyOverride(HKEY override, const
wchar_t* temp_name);
On 2011/08/18 22:42:47, Paweł Hajdan Jr. wrote:
> nit: Why not const std::wstring& ?

Done.

http://codereview.chromium.org/7669061/diff/1/base/test/test_reg_util_win.h#n...
base/test/test_reg_util_win.h:31: };
On 2011/08/18 22:42:47, Paweł Hajdan Jr. wrote:
> nit: DISALLOW_COPY_AND_ASSIGN

Done.

[Paweł Hajdan Jr., 2011-08-22 19:42:58]

http://codereview.chromium.org/7669061/diff/11002/chrome/installer/util/produ...
File chrome/installer/util/product_state_unittest.cc (right):

http://codereview.chromium.org/7669061/diff/11002/chrome/installer/util/produ...
chrome/installer/util/product_state_unittest.cc:99:
ScopedRegistryKeyOverride::DeleteAllTempKeys();
And this doesn't use the cleanup scoper. Could you try designing it in a way
that it's _impossible_ not to use the cleanup scoper? This applies to entire CL,
few other files also fail to use the scoped cleanup.

[Roger Tawa OOO till Jul 10th, 2011-08-22 20:19:17]

http://codereview.chromium.org/7669061/diff/11002/chrome/installer/util/produ...
File chrome/installer/util/product_state_unittest.cc (right):

http://codereview.chromium.org/7669061/diff/11002/chrome/installer/util/produ...
chrome/installer/util/product_state_unittest.cc:99:
ScopedRegistryKeyOverride::DeleteAllTempKeys();
On 2011/08/22 19:42:58, Paweł Hajdan Jr. wrote:
> And this doesn't use the cleanup scoper. Could you try designing it in a way
> that it's _impossible_ not to use the cleanup scoper? This applies to entire
CL,
> few other files also fail to use the scoped cleanup.

This code properly handles cleanup of the temp registry key already, so no need
to change it.  Likewise for the other files which I did not change.  I did use
the scoper in the places where it really helped.

I don't want to make it impossible to not use the cleanup scoper.

[Roger Tawa OOO till Jul 10th, 2011-08-23 14:43:24]

Hi Tommi,

To force users not to use ScopedRegistryKeyOverride directly and go through a
manager I had to make more significant changes to the chrome frame tests and
installer tests.  Could you please take another look?  Thanks.

[tommi (sloooow) - chröme, 2011-08-23 14:46:25]

Absolutely not!  I kid I kid... I'll take a look tonight.

On Tue, Aug 23, 2011 at 4:43 PM, <rogerta@chromium.org> wrote:

> Hi Tommi,
>
> To force users not to use ScopedRegistryKeyOverride directly and go through
> a
> manager I had to make more significant changes to the chrome frame tests
> and
> installer tests.  Could you please take another look?  Thanks.
>
>
>
>
http://codereview.chromium.**org/7669061/<http://codereview.chromium.org/7669...
>

[Paweł Hajdan Jr., 2011-08-23 18:19:05]

LGTM, thanks!

http://codereview.chromium.org/7669061/diff/13005/base/test/test_reg_util_win.h
File base/test/test_reg_util_win.h (right):

http://codereview.chromium.org/7669061/diff/13005/base/test/test_reg_util_win...
base/test/test_reg_util_win.h:23: // Keeps track of one override.
nit: How about just moving this down to the private section?

[Roger Tawa OOO till Jul 10th, 2011-08-23 18:33:48]

Thanks.  Will wait for Tommi's l g t m.

http://codereview.chromium.org/7669061/diff/13005/base/test/test_reg_util_win.h
File base/test/test_reg_util_win.h (right):

http://codereview.chromium.org/7669061/diff/13005/base/test/test_reg_util_win...
base/test/test_reg_util_win.h:23: // Keeps track of one override.
On 2011/08/23 18:19:05, Paweł Hajdan Jr. wrote:
> nit: How about just moving this down to the private section?

Done.

[tommi (sloooow) - chröme, 2011-08-23 20:33:24]

lgtm!

http://codereview.chromium.org/7669061/diff/17001/base/test/test_reg_util_win.cc
File base/test/test_reg_util_win.cc (right):

http://codereview.chromium.org/7669061/diff/17001/base/test/test_reg_util_win...
base/test/test_reg_util_win.cc:20: DCHECK(temp_name_.length() > 0);
nit: DCHECK(!temp_name.empty());

http://codereview.chromium.org/7669061/diff/17001/chrome/installer/util/produ...
File chrome/installer/util/product_unittest.h (left):

http://codereview.chromium.org/7669061/diff/17001/chrome/installer/util/produ...
chrome/installer/util/product_unittest.h:33: // moved to some common test
utility file.
w00t!

http://codereview.chromium.org/7669061/diff/17001/chrome_frame/test/chrome_fr...
File chrome_frame/test/chrome_frame_test_utils.h (left):

http://codereview.chromium.org/7669061/diff/17001/chrome_frame/test/chrome_fr...
chrome_frame/test/chrome_frame_test_utils.h:315: 
empty line removed by accident?

[Roger Tawa OOO till Jul 10th, 2011-08-23 21:08:02]

Thanks guys.  Will commit shortly.

http://codereview.chromium.org/7669061/diff/17001/base/test/test_reg_util_win.cc
File base/test/test_reg_util_win.cc (right):

http://codereview.chromium.org/7669061/diff/17001/base/test/test_reg_util_win...
base/test/test_reg_util_win.cc:20: DCHECK(temp_name_.length() > 0);
On 2011/08/23 20:33:24, tommi wrote:
> nit: DCHECK(!temp_name.empty());

Done.

http://codereview.chromium.org/7669061/diff/17001/chrome/installer/util/produ...
File chrome/installer/util/product_unittest.h (left):

http://codereview.chromium.org/7669061/diff/17001/chrome/installer/util/produ...
chrome/installer/util/product_unittest.h:33: // moved to some common test
utility file.
On 2011/08/23 20:33:24, tommi wrote:
> w00t!

Done.

http://codereview.chromium.org/7669061/diff/17001/chrome_frame/test/chrome_fr...
File chrome_frame/test/chrome_frame_test_utils.h (left):

http://codereview.chromium.org/7669061/diff/17001/chrome_frame/test/chrome_fr...
chrome_frame/test/chrome_frame_test_utils.h:315: 
On 2011/08/23 20:33:24, tommi wrote:
> empty line removed by accident?

Oops, fixed.

[commit-bot: I haz the power, 2011-08-23 21:28:49]

Presubmit check for 7669061-18016 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: base/base.gyp

Presubmit checks took 4.0s to calculate.

[Mark Mentovai, 2011-08-23 21:57:33]

LGTM for OWNERS approval in base only.

[commit-bot: I haz the power, 2011-08-24 20:02:49]

Change committed as 98100