Merged PolicyServiceWatcher into PolicyWatcher.

Merged PolicyServiceWatcher into PolicyWatcher.

BUG=368321
Committed: https://crrev.com/a9376cda48ce55f3b7cec6a63a4c5864871718ad
Cr-Commit-Position: refs/heads/master@{#313570}

[Łukasz Anforowicz, 2015-01-26 17:45:10]

lukasza@chromium.org changed reviewers:
+ sergeyu@chromium.org

[Łukasz Anforowicz, 2015-01-26 17:45:11]

Sergey, could you please take a look at the changelist that merges
PolicyServiceWatcher into PolicyWatcher?

Thanks,

Lukasz

https://codereview.chromium.org/867463006/diff/1/remoting/host/policy_hack/po...
File remoting/host/policy_hack/policy_watcher.cc (right):

https://codereview.chromium.org/867463006/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.cc:92: 
GetPolicyNamespace is copy&pasted from policy_service_watcher.cc

https://codereview.chromium.org/867463006/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.cc:229: }
Everything below is copy&pasted from policy_service_watcher.cc except:
1. s/PolicyServiceWatcher/PolicyWatcher/g
2. minor changes to the form used by constructors (in order to reuse a "base"
constructor, I had to move field initialization of owner_policy_provider [etc]
into field assignment in the body of the constructor)

[Sergey Ulanov, 2015-01-26 18:20:01]

look mostly good, I just have some style nits

https://codereview.chromium.org/867463006/diff/1/remoting/host/policy_hack/po...
File remoting/host/policy_hack/policy_watcher.cc (right):

https://codereview.chromium.org/867463006/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.cc:40: using namespace policy;
style guide prohibits "using namespace":
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces

But you can replace it with "using policy::key;".

https://codereview.chromium.org/867463006/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.cc:95:
PolicyWatcher::PolicyWatcher(const scoped_refptr<base::SingleThreadTaskRunner>&
nit: Move this constructructor next to the other constructors below?

https://codereview.chromium.org/867463006/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.cc:218: PolicyWatcher::PolicyWatcher(
This class has 3 private constructors. Consider replacing them with a single
constructor that takes all the arguments and pass nullptr for some of them. Even
if you prefer to keep multiple constructors you don't need all 3. This
constructor can call the one above, so you can merge the first two constructors.

https://codereview.chromium.org/867463006/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.cc:242: it++) {
s/it++/++it/
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Preincrement_...

https://codereview.chromium.org/867463006/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.cc:320: // Always read the Chrome
policies (even on Chromium) so that policy
This comment applies to Win and OSX too. Move it out of the ifdef?

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

https://codereview.chromium.org/867463006/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.h:97: // TODO(lukasza):
s/Internal/OnWatcherThread/g
maybe rename them in this CL?

[Łukasz Anforowicz, 2015-01-26 19:17:07]

Patchset #2 (id:20001) has been deleted

[Łukasz Anforowicz, 2015-01-26 19:24:20]

Thanks for the feedback Sergey.  Your comments should be addressed in patchset
#2.  Could you please take another look?

-Lukasz

https://codereview.chromium.org/867463006/diff/1/remoting/host/policy_hack/po...
File remoting/host/policy_hack/policy_watcher.cc (right):

https://codereview.chromium.org/867463006/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.cc:40: using namespace policy;
On 2015/01/26 18:20:01, Sergey Ulanov wrote:
> style guide prohibits "using namespace":
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces
> 
> But you can replace it with "using policy::key;". 

Oops.  Fixed.  FWIW, I was just retaining it from policy_watcher_chromeos.cc.

https://codereview.chromium.org/867463006/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.cc:95:
PolicyWatcher::PolicyWatcher(const scoped_refptr<base::SingleThreadTaskRunner>&
On 2015/01/26 18:20:01, Sergey Ulanov wrote:
> nit: Move this constructructor next to the other constructors below?

Done.

https://codereview.chromium.org/867463006/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.cc:218: PolicyWatcher::PolicyWatcher(
On 2015/01/26 18:20:01, Sergey Ulanov wrote:
> This class has 3 private constructors. Consider replacing them with a single
> constructor that takes all the arguments and pass nullptr for some of them.
Even
> if you prefer to keep multiple constructors you don't need all 3. This
> constructor can call the one above, so you can merge the first two
constructors.

I merged into a single constructor.

https://codereview.chromium.org/867463006/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.cc:242: it++) {
On 2015/01/26 18:20:01, Sergey Ulanov wrote:
> s/it++/++it/
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Preincrement_...

Done.

I also looked into whether I can use "auto" here, but there is no way to
explicitly say that I want a "const" iterator, so I decided not to do it. 
("auto" would still give a "const" iterator because "current" is "const", but
this seems like too much magic the reader of the code has to be aware of... :-/
).

https://codereview.chromium.org/867463006/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.cc:320: // Always read the Chrome
policies (even on Chromium) so that policy
On 2015/01/26 18:20:01, Sergey Ulanov wrote:
> This comment applies to Win and OSX too. Move it out of the ifdef?

I moved it slightly higher + added a note that the comment applies to all 3
cases.  Note that this behavior is different from behavior of the browser and
I'll probably have to follow-up on this later on (i.e. Mattias was surprised by
this behavior here).

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

https://codereview.chromium.org/867463006/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.h:97: // TODO(lukasza):
s/Internal/OnWatcherThread/g
On 2015/01/26 18:20:01, Sergey Ulanov wrote:
> maybe rename them in this CL?

Done.  Initially I wanted to just make this changelist a dumb merge with little
changes of content being merged.  OTOH, with the merging of the constructors and
getting rid of "using namespace policy", it is already a quite busy changelist,
so it is probably better to make this as complete as possible.  OTOH, I probably
want to remove the type-checking in a separate changelist (as this is not simple
refactoring as the other things within the current changelist).

[Sergey Ulanov, 2015-01-28 07:07:53]

lgtm

https://codereview.chromium.org/867463006/diff/1/remoting/host/policy_hack/po...
File remoting/host/policy_hack/policy_watcher.cc (right):

https://codereview.chromium.org/867463006/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.cc:40: using namespace policy;
On 2015/01/26 19:24:20, lukasza wrote:
> On 2015/01/26 18:20:01, Sergey Ulanov wrote:
> > style guide prohibits "using namespace":
> > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces
> > 
> > But you can replace it with "using policy::key;". 
> 
> Oops.  Fixed.  FWIW, I was just retaining it from policy_watcher_chromeos.cc.

If I remember correctly the style guide did allow it in the past.  Don't know
when that section was changed.

https://codereview.chromium.org/867463006/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.cc:242: it++) {
On 2015/01/26 19:24:20, lukasza wrote:
> On 2015/01/26 18:20:01, Sergey Ulanov wrote:
> > s/it++/++it/
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Preincrement_...
> 
> Done.
> 
> I also looked into whether I can use "auto" here, but there is no way to
> explicitly say that I want a "const" iterator, so I decided not to do it. 
> ("auto" would still give a "const" iterator because "current" is "const", but
> this seems like too much magic the reader of the code has to be aware of...
:-/
> ).

I don't think it's essential that the iterator is const here, so if auto works
here I think it would make this code more readable. Up to you.

[Łukasz Anforowicz, 2015-01-28 17:33:22]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2015-01-28 17:34:27]

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

[commit-bot: I haz the power, 2015-01-28 18:31:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-28 18:31:11]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Łukasz Anforowicz, 2015-01-28 19:23:31]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2015-01-28 19:24:28]

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

[commit-bot: I haz the power, 2015-01-28 20:29:36]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-01-28 20:30:49]

Patchset 4 (id:??) landed as
https://crrev.com/a9376cda48ce55f3b7cec6a63a4c5864871718ad
Cr-Commit-Position: refs/heads/master@{#313570}