Add a platform policy provider for Android.

Add a platform policy provider for Android.

BUG=340704
TBR=ben@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249618

[Bernhard Bauer, 2014-02-05 19:22:20]

Please review. Thanks!

[Joao da Silva, 2014-02-05 20:22:26]

Other than the behavior of IsInitializationComplete(), this class is mostly like
the
ProxyPolicyProvider. If that one could work for you then try using it instead.

IIUC you're going to call GetPlatformProvider() and cast it; and the
initialized_ flag uses the global singleton because you can't pass it into
ChromeBrowserPolicyConnector::CreatePlatformProvider(), is that right?

If so then you may consider subclassing BrowserPolicyConnector or
ChromeBrowserPolicyConnector with an AndroidBrowserPolicyConnector (we already
do this for ChromeOS and iOS) so that you can have a getter with the right type,
and you can also pass parameters to your provider at construction time.

You can then have a getter for that connector in
browser_process_platform_part_android.cc (similar to
browser_process_platform_part_chromeos.cc, for example) and then


g_browser_process->platform_part()->android_browser_policy_connector()->GetProvider()->SetDelegate/Flag/etc

WDYT?

https://codereview.chromium.org/155923002/diff/180001/chrome/browser/policy/c...
File chrome/browser/policy/chrome_browser_policy_connector.cc (right):

https://codereview.chromium.org/155923002/diff/180001/chrome/browser/policy/c...
chrome/browser/policy/chrome_browser_policy_connector.cc:186: return new
PolicyProviderAndroid();
Question: how will you get this provider to set its delegate? Will you call
GetPlatformProvider() and cast it?

https://codereview.chromium.org/155923002/diff/180001/components/policy/core/...
File components/policy/core/common/policy_provider_android.cc (right):

https://codereview.chromium.org/155923002/diff/180001/components/policy/core/...
components/policy/core/common/policy_provider_android.cc:19:
PolicyProviderAndroid::~PolicyProviderAndroid() {}
nit: newline above this one

https://codereview.chromium.org/155923002/diff/180001/components/policy/core/...
components/policy/core/common/policy_provider_android.cc:46: return
initialized_;
Other than the behavior of this flag, this class is mostly like the
ProxyPolicyProvider.

If that one could work for you then try using it instead.

https://codereview.chromium.org/155923002/diff/180001/components/policy/core/...
components/policy/core/common/policy_provider_android.cc:52: }
else {
  call UpdatePolicy() with a copy of the current bundle (see
ProxyPolicyProvider::RefreshPolicies)
}

This is required because a call to RefreshPolicies must always trigger a
notification to observers of this provider, eventually. You should also make
sure that the delegate calls SetPolicies() when it gets RefreshPolicies() called
(please add a comment documenting this to the delegate)

https://codereview.chromium.org/155923002/diff/180001/components/policy/core/...
File components/policy/core/common/policy_provider_android_delegate.h (right):

https://codereview.chromium.org/155923002/diff/180001/components/policy/core/...
components/policy/core/common/policy_provider_android_delegate.h:10: #include
"base/memory/weak_ptr.h"
not used

https://codereview.chromium.org/155923002/diff/180001/components/policy/core/...
components/policy/core/common/policy_provider_android_delegate.h:12: #include
"components/policy/core/common/policy_service.h"
not used

https://codereview.chromium.org/155923002/diff/180001/components/policy/core/...
components/policy/core/common/policy_provider_android_delegate.h:18: class
POLICY_EXPORT PolicyProviderAndroidDelegate {
We usually have Observers and Delegates like this declared inside the main
class. Can you move this to PolicyProviderAndroid::Delegate?

And a suggestion (but I don't know how the delegate looks like internally, so
feel free to bypass the suggestion):

Could PolicyProviderAndroid own the delegate, and also get it passed in the
ctor?

[Bernhard Bauer, 2014-02-05 22:20:19]

Thanks for the review!

On 2014/02/05 20:22:26, Joao da Silva wrote:
> Other than the behavior of IsInitializationComplete(), this class is mostly
like
> the
> ProxyPolicyProvider. If that one could work for you then try using it instead.
> 
> IIUC you're going to call GetPlatformProvider() and cast it; and the
> initialized_ flag uses the global singleton because you can't pass it into
> ChromeBrowserPolicyConnector::CreatePlatformProvider(), is that right?

That's mostly correct, there is one additional thing though: The native
downstream code is called very early on, and I don't think the browser policy
connector already exists at that point. If BrowserProcessPlatformPartAndroid
already exists though and it's okay for me to put stuff in there, I could
probably create the PolicyProviderAndroidDelegate and store it there, then pass
it to the policy provider at construction like you suggest below. 

> If so then you may consider subclassing BrowserPolicyConnector or
> ChromeBrowserPolicyConnector with an AndroidBrowserPolicyConnector (we already
> do this for ChromeOS and iOS) so that you can have a getter with the right
type,
> and you can also pass parameters to your provider at construction time.
> 
> You can then have a getter for that connector in
> browser_process_platform_part_android.cc (similar to
> browser_process_platform_part_chromeos.cc, for example) and then
> 
> 
>
g_browser_process->platform_part()->android_browser_policy_connector()->GetProvider()->SetDelegate/Flag/etc
> 
> WDYT?

https://codereview.chromium.org/155923002/diff/180001/components/policy/core/...
File components/policy/core/common/policy_provider_android.cc (right):

https://codereview.chromium.org/155923002/diff/180001/components/policy/core/...
components/policy/core/common/policy_provider_android.cc:46: return
initialized_;
On 2014/02/05 20:22:26, Joao da Silva wrote:
> Other than the behavior of this flag, this class is mostly like the
> ProxyPolicyProvider.
> 
> If that one could work for you then try using it instead.

Hm, so then downstream I would implement another ConfigurationPolicyProvider?
That should work, but yeah, we'd need to do something about the delayed
initialization flag.

https://codereview.chromium.org/155923002/diff/180001/components/policy/core/...
components/policy/core/common/policy_provider_android.cc:52: }
On 2014/02/05 20:22:26, Joao da Silva wrote:
> else {
>   call UpdatePolicy() with a copy of the current bundle (see
> ProxyPolicyProvider::RefreshPolicies)
> }
> 
> This is required because a call to RefreshPolicies must always trigger a
> notification to observers of this provider, eventually. You should also make
> sure that the delegate calls SetPolicies() when it gets RefreshPolicies()
called
> (please add a comment documenting this to the delegate)

Oh, good catch! Yeah, I will do that.

https://codereview.chromium.org/155923002/diff/180001/components/policy/core/...
File components/policy/core/common/policy_provider_android_delegate.h (right):

https://codereview.chromium.org/155923002/diff/180001/components/policy/core/...
components/policy/core/common/policy_provider_android_delegate.h:18: class
POLICY_EXPORT PolicyProviderAndroidDelegate {
On 2014/02/05 20:22:26, Joao da Silva wrote:
> We usually have Observers and Delegates like this declared inside the main
> class. Can you move this to PolicyProviderAndroid::Delegate?
> 
> And a suggestion (but I don't know how the delegate looks like internally, so
> feel free to bypass the suggestion):
> 
> Could PolicyProviderAndroid own the delegate, and also get it passed in the
> ctor?

Hm, currently the lifetime of the delegate is coupled to the Java classes, i.e.
it is constructed and destructed from Java. OTOH, we could probably construct it
beforehand, and then just get a reference to it for access from Java... I'll
look into it tomorrow.

[Joao da Silva, 2014-02-06 11:05:41]

The BPC is created quite early on; it's guaranteed to be created before
local_state (so that policy can override prefs there from the beginning). The
platform_part is created even earlier than that. So if you don't need to
override something even earlier than local_state then that should work.

It's fine for me to have the delegate+new provider or to reuse the
ProxyPolicyProvider; I don't know how downstream looks like and I trust your
judgement about what's best here.

[Bernhard Bauer, 2014-02-06 15:00:18]

OK, I tried adding a BrowserPolicyConnectorAndroid, but it turns out we still
need to static cast to it in BrowserProcessPlatformPart, so we wouldn't reduce
the number of static casts. Would you be okay with the CL in this form?

https://codereview.chromium.org/155923002/diff/180001/components/policy/core/...
File components/policy/core/common/policy_provider_android.cc (right):

https://codereview.chromium.org/155923002/diff/180001/components/policy/core/...
components/policy/core/common/policy_provider_android.cc:19:
PolicyProviderAndroid::~PolicyProviderAndroid() {}
On 2014/02/05 20:22:26, Joao da Silva wrote:
> nit: newline above this one

Done.

https://codereview.chromium.org/155923002/diff/180001/components/policy/core/...
components/policy/core/common/policy_provider_android.cc:52: }
On 2014/02/05 22:20:19, Bernhard Bauer wrote:
> On 2014/02/05 20:22:26, Joao da Silva wrote:
> > else {
> >   call UpdatePolicy() with a copy of the current bundle (see
> > ProxyPolicyProvider::RefreshPolicies)
> > }
> > 
> > This is required because a call to RefreshPolicies must always trigger a
> > notification to observers of this provider, eventually. You should also make
> > sure that the delegate calls SetPolicies() when it gets RefreshPolicies()
> called
> > (please add a comment documenting this to the delegate)
> 
> Oh, good catch! Yeah, I will do that.

Done.

https://codereview.chromium.org/155923002/diff/180001/components/policy/core/...
File components/policy/core/common/policy_provider_android_delegate.h (right):

https://codereview.chromium.org/155923002/diff/180001/components/policy/core/...
components/policy/core/common/policy_provider_android_delegate.h:10: #include
"base/memory/weak_ptr.h"
On 2014/02/05 20:22:26, Joao da Silva wrote:
> not used

Removed.

https://codereview.chromium.org/155923002/diff/180001/components/policy/core/...
components/policy/core/common/policy_provider_android_delegate.h:12: #include
"components/policy/core/common/policy_service.h"
On 2014/02/05 20:22:26, Joao da Silva wrote:
> not used

Removed.

[Joao da Silva, 2014-02-06 15:38:26]

lgtm

Thanks for doing this!

https://codereview.chromium.org/155923002/diff/290001/components/policy/core/...
File components/policy/core/common/policy_provider_android_unittest.cc (right):

https://codereview.chromium.org/155923002/diff/290001/components/policy/core/...
components/policy/core/common/policy_provider_android_unittest.cc:30:
~MockPolicyProviderAndroidDelegate() {}
virtual

https://codereview.chromium.org/155923002/diff/290001/components/policy/core/...
components/policy/core/common/policy_provider_android_unittest.cc:69:
MockPolicyProviderAndroidDelegate delegate_;
not used

[Bernhard Bauer, 2014-02-06 16:34:57]

You're welcome! I'm glad we can get this done quickly.

TBR: Ben for components GYP changes (adding a new test)

https://codereview.chromium.org/155923002/diff/290001/components/policy/core/...
File components/policy/core/common/policy_provider_android_unittest.cc (right):

https://codereview.chromium.org/155923002/diff/290001/components/policy/core/...
components/policy/core/common/policy_provider_android_unittest.cc:30:
~MockPolicyProviderAndroidDelegate() {}
On 2014/02/06 15:38:27, Joao da Silva wrote:
> virtual

Done.

https://codereview.chromium.org/155923002/diff/290001/components/policy/core/...
components/policy/core/common/policy_provider_android_unittest.cc:69:
MockPolicyProviderAndroidDelegate delegate_;
On 2014/02/06 15:38:27, Joao da Silva wrote:
> not used

Removed.

[Bernhard Bauer, 2014-02-06 16:35:02]

The CQ bit was checked by bauerb@chromium.org

[commit-bot: I haz the power, 2014-02-06 16:37:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/155923002/420001

[commit-bot: I haz the power, 2014-02-07 02:13:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/155923002/420001

[commit-bot: I haz the power, 2014-02-07 08:17:18]

Change committed as 249618