Enable invalidations for arbitrary objects on Android.

Enable invalidations for arbitrary objects on Android.

Enable the native InvalidationServiceAndroid to call into the Java-side InvalidationController in order to register for invalidations for arbitrary objects. Currently only Sync objects can be registered for. This change is motivated by the need to receive invalidations for enterprise user policy on Android.

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

[nyquist, 2013-09-05 22:50:08]

https://codereview.chromium.org/23643002/diff/10001/chrome/browser/invalidati...
File chrome/browser/invalidation/invalidation_service_android_unittest.cc
(right):

https://codereview.chromium.org/23643002/diff/10001/chrome/browser/invalidati...
chrome/browser/invalidation/invalidation_service_android_unittest.cc:103:
syncer::ObjectIdSet::const_iterator id;
Nit: Do you want to extract a method, and pass in true/false for the nicely
named methods to that, and do something like:
if (IsRegistered(*id) == expected_registration_state) { ...
?

I am fine with keeping as is too though since it's only 5 lines

And regardless, I really like the naming like you have it today for the methods
that are called from the test.

https://codereview.chromium.org/23643002/diff/10001/chrome/browser/invalidati...
chrome/browser/invalidation/invalidation_service_android_unittest.cc:137:
syncer::ObjectIdSet OBJECT_SET_D;
Nit: OBJECT_SET_SYNC?

Fine with keeping as is though.

https://codereview.chromium.org/23643002/diff/10001/chrome/browser/invalidati...
chrome/browser/invalidation/invalidation_service_android_unittest.cc:140:
syncer::ObjectIdSet OBJECT_SET_E;
Nit: OBJECT_SET_A_UNION_D or OBJECT_SET_A_UNION_SYNC?

Fine with keeping as is though.

https://codereview.chromium.org/23643002/diff/10001/sync/android/java/src/org...
File
sync/android/java/src/org/chromium/sync/notifier/InvalidationPreferences.java
(right):

https://codereview.chromium.org/23643002/diff/10001/sync/android/java/src/org...
sync/android/java/src/org/chromium/sync/notifier/InvalidationPreferences.java:118:
@Nullable public Set<ObjectId> getSavedObjectIds() {
Nit: \n before public

https://codereview.chromium.org/23643002/diff/10001/sync/android/java/src/org...
sync/android/java/src/org/chromium/sync/notifier/InvalidationPreferences.java:183:
* Converts the given object id string stored in preferences to a object id.
Nit: an object id

https://codereview.chromium.org/23643002/diff/10001/sync/android/java/src/org...
File sync/android/java/src/org/chromium/sync/notifier/InvalidationService.java
(right):

https://codereview.chromium.org/23643002/diff/10001/sync/android/java/src/org...
sync/android/java/src/org/chromium/sync/notifier/InvalidationService.java:322:
private Set<ObjectId> joinRegistrations(Set<ObjectId> syncRegistrations,
static

https://codereview.chromium.org/23643002/diff/10001/sync/android/javatests/sr...
File
sync/android/javatests/src/org/chromium/sync/notifier/InvalidationControllerTest.java
(right):

https://codereview.chromium.org/23643002/diff/10001/sync/android/javatests/sr...
sync/android/javatests/src/org/chromium/sync/notifier/InvalidationControllerTest.java:287:
InvalidationController controller = new InvalidationController(mContext);
Nit: Indent should be 4.

https://codereview.chromium.org/23643002/diff/10001/sync/android/javatests/sr...
sync/android/javatests/src/org/chromium/sync/notifier/InvalidationControllerTest.java:288:
controller.setRegisteredObjectIds(new int[] { 1, 2 }, new String[] { "a", "b"
});
Nit:
controller.setRegisteredObjectIds(new int[] {1, 2}, new String[] {"a", "b"});

https://codereview.chromium.org/23643002/diff/10001/sync/android/javatests/sr...
File
sync/android/javatests/src/org/chromium/sync/notifier/InvalidationServiceTest.java
(right):

https://codereview.chromium.org/23643002/diff/10001/sync/android/javatests/sr...
sync/android/javatests/src/org/chromium/sync/notifier/InvalidationServiceTest.java:614:
Intent registrationIntent = IntentProtocol.createRegisterIntent(account, new
int[] { 1, 2 },
Nit: No spaces after { or before } in array initialization (throughout this
method).

[Steve Condie, 2013-09-06 20:05:22]

https://codereview.chromium.org/23643002/diff/10001/chrome/browser/invalidati...
File chrome/browser/invalidation/invalidation_service_android_unittest.cc
(right):

https://codereview.chromium.org/23643002/diff/10001/chrome/browser/invalidati...
chrome/browser/invalidation/invalidation_service_android_unittest.cc:103:
syncer::ObjectIdSet::const_iterator id;
On 2013/09/05 22:50:08, nyquist wrote:
> Nit: Do you want to extract a method, and pass in true/false for the nicely
> named methods to that, and do something like:
> if (IsRegistered(*id) == expected_registration_state) { ...
> ?
> 
> I am fine with keeping as is too though since it's only 5 lines
> 
> And regardless, I really like the naming like you have it today for the
methods
> that are called from the test.

Done.

https://codereview.chromium.org/23643002/diff/10001/chrome/browser/invalidati...
chrome/browser/invalidation/invalidation_service_android_unittest.cc:137:
syncer::ObjectIdSet OBJECT_SET_D;
On 2013/09/05 22:50:08, nyquist wrote:
> Nit: OBJECT_SET_SYNC?
> 
> Fine with keeping as is though.

Done.

https://codereview.chromium.org/23643002/diff/10001/chrome/browser/invalidati...
chrome/browser/invalidation/invalidation_service_android_unittest.cc:140:
syncer::ObjectIdSet OBJECT_SET_E;
On 2013/09/05 22:50:08, nyquist wrote:
> Nit: OBJECT_SET_A_UNION_D or OBJECT_SET_A_UNION_SYNC?
> 
> Fine with keeping as is though.

How about OBJECT_SET_MIXED to keep the name a bit shorter?

https://codereview.chromium.org/23643002/diff/10001/sync/android/java/src/org...
File
sync/android/java/src/org/chromium/sync/notifier/InvalidationPreferences.java
(right):

https://codereview.chromium.org/23643002/diff/10001/sync/android/java/src/org...
sync/android/java/src/org/chromium/sync/notifier/InvalidationPreferences.java:118:
@Nullable public Set<ObjectId> getSavedObjectIds() {
On 2013/09/05 22:50:08, nyquist wrote:
> Nit: \n before public

Done.

https://codereview.chromium.org/23643002/diff/10001/sync/android/java/src/org...
sync/android/java/src/org/chromium/sync/notifier/InvalidationPreferences.java:183:
* Converts the given object id string stored in preferences to a object id.
On 2013/09/05 22:50:08, nyquist wrote:
> Nit: an object id

Done.

https://codereview.chromium.org/23643002/diff/10001/sync/android/java/src/org...
File sync/android/java/src/org/chromium/sync/notifier/InvalidationService.java
(right):

https://codereview.chromium.org/23643002/diff/10001/sync/android/java/src/org...
sync/android/java/src/org/chromium/sync/notifier/InvalidationService.java:322:
private Set<ObjectId> joinRegistrations(Set<ObjectId> syncRegistrations,
On 2013/09/05 22:50:08, nyquist wrote:
> static

Done.

https://codereview.chromium.org/23643002/diff/10001/sync/android/javatests/sr...
File
sync/android/javatests/src/org/chromium/sync/notifier/InvalidationControllerTest.java
(right):

https://codereview.chromium.org/23643002/diff/10001/sync/android/javatests/sr...
sync/android/javatests/src/org/chromium/sync/notifier/InvalidationControllerTest.java:287:
InvalidationController controller = new InvalidationController(mContext);
On 2013/09/05 22:50:08, nyquist wrote:
> Nit: Indent should be 4.

Done.

https://codereview.chromium.org/23643002/diff/10001/sync/android/javatests/sr...
sync/android/javatests/src/org/chromium/sync/notifier/InvalidationControllerTest.java:288:
controller.setRegisteredObjectIds(new int[] { 1, 2 }, new String[] { "a", "b"
});
On 2013/09/05 22:50:08, nyquist wrote:
> Nit:
> controller.setRegisteredObjectIds(new int[] {1, 2}, new String[] {"a", "b"});

Done.

https://codereview.chromium.org/23643002/diff/10001/sync/android/javatests/sr...
File
sync/android/javatests/src/org/chromium/sync/notifier/InvalidationServiceTest.java
(right):

https://codereview.chromium.org/23643002/diff/10001/sync/android/javatests/sr...
sync/android/javatests/src/org/chromium/sync/notifier/InvalidationServiceTest.java:614:
Intent registrationIntent = IntentProtocol.createRegisterIntent(account, new
int[] { 1, 2 },
On 2013/09/05 22:50:08, nyquist wrote:
> Nit: No spaces after { or before } in array initialization (throughout this
> method).

Done.

[nyquist, 2013-09-07 05:10:46]

lgtm

https://codereview.chromium.org/23643002/diff/10001/chrome/browser/invalidati...
File chrome/browser/invalidation/invalidation_service_android_unittest.cc
(right):

https://codereview.chromium.org/23643002/diff/10001/chrome/browser/invalidati...
chrome/browser/invalidation/invalidation_service_android_unittest.cc:140:
syncer::ObjectIdSet OBJECT_SET_E;
On 2013/09/06 20:05:22, stepco wrote:
> On 2013/09/05 22:50:08, nyquist wrote:
> > Nit: OBJECT_SET_A_UNION_D or OBJECT_SET_A_UNION_SYNC?
> > 
> > Fine with keeping as is though.
> 
> How about OBJECT_SET_MIXED to keep the name a bit shorter?

Great idea!

[Steve Condie, 2013-09-09 17:40:35]

Added Marcus for jni_array changes (added methods to create java int arrays).
Added Richard for invalidation_service_android changes.

[rlarocque, 2013-09-09 18:32:22]

https://codereview.chromium.org/23643002/diff/24001/chrome/browser/invalidati...
File chrome/browser/invalidation/invalidation_service_android.cc (right):

https://codereview.chromium.org/23643002/diff/24001/chrome/browser/invalidati...
chrome/browser/invalidation/invalidation_service_android.cc:119: if
(!update_registration_callback_.is_null()) {
I'd prefer to keep test code out of this class, if possible.

Come to think of it, it would also be nice to keep the JNI code quarantined
somewhere.

Could this code be refactored so that we have some sort of
"AndroidInvalidationRegistrationTracker" interface that this class communicates
with?  (Feel free to pick a better name for it.)  You could use this both to
hide the JNI bits and to inject a mock instance for testing.

https://codereview.chromium.org/23643002/diff/24001/chrome/browser/invalidati...
chrome/browser/invalidation/invalidation_service_android.cc:150: bool
InvalidationServiceAndroid::IsRegistrationRequired(
Could we put this filtering logic on the Java side?

It's the Java classes that have the sync-specific registration logic.  It seems
logical that they should also manage the filtering that is required by this
sync-specific special case.

https://codereview.chromium.org/23643002/diff/24001/chrome/browser/invalidati...
File chrome/browser/invalidation/invalidation_service_android.h (right):

https://codereview.chromium.org/23643002/diff/24001/chrome/browser/invalidati...
chrome/browser/invalidation/invalidation_service_android.h:95: #if
defined(OS_ANDROID)
This shouldn't be necessary.  I think there's some gyp magic that makes this
class only compile on Android because it has an "android" suffix in its name.

See also the comment about hiding JNI code.  I think these members should be
pulled out into a separate class.

[bulach, 2013-09-09 18:39:23]

base/android lgtm, thanks for adding the tests too! :)

[Steve Condie, 2013-09-09 22:58:25]

https://codereview.chromium.org/23643002/diff/24001/chrome/browser/invalidati...
File chrome/browser/invalidation/invalidation_service_android.cc (right):

https://codereview.chromium.org/23643002/diff/24001/chrome/browser/invalidati...
chrome/browser/invalidation/invalidation_service_android.cc:119: if
(!update_registration_callback_.is_null()) {
On 2013/09/09 18:32:23, rlarocque wrote:
> I'd prefer to keep test code out of this class, if possible.
> 
> Come to think of it, it would also be nice to keep the JNI code quarantined
> somewhere.
> 
> Could this code be refactored so that we have some sort of
> "AndroidInvalidationRegistrationTracker" interface that this class
communicates
> with?  (Feel free to pick a better name for it.)  You could use this both to
> hide the JNI bits and to inject a mock instance for testing.

Done. For lack of a better name, I called it InvalidationControllerAndroid, as
it is essentially just a wrapper for the Java-side InvalidationController class.
Now the unit test can subclass InvalidationControllerAndroid for a clean way to
inject a mock.

https://codereview.chromium.org/23643002/diff/24001/chrome/browser/invalidati...
chrome/browser/invalidation/invalidation_service_android.cc:150: bool
InvalidationServiceAndroid::IsRegistrationRequired(
On 2013/09/09 18:32:23, rlarocque wrote:
> Could we put this filtering logic on the Java side?
> 
> It's the Java classes that have the sync-specific registration logic.  It
seems
> logical that they should also manage the filtering that is required by this
> sync-specific special case.

I think that's a good idea; done. My only concern was the overhead with calling
into Java and writing to preferences (where registrations are persisted) extra
times, but I think the simplicity of the new approach is worth it, and there
should only be a small number of calls to update registrations anyway.

https://codereview.chromium.org/23643002/diff/24001/chrome/browser/invalidati...
File chrome/browser/invalidation/invalidation_service_android.h (right):

https://codereview.chromium.org/23643002/diff/24001/chrome/browser/invalidati...
chrome/browser/invalidation/invalidation_service_android.h:95: #if
defined(OS_ANDROID)
On 2013/09/09 18:32:23, rlarocque wrote:
> This shouldn't be necessary.  I think there's some gyp magic that makes this
> class only compile on Android because it has an "android" suffix in its name.
> 
> See also the comment about hiding JNI code.  I think these members should be
> pulled out into a separate class.

These are removed now, but the issue was that even though the .cc file isn't
compiled for other platforms, the .h file is included unconditionally in .cc
files which are compiled for other platforms.

[rlarocque, 2013-09-09 23:22:22]

Much better.  C++ code LGTM.

[Steve Condie, 2013-09-09 23:32:37]

Thanks Richard. Tommy do the changes since you LGTM'ed look good to you?
Essentially, the C++ side was simplified by moving the code which filters out
the Sync object ids to the Java InvalidationController.

[nyquist, 2013-09-10 01:42:48]

lgtm

https://codereview.chromium.org/23643002/diff/58001/chrome/browser/invalidati...
File chrome/browser/invalidation/invalidation_controller_android.cc (right):

https://codereview.chromium.org/23643002/diff/58001/chrome/browser/invalidati...
chrome/browser/invalidation/invalidation_controller_android.cc:25: if
(invalidation_controller_.is_null()) {
Could this be done in the constructor instead?

https://codereview.chromium.org/23643002/diff/58001/sync/android/java/src/org...
File
sync/android/java/src/org/chromium/sync/notifier/InvalidationController.java
(right):

https://codereview.chromium.org/23643002/diff/58001/sync/android/java/src/org...
sync/android/java/src/org/chromium/sync/notifier/InvalidationController.java:143:
objectNames.get(i).getBytes()));
Nit: Java indenting should be 8 from the start of the previous line. How about
instead doing:

objectIds.add(ObjectId.newInstance(
        objectSources.get(i), objectNames.get(i).getBytes()));

or:

objectIds.add(
        ObjectId.newInstance(objectSources.get(i),
objectNames.get(i).getBytes()));


Ted made me do it.

[nyquist, 2013-09-10 09:48:04]

https://codereview.chromium.org/23643002/diff/58001/chrome/browser/invalidati...
File chrome/browser/invalidation/invalidation_controller_android.cc (right):

https://codereview.chromium.org/23643002/diff/58001/chrome/browser/invalidati...
chrome/browser/invalidation/invalidation_controller_android.cc:25: if
(invalidation_controller_.is_null()) {
On 2013/09/10 01:42:49, nyquist wrote:
> Could this be done in the constructor instead?

Disregard this since this is not always called. Doing lazy init is probably
better.

[Steve Condie, 2013-09-10 17:49:54]

https://codereview.chromium.org/23643002/diff/58001/chrome/browser/invalidati...
File chrome/browser/invalidation/invalidation_controller_android.cc (right):

https://codereview.chromium.org/23643002/diff/58001/chrome/browser/invalidati...
chrome/browser/invalidation/invalidation_controller_android.cc:25: if
(invalidation_controller_.is_null()) {
On 2013/09/10 09:48:05, nyquist wrote:
> On 2013/09/10 01:42:49, nyquist wrote:
> > Could this be done in the constructor instead?
> 
> Disregard this since this is not always called. Doing lazy init is probably
> better.

Ok, I will leave as is.

https://codereview.chromium.org/23643002/diff/58001/sync/android/java/src/org...
File
sync/android/java/src/org/chromium/sync/notifier/InvalidationController.java
(right):

https://codereview.chromium.org/23643002/diff/58001/sync/android/java/src/org...
sync/android/java/src/org/chromium/sync/notifier/InvalidationController.java:143:
objectNames.get(i).getBytes()));
On 2013/09/10 01:42:49, nyquist wrote:
> Nit: Java indenting should be 8 from the start of the previous line. How about
> instead doing:
> 
> objectIds.add(ObjectId.newInstance(
>         objectSources.get(i), objectNames.get(i).getBytes()));
> 
> or:
> 
> objectIds.add(
>         ObjectId.newInstance(objectSources.get(i),
> objectNames.get(i).getBytes()));
> 
> 
> Ted made me do it.

Done.

[commit-bot: I haz the power, 2013-09-10 17:55:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stepco@chromium.org/23643002/73001

[commit-bot: I haz the power, 2013-09-10 18:06:14]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2013-09-10 18:32:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stepco@chromium.org/23643002/89001

[commit-bot: I haz the power, 2013-09-10 21:49:11]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-09-10 23:01:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stepco@chromium.org/23643002/89001

[commit-bot: I haz the power, 2013-09-11 00:20:29]

Change committed as 222417