Adds basic group policy support. Resubmitting r33093 with a new unit test....

Adds basic group policy support.  Resubmitting r33093 with a new unit test.

R=kuchhal
TEST=unit test included
BUG=19374
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=34008

[Glenn Wilson, 2009-12-01 23:07:43]

Here's another shot at this patch -- I modified the unit test to fail gracefully
if it cannot set registry keys.

[kuchhal, 2009-12-02 17:51:38]

lgtm.

I have only looked at the unit test file in this round since apparently thats
the only file that was changed.

My only concern is that these tests wont ever run on our continuous buildbots
(right?). Would be nice to use some sort of dummy key or something to actually
make them run.

http://codereview.chromium.org/450037/diff/2001/2006
File chrome/browser/group_policy_unittest.cc (right):

http://codereview.chromium.org/450037/diff/2001/2006#newcode18
chrome/browser/group_policy_unittest.cc:18: bool can_set_hkcu;
rename to can_set_hklm_ and can_set_hkcu_

[Glenn Wilson, 2009-12-02 22:48:17]

On 2009/12/02 17:51:38, kuchhal wrote:
> lgtm.
> 
> I have only looked at the unit test file in this round since apparently thats
> the only file that was changed.
> 
> My only concern is that these tests wont ever run on our continuous buildbots
> (right?). Would be nice to use some sort of dummy key or something to actually
> make them run.
> 

This would be nice, yes.  M-A had the suggestion that we use this:
http://msdn.microsoft.com/en-us/library/ms724901(VS.85).aspx

Would you prefer that I go that route instead to make sure the tests always run
fully?  I'd worry that even with this API, we'll still run into auth issues.

> http://codereview.chromium.org/450037/diff/2001/2006
> File chrome/browser/group_policy_unittest.cc (right):
> 
> http://codereview.chromium.org/450037/diff/2001/2006#newcode18
> chrome/browser/group_policy_unittest.cc:18: bool can_set_hkcu;
> rename to can_set_hklm_ and can_set_hkcu_

Done.

[kuchhal, 2009-12-02 22:56:31]

It does look promising. Can buildbots write to hkcu? Or any other registry?
Even with this API it looks like you will need write permission to atleast
one registry key.

On Wed, Dec 2, 2009 at 2:48 PM, <gwilson@chromium.org> wrote:

> On 2009/12/02 17:51:38, kuchhal wrote:
>
>> lgtm.
>>
>
>  I have only looked at the unit test file in this round since apparently
>> thats
>> the only file that was changed.
>>
>
>  My only concern is that these tests wont ever run on our continuous
>> buildbots
>> (right?). Would be nice to use some sort of dummy key or something to
>> actually
>> make them run.
>>
>
>
> This would be nice, yes.  M-A had the suggestion that we use this:
> http://msdn.microsoft.com/en-us/library/ms724901(VS.85).aspx
>
> Would you prefer that I go that route instead to make sure the tests always
> run
> fully?  I'd worry that even with this API, we'll still run into auth
> issues.
>
>
>  http://codereview.chromium.org/450037/diff/2001/2006
>> File chrome/browser/group_policy_unittest.cc (right):
>>
>
>  http://codereview.chromium.org/450037/diff/2001/2006#newcode18
>> chrome/browser/group_policy_unittest.cc:18: bool can_set_hkcu;
>> rename to can_set_hklm_ and can_set_hkcu_
>>
>
> Done.
>
>
>
> http://codereview.chromium.org/450037
>

[Nicolas Sylvain, 2009-12-02 23:29:42]

On Wed, Dec 2, 2009 at 2:55 PM, Rahul Kuchhal <kuchhal@chromium.org> wrote:

> It does look promising. Can buildbots write to hkcu? Or any other registry?
> Even with this API it looks like you will need write permission to atleast
> one registry key.

Buildbots are running as normal users, with normal permissions, so they
should be able to write to hkcu

Nicolas


>
>
> On Wed, Dec 2, 2009 at 2:48 PM, <gwilson@chromium.org> wrote:
>
>> On 2009/12/02 17:51:38, kuchhal wrote:
>>
>>> lgtm.
>>>
>>
>>  I have only looked at the unit test file in this round since apparently
>>> thats
>>> the only file that was changed.
>>>
>>
>>  My only concern is that these tests wont ever run on our continuous
>>> buildbots
>>> (right?). Would be nice to use some sort of dummy key or something to
>>> actually
>>> make them run.
>>>
>>
>>
>> This would be nice, yes.  M-A had the suggestion that we use this:
>> http://msdn.microsoft.com/en-us/library/ms724901(VS.85).aspx
>>
>> Would you prefer that I go that route instead to make sure the tests
>> always run
>> fully?  I'd worry that even with this API, we'll still run into auth
>> issues.
>>
>>
>>  http://codereview.chromium.org/450037/diff/2001/2006
>>> File chrome/browser/group_policy_unittest.cc (right):
>>>
>>
>>  http://codereview.chromium.org/450037/diff/2001/2006#newcode18
>>> chrome/browser/group_policy_unittest.cc:18: bool can_set_hkcu;
>>> rename to can_set_hklm_ and can_set_hkcu_
>>>
>>
>> Done.
>>
>>
>>
>> http://codereview.chromium.org/450037
>>
>
>

[Glenn Wilson, 2009-12-03 19:33:01]

On Wed, Dec 2, 2009 at 3:29 PM, Nicolas Sylvain <nsylvain@chromium.org>wrote:

>
>
> On Wed, Dec 2, 2009 at 2:55 PM, Rahul Kuchhal <kuchhal@chromium.org>wrote:
>
>> It does look promising. Can buildbots write to hkcu? Or any other
>> registry? Even with this API it looks like you will need write permission to
>> atleast one registry key.
>
> Buildbots are running as normal users, with normal permissions, so they
> should be able to write to hkcu
>
>
Yes, to test anything at all, the unit test will need to be able to write to
hkcu (which it probably already can, per Nicolas).  It's possible that even
after implementing with this API, it would still only be able to write to
hkcu, which would give us no increase in coverage.  Should I give it a shot
anyway?




> Nicolas
>
>
>>
>>
>> On Wed, Dec 2, 2009 at 2:48 PM, <gwilson@chromium.org> wrote:
>>
>>> On 2009/12/02 17:51:38, kuchhal wrote:
>>>
>>>> lgtm.
>>>>
>>>
>>>  I have only looked at the unit test file in this round since apparently
>>>> thats
>>>> the only file that was changed.
>>>>
>>>
>>>  My only concern is that these tests wont ever run on our continuous
>>>> buildbots
>>>> (right?). Would be nice to use some sort of dummy key or something to
>>>> actually
>>>> make them run.
>>>>
>>>
>>>
>>> This would be nice, yes.  M-A had the suggestion that we use this:
>>> http://msdn.microsoft.com/en-us/library/ms724901(VS.85).aspx
>>>
>>> Would you prefer that I go that route instead to make sure the tests
>>> always run
>>> fully?  I'd worry that even with this API, we'll still run into auth
>>> issues.
>>>
>>>
>>>  http://codereview.chromium.org/450037/diff/2001/2006
>>>> File chrome/browser/group_policy_unittest.cc (right):
>>>>
>>>
>>>  http://codereview.chromium.org/450037/diff/2001/2006#newcode18
>>>> chrome/browser/group_policy_unittest.cc:18: bool can_set_hkcu;
>>>> rename to can_set_hklm_ and can_set_hkcu_
>>>>
>>>
>>> Done.
>>>
>>>
>>>
>>> http://codereview.chromium.org/450037
>>>
>>
>>
>

[kuchhal, 2009-12-03 22:30:53]

I think it is fine for now since the tests are exactly same for HKLM and HKCU.
In future if we have HKLM only tests that can not be run on buildbots, it would
be good to use HKCU on buildbots for testing.

For now lgtm.