Remove raw handles from base::win::RegKey

Remove raw handles from base::win::RegKey

BUG=419210, 423634
R=cpu@chromium.org, eroman@chromium.org, sky@chromium.org

Committed: https://crrev.com/1aa0fa75b65f403e08ae0f3f2fcb053c02cd9ef2
Cr-Commit-Position: refs/heads/master@{#299737}

Committed: https://crrev.com/6f315b85b9126cdbbeee5457a7fa1dbeed058105
Cr-Commit-Position: refs/heads/master@{#300036}

[rvargas (doing something else), 2014-10-07 02:28:25]

rvargas@chromium.org changed reviewers:
+ cpu@chromium.org

[rvargas (doing something else), 2014-10-07 02:28:25]

cpu: please take a look at base.

[cpu_(ooo_6.6-7.5), 2014-10-08 21:36:46]

otherwise looks nice.

Well, there is the issue of :: on the windows calls.

https://codereview.chromium.org/632833002/diff/20001/base/win/registry.h
File base/win/registry.h (right):

https://codereview.chromium.org/632833002/diff/20001/base/win/registry.h#newc...
base/win/registry.h:28: class BASE_EXPORT RegKey : private
ObjectWatcher::Delegate {
do we allow private inheritance?

[rvargas (doing something else), 2014-10-08 23:01:46]

Thanks. PTAL.

> Well, there is the issue of :: on the windows calls.

Isn't that a thing of the past? It is not universally followed in our code (and
TBH I don't see the value... and it mostly causes extra line breaks)

https://codereview.chromium.org/632833002/diff/20001/base/win/registry.h
File base/win/registry.h (right):

https://codereview.chromium.org/632833002/diff/20001/base/win/registry.h#newc...
base/win/registry.h:28: class BASE_EXPORT RegKey : private
ObjectWatcher::Delegate {
On 2014/10/08 21:36:46, cpu wrote:
> do we allow private inheritance?

nope :(
Changed

[cpu_(ooo_6.6-7.5), 2014-10-08 23:28:19]

https://codereview.chromium.org/632833002/diff/40001/base/win/registry.cc
File base/win/registry.cc (right):

https://codereview.chromium.org/632833002/diff/40001/base/win/registry.cc#new...
base/win/registry.cc:46: private:
now that you have this helper guy seems we want the object_watcher_ here as a
member.

[rvargas (doing something else), 2014-10-09 00:18:07]

Patchset #4 (id:60001) has been deleted

[rvargas (doing something else), 2014-10-09 00:22:47]

ptal

https://codereview.chromium.org/632833002/diff/40001/base/win/registry.cc
File base/win/registry.cc (right):

https://codereview.chromium.org/632833002/diff/40001/base/win/registry.cc#new...
base/win/registry.cc:46: private:
On 2014/10/08 23:28:19, cpu wrote:
> now that you have this helper guy seems we want the object_watcher_ here as a
> member.

done

I wanted a trivial adaptor between a delegate interface that cannot be part of
the the key and the key.

[cpu_(ooo_6.6-7.5), 2014-10-09 19:36:26]

lgtm

[rvargas (doing something else), 2014-10-10 20:23:16]

rvargas@chromium.org changed reviewers:
+ eroman@chromium.org, sky@chromium.org

[rvargas (doing something else), 2014-10-10 20:23:56]

+ eroman -> net
+ sky -> content

[eroman, 2014-10-10 21:34:36]

https://codereview.chromium.org/632833002/diff/80001/base/win/registry.cc
File base/win/registry.cc (right):

https://codereview.chromium.org/632833002/diff/80001/base/win/registry.cc#new...
base/win/registry.cc:63: return true;
This doesn't match my expectation of what StartWatching should return.

Here is the scenario I am thinking about:

 (1) StartWatching() is called the first time, and fails on
RegNotifyChangeKeyValue(). watch_event_ will be initialized to something valid,
however the key is not being watched.

 (2) StartWatching() is called again in attempt to re-try. This time it returns
true from this line. However the registry key is still now being watched.

https://codereview.chromium.org/632833002/diff/80001/base/win/registry.h
File base/win/registry.h (right):

https://codereview.chromium.org/632833002/diff/80001/base/win/registry.h#newc...
base/win/registry.h:30: // Called from the MessageLoop when the key changes. To
continue watching the
This comment seems more appropriate as part of StartWatching(). In particular
mentioning that the callback is only called once.

https://codereview.chromium.org/632833002/diff/80001/base/win/registry.h#newc...
base/win/registry.h:156: ChangeCallback callback_;
I suggest keeping this as an internal detail of Watcher. Similarly, move
OnObjectSignaled into Watcher.

https://codereview.chromium.org/632833002/diff/80001/net/dns/dns_config_servi...
File net/dns/dns_config_service_win.cc (right):

https://codereview.chromium.org/632833002/diff/80001/net/dns/dns_config_servi...
net/dns/dns_config_service_win.cc:739: if (!succeeded) {
What is the reason for this change?

https://codereview.chromium.org/632833002/diff/80001/net/proxy/proxy_config_s...
File net/proxy/proxy_config_service_win.cc (right):

https://codereview.chromium.org/632833002/diff/80001/net/proxy/proxy_config_s...
net/proxy/proxy_config_service_win.cc:101: scoped_ptr<base::win::RegKey>
entry(new base::win::RegKey);
rename entry --> key

https://codereview.chromium.org/632833002/diff/80001/net/proxy/proxy_config_s...
net/proxy/proxy_config_service_win.cc:134: keys_to_watch_.erase(it);
Looks like the pre-existing code is bad, and is leaking the entry.
It is a pre-existing bug, but do you want to address it as part of this change?

https://codereview.chromium.org/632833002/diff/80001/net/proxy/proxy_config_s...
File net/proxy/proxy_config_service_win.h (right):

https://codereview.chromium.org/632833002/diff/80001/net/proxy/proxy_config_s...
net/proxy/proxy_config_service_win.h:58: typedef std::vector<base::win::RegKey*>
KeyEntryList;
Can you rename this to RegKeyList ?

https://codereview.chromium.org/632833002/diff/80001/net/proxy/proxy_config_s...
net/proxy/proxy_config_service_win.h:63: // Creates a new KeyEntry and appends
it to |keys_to_watch_|. If the key
Remove reference to KeyEntry

https://codereview.chromium.org/632833002/diff/80001/net/proxy/proxy_config_s...
net/proxy/proxy_config_service_win.h:67: // ObjectWatcher::Delegate methods:
This line is no longer applicable.

[eroman, 2014-10-10 21:41:58]

https://codereview.chromium.org/632833002/diff/80001/net/proxy/proxy_config_s...
File net/proxy/proxy_config_service_win.cc (right):

https://codereview.chromium.org/632833002/diff/80001/net/proxy/proxy_config_s...
net/proxy/proxy_config_service_win.cc:122: KeyEntryList::iterator it;
Looks like the pre-existing code is bad and doesn't initialize "it".

I suggest changing this to
 it = std::find(keys_to_watch_.begin(), keys_to_watch_.end(), key);

[rvargas (doing something else), 2014-10-10 22:22:06]

Thanks

https://codereview.chromium.org/632833002/diff/80001/base/win/registry.cc
File base/win/registry.cc (right):

https://codereview.chromium.org/632833002/diff/80001/base/win/registry.cc#new...
base/win/registry.cc:63: return true;
On 2014/10/10 21:34:35, eroman wrote:
> This doesn't match my expectation of what StartWatching should return.
> 
> Here is the scenario I am thinking about:
> 
>  (1) StartWatching() is called the first time, and fails on
> RegNotifyChangeKeyValue(). watch_event_ will be initialized to something
valid,
> however the key is not being watched.

That failures closes watch_event_ (line 81)
> 
>  (2) StartWatching() is called again in attempt to re-try. This time it
returns
> true from this line. However the registry key is still now being watched.

That's the behavior for XP. Once a key is registered, the event continues to be
set every time there is a change until the key is closed.

https://codereview.chromium.org/632833002/diff/80001/base/win/registry.h
File base/win/registry.h (right):

https://codereview.chromium.org/632833002/diff/80001/base/win/registry.h#newc...
base/win/registry.h:30: // Called from the MessageLoop when the key changes. To
continue watching the
On 2014/10/10 21:34:35, eroman wrote:
> This comment seems more appropriate as part of StartWatching(). In particular
> mentioning that the callback is only called once.

Moved the last part to StartWatching() but I'm not sure if it is worth keeping
it given that this is the standard behavior (calling a callback only once)

https://codereview.chromium.org/632833002/diff/80001/base/win/registry.h#newc...
base/win/registry.h:156: ChangeCallback callback_;
On 2014/10/10 21:34:35, eroman wrote:
> I suggest keeping this as an internal detail of Watcher. Similarly, move
> OnObjectSignaled into Watcher.

Done.

https://codereview.chromium.org/632833002/diff/80001/net/dns/dns_config_servi...
File net/dns/dns_config_service_win.cc (right):

https://codereview.chromium.org/632833002/diff/80001/net/dns/dns_config_servi...
net/dns/dns_config_service_win.cc:739: if (!succeeded) {
On 2014/10/10 21:34:35, eroman wrote:
> What is the reason for this change?

|succeeded| means that the next notification was set, not that the in-progress
notification failed. If we are here, a registry key changed.

https://codereview.chromium.org/632833002/diff/80001/net/proxy/proxy_config_s...
File net/proxy/proxy_config_service_win.cc (right):

https://codereview.chromium.org/632833002/diff/80001/net/proxy/proxy_config_s...
net/proxy/proxy_config_service_win.cc:122: KeyEntryList::iterator it;
On 2014/10/10 21:41:58, eroman wrote:
> Looks like the pre-existing code is bad and doesn't initialize "it".

dos it on the loop

> I suggest changing this to
>  it = std::find(keys_to_watch_.begin(), keys_to_watch_.end(), key);

done

https://codereview.chromium.org/632833002/diff/80001/net/proxy/proxy_config_s...
net/proxy/proxy_config_service_win.cc:134: keys_to_watch_.erase(it);
On 2014/10/10 21:34:35, eroman wrote:
> Looks like the pre-existing code is bad, and is leaking the entry.
> It is a pre-existing bug, but do you want to address it as part of this
change?

Done.

https://codereview.chromium.org/632833002/diff/80001/net/proxy/proxy_config_s...
File net/proxy/proxy_config_service_win.h (right):

https://codereview.chromium.org/632833002/diff/80001/net/proxy/proxy_config_s...
net/proxy/proxy_config_service_win.h:58: typedef std::vector<base::win::RegKey*>
KeyEntryList;
On 2014/10/10 21:34:35, eroman wrote:
> Can you rename this to RegKeyList ?

Done.

https://codereview.chromium.org/632833002/diff/80001/net/proxy/proxy_config_s...
net/proxy/proxy_config_service_win.h:63: // Creates a new KeyEntry and appends
it to |keys_to_watch_|. If the key
On 2014/10/10 21:34:35, eroman wrote:
> Remove reference to KeyEntry

Done.

https://codereview.chromium.org/632833002/diff/80001/net/proxy/proxy_config_s...
net/proxy/proxy_config_service_win.h:67: // ObjectWatcher::Delegate methods:
On 2014/10/10 21:34:36, eroman wrote:
> This line is no longer applicable.

Done.

[eroman, 2014-10-10 22:35:12]

lgtm

https://codereview.chromium.org/632833002/diff/80001/net/proxy/proxy_config_s...
File net/proxy/proxy_config_service_win.cc (right):

https://codereview.chromium.org/632833002/diff/80001/net/proxy/proxy_config_s...
net/proxy/proxy_config_service_win.cc:122: KeyEntryList::iterator it;
On 2014/10/10 22:22:05, rvargas wrote:
> On 2014/10/10 21:41:58, eroman wrote:
> > Looks like the pre-existing code is bad and doesn't initialize "it".
> 
> dos it on the loop
> 
> > I suggest changing this to
> >  it = std::find(keys_to_watch_.begin(), keys_to_watch_.end(), key);
> 
> done

You are right about the loop part; brain fail on my part :(

Thanks for changing to std::find() at any rate!

[sky, 2014-10-13 16:51:38]

sky@chromium.org changed reviewers:
+ jam@chromium.org
- sky@chromium.org

[sky, 2014-10-13 16:51:47]

sky->jam

[jam, 2014-10-15 16:05:39]

lgtm

[rvargas (doing something else), 2014-10-15 18:50:53]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2014-10-15 18:52:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/632833002/330001

[commit-bot: I haz the power, 2014-10-15 19:16:56]

Committed patchset #6 (id:330001)

[commit-bot: I haz the power, 2014-10-15 19:17:47]

Patchset 6 (id:??) landed as
https://crrev.com/1aa0fa75b65f403e08ae0f3f2fcb053c02cd9ef2
Cr-Commit-Position: refs/heads/master@{#299737}

[dcheng, 2014-10-16 03:04:40]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2014-10-16 03:04:40]

Not sure why, but ChangeCallback is consistently timing out on the XP Tests (1).
See
https://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/build...

[dcheng, 2014-10-16 03:05:10]

A revert of this CL (patchset #6 id:330001) has been created in
https://codereview.chromium.org/652903005/ by dcheng@chromium.org.

The reason for reverting is: Causing base_unittests failures on XP Tests (1).

[rvargas (doing something else), 2014-10-16 22:12:25]

cpu, do you mind looking at registry.cc again?

Patch set 6 landed/was reverted. PS 7 is the fix

[cpu_(ooo_6.6-7.5), 2014-10-16 22:43:59]

lgtm

[rvargas (doing something else), 2014-10-16 22:44:48]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2014-10-16 22:46:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/632833002/350001

[commit-bot: I haz the power, 2014-10-17 01:29:11]

Committed patchset #7 (id:350001)

[commit-bot: I haz the power, 2014-10-17 01:29:56]

Patchset 7 (id:??) landed as
https://crrev.com/6f315b85b9126cdbbeee5457a7fa1dbeed058105
Cr-Commit-Position: refs/heads/master@{#300036}