Move AppRestriction to Policy code out of //chrome

Move AppRestriction to Policy code out of //chrome

This patch extracts the code related to transforming the Java
objects coming from the AppRestrictionProvider to a PolicyBundle
in the native code.

It now lives in a new class in the policy components.

BUG=506809
Committed: https://crrev.com/733516186f56ce8c3c315a1c5480951a21301e29
Cr-Commit-Position: refs/heads/master@{#338859}

[dgn, 2015-07-03 15:37:53]

dgn@chromium.org changed reviewers:
+ bartfab@chromium.org, bauerb@chromium.org

[Bernhard Bauer, 2015-07-03 16:00:33]

Why does the JNI code have to live in the embedder?

[bartfab (slow), 2015-07-03 16:55:49]

I reviewed everything except the unit tests. This is as far as I got today.

https://codereview.chromium.org/1220683008/diff/20001/chrome/browser/android/...
File chrome/browser/android/policy/policy_manager.cc (right):

https://codereview.chromium.org/1220683008/diff/20001/chrome/browser/android/...
chrome/browser/android/policy/policy_manager.cc:17:
g_browser_process->browser_policy_connector()->GetPlatformProvider());
Nit: #include "chrome/browser/browser_process_platform_part.h"

https://codereview.chromium.org/1220683008/diff/20001/chrome/browser/android/...
chrome/browser/android/policy/policy_manager.cc:24: return
policy_provider_->GetChromeSchema()->GetKnownProperty(key);
Nit: #include "components/policy/core/common/schema.h"

https://codereview.chromium.org/1220683008/diff/20001/chrome/browser/android/...
chrome/browser/android/policy/policy_manager.cc:32: JNIEnv* env =
AttachCurrentThread();
Nit: #include "base/android/jni_android.h"

https://codereview.chromium.org/1220683008/diff/20001/chrome/browser/android/...
File chrome/browser/android/policy/policy_manager.h (right):

https://codereview.chromium.org/1220683008/diff/20001/chrome/browser/android/...
chrome/browser/android/policy/policy_manager.h:21: // AppRestrictionsImporter:
1: Why are these two methods public?
2: Nit: Prefix with "policy::"

https://codereview.chromium.org/1220683008/diff/20001/chrome/browser/android/...
chrome/browser/android/policy/policy_manager.h:30: ~PolicyManager() override;
Nit: There should be no need to override the destructor here.

https://codereview.chromium.org/1220683008/diff/20001/chrome/browser/android/...
chrome/browser/android/policy/policy_manager.h:34:
policy::PolicyProviderAndroid* policy_provider_;
Nit: Forward-declare |policy::PolicyProviderAndroid|.

https://codereview.chromium.org/1220683008/diff/20001/chrome/browser/android/...
chrome/browser/android/policy/policy_manager.h:36:
DISALLOW_COPY_AND_ASSIGN(PolicyManager);
Nit: #include "base/macros.h"

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
File components/policy/core/browser/android/app_restrictions_importer.cc
(right):

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.cc:11: #include
"base/prefs/pref_service.h"
Nit: Not used.

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.cc:24:
AppRestrictionsImporter::~AppRestrictionsImporter() {}
Please move this and the following method so that declaration orde rmatches
definition order.

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.cc:27: return
PolicyNamespace(POLICY_DOMAIN_CHROME, std::string());
Nit: #include "components/policy/core/common/policy_namespace.h"

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.cc:35: new
base::FundamentalValue(static_cast<bool>(value)));
Nit: #include "base/values.h"

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.cc:58:
std::vector<base::string16> string_vector;
Nit 1: #include <vector>
Nit 2: #include "base/strings/string16.h"

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.cc:61:
base::ListValue* list_values = new base::ListValue();
Nit: Use a scoped_ptr to clarify ownership.

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.cc:67:
PolicyProviderAndroid* policy_provider = GetPolicyProvider();
Nit: const pointer.

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.cc:77:
base::Value* raw_value,
Nit: Indentation.

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.cc:78: const
Schema& schema) {
Nit: #include "components/policy/core/common/schema.h"

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.cc:133:
NOTREACHED();
Nit: #include "base/logging.h"

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.cc:157: if
(!GetPolicyProvider())
Why does it matter here whether we have a policy provider?

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.cc:160: Schema
schema = GetPolicySchema(key);
Nit: const.

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.cc:162:
PolicyNamespace ns = GetPolicyNamespace();
Nit: const.

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.cc:163:
policy_bundle_->Get(ns).Set(key,
Nit: #include "components/policy/core/common/policy_map.h"

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.cc:164:
POLICY_LEVEL_MANDATORY,
Nit: #include "components/policy/core/common/policy_types.h"

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
File components/policy/core/browser/android/app_restrictions_importer.h (right):

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.h:9: #include
"base/memory/scoped_ptr.h"
Nit: Add blank line above.

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.h:23: class
POLICY_EXPORT AppRestrictionsImporter {
Nit: Add a short comment that explains what this class does.

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.h:57: static
base::Value* ConvertValueToSchema(base::Value* raw_value,
Please change |raw_value| and the return value to scoped_ptrs, so that ownership
is explicit.

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.h:62: //
Returns the Schema for policy named |key|
Nit 1: s/for/for the/
Nit 2: Add a full stop at the end.

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.h:63: virtual
Schema GetPolicySchema(const std::string& key) = 0;
1: Nit: #include <string>
2: Why not use GetPolicyProvider()->GetChromeSchema()->GetKnownProperty(key)?

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.h:69: //
returns an empty namespace in the domain policy::POLICY_DOMAIN_CHROME
Nit 1: Add a full stop at the end.
Nit 2: That namespace is simply the name Chrome namespace. Other namespaces are
used for special types of policies, e.g. policy for extensions (which does not
exist on Android).

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.h:70: virtual
PolicyNamespace GetPolicyNamespace();
Why does this method exist, if nobody ever overrides it? Why can you not
hard-code the policy namespace?

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.h:73:
scoped_ptr<policy::PolicyBundle> policy_bundle_;
Nit: s/policy:://

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.h:77:
DISALLOW_COPY_AND_ASSIGN(AppRestrictionsImporter);
Nit: #include "base/macros.h"

[dgn, 2015-07-06 08:54:05]

On 2015/07/03 at 16:00:33, bauerb wrote:
> Why does the JNI code have to live in the embedder?

Hum good point. I wanted to stay close to the current implementation. It makes
it feel awkward though, that's true. I'll update it with something better.

[Bernhard Bauer, 2015-07-06 08:55:45]

On 2015/07/06 08:54:05, dgn wrote:
> On 2015/07/03 at 16:00:33, bauerb wrote:
> > Why does the JNI code have to live in the embedder?
> 
> Hum good point. I wanted to stay close to the current implementation. It makes
> it feel awkward though, that's true. I'll update it with something better.

I think the current implementation is still a bit an artifact of back when
PolicyManager was downstream, so we needed an additional abstraction layer for
the upstream/downstream split. We can probably simplify that now.

[dgn, 2015-07-06 17:34:04]

https://codereview.chromium.org/1220683008/diff/20001/chrome/browser/android/...
File chrome/browser/android/policy/policy_manager.cc (right):

https://codereview.chromium.org/1220683008/diff/20001/chrome/browser/android/...
chrome/browser/android/policy/policy_manager.cc:17:
g_browser_process->browser_policy_connector()->GetPlatformProvider());
On 2015/07/03 at 16:55:47, bartfab wrote:
> Nit: #include "chrome/browser/browser_process_platform_part.h"

Done. I don't understand why we need that though. browser_policy_connector() is
already in browser_process.h, so why do we add this one?

https://codereview.chromium.org/1220683008/diff/20001/chrome/browser/android/...
chrome/browser/android/policy/policy_manager.cc:24: return
policy_provider_->GetChromeSchema()->GetKnownProperty(key);
On 2015/07/03 at 16:55:47, bartfab wrote:
> Nit: #include "components/policy/core/common/schema.h"

Done.

https://codereview.chromium.org/1220683008/diff/20001/chrome/browser/android/...
chrome/browser/android/policy/policy_manager.cc:32: JNIEnv* env =
AttachCurrentThread();
On 2015/07/03 at 16:55:47, bartfab wrote:
> Nit: #include "base/android/jni_android.h"

Done.

https://codereview.chromium.org/1220683008/diff/20001/chrome/browser/android/...
File chrome/browser/android/policy/policy_manager.h (right):

https://codereview.chromium.org/1220683008/diff/20001/chrome/browser/android/...
chrome/browser/android/policy/policy_manager.h:21: // AppRestrictionsImporter:
On 2015/07/03 at 16:55:47, bartfab wrote:
> 1: Why are these two methods public?
> 2: Nit: Prefix with "policy::"

Made them private.

https://codereview.chromium.org/1220683008/diff/20001/chrome/browser/android/...
chrome/browser/android/policy/policy_manager.h:30: ~PolicyManager() override;
On 2015/07/03 at 16:55:47, bartfab wrote:
> Nit: There should be no need to override the destructor here.

Done.

https://codereview.chromium.org/1220683008/diff/20001/chrome/browser/android/...
chrome/browser/android/policy/policy_manager.h:34:
policy::PolicyProviderAndroid* policy_provider_;
On 2015/07/03 at 16:55:47, bartfab wrote:
> Nit: Forward-declare |policy::PolicyProviderAndroid|.

Done.

https://codereview.chromium.org/1220683008/diff/20001/chrome/browser/android/...
chrome/browser/android/policy/policy_manager.h:36:
DISALLOW_COPY_AND_ASSIGN(PolicyManager);
On 2015/07/03 at 16:55:47, bartfab wrote:
> Nit: #include "base/macros.h"

Done.

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
File components/policy/core/browser/android/app_restrictions_importer.cc
(right):

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.cc:11: #include
"base/prefs/pref_service.h"
On 2015/07/03 at 16:55:48, bartfab wrote:
> Nit: Not used.

Done.

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.cc:24:
AppRestrictionsImporter::~AppRestrictionsImporter() {}
On 2015/07/03 at 16:55:47, bartfab wrote:
> Please move this and the following method so that declaration orde rmatches
definition order.

Done.

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.cc:27: return
PolicyNamespace(POLICY_DOMAIN_CHROME, std::string());
On 2015/07/03 at 16:55:48, bartfab wrote:
> Nit: #include "components/policy/core/common/policy_namespace.h"

Done.

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.cc:35: new
base::FundamentalValue(static_cast<bool>(value)));
On 2015/07/03 at 16:55:47, bartfab wrote:
> Nit: #include "base/values.h"

Done.

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.cc:58:
std::vector<base::string16> string_vector;
On 2015/07/03 at 16:55:47, bartfab wrote:
> Nit 1: #include <vector>
> Nit 2: #include "base/strings/string16.h"

Done.

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.cc:77:
base::Value* raw_value,
On 2015/07/03 at 16:55:47, bartfab wrote:
> Nit: Indentation.

Done.

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.cc:78: const
Schema& schema) {
On 2015/07/03 at 16:55:48, bartfab wrote:
> Nit: #include "components/policy/core/common/schema.h"

Done.

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.cc:133:
NOTREACHED();
On 2015/07/03 at 16:55:47, bartfab wrote:
> Nit: #include "base/logging.h"

Done.

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.cc:157: if
(!GetPolicyProvider())
On 2015/07/03 at 16:55:48, bartfab wrote:
> Why does it matter here whether we have a policy provider?

We want to avoid crashing on a null pointer. Well, that's how it was before, and
how it is now, since GetPolicySchema is now replaced by the previous code.

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.cc:160: Schema
schema = GetPolicySchema(key);
On 2015/07/03 at 16:55:48, bartfab wrote:
> Nit: const.

Done.

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.cc:162:
PolicyNamespace ns = GetPolicyNamespace();
On 2015/07/03 at 16:55:48, bartfab wrote:
> Nit: const.

Done.

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.cc:163:
policy_bundle_->Get(ns).Set(key,
On 2015/07/03 at 16:55:47, bartfab wrote:
> Nit: #include "components/policy/core/common/policy_map.h"

Done.

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.cc:164:
POLICY_LEVEL_MANDATORY,
On 2015/07/03 at 16:55:48, bartfab wrote:
> Nit: #include "components/policy/core/common/policy_types.h"

Done.

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
File components/policy/core/browser/android/app_restrictions_importer.h (right):

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.h:9: #include
"base/memory/scoped_ptr.h"
On 2015/07/03 at 16:55:48, bartfab wrote:
> Nit: Add blank line above.

Done.

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.h:23: class
POLICY_EXPORT AppRestrictionsImporter {
On 2015/07/03 at 16:55:48, bartfab wrote:
> Nit: Add a short comment that explains what this class does.

Done.

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.h:57: static
base::Value* ConvertValueToSchema(base::Value* raw_value,
On 2015/07/03 at 16:55:48, bartfab wrote:
> Please change |raw_value| and the return value to scoped_ptrs, so that
ownership is explicit.

Done.

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.h:62: //
Returns the Schema for policy named |key|
On 2015/07/03 at 16:55:48, bartfab wrote:
> Nit 1: s/for/for the/
> Nit 2: Add a full stop at the end.

Removed the method. (see next comment)

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.h:63: virtual
Schema GetPolicySchema(const std::string& key) = 0;
On 2015/07/03 at 16:55:48, bartfab wrote:
> 1: Nit: #include <string>
> 2: Why not use GetPolicyProvider()->GetChromeSchema()->GetKnownProperty(key)?

I wanted to remove explicit chrome references, by abstracting the namespace and
domain so that webview can specify different ones. It looks like it won't be the
case, so I'll remove GetPolicySchema and GetPolicyNamespace.

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.h:69: //
returns an empty namespace in the domain policy::POLICY_DOMAIN_CHROME
On 2015/07/03 at 16:55:48, bartfab wrote:
> Nit 1: Add a full stop at the end.
> Nit 2: That namespace is simply the name Chrome namespace. Other namespaces
are used for special types of policies, e.g. policy for extensions (which does
not exist on Android).

Done.

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.h:70: virtual
PolicyNamespace GetPolicyNamespace();
On 2015/07/03 at 16:55:48, bartfab wrote:
> Why does this method exist, if nobody ever overrides it? Why can you not
hard-code the policy namespace?

Done.

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.h:73:
scoped_ptr<policy::PolicyBundle> policy_bundle_;
On 2015/07/03 at 16:55:48, bartfab wrote:
> Nit: s/policy:://

Done.

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.h:77:
DISALLOW_COPY_AND_ASSIGN(AppRestrictionsImporter);
On 2015/07/03 at 16:55:48, bartfab wrote:
> Nit: #include "base/macros.h"

Done.

[dgn, 2015-07-07 11:31:17]

PTAL

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
File components/policy/core/browser/android/app_restrictions_importer.cc
(right):

https://codereview.chromium.org/1220683008/diff/20001/components/policy/core/...
components/policy/core/browser/android/app_restrictions_importer.cc:67:
PolicyProviderAndroid* policy_provider = GetPolicyProvider();
On 2015/07/03 at 16:55:47, bartfab wrote:
> Nit: const pointer.

Do you mean PolicyProviderAndroid* const policy_provider? I can't const the
PolicyProviderAndroid because of SetPolicies.

[Bernhard Bauer, 2015-07-08 09:54:34]

https://codereview.chromium.org/1220683008/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/policy/PolicyManager.java
(right):

https://codereview.chromium.org/1220683008/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/policy/PolicyManager.java:32:
mAppRestrictionsImporter.setNativeDelegate(mNativePolicyManager);
Can't you just pass the native pointer into the constructor?

https://codereview.chromium.org/1220683008/diff/100001/components/policy/andr...
File
components/policy/android/java/src/org/chromium/policy/AppRestrictionsImporter.java
(right):

https://codereview.chromium.org/1220683008/diff/100001/components/policy/andr...
components/policy/android/java/src/org/chromium/policy/AppRestrictionsImporter.java:10:
* TODO: Insert description here. (generated by dgn)
:)

https://codereview.chromium.org/1220683008/diff/100001/components/policy/andr...
components/policy/android/java/src/org/chromium/policy/AppRestrictionsImporter.java:13:
public class AppRestrictionsImporter {
Nit: Name this ...Converter?

https://codereview.chromium.org/1220683008/diff/100001/components/policy/andr...
components/policy/android/java/src/org/chromium/policy/AppRestrictionsImporter.java:23:
if (mNativeAppRestrictionsImporter == 0) return;
Can this actually happen?

https://codereview.chromium.org/1220683008/diff/100001/components/policy/core...
File components/policy/core/browser/BUILD.gn (right):

https://codereview.chromium.org/1220683008/diff/100001/components/policy/core...
components/policy/core/browser/BUILD.gn:64:
"android/app_restrictions_importer.cc",
I think you can just add these source in the regular list, and they should be
filtered out on non-Android platforms based on their name? Same in the GYP file.

https://codereview.chromium.org/1220683008/diff/100001/components/policy/core...
File components/policy/core/browser/android/app_restrictions_importer.cc
(right):

https://codereview.chromium.org/1220683008/diff/100001/components/policy/core...
components/policy/core/browser/android/app_restrictions_importer.cc:181: return
reinterpret_cast<intptr_t>(new AppRestrictionsImporter(delegate));
When is this object destroyed?

The whole object lifetime situation in this CL seems a bit convoluted, with
passing native pointers through Java code back to native code (see also my other
comment about accessing the delegate). Could the PolicyManager just own this
class directly?

https://codereview.chromium.org/1220683008/diff/100001/components/policy/core...
File components/policy/core/browser/android/app_restrictions_importer.h (right):

https://codereview.chromium.org/1220683008/diff/100001/components/policy/core...
components/policy/core/browser/android/app_restrictions_importer.h:28: class
POLICY_EXPORT AppRestrictionsImporterDelegate {
Move this into an android namespace?

https://codereview.chromium.org/1220683008/diff/100001/components/policy/core...
components/policy/core/browser/android/app_restrictions_importer.h:30:
AppRestrictionsImporterDelegate(){};
Space before braces, and no semicolon afterwards.

https://codereview.chromium.org/1220683008/diff/100001/components/policy/core...
components/policy/core/browser/android/app_restrictions_importer.h:33: virtual
PolicyProviderAndroid* GetPolicyProvider() = 0;
In general, instead of interfaces with only one method you can often use
callbacks. In this particular case, why do you pass in a delegate with a single
method that returns a PolicyProviderAndroid instead of just passing in the
PolicyProviderAndroid?

https://codereview.chromium.org/1220683008/diff/100001/components/policy/core...
components/policy/core/browser/android/app_restrictions_importer.h:36: virtual
~AppRestrictionsImporterDelegate(){};
Space before braces, and no semicolon afterwards.

https://codereview.chromium.org/1220683008/diff/100001/components/policy/core...
components/policy/core/browser/android/app_restrictions_importer.h:82:
~AppRestrictionsImporter();
Is this class going to be subclassed? If not, you can make the constructor
private.

https://codereview.chromium.org/1220683008/diff/100001/components/policy/core...
components/policy/core/browser/android/app_restrictions_importer.h:88:
AppRestrictionsImporterDelegate* delegate_;
How do you guarantee that this object will be alive when you access it?

https://codereview.chromium.org/1220683008/diff/100001/components/policy/core...
File components/policy/core/browser/android/component_jni_registrar.h (right):

https://codereview.chromium.org/1220683008/diff/100001/components/policy/core...
components/policy/core/browser/android/component_jni_registrar.h:13: 
Nit: no empty line between named namespaces.

[dgn, 2015-07-08 11:25:45]

https://codereview.chromium.org/1220683008/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/policy/PolicyManager.java
(right):

https://codereview.chromium.org/1220683008/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/policy/PolicyManager.java:32:
mAppRestrictionsImporter.setNativeDelegate(mNativePolicyManager);
On 2015/07/08 at 09:54:33, Bernhard Bauer wrote:
> Can't you just pass the native pointer into the constructor?

It should not cause any issue, since providers are registered after
initializeNative. Will do.

https://codereview.chromium.org/1220683008/diff/100001/components/policy/andr...
File
components/policy/android/java/src/org/chromium/policy/AppRestrictionsImporter.java
(right):

https://codereview.chromium.org/1220683008/diff/100001/components/policy/andr...
components/policy/android/java/src/org/chromium/policy/AppRestrictionsImporter.java:13:
public class AppRestrictionsImporter {
On 2015/07/08 at 09:54:33, Bernhard Bauer wrote:
> Nit: Name this ...Converter?

PolicyConverter even. This class will not be used only with app restrictions

https://codereview.chromium.org/1220683008/diff/100001/components/policy/andr...
components/policy/android/java/src/org/chromium/policy/AppRestrictionsImporter.java:23:
if (mNativeAppRestrictionsImporter == 0) return;
On 2015/07/08 at 09:54:33, Bernhard Bauer wrote:
> Can this actually happen?

Not normally. If I set the delegate in the constructor it won't be possible.

https://codereview.chromium.org/1220683008/diff/100001/components/policy/core...
File components/policy/core/browser/android/app_restrictions_importer.cc
(right):

https://codereview.chromium.org/1220683008/diff/100001/components/policy/core...
components/policy/core/browser/android/app_restrictions_importer.cc:181: return
reinterpret_cast<intptr_t>(new AppRestrictionsImporter(delegate));
On 2015/07/08 at 09:54:33, Bernhard Bauer wrote:
> When is this object destroyed?
> 
> The whole object lifetime situation in this CL seems a bit convoluted, with
passing native pointers through Java code back to native code (see also my other
comment about accessing the delegate). Could the PolicyManager just own this
class directly?

PolicyManager (java) owns AppRestrictionsImporter (java) which owns
AppRestrictionsImporter (cc). Or do you mean PolicyManager (cc) should own
AppRestrictionsImporter (cc) ? Interactions need to go through Java anyway to
get the bundles etc. I feel like it would make initialization more awkward by
having chrome initialize native and java side of the class separately and put
them together... 

But yeah, Destroy is missing and should be triggered by PolicyManager#destroy()

https://codereview.chromium.org/1220683008/diff/100001/components/policy/core...
File components/policy/core/browser/android/app_restrictions_importer.h (right):

https://codereview.chromium.org/1220683008/diff/100001/components/policy/core...
components/policy/core/browser/android/app_restrictions_importer.h:33: virtual
PolicyProviderAndroid* GetPolicyProvider() = 0;
On 2015/07/08 at 09:54:33, Bernhard Bauer wrote:
> In general, instead of interfaces with only one method you can often use
callbacks. In this particular case, why do you pass in a delegate with a single
method that returns a PolicyProviderAndroid instead of just passing in the
PolicyProviderAndroid?

A method allows to know when the pointer has been cleared by
PolicyManager::PolicyProviderShutdown(). Otherwise I would have to add another
mechanism to cancel PolicyProviderAndroid::SetPolicies is supposed to be dead.
It looks like it happens only while destroying stuff, so a WeakPtr on
PolicyProviderAndroid instead of delegate and callback might be enough. I'll
test that.

https://codereview.chromium.org/1220683008/diff/100001/components/policy/core...
components/policy/core/browser/android/app_restrictions_importer.h:88:
AppRestrictionsImporterDelegate* delegate_;
On 2015/07/08 at 09:54:33, Bernhard Bauer wrote:
> How do you guarantee that this object will be alive when you access it?

Calls to this object's methods are initiated from the PolicyManager, so it
should be alive. Also, that's the object that owns it (indirectly, from Java. I
need to handle PolicyManager#destroy() better.)

[dgn, 2015-07-08 18:16:01]

PTAL

https://codereview.chromium.org/1220683008/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/policy/PolicyManager.java
(right):

https://codereview.chromium.org/1220683008/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/policy/PolicyManager.java:32:
mAppRestrictionsImporter.setNativeDelegate(mNativePolicyManager);
On 2015/07/08 at 11:25:44, dgn wrote:
> On 2015/07/08 at 09:54:33, Bernhard Bauer wrote:
> > Can't you just pass the native pointer into the constructor?
> 
> It should not cause any issue, since providers are registered after
initializeNative. Will do.

The reverse is now done. The PolicyConverter is created via the native code and
is sent back to java.

https://codereview.chromium.org/1220683008/diff/100001/components/policy/andr...
File
components/policy/android/java/src/org/chromium/policy/AppRestrictionsImporter.java
(right):

https://codereview.chromium.org/1220683008/diff/100001/components/policy/andr...
components/policy/android/java/src/org/chromium/policy/AppRestrictionsImporter.java:10:
* TODO: Insert description here. (generated by dgn)
On 2015/07/08 at 09:54:33, Bernhard Bauer wrote:
> :)

Done.

https://codereview.chromium.org/1220683008/diff/100001/components/policy/core...
File components/policy/core/browser/android/app_restrictions_importer.cc
(right):

https://codereview.chromium.org/1220683008/diff/100001/components/policy/core...
components/policy/core/browser/android/app_restrictions_importer.cc:181: return
reinterpret_cast<intptr_t>(new AppRestrictionsImporter(delegate));

Now it goes like:

PolicyManager(java) creates PolicyManager(cc) which creates PolicyConverter (cc)
which creates PolicyConverter (java), the reference to that last object is then
tranferred all the way back to PolicyManager(java)

https://codereview.chromium.org/1220683008/diff/100001/components/policy/core...
File components/policy/core/browser/android/app_restrictions_importer.h (right):

https://codereview.chromium.org/1220683008/diff/100001/components/policy/core...
components/policy/core/browser/android/app_restrictions_importer.h:28: class
POLICY_EXPORT AppRestrictionsImporterDelegate {
On 2015/07/08 at 09:54:33, Bernhard Bauer wrote:
> Move this into an android namespace?

Removed.

https://codereview.chromium.org/1220683008/diff/100001/components/policy/core...
components/policy/core/browser/android/app_restrictions_importer.h:30:
AppRestrictionsImporterDelegate(){};
On 2015/07/08 at 09:54:33, Bernhard Bauer wrote:
> Space before braces, and no semicolon afterwards.

Removed.

https://codereview.chromium.org/1220683008/diff/100001/components/policy/core...
components/policy/core/browser/android/app_restrictions_importer.h:33: virtual
PolicyProviderAndroid* GetPolicyProvider() = 0;
Removed.

https://codereview.chromium.org/1220683008/diff/100001/components/policy/core...
components/policy/core/browser/android/app_restrictions_importer.h:36: virtual
~AppRestrictionsImporterDelegate(){};
On 2015/07/08 at 09:54:33, Bernhard Bauer wrote:
> Space before braces, and no semicolon afterwards.

Removed.

https://codereview.chromium.org/1220683008/diff/100001/components/policy/core...
components/policy/core/browser/android/app_restrictions_importer.h:82:
~AppRestrictionsImporter();
On 2015/07/08 at 09:54:33, Bernhard Bauer wrote:
> Is this class going to be subclassed? If not, you can make the constructor
private.

The constructor is called from PolicyManager now. And destructor needs to be
public for scoped_ptr to call it.

https://codereview.chromium.org/1220683008/diff/100001/components/policy/core...
File components/policy/core/browser/android/component_jni_registrar.h (right):

https://codereview.chromium.org/1220683008/diff/100001/components/policy/core...
components/policy/core/browser/android/component_jni_registrar.h:13: 
On 2015/07/08 at 09:54:33, Bernhard Bauer wrote:
> Nit: no empty line between named namespaces.

Done.

[Bernhard Bauer, 2015-07-09 08:53:58]

Nice!

https://codereview.chromium.org/1220683008/diff/120001/chrome/browser/android...
File chrome/browser/android/policy/policy_manager.h (right):

https://codereview.chromium.org/1220683008/diff/120001/chrome/browser/android...
chrome/browser/android/policy/policy_manager.h:18: class PolicyConverter;
Add an empty line after this one, and add the namespace comment back.

https://codereview.chromium.org/1220683008/diff/120001/components/policy/andr...
File components/policy/android/java/src/org/chromium/policy/PolicyConverter.java
(right):

https://codereview.chromium.org/1220683008/diff/120001/components/policy/andr...
components/policy/android/java/src/org/chromium/policy/PolicyConverter.java:22:
private long mNativePolicyConverter = 0;
You can remove the default value.

https://codereview.chromium.org/1220683008/diff/120001/components/policy/core...
File components/policy/core/browser/android/policy_converter.cc (right):

https://codereview.chromium.org/1220683008/diff/120001/components/policy/core...
components/policy/core/browser/android/policy_converter.cc:176:
ConvertValueToSchema(scoped_ptr<base::Value>(value), schema).release(),
You can use make_scoped_ptr() here.

https://codereview.chromium.org/1220683008/diff/120001/components/policy/core...
File components/policy/core/browser/android/policy_converter.h (right):

https://codereview.chromium.org/1220683008/diff/120001/components/policy/core...
components/policy/core/browser/android/policy_converter.h:28: // Populates
natives policies from Java values key/value pairs. With its
I would move this into the android namespace, and make the JNI registration
method a static method.

https://codereview.chromium.org/1220683008/diff/120001/components/policy/core...
components/policy/core/browser/android/policy_converter.h:33:
PolicyConverter(const Schema* policy_schema);
Single-argument constructors should be marked explicit.

https://codereview.chromium.org/1220683008/diff/120001/components/policy/core...
components/policy/core/browser/android/policy_converter.h:78:
JavaObjectWeakGlobalRef weak_java_obj_;
Make this a strong reference? I'm a bit worried about a garbage collection being
triggered between the time the Java object is created and the time it's stored
in the PolicyManager.

[dgn, 2015-07-09 10:53:41]

PTAL

https://codereview.chromium.org/1220683008/diff/120001/chrome/browser/android...
File chrome/browser/android/policy/policy_manager.h (right):

https://codereview.chromium.org/1220683008/diff/120001/chrome/browser/android...
chrome/browser/android/policy/policy_manager.h:18: class PolicyConverter;
On 2015/07/09 at 08:53:58, Bernhard Bauer wrote:
> Add an empty line after this one, and add the namespace comment back.

Done.

https://codereview.chromium.org/1220683008/diff/120001/components/policy/andr...
File components/policy/android/java/src/org/chromium/policy/PolicyConverter.java
(right):

https://codereview.chromium.org/1220683008/diff/120001/components/policy/andr...
components/policy/android/java/src/org/chromium/policy/PolicyConverter.java:22:
private long mNativePolicyConverter = 0;
On 2015/07/09 at 08:53:58, Bernhard Bauer wrote:
> You can remove the default value.

Done.

https://codereview.chromium.org/1220683008/diff/120001/components/policy/core...
File components/policy/core/browser/android/policy_converter.cc (right):

https://codereview.chromium.org/1220683008/diff/120001/components/policy/core...
components/policy/core/browser/android/policy_converter.cc:176:
ConvertValueToSchema(scoped_ptr<base::Value>(value), schema).release(),
On 2015/07/09 at 08:53:58, Bernhard Bauer wrote:
> You can use make_scoped_ptr() here.

Done.

https://codereview.chromium.org/1220683008/diff/120001/components/policy/core...
File components/policy/core/browser/android/policy_converter.h (right):

https://codereview.chromium.org/1220683008/diff/120001/components/policy/core...
components/policy/core/browser/android/policy_converter.h:28: // Populates
natives policies from Java values key/value pairs. With its
On 2015/07/09 at 08:53:58, Bernhard Bauer wrote:
> I would move this into the android namespace, and make the JNI registration
method a static method.

Done. Also moved the methods around to have a consistent def/decl order with the
cc file.

https://codereview.chromium.org/1220683008/diff/120001/components/policy/core...
components/policy/core/browser/android/policy_converter.h:33:
PolicyConverter(const Schema* policy_schema);
On 2015/07/09 at 08:53:58, Bernhard Bauer wrote:
> Single-argument constructors should be marked explicit.

Done.

https://codereview.chromium.org/1220683008/diff/120001/components/policy/core...
components/policy/core/browser/android/policy_converter.h:78:
JavaObjectWeakGlobalRef weak_java_obj_;
On 2015/07/09 at 08:53:58, Bernhard Bauer wrote:
> Make this a strong reference? I'm a bit worried about a garbage collection
being triggered between the time the Java object is created and the time it's
stored in the PolicyManager.

Done.

[Bernhard Bauer, 2015-07-09 10:58:15]

lgtm

[bartfab (slow), 2015-07-10 12:52:50]

bartfab@chromium.org changed reviewers:
+ mnissler@chromium.org

[bartfab (slow), 2015-07-10 12:52:51]

I am sorry, I did not manage to review this before my vacation. Mattias, could
you take it over?

[Mattias Nissler (ping if slow), 2015-07-13 09:46:05]

https://codereview.chromium.org/1220683008/diff/140001/components/policy/core...
File components/policy/core/browser/DEPS (right):

https://codereview.chromium.org/1220683008/diff/140001/components/policy/core...
components/policy/core/browser/DEPS:7: "+jni",
Shouldn't this rather go into a DEPS file in the android subdirectory? We don't
want policy/core/browser to depend on android stuff in general.

https://codereview.chromium.org/1220683008/diff/140001/components/policy/core...
File components/policy/core/browser/android/policy_converter.cc (right):

https://codereview.chromium.org/1220683008/diff/140001/components/policy/core...
components/policy/core/browser/android/policy_converter.cc:20: #include
"components/policy/core/common/policy_provider_android.h"
needed?

https://codereview.chromium.org/1220683008/diff/140001/components/policy/core...
components/policy/core/browser/android/policy_converter.cc:85:
list_values->AppendStrings(string_vector);
Double conversion via std::vector seems convoluted, can't we go from
jobjectArray to ListValue directly?

https://codereview.chromium.org/1220683008/diff/140001/components/policy/core...
File components/policy/core/browser/android/policy_converter.h (right):

https://codereview.chromium.org/1220683008/diff/140001/components/policy/core...
components/policy/core/browser/android/policy_converter.h:31: // Populates
natives policies from Java values key/value pairs. With its
nit: remove "values"

https://codereview.chromium.org/1220683008/diff/140001/components/policy/core...
components/policy/core/browser/android/policy_converter.h:33: // |PolicyBundle|s
and updating the native policies.
nit: I don't see how this class would "update the native policies"?

https://codereview.chromium.org/1220683008/diff/140001/components/policy/core...
components/policy/core/browser/android/policy_converter.h:40: // call to this
method. The caller gains ownership of the bundle.
nit: Ownership comment is redundant with scoped_ptr return type.

https://codereview.chromium.org/1220683008/diff/140001/components/policy/core...
components/policy/core/browser/android/policy_converter.h:86: void
SetPolicyValue(const std::string& key, base::Value* raw_value);
Should have a comment saying that it takes ownership of |raw_value|, or better,
pass it as scoped_ptr.

[Bernhard Bauer, 2015-07-13 10:46:03]

https://codereview.chromium.org/1220683008/diff/140001/components/policy/core...
File components/policy/core/browser/android/policy_converter.cc (right):

https://codereview.chromium.org/1220683008/diff/140001/components/policy/core...
components/policy/core/browser/android/policy_converter.cc:85:
list_values->AppendStrings(string_vector);
On 2015/07/13 09:46:05, Mattias Nissler wrote:
> Double conversion via std::vector seems convoluted, can't we go from
> jobjectArray to ListValue directly?

Short answer: No :)

Longer answer: We need to convert a Java String[] to a native base::ListValue,
which has a list of base::Value internally, so we first need to convert all the
Java strings, then wrap each of them in a base::StringValue and insert them into
the ListValue.

[Mattias Nissler (ping if slow), 2015-07-13 11:27:13]

https://codereview.chromium.org/1220683008/diff/140001/components/policy/core...
File components/policy/core/browser/android/policy_converter.cc (right):

https://codereview.chromium.org/1220683008/diff/140001/components/policy/core...
components/policy/core/browser/android/policy_converter.cc:85:
list_values->AppendStrings(string_vector);
On 2015/07/13 10:46:03, Bernhard Bauer wrote:
> On 2015/07/13 09:46:05, Mattias Nissler wrote:
> > Double conversion via std::vector seems convoluted, can't we go from
> > jobjectArray to ListValue directly?
> 
> Short answer: No :)
> 
> Longer answer: We need to convert a Java String[] to a native base::ListValue,
> which has a list of base::Value internally, so we first need to convert all
the
> Java strings, then wrap each of them in a base::StringValue and insert them
into
> the ListValue.

Note that your longer answer specifically doesn't mention the intermediate
std::vector<string>. Why not just loop over |array| and insert it intoe
|list_values| (which is named misleadingly btw.)?

[dgn, 2015-07-14 12:50:26]

PTAL

https://codereview.chromium.org/1220683008/diff/140001/components/policy/core...
File components/policy/core/browser/DEPS (right):

https://codereview.chromium.org/1220683008/diff/140001/components/policy/core...
components/policy/core/browser/DEPS:7: "+jni",
On 2015/07/13 09:46:05, Mattias Nissler wrote:
> Shouldn't this rather go into a DEPS file in the android subdirectory? We
don't
> want policy/core/browser to depend on android stuff in general.

Done.

https://codereview.chromium.org/1220683008/diff/140001/components/policy/core...
File components/policy/core/browser/android/policy_converter.cc (right):

https://codereview.chromium.org/1220683008/diff/140001/components/policy/core...
components/policy/core/browser/android/policy_converter.cc:20: #include
"components/policy/core/common/policy_provider_android.h"
On 2015/07/13 09:46:05, Mattias Nissler wrote:
> needed?

Nope, removed also some forward declarations in the .h

https://codereview.chromium.org/1220683008/diff/140001/components/policy/core...
components/policy/core/browser/android/policy_converter.cc:85:
list_values->AppendStrings(string_vector);
On 2015/07/13 11:27:13, Mattias Nissler wrote:
> On 2015/07/13 10:46:03, Bernhard Bauer wrote:
> > On 2015/07/13 09:46:05, Mattias Nissler wrote:
> > > Double conversion via std::vector seems convoluted, can't we go from
> > > jobjectArray to ListValue directly?
> > 
> > Short answer: No :)
> > 
> > Longer answer: We need to convert a Java String[] to a native
base::ListValue,
> > which has a list of base::Value internally, so we first need to convert all
> the
> > Java strings, then wrap each of them in a base::StringValue and insert them
> into
> > the ListValue.
> 
> Note that your longer answer specifically doesn't mention the intermediate
> std::vector<string>. Why not just loop over |array| and insert it intoe
> |list_values| (which is named misleadingly btw.)?

Added it as a separate method. I would have been more comfortable with just
using the provided function from //base and I'm not super convinced not creating
the vector makes a big difference, be there it is.

https://codereview.chromium.org/1220683008/diff/140001/components/policy/core...
File components/policy/core/browser/android/policy_converter.h (right):

https://codereview.chromium.org/1220683008/diff/140001/components/policy/core...
components/policy/core/browser/android/policy_converter.h:31: // Populates
natives policies from Java values key/value pairs. With its
On 2015/07/13 09:46:05, Mattias Nissler wrote:
> nit: remove "values"

Done.

https://codereview.chromium.org/1220683008/diff/140001/components/policy/core...
components/policy/core/browser/android/policy_converter.h:33: // |PolicyBundle|s
and updating the native policies.
On 2015/07/13 09:46:05, Mattias Nissler wrote:
> nit: I don't see how this class would "update the native policies"?

I forgot to remove this after moving FlushPolicies out of this class. Done.

https://codereview.chromium.org/1220683008/diff/140001/components/policy/core...
components/policy/core/browser/android/policy_converter.h:40: // call to this
method. The caller gains ownership of the bundle.
On 2015/07/13 09:46:05, Mattias Nissler wrote:
> nit: Ownership comment is redundant with scoped_ptr return type.

Done.

https://codereview.chromium.org/1220683008/diff/140001/components/policy/core...
components/policy/core/browser/android/policy_converter.h:86: void
SetPolicyValue(const std::string& key, base::Value* raw_value);
On 2015/07/13 09:46:05, Mattias Nissler wrote:
> Should have a comment saying that it takes ownership of |raw_value|, or
better,
> pass it as scoped_ptr.

Done.

[Bernhard Bauer, 2015-07-14 13:24:14]

https://codereview.chromium.org/1220683008/diff/180001/components/policy/core...
File components/policy/core/browser/android/policy_converter.h (right):

https://codereview.chromium.org/1220683008/diff/180001/components/policy/core...
components/policy/core/browser/android/policy_converter.h:73: // Returns an
empty ListValue in if there is a problem with the input array.
What problem would that be? In general, it would be better to return e.g. null
if the input can't be converted, so that the caller can decide how to handle
failure.

https://codereview.chromium.org/1220683008/diff/180001/components/policy/core...
File components/policy/core/browser/android/policy_converter_unittest.cc
(right):

https://codereview.chromium.org/1220683008/diff/180001/components/policy/core...
components/policy/core/browser/android/policy_converter_unittest.cc:70:
EXPECT_TRUE(base::JSONWriter::Write(*(list.get()), &json_string));
This can be just *list.

[dgn, 2015-07-14 15:56:53]

https://codereview.chromium.org/1220683008/diff/180001/components/policy/core...
File components/policy/core/browser/android/policy_converter.h (right):

https://codereview.chromium.org/1220683008/diff/180001/components/policy/core...
components/policy/core/browser/android/policy_converter.h:73: // Returns an
empty ListValue in if there is a problem with the input array.
On 2015/07/14 13:24:14, Bernhard Bauer wrote:
> What problem would that be? In general, it would be better to return e.g. null
> if the input can't be converted, so that the caller can decide how to handle
> failure.

I looked at base/jni_string.h's behaviour, but functions populate other
variables rather than allocating memory so I guess that's not where I should
have looked. I only tested null array here. In the current case, this can not
happen.

https://codereview.chromium.org/1220683008/diff/180001/components/policy/core...
File components/policy/core/browser/android/policy_converter_unittest.cc
(right):

https://codereview.chromium.org/1220683008/diff/180001/components/policy/core...
components/policy/core/browser/android/policy_converter_unittest.cc:70:
EXPECT_TRUE(base::JSONWriter::Write(*(list.get()), &json_string));
On 2015/07/14 13:24:14, Bernhard Bauer wrote:
> This can be just *list.

Done.

[Bernhard Bauer, 2015-07-14 16:10:08]

https://codereview.chromium.org/1220683008/diff/200001/components/policy/core...
File components/policy/core/browser/android/policy_converter.cc (right):

https://codereview.chromium.org/1220683008/diff/200001/components/policy/core...
components/policy/core/browser/android/policy_converter.cc:97: size_t len =
static_cast<size_t>(std::max(0, env->GetArrayLength(array)));
I would also use SafeGetArrayLength, because it shouldn't happen that |array|
isn't actually a valid Java array either.

[dgn, 2015-07-14 17:13:32]

https://codereview.chromium.org/1220683008/diff/200001/components/policy/core...
File components/policy/core/browser/android/policy_converter.cc (right):

https://codereview.chromium.org/1220683008/diff/200001/components/policy/core...
components/policy/core/browser/android/policy_converter.cc:97: size_t len =
static_cast<size_t>(std::max(0, env->GetArrayLength(array)));
On 2015/07/14 16:10:08, Bernhard Bauer wrote:
> I would also use SafeGetArrayLength, because it shouldn't happen that |array|
> isn't actually a valid Java array either.

I didn't because it's an internal method. Changed to return a nullptr rather
than an empty list

[dgn, 2015-07-14 17:15:46]

https://codereview.chromium.org/1220683008/diff/220001/components/policy/core...
File components/policy/core/browser/android/policy_converter.cc (right):

https://codereview.chromium.org/1220683008/diff/220001/components/policy/core...
components/policy/core/browser/android/policy_converter.cc:94: // GetArrayLength
Could return -1 is array is not a valid Java array
TODO at next patch: s/Could return -1 is array/could return -1 if array

[Bernhard Bauer, 2015-07-14 17:22:42]

https://codereview.chromium.org/1220683008/diff/200001/components/policy/core...
File components/policy/core/browser/android/policy_converter.cc (right):

https://codereview.chromium.org/1220683008/diff/200001/components/policy/core...
components/policy/core/browser/android/policy_converter.cc:97: size_t len =
static_cast<size_t>(std::max(0, env->GetArrayLength(array)));
On 2015/07/14 17:13:32, dgn wrote:
> On 2015/07/14 16:10:08, Bernhard Bauer wrote:
> > I would also use SafeGetArrayLength, because it shouldn't happen that
|array|
> > isn't actually a valid Java array either.
> 
> I didn't because it's an internal method. 

Oh, darn. Still, we should probably copy that method (or make it visible?).
Specifically, if something is passed in that isn't a Java object array, things
have gone *seriously* wrong, so we should at the very least have the DCHECK that
is in the SafeGetArrayLength implementation.

[dgn, 2015-07-14 17:50:19]

https://codereview.chromium.org/1220683008/diff/200001/components/policy/core...
File components/policy/core/browser/android/policy_converter.cc (right):

https://codereview.chromium.org/1220683008/diff/200001/components/policy/core...
components/policy/core/browser/android/policy_converter.cc:97: size_t len =
static_cast<size_t>(std::max(0, env->GetArrayLength(array)));
On 2015/07/14 17:22:42, Bernhard Bauer wrote:
> On 2015/07/14 17:13:32, dgn wrote:
> > On 2015/07/14 16:10:08, Bernhard Bauer wrote:
> > > I would also use SafeGetArrayLength, because it shouldn't happen that
> |array|
> > > isn't actually a valid Java array either.
> > 
> > I didn't because it's an internal method. 
> 
> Oh, darn. Still, we should probably copy that method (or make it visible?).
> Specifically, if something is passed in that isn't a Java object array, things
> have gone *seriously* wrong, so we should at the very least have the DCHECK
that
> is in the SafeGetArrayLength implementation.

Done.

[Bernhard Bauer, 2015-07-14 17:57:29]

https://codereview.chromium.org/1220683008/diff/240001/components/policy/core...
File components/policy/core/browser/android/policy_converter.cc (right):

https://codereview.chromium.org/1220683008/diff/240001/components/policy/core...
components/policy/core/browser/android/policy_converter.cc:96: NOTREACHED() <<
"Invalid array length: " << length;
Just DCHECK it. If you have a for-loop below, you're going to do zero iterations
of it anyway, right? And a DCHECK failure is not something that needs to be
handled in a production build (see
http://dev.chromium.org/developers/coding-style/#TOC-CHECK-DCHECK-and-NOTREAC...).

[dgn, 2015-07-14 19:15:16]

https://codereview.chromium.org/1220683008/diff/240001/components/policy/core...
File components/policy/core/browser/android/policy_converter.cc (right):

https://codereview.chromium.org/1220683008/diff/240001/components/policy/core...
components/policy/core/browser/android/policy_converter.cc:96: NOTREACHED() <<
"Invalid array length: " << length;
On 2015/07/14 17:57:28, Bernhard Bauer wrote:
> Just DCHECK it. If you have a for-loop below, you're going to do zero
iterations
> of it anyway, right? And a DCHECK failure is not something that needs to be
> handled in a production build (see
>
http://dev.chromium.org/developers/coding-style/#TOC-CHECK-DCHECK-and-NOTREAC...).

thanks, understood.

[Bernhard Bauer, 2015-07-15 08:25:27]

Thanks! LGTM still holds.

[dgn, 2015-07-15 10:23:46]

Thank you!

mnissler@: PTAL

[Mattias Nissler (ping if slow), 2015-07-15 13:22:12]

LGTM

[dgn, 2015-07-15 13:45:09]

The CQ bit was checked by dgn@chromium.org

[dgn, 2015-07-15 13:45:09]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
mnissler@chromium.org
Link to the patchset: https://codereview.chromium.org/1220683008/#ps280001
(title: "Rebase")

[commit-bot: I haz the power, 2015-07-15 13:45:28]

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

[dgn, 2015-07-15 13:45:30]

Thank you very much for the review!

[commit-bot: I haz the power, 2015-07-15 14:56:15]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-07-15 14:58:16]

Patchset 15 (id:??) landed as
https://crrev.com/733516186f56ce8c3c315a1c5480951a21301e29
Cr-Commit-Position: refs/heads/master@{#338859}