Update instrumentation tests to use PreTestHooks for policies

Update webview and chrome instrumentation test to use PreTestHooks for policies

- Add utilities to set policies for tests using anotation and pre
test hooks
- Add AwInstrumentationTestRunner and update
ChromeInstrumentationTestRunner to register said hooks
- Add classes to simplify building policy bundles

BUG=542859
Committed: https://crrev.com/738fa38d347a0f3f71a41c23cb4f03618e071d7c
Cr-Commit-Position: refs/heads/master@{#358829}

[dgn, 2015-10-02 18:25:25]

dgn@chromium.org changed reviewers:
+ aberent@chromium.org

[dgn, 2015-10-02 18:25:25]

PTAL

[aberent, 2015-10-07 11:13:02]

https://codereview.chromium.org/1387633002/diff/1/base/test/android/javatests...
File
base/test/android/javatests/src/org/chromium/base/test/BaseInstrumentationTestRunner.java
(right):

https://codereview.chromium.org/1387633002/diff/1/base/test/android/javatests...
base/test/android/javatests/src/org/chromium/base/test/BaseInstrumentationTestRunner.java:59:
* Adds the desired hooks to result. Subclasses can add additional ones.
Are SkipChecks really hooks? While combining the two is probably sensible from a
practical point of view, maybe the comment could be made more explicit.

https://codereview.chromium.org/1387633002/diff/1/chrome/android/BUILD.gn
File chrome/android/BUILD.gn (right):

https://codereview.chromium.org/1387633002/diff/1/chrome/android/BUILD.gn#new...
chrome/android/BUILD.gn:317:
"//components/policy/android:policy_java_test_support",
Should there be a matching change to chrome_apk.gyp:chrome_shared_test_java?

https://codereview.chromium.org/1387633002/diff/1/components/policy/android/j...
File
components/policy/android/java/src/org/chromium/policy/AbstractAppRestrictionsProvider.java
(right):

https://codereview.chromium.org/1387633002/diff/1/components/policy/android/j...
components/policy/android/java/src/org/chromium/policy/AbstractAppRestrictionsProvider.java:108:
if (cachedResult != null && cachedResult.getBoolean(STICKY_CACHE_KEY)) {
If the cachedResult is null then this will take the result of
getApplicationRestriction even in a test. Is this what you intend?

Do you want STICKY_CACHE_KEY to last between tests? If not, would it be better
to just use a local variable (static if necessary) to change the caching
behavior?

[dgn, 2015-10-07 14:36:57]

https://codereview.chromium.org/1387633002/diff/1/base/test/android/javatests...
File
base/test/android/javatests/src/org/chromium/base/test/BaseInstrumentationTestRunner.java
(right):

https://codereview.chromium.org/1387633002/diff/1/base/test/android/javatests...
base/test/android/javatests/src/org/chromium/base/test/BaseInstrumentationTestRunner.java:59:
* Adds the desired hooks to result. Subclasses can add additional ones.
On 2015/10/07 11:13:02, aberent wrote:
> Are SkipChecks really hooks? While combining the two is probably sensible from
a
> practical point of view, maybe the comment could be made more explicit.

They are hooks in the general sense, just not pretest ones. I added more to the
description.

https://codereview.chromium.org/1387633002/diff/1/chrome/android/BUILD.gn
File chrome/android/BUILD.gn (right):

https://codereview.chromium.org/1387633002/diff/1/chrome/android/BUILD.gn#new...
chrome/android/BUILD.gn:317:
"//components/policy/android:policy_java_test_support",
On 2015/10/07 11:13:02, aberent wrote:
> Should there be a matching change to chrome_apk.gyp:chrome_shared_test_java?

Thanks, it was not needed since the dependency is added via 
//chrome/test/android:chrome_java_test_support

https://codereview.chromium.org/1387633002/diff/1/components/policy/android/j...
File
components/policy/android/java/src/org/chromium/policy/AbstractAppRestrictionsProvider.java
(right):

https://codereview.chromium.org/1387633002/diff/1/components/policy/android/j...
components/policy/android/java/src/org/chromium/policy/AbstractAppRestrictionsProvider.java:108:
if (cachedResult != null && cachedResult.getBoolean(STICKY_CACHE_KEY)) {
As I understand it, the only way to get null would be to set the cache on a
context that's not from chrome. I'm not sure that's possible in the tests, but
it should be noticeable fairly easily in the CQ.

Currently STICKY_CACHE_KEY already doesn't last between tests and I set it only
if there is something to put in the cached policy bundle. Maybe I should enforce
it for all tests?

Instead of putting the policies in the cache I could bypass the cache with a
local variable, yes. Do you think it's better? I also tried to avoid test-only
members and the like but if it makes things less clear...

[dgn, 2015-10-14 17:22:36]

dgn@chromium.org changed reviewers:
+ sgurun@chromium.org

[dgn, 2015-10-14 17:22:36]

PTAL

[aberent, 2015-10-15 14:12:25]

lgtm

https://codereview.chromium.org/1387633002/diff/1/components/policy/android/j...
File
components/policy/android/java/src/org/chromium/policy/AbstractAppRestrictionsProvider.java
(right):

https://codereview.chromium.org/1387633002/diff/1/components/policy/android/j...
components/policy/android/java/src/org/chromium/policy/AbstractAppRestrictionsProvider.java:108:
if (cachedResult != null && cachedResult.getBoolean(STICKY_CACHE_KEY)) {
On 2015/10/07 14:36:57, dgn wrote:
> As I understand it, the only way to get null would be to set the cache on a
> context that's not from chrome. I'm not sure that's possible in the tests, but
> it should be noticeable fairly easily in the CQ.
> 
> Currently STICKY_CACHE_KEY already doesn't last between tests and I set it
only
> if there is something to put in the cached policy bundle. Maybe I should
enforce
> it for all tests?
> 
> Instead of putting the policies in the cache I could bypass the cache with a
> local variable, yes. Do you think it's better? I also tried to avoid test-only
> members and the like but if it makes things less clear...

I don't like test only anything, but if you need something a test only member
seems simpler than a test only cache key.

[sgurun-gerrit only, 2015-10-16 00:35:52]

On 2015/10/15 14:12:25, aberent wrote:
> lgtm
> 
>
https://codereview.chromium.org/1387633002/diff/1/components/policy/android/j...
> File
>
components/policy/android/java/src/org/chromium/policy/AbstractAppRestrictionsProvider.java
> (right):
> 
>
https://codereview.chromium.org/1387633002/diff/1/components/policy/android/j...
>
components/policy/android/java/src/org/chromium/policy/AbstractAppRestrictionsProvider.java:108:
> if (cachedResult != null && cachedResult.getBoolean(STICKY_CACHE_KEY)) {
> On 2015/10/07 14:36:57, dgn wrote:
> > As I understand it, the only way to get null would be to set the cache on a
> > context that's not from chrome. I'm not sure that's possible in the tests,
but
> > it should be noticeable fairly easily in the CQ.
> > 
> > Currently STICKY_CACHE_KEY already doesn't last between tests and I set it
> only
> > if there is something to put in the cached policy bundle. Maybe I should
> enforce
> > it for all tests?
> > 
> > Instead of putting the policies in the cache I could bypass the cache with a
> > local variable, yes. Do you think it's better? I also tried to avoid
test-only
> > members and the like but if it makes things less clear...
> 
> I don't like test only anything, but if you need something a test only member
> seems simpler than a test only cache key.

lgtm, I only looked at android_webview.

[dgn, 2015-10-16 17:17:15]

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

[dgn, 2015-10-16 17:17:15]

yfriedman@chromium.org: Please review changes in //chrome/test It's the follow
up to https://codereview.chromium.org/1386353004/ I sent you earlier this week.

bartfab@chromium.org: Please review changes in //component/policy*

[Yaron, 2015-10-16 17:51:53]

https://codereview.chromium.org/1387633002/diff/100001/chrome/test/android/ja...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java
(right):

https://codereview.chromium.org/1387633002/diff/100001/chrome/test/android/ja...
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:219:
result.addPreTestHook(Policies.getRegistrationHook());
Do all chrome tests need these policies set up? Could you restrict it to a
policy test suite base class for classes which depend on it?

Also, the CL description only talks about webview (hence my confusion :)

[dgn, 2015-10-16 20:12:29]

On 2015/10/16 17:51:53, Yaron wrote:
>
https://codereview.chromium.org/1387633002/diff/100001/chrome/test/android/ja...
> File
>
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java
> (right):
> 
>
https://codereview.chromium.org/1387633002/diff/100001/chrome/test/android/ja...
>
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:219:
> result.addPreTestHook(Policies.getRegistrationHook());
> Do all chrome tests need these policies set up? Could you restrict it to a
> policy test suite base class for classes which depend on it?
> 
> Also, the CL description only talks about webview (hence my confusion :)

It's just like @CommandLineFlags, if there are policies set via annotations for
the test class or method it will use it, otherwise it does nothing. The
registration is just to tell the test runner to read the annotations. So in case
some test somewhere in Chrome needs to set policies/app restrictions, they would
only need to add them rather than doing all the registration stuff.
So in this change, I enabled the @Policies annotations for webview and chrome,
but some values are set only in PolicyUrlFilteringTest

[dgn, 2015-10-16 20:13:54]

On 2015/10/16 20:12:29, dgn wrote:
> On 2015/10/16 17:51:53, Yaron wrote:
> >
>
https://codereview.chromium.org/1387633002/diff/100001/chrome/test/android/ja...
> > File
> >
>
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1387633002/diff/100001/chrome/test/android/ja...
> >
>
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:219:
> > result.addPreTestHook(Policies.getRegistrationHook());
> > Do all chrome tests need these policies set up? Could you restrict it to a
> > policy test suite base class for classes which depend on it?
> > 
> > Also, the CL description only talks about webview (hence my confusion :)
> 
> It's just like @CommandLineFlags, if there are policies set via annotations
for
> the test class or method it will use it, otherwise it does nothing. The
> registration is just to tell the test runner to read the annotations. So in
case
> some test somewhere in Chrome needs to set policies/app restrictions, they
would
> only need to add them rather than doing all the registration stuff.
> So in this change, I enabled the @Policies annotations for webview and chrome,
> but some values are set only in PolicyUrlFilteringTest

Also, I have them in the InstrumentationTest class because they need to run
before the application is created to fill the policy cache. Setting them in the
TestCase would be too late I think

[dgn, 2015-10-16 20:13:56]

On 2015/10/16 20:12:29, dgn wrote:
> On 2015/10/16 17:51:53, Yaron wrote:
> >
>
https://codereview.chromium.org/1387633002/diff/100001/chrome/test/android/ja...
> > File
> >
>
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1387633002/diff/100001/chrome/test/android/ja...
> >
>
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:219:
> > result.addPreTestHook(Policies.getRegistrationHook());
> > Do all chrome tests need these policies set up? Could you restrict it to a
> > policy test suite base class for classes which depend on it?
> > 
> > Also, the CL description only talks about webview (hence my confusion :)
> 
> It's just like @CommandLineFlags, if there are policies set via annotations
for
> the test class or method it will use it, otherwise it does nothing. The
> registration is just to tell the test runner to read the annotations. So in
case
> some test somewhere in Chrome needs to set policies/app restrictions, they
would
> only need to add them rather than doing all the registration stuff.
> So in this change, I enabled the @Policies annotations for webview and chrome,
> but some values are set only in PolicyUrlFilteringTest

Also, I have them in the InstrumentationTest class because they need to run
before the application is created to fill the policy cache. Setting them in the
TestCase would be too late I think

[Yaron, 2015-10-19 15:47:33]

On 2015/10/16 20:12:29, dgn wrote:
> On 2015/10/16 17:51:53, Yaron wrote:
> >
>
https://codereview.chromium.org/1387633002/diff/100001/chrome/test/android/ja...
> > File
> >
>
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1387633002/diff/100001/chrome/test/android/ja...
> >
>
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:219:
> > result.addPreTestHook(Policies.getRegistrationHook());
> > Do all chrome tests need these policies set up? Could you restrict it to a
> > policy test suite base class for classes which depend on it?
> > 
> > Also, the CL description only talks about webview (hence my confusion :)
> 
> It's just like @CommandLineFlags, if there are policies set via annotations
for
> the test class or method it will use it, otherwise it does nothing. The
> registration is just to tell the test runner to read the annotations. So in
case
> some test somewhere in Chrome needs to set policies/app restrictions, they
would
> only need to add them rather than doing all the registration stuff.
> So in this change, I enabled the @Policies annotations for webview and chrome,
> but some values are set only in PolicyUrlFilteringTest

Sure, I understood that it's only set in some places, but I figured you could
limit the infrastructure itself to a test base class as well. Per your other
comment, that may not be possible so it's fine.

lgtm (but please make the fact that this affects chrome more apparent in CL
description)

[dgn, 2015-10-19 15:59:43]

Description was changed from

==========
Update webview instrumentation test to use PreTestHooks for policies
    
- Add utilities to set policies for tests using anotation and pre
test hooks
- Add AwInstrumentationTestRunner to register said hooks
- Add classes to simplify building policy bundles

BUG=542859
==========

to

==========
Update webview and chrome instrumentation test to use PreTestHooks for policies
    
- Add utilities to set policies for tests using anotation and pre
test hooks
- Add AwInstrumentationTestRunner and update
ChromeInstrumentationTestRunner to register said hooks
- Add classes to simplify building policy bundles

BUG=542859
==========

[dgn, 2015-10-19 16:00:35]

On 2015/10/19 15:47:33, Yaron wrote:
> On 2015/10/16 20:12:29, dgn wrote:
> > On 2015/10/16 17:51:53, Yaron wrote:
> > >
> >
>
https://codereview.chromium.org/1387633002/diff/100001/chrome/test/android/ja...
> > > File
> > >
> >
>
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1387633002/diff/100001/chrome/test/android/ja...
> > >
> >
>
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:219:
> > > result.addPreTestHook(Policies.getRegistrationHook());
> > > Do all chrome tests need these policies set up? Could you restrict it to a
> > > policy test suite base class for classes which depend on it?
> > > 
> > > Also, the CL description only talks about webview (hence my confusion :)
> > 
> > It's just like @CommandLineFlags, if there are policies set via annotations
> for
> > the test class or method it will use it, otherwise it does nothing. The
> > registration is just to tell the test runner to read the annotations. So in
> case
> > some test somewhere in Chrome needs to set policies/app restrictions, they
> would
> > only need to add them rather than doing all the registration stuff.
> > So in this change, I enabled the @Policies annotations for webview and
chrome,
> > but some values are set only in PolicyUrlFilteringTest
> 
> Sure, I understood that it's only set in some places, but I figured you could
> limit the infrastructure itself to a test base class as well. Per your other
> comment, that may not be possible so it's fine.
> 
> lgtm (but please make the fact that this affects chrome more apparent in CL
> description)

Thanks, done.

[bartfab (slow), 2015-10-20 00:44:39]

https://codereview.chromium.org/1387633002/diff/100001/components/policy/andr...
File
components/policy/android/javatests/src/org/chromium/policy/test/PolicyData.java
(right):

https://codereview.chromium.org/1387633002/diff/100001/components/policy/andr...
components/policy/android/javatests/src/org/chromium/policy/test/PolicyData.java:39:
int result = 1;
1) Nit: Why do you need to feed this constant into the next line? Why can the
next line not simply read:

int result = prime + ...

2) Why do you need the prime at all? It just offsets mKey.hashCode() by a fixed
value.

https://codereview.chromium.org/1387633002/diff/100001/components/policy/andr...
components/policy/android/javatests/src/org/chromium/policy/test/PolicyData.java:102:
String joinedStrings = TextUtils.join("\",\"", mValue);
Nit: Does this escape commas, square brackets, etc. in the values?

https://codereview.chromium.org/1387633002/diff/100001/components/policy/andr...
File
components/policy/android/javatests/src/org/chromium/policy/test/annotations/Policies.java
(right):

https://codereview.chromium.org/1387633002/diff/100001/components/policy/andr...
components/policy/android/javatests/src/org/chromium/policy/test/annotations/Policies.java:90:
Assert.assertNull("There can be at most one type of value for the policy",
data);
Nit: This check is unreachable.

https://codereview.chromium.org/1387633002/diff/100001/components/policy/andr...
components/policy/android/javatests/src/org/chromium/policy/test/annotations/Policies.java:94:
if (item.stringArray().length != 0) {
Nit: isEmpty() is more efficient if you do not actually care about the number of
items.

[dgn, 2015-10-20 10:09:46]

PTAL

https://codereview.chromium.org/1387633002/diff/100001/chrome/test/android/ja...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java
(right):

https://codereview.chromium.org/1387633002/diff/100001/chrome/test/android/ja...
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:219:
result.addPreTestHook(Policies.getRegistrationHook());
On 2015/10/16 17:51:52, Yaron wrote:
> Do all chrome tests need these policies set up? Could you restrict it to a
> policy test suite base class for classes which depend on it?
> 
> Also, the CL description only talks about webview (hence my confusion :)

We want to set the cached policies before the application set up, so doing it
before the test case is initialized makes sure we do it early enough.
Updated the description

https://codereview.chromium.org/1387633002/diff/100001/components/policy/andr...
File
components/policy/android/javatests/src/org/chromium/policy/test/PolicyData.java
(right):

https://codereview.chromium.org/1387633002/diff/100001/components/policy/andr...
components/policy/android/javatests/src/org/chromium/policy/test/PolicyData.java:39:
int result = 1;
On 2015/10/20 00:44:39, bartfab (slow) wrote:
> 1) Nit: Why do you need to feed this constant into the next line? Why can the
> next line not simply read:
> 
> int result = prime + ...
> 
> 2) Why do you need the prime at all? It just offsets mKey.hashCode() by a
fixed
> value.

I just used the code generated by eclipse. But it's true that since 
I'm going with items being equal when their keys are, I could just
return the key's hash code

Done.

https://codereview.chromium.org/1387633002/diff/100001/components/policy/andr...
components/policy/android/javatests/src/org/chromium/policy/test/PolicyData.java:102:
String joinedStrings = TextUtils.join("\",\"", mValue);
On 2015/10/20 00:44:39, bartfab (slow) wrote:
> Nit: Does this escape commas, square brackets, etc. in the values?

No. But that's to be used for tests, I think it's ok.

https://codereview.chromium.org/1387633002/diff/100001/components/policy/andr...
File
components/policy/android/javatests/src/org/chromium/policy/test/annotations/Policies.java
(right):

https://codereview.chromium.org/1387633002/diff/100001/components/policy/andr...
components/policy/android/javatests/src/org/chromium/policy/test/annotations/Policies.java:90:
Assert.assertNull("There can be at most one type of value for the policy",
data);
On 2015/10/20 00:44:39, bartfab (slow) wrote:
> Nit: This check is unreachable.

True, I put it there for consistency with the other check. In case more types
are added, potentially before, the validity check is already done and does not
risk to be forgotten. But maybe I should just trust the editor (future me) to do
the right thing?

https://codereview.chromium.org/1387633002/diff/100001/components/policy/andr...
components/policy/android/javatests/src/org/chromium/policy/test/annotations/Policies.java:94:
if (item.stringArray().length != 0) {
On 2015/10/20 00:44:39, bartfab (slow) wrote:
> Nit: isEmpty() is more efficient if you do not actually care about the number
of
> items.

It's a String[], not a List, so there is no isEmpty()

[aberent, 2015-10-20 10:41:22]

Still lgtm

[dgn, 2015-10-26 13:52:11]

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

[dgn, 2015-10-26 13:52:11]

bauerb@: PTAL

[dgn, 2015-10-26 13:52:12]

bauerb@: PTAL

[Bernhard Bauer, 2015-11-06 10:25:35]

https://codereview.chromium.org/1387633002/diff/120001/chrome/test/android/ja...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/MultiActivityTestBase.java
(right):

https://codereview.chromium.org/1387633002/diff/120001/chrome/test/android/ja...
chrome/test/android/javatests/src/org/chromium/chrome/test/MultiActivityTestBase.java:141:
Policies.getRegistrationHook().run(mContext, getClass().getMethod(getName()));
Why do we need to do this if we already add it to the test runners?

https://codereview.chromium.org/1387633002/diff/120001/components/policy/andr...
File
components/policy/android/javatests/src/org/chromium/policy/test/PolicyData.java
(right):

https://codereview.chromium.org/1387633002/diff/120001/components/policy/andr...
components/policy/android/javatests/src/org/chromium/policy/test/PolicyData.java:19:
public final String mKey;
Add an accessor for this?

https://codereview.chromium.org/1387633002/diff/120001/components/policy/andr...
components/policy/android/javatests/src/org/chromium/policy/test/PolicyData.java:87:
* {@link PolicyData} for the {@link String } array type.
Nit: no space before the closing brace.

https://codereview.chromium.org/1387633002/diff/120001/components/policy/andr...
components/policy/android/javatests/src/org/chromium/policy/test/PolicyData.java:99:
String joinedStrings = TextUtils.join("\",\"", mValue);
This isn't really correct... the string value is supposed to be the JSON
serialization. If you keep this, you should at least update the documentation to
state that things like quotes are not allowed inside the strings, but really I'd
prefer to just use JSON. Android actually has two different JSON APIs you could
use here (http://developer.android.com/reference/android/util/JsonWriter.html
and http://developer.android.com/reference/org/json/JSONArray.html).

https://codereview.chromium.org/1387633002/diff/120001/components/policy/andr...
File
components/policy/android/javatests/src/org/chromium/policy/test/annotations/Policies.java
(right):

https://codereview.chromium.org/1387633002/diff/120001/components/policy/andr...
components/policy/android/javatests/src/org/chromium/policy/test/annotations/Policies.java:78:
String string() default "";
Could you use null as the default value?

https://codereview.chromium.org/1387633002/diff/120001/components/policy/andr...
components/policy/android/javatests/src/org/chromium/policy/test/annotations/Policies.java:99:
if (data == null) data = new PolicyData.Undefined(item.key());
What is the Undefined item going to do? Couldn't we just skip it?

[dgn, 2015-11-06 16:07:08]

https://codereview.chromium.org/1387633002/diff/120001/chrome/test/android/ja...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/MultiActivityTestBase.java
(right):

https://codereview.chromium.org/1387633002/diff/120001/chrome/test/android/ja...
chrome/test/android/javatests/src/org/chromium/chrome/test/MultiActivityTestBase.java:141:
Policies.getRegistrationHook().run(mContext, getClass().getMethod(getName()));
On 2015/11/06 10:25:34, Bernhard Bauer wrote:
> Why do we need to do this if we already add it to the test runners?

We don't. I followed what was done with CommandLineFlags, but this is not needed
for Policies here. Removed.

https://codereview.chromium.org/1387633002/diff/120001/components/policy/andr...
File
components/policy/android/javatests/src/org/chromium/policy/test/PolicyData.java
(right):

https://codereview.chromium.org/1387633002/diff/120001/components/policy/andr...
components/policy/android/javatests/src/org/chromium/policy/test/PolicyData.java:19:
public final String mKey;
On 2015/11/06 10:25:34, Bernhard Bauer wrote:
> Add an accessor for this?

Done.

https://codereview.chromium.org/1387633002/diff/120001/components/policy/andr...
components/policy/android/javatests/src/org/chromium/policy/test/PolicyData.java:87:
* {@link PolicyData} for the {@link String } array type.
On 2015/11/06 10:25:34, Bernhard Bauer wrote:
> Nit: no space before the closing brace.

Done.

https://codereview.chromium.org/1387633002/diff/120001/components/policy/andr...
components/policy/android/javatests/src/org/chromium/policy/test/PolicyData.java:99:
String joinedStrings = TextUtils.join("\",\"", mValue);
On 2015/11/06 10:25:34, Bernhard Bauer wrote:
> This isn't really correct... the string value is supposed to be the JSON
> serialization. If you keep this, you should at least update the documentation
to
> state that things like quotes are not allowed inside the strings, but really
I'd
> prefer to just use JSON. Android actually has two different JSON APIs you
could
> use here (http://developer.android.com/reference/android/util/JsonWriter.html
> and http://developer.android.com/reference/org/json/JSONArray.html).

Thanks! I didn't think about checking that but it's much better indeed. Done.

https://codereview.chromium.org/1387633002/diff/120001/components/policy/andr...
File
components/policy/android/javatests/src/org/chromium/policy/test/annotations/Policies.java
(right):

https://codereview.chromium.org/1387633002/diff/120001/components/policy/andr...
components/policy/android/javatests/src/org/chromium/policy/test/annotations/Policies.java:78:
String string() default "";
On 2015/11/06 10:25:34, Bernhard Bauer wrote:
> Could you use null as the default value?

I tried, but that's not allowed: "The value for annotation attribute
Policies.Item.string must be a constant expression"
http://stackoverflow.com/q/1178104/5247260

https://codereview.chromium.org/1387633002/diff/120001/components/policy/andr...
components/policy/android/javatests/src/org/chromium/policy/test/annotations/Policies.java:99:
if (data == null) data = new PolicyData.Undefined(item.key());
On 2015/11/06 10:25:35, Bernhard Bauer wrote:
> What is the Undefined item going to do? Couldn't we just skip it?

It's not going to do anything when transforming it to a bundle, but for
Policies.Remove, we do want to keep the Undefined items to know which keys to
remove.

[Bernhard Bauer, 2015-11-06 16:18:00]

https://codereview.chromium.org/1387633002/diff/120001/components/policy/andr...
File
components/policy/android/javatests/src/org/chromium/policy/test/annotations/Policies.java
(right):

https://codereview.chromium.org/1387633002/diff/120001/components/policy/andr...
components/policy/android/javatests/src/org/chromium/policy/test/annotations/Policies.java:78:
String string() default "";
On 2015/11/06 16:07:08, dgn wrote:
> On 2015/11/06 10:25:34, Bernhard Bauer wrote:
> > Could you use null as the default value?
> 
> I tried, but that's not allowed: "The value for annotation attribute
> Policies.Item.string must be a constant expression"
> http://stackoverflow.com/q/1178104/5247260

Ugh, Java. The language that allows assigning a value (null) which is not
actually a member of a type to that type.

https://codereview.chromium.org/1387633002/diff/120001/components/policy/andr...
components/policy/android/javatests/src/org/chromium/policy/test/annotations/Policies.java:99:
if (data == null) data = new PolicyData.Undefined(item.key());
On 2015/11/06 16:07:08, dgn wrote:
> On 2015/11/06 10:25:35, Bernhard Bauer wrote:
> > What is the Undefined item going to do? Couldn't we just skip it?
> 
> It's not going to do anything when transforming it to a bundle, but for
> Policies.Remove, we do want to keep the Undefined items to know which keys to
> remove.

Oh, I see. Hm, would it then ever actually happen that we _add_ an undefined
item? Or could we throw an assertion in putInBundle()?

https://codereview.chromium.org/1387633002/diff/140001/components/policy/andr...
File
components/policy/android/javatests/src/org/chromium/policy/test/PolicyData.java
(right):

https://codereview.chromium.org/1387633002/diff/140001/components/policy/andr...
components/policy/android/javatests/src/org/chromium/policy/test/PolicyData.java:114:
for (String s: mValue) {
Nit: space before colon.

[dgn, 2015-11-06 17:27:36]

PTAL

https://codereview.chromium.org/1387633002/diff/120001/components/policy/andr...
File
components/policy/android/javatests/src/org/chromium/policy/test/annotations/Policies.java
(right):

https://codereview.chromium.org/1387633002/diff/120001/components/policy/andr...
components/policy/android/javatests/src/org/chromium/policy/test/annotations/Policies.java:99:
if (data == null) data = new PolicyData.Undefined(item.key());
On 2015/11/06 16:17:59, Bernhard Bauer wrote:
> On 2015/11/06 16:07:08, dgn wrote:
> > On 2015/11/06 10:25:35, Bernhard Bauer wrote:
> > > What is the Undefined item going to do? Couldn't we just skip it?
> > 
> > It's not going to do anything when transforming it to a bundle, but for
> > Policies.Remove, we do want to keep the Undefined items to know which keys
to
> > remove.
> 
> Oh, I see. Hm, would it then ever actually happen that we _add_ an undefined
> item? Or could we throw an assertion in putInBundle()?

You're right, it should not. Replaced the noop with an assertion.

[Bernhard Bauer, 2015-11-09 00:02:49]

https://codereview.chromium.org/1387633002/diff/160001/components/policy/andr...
File
components/policy/android/javatests/src/org/chromium/policy/test/PolicyData.java
(right):

https://codereview.chromium.org/1387633002/diff/160001/components/policy/andr...
components/policy/android/javatests/src/org/chromium/policy/test/PolicyData.java:45:
return (mKey == null) ? 0 : mKey.hashCode();
Can we assert that the key is non-null instead?

https://codereview.chromium.org/1387633002/diff/160001/components/policy/andr...
components/policy/android/javatests/src/org/chromium/policy/test/PolicyData.java:90:
Assert.fail("PolicyData.Undefined should not be put in bundles.");
This message could be a bit clearer. If this happens, it's likely that someone
who wrote a test just forgot to set a value for a policy they added, and they
have no idea what PolicyData.Undefined is.

https://codereview.chromium.org/1387633002/diff/160001/components/policy/andr...
File
components/policy/android/junit/src/org/chromium/policy/test/annotations/PoliciesTest.java
(right):

https://codereview.chromium.org/1387633002/diff/160001/components/policy/andr...
components/policy/android/junit/src/org/chromium/policy/test/annotations/PoliciesTest.java:31:
// Simple element, removing an annotation just has no effect
Hm… now that I see this: would it make sense to be a bit stricter here? To be
specific, is there a class of error we could detect by being more strict? Or is
there a case where being lenient is more useful?

https://codereview.chromium.org/1387633002/diff/160001/components/policy/andr...
components/policy/android/junit/src/org/chromium/policy/test/annotations/PoliciesTest.java:49:
// Annotations are properly deduped
You could also test that additions with the same key get the value of the last
one (so it's like a put operation on a map).

https://codereview.chromium.org/1387633002/diff/160001/components/policy/andr...
components/policy/android/junit/src/org/chromium/policy/test/annotations/PoliciesTest.java:58:
//
Remove.

[dgn, 2015-11-09 16:49:05]

https://codereview.chromium.org/1387633002/diff/160001/components/policy/andr...
File
components/policy/android/javatests/src/org/chromium/policy/test/PolicyData.java
(right):

https://codereview.chromium.org/1387633002/diff/160001/components/policy/andr...
components/policy/android/javatests/src/org/chromium/policy/test/PolicyData.java:45:
return (mKey == null) ? 0 : mKey.hashCode();
On 2015/11/09 00:02:49, Bernhard Bauer wrote:
> Can we assert that the key is non-null instead?

Removed.

https://codereview.chromium.org/1387633002/diff/160001/components/policy/andr...
components/policy/android/javatests/src/org/chromium/policy/test/PolicyData.java:90:
Assert.fail("PolicyData.Undefined should not be put in bundles.");
On 2015/11/09 00:02:49, Bernhard Bauer wrote:
> This message could be a bit clearer. If this happens, it's likely that someone
> who wrote a test just forgot to set a value for a policy they added, and they
> have no idea what PolicyData.Undefined is. 

Done.

https://codereview.chromium.org/1387633002/diff/160001/components/policy/andr...
File
components/policy/android/junit/src/org/chromium/policy/test/annotations/PoliciesTest.java
(right):

https://codereview.chromium.org/1387633002/diff/160001/components/policy/andr...
components/policy/android/junit/src/org/chromium/policy/test/annotations/PoliciesTest.java:31:
// Simple element, removing an annotation just has no effect
On 2015/11/09 00:02:49, Bernhard Bauer wrote:
> Hm… now that I see this: would it make sense to be a bit stricter here? To be
> specific, is there a class of error we could detect by being more strict? Or
is
> there a case where being lenient is more useful?

- Pros to being lenient: It could allow interfaces to remove some properties
without caring about whether they we already present. I'm not sure why that
would be something useful. Annotations on methods are not inherited, for classes
we know what the parent type is, so being lenient would not be necessary.

- Errors that could be detected: Assumptions about parent types having certain
annotation? That probably won't change anything though. It's a test only thing,
and tests already would rely on the annotations so they should fail if something
they rely on in the setup changes. If they don't fail, the annotation was
probably unnecessary and should be removed. So that is a nice consequence

I don't think it would change much overall, but I probably missed some cases.
Right now the implementation is a simple removeAll which is neat, checking
unmatched annotations would be less so. I'll change it if you think it's
important.

https://codereview.chromium.org/1387633002/diff/160001/components/policy/andr...
components/policy/android/junit/src/org/chromium/policy/test/annotations/PoliciesTest.java:49:
// Annotations are properly deduped
On 2015/11/09 00:02:49, Bernhard Bauer wrote:
> You could also test that additions with the same key get the value of the last
> one (so it's like a put operation on a map).

Sets assume that objects are identical, and so overriding with a new one doesn't
make sense. Instead of doing it manually, I rebased the implementation on a map,
which is more consistent with the way I use the objects (identify them by key
only)

https://codereview.chromium.org/1387633002/diff/160001/components/policy/andr...
components/policy/android/junit/src/org/chromium/policy/test/annotations/PoliciesTest.java:58:
//
On 2015/11/09 00:02:49, Bernhard Bauer wrote:
> Remove.

Done.

[Bernhard Bauer, 2015-11-09 20:20:28]

LGTM, thanks!

[dgn, 2015-11-10 12:08:28]

dgn@chromium.org changed reviewers:
+ atwilson@chromium.org

[dgn, 2015-11-10 12:08:29]

bartfab@, atwilson@: The CL has been reviewed by other owners, bauerb@ also
reviewed to the //component/policy part. Can you please rubberstamp? thanks.

[Andrew T Wilson (Slow), 2015-11-10 12:57:02]

lgtm

[dgn, 2015-11-10 13:38:53]

The CQ bit was checked by dgn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-10 13:39:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1387633002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1387633002/180001

[commit-bot: I haz the power, 2015-11-10 14:19:12]

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

[commit-bot: I haz the power, 2015-11-10 14:19:13]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dgn, 2015-11-10 15:28:54]

Thanks! Landing it.

[dgn, 2015-11-10 15:29:03]

The CQ bit was checked by dgn@chromium.org

[dgn, 2015-11-10 15:29:03]

The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org,
yfriedman@chromium.org, aberent@chromium.org
Link to the patchset: https://codereview.chromium.org/1387633002/#ps180001
(title: "Address comments, use a map instead of hacking PolicyData's equality")

[commit-bot: I haz the power, 2015-11-10 15:29:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1387633002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1387633002/180001

[commit-bot: I haz the power, 2015-11-10 15:34:02]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-11-10 15:34:56]

Patchset 10 (id:??) landed as
https://crrev.com/738fa38d347a0f3f71a41c23cb4f03618e071d7c
Cr-Commit-Position: refs/heads/master@{#358829}