Enable invalidations for arbitrary objects on Android.

chrome/browser/invalidation/invalidation_service_android.cc

Index: chrome/browser/invalidation/invalidation_service_android.cc
diff --git a/chrome/browser/invalidation/invalidation_service_android.cc b/chrome/browser/invalidation/invalidation_service_android.cc
index b12f1237388bc1cb7879037c64cfd7d49f717e54..8ccac1c8780b4813d0bb54cb9f71515bae32ae12 100644
--- a/chrome/browser/invalidation/invalidation_service_android.cc
+++ b/chrome/browser/invalidation/invalidation_service_android.cc
@@ -4,8 +4,11 @@
 
 #include "chrome/browser/invalidation/invalidation_service_android.h"
 
+#include "base/android/jni_array.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "content/public/browser/notification_service.h"
+#include "google/cacheinvalidation/types.pb.h"
+#include "sync/jni/InvalidationController_jni.h"
 
 namespace invalidation {
 
@@ -28,7 +31,13 @@ void InvalidationServiceAndroid::UpdateRegisteredInvalidationIds(
     syncer::InvalidationHandler* handler,
     const syncer::ObjectIdSet& ids) {
   DCHECK(CalledOnValidThread());
+  // Registration with the Android invalidation controller is needed if any of
+  // the objects in the current or replacement object sets require it.
+  bool isRegistrationRequired = IsRegistrationRequired(ids) ||
+      IsRegistrationRequired(invalidator_registrar_.GetRegisteredIds(handler));
   invalidator_registrar_.UpdateRegisteredIds(handler, ids);
+  if (isRegistrationRequired)
+    UpdateRegistration();
 }
 
 void InvalidationServiceAndroid::UnregisterInvalidationHandler(
@@ -88,4 +97,65 @@ void InvalidationServiceAndroid::TriggerStateChangeForTest(
   invalidator_registrar_.UpdateInvalidatorState(invalidator_state_);
 }
 
+void InvalidationServiceAndroid::SetUpdateRegistrationCallbackForTest(
+    const UpdateRegistrationCallback& callback) {
+  update_registration_callback_ = callback;
+}
+
+void InvalidationServiceAndroid::UpdateRegistration() {
+  // Create a list of object sources and names which require registration.
+  std::vector<int> sources;
+  std::vector<std::string> names;
+  syncer::ObjectIdSet ids = invalidator_registrar_.GetAllRegisteredIds();
+  syncer::ObjectIdSet::const_iterator id;
+  for (id = ids.begin(); id != ids.end(); ++id) {
+    if (IsRegistrationRequired(*id)) {
+      sources.push_back(id->source());
+      names.push_back(id->name());
+    }
+  }
+
+  // Invoke callback for testing if provided.
+  if (!update_registration_callback_.is_null()) {

[rlarocque, 2013/09/09 18:32:23]

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.

[Steve Condie, 2013/09/09 22:58:26]

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.

+    update_registration_callback_.Run(sources, names);
+    return;
+  }
+
+  // Get a reference to the invalidation controller.
+  JNIEnv* env = base::android::AttachCurrentThread();
+  DCHECK(env);
+  if (invalidation_controller_.is_null()) {
+    invalidation_controller_.Reset(Java_InvalidationController_get(
+        env,
+        base::android::GetApplicationContext()));
+  }
+
+  // Update the registration with the invalidation controller.
+  Java_InvalidationController_setRegisteredObjectIds(
+      env,
+      invalidation_controller_.obj(),
+      base::android::ToJavaIntArray(env, sources).obj(),
+      base::android::ToJavaArrayOfStrings(env, names).obj());
+}
+
+bool InvalidationServiceAndroid::IsRegistrationRequired(
+    const syncer::ObjectIdSet& ids) {
+  syncer::ObjectIdSet::const_iterator id;
+  for (id = ids.begin(); id != ids.end(); ++id)
+    if (IsRegistrationRequired(*id))
+      return true;
+  return false;
+}
+
+bool InvalidationServiceAndroid::IsRegistrationRequired(

[rlarocque, 2013/09/09 18:32:23]

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.

[Steve Condie, 2013/09/09 22:58:26]

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.

+    const invalidation::ObjectId& id) {
+  // Sync types are registered on the Java side, so only non-Sync objects must
+  // be registered with the Android invalidation controller by this class.
+  return id.source() != ipc::invalidation::ObjectSource::CHROME_SYNC;
+}
+
+bool RegisterInvalidationController(JNIEnv* env) {
+  return RegisterNativesImpl(env);
+}
+
 } //  namespace invalidation