Refactor PolicyWatcher to allow mocking

Refactor PolicyWatcher to allow mocking

Supplanted by 2764333003

[rkjnsn, 2017-02-23 02:02:37]

rkjnsn@chromium.org changed reviewers:
+ jamiewalch@chromium.org, sergeyu@chromium.org

[rkjnsn, 2017-02-23 02:02:37]

Is this a good approach?

[Sergey Ulanov, 2017-02-23 19:18:55]

Yes, this is a good approach. I suggest moving the Impl class to a separate file
(policy_watcher.cc shouldn't needed at all).

https://codereview.chromium.org/2710023003/diff/1/remoting/host/policy_watcher.h
File remoting/host/policy_watcher.h (right):

https://codereview.chromium.org/2710023003/diff/1/remoting/host/policy_watche...
remoting/host/policy_watcher.h:38: virtual ~PolicyWatcher() = 0;
s/=0/{}/
I didn't know destructor can be pure virtual, but I don't think we use it
anywhere else, and for consistency it's better to define an empty destructor.

https://codereview.chromium.org/2710023003/diff/1/remoting/host/policy_watche...
remoting/host/policy_watcher.h:79: PolicyWatcher();
Usually for interfaces the constructor should have an empty inline definition,
or you can omit it completely.

https://codereview.chromium.org/2710023003/diff/1/remoting/host/policy_watche...
remoting/host/policy_watcher.h:96: virtual const base::DictionaryValue*
GetDefaultValues() const = 0;
I don't think these private methods need to be here. Only Impl class needs them.

https://google.github.io/styleguide/cppguide.html#Interfaces

[rkjnsn, 2017-02-23 22:28:10]

On 2017/02/23 19:18:55, Sergey Ulanov wrote:
> Yes, this is a good approach. I suggest moving the Impl class to a separate
file
> (policy_watcher.cc shouldn't needed at all).

Currently, policy_watcher.cc holds the definition of PolicyWatcher::Create and
PolicyWatcher::CreateFromPolicyLoader, which in turn construct instances of
PolicyWatcherImpl.

The overall approach I went for here, and which I was asking about, was to make
the minimal changes to the creation and usage of PolicyWatcher while still
allowing it's behavior to be overridden when needed for testing. Thus, I was
considering PolicyWatcherImpl to be more of an implementation detail of
PolicyWatcher's default behaviour. (That's why I put PolicyWatcherImpl in the
anonymous namespace in policy_watcher.cc, and GetPolicySchema and
GetDefaultValues are exposed through PolicyWatcher for testing.)

Another approach, which it sounds like you might prefer, would be to more fully
extract an isolated interface. It that case, I'd move the existing
implementation to SystemPolicyWatcher or some such and leave PolicyWatcher as
just an interface, then replace calls to PolicyWatcher::Create with
SystemPolicyWatcher::Create.

As I mentioned last night, though, I realized that just exposing
CreateFromPolicyLoader might be sufficient for the it2me domain validation test
I'm trying to write, so I'll probably just try to do that unless there are other
reasons to split PolicyWatcher or we don't want to expose CreateFromPolicyLoader
for some reason.

https://codereview.chromium.org/2710023003/diff/1/remoting/host/policy_watcher.h
File remoting/host/policy_watcher.h (right):

https://codereview.chromium.org/2710023003/diff/1/remoting/host/policy_watche...
remoting/host/policy_watcher.h:38: virtual ~PolicyWatcher() = 0;
On 2017/02/23 19:18:55, Sergey Ulanov wrote:
> s/=0/{}/
> I didn't know destructor can be pure virtual, but I don't think we use it
> anywhere else, and for consistency it's better to define an empty destructor.

Pure virtual destructors are interesting beasts. Unlike pure virtual methods,
you still have to define them. (There's a definition in policy_watcher.cc) Their
main effect is to make a class abstract even if all virtual methods have default
implementations.

In some code bases, the convention is to always make the destructor of abstract
classes pure virtual to make it easy to tell at a glance whether the class is
abstract or not. However, it looks like Chromium prefers the opposite, so I'll
change it.

https://codereview.chromium.org/2710023003/diff/1/remoting/host/policy_watche...
remoting/host/policy_watcher.h:79: PolicyWatcher();
On 2017/02/23 19:18:55, Sergey Ulanov wrote:
> Usually for interfaces the constructor should have an empty inline definition,
> or you can omit it completely.

Okay, I'll move it inline.

[Sergey Ulanov, 2017-03-18 00:22:40]

What's the status of this CL? Is there anything that blocks you from moving the
code to an Impl class?

[rkjnsn, 2017-04-06 17:06:05]

Description was changed from

==========
Refactor PolicyWatcher to allow mocking
==========

to

==========
Refactor PolicyWatcher to allow mocking

Supplanted by 2764333003
==========