Convert ListenerRegistration to the new JNI framework

wilhuff · a-maurice · commit 782ec2a800a8 · 2020-09-29T19:11:07.000-07:00
PiperOrigin-RevId: 333539837
diff --git a/firestore/src/android/document_reference_android.cc b/firestore/src/android/document_reference_android.cc
@@ -176,8 +176,7 @@ ListenerRegistration DocumentReferenceInternal::AddSnapshotListener(
 
   if (!env.ok() || !java_registration) return {};
   return ListenerRegistration(new ListenerRegistrationInternal(
-      firestore_, listener, passing_listener_ownership,
-      java_registration.get()));
+      firestore_, listener, passing_listener_ownership, java_registration));
 }
 
 Class DocumentReferenceInternal::GetClass() { return Class(clazz); }
diff --git a/firestore/src/android/firestore_android.cc b/firestore/src/android/firestore_android.cc
@@ -176,7 +176,6 @@ bool FirestoreInternal::Initialize(App* app) {
     if (!(  // Call Initialize on each Firestore internal class.
             FieldValueInternal::Initialize(app) &&
             FirebaseFirestoreExceptionInternal::Initialize(app) &&
-            ListenerRegistrationInternal::Initialize(app) &&
             TransactionInternal::Initialize(app) && Wrapper::Initialize(app) &&
             // Initialize those embedded Firestore internal classes.
             InitializeEmbeddedClasses(app, loader))) {
@@ -204,6 +203,7 @@ bool FirestoreInternal::Initialize(App* app) {
     DocumentSnapshotInternal::Initialize(loader);
     FieldPathConverter::Initialize(loader);
     GeoPointInternal::Initialize(loader);
+    ListenerRegistrationInternal::Initialize(loader);
     MetadataChangesInternal::Initialize(loader);
     QueryInternal::Initialize(loader);
     QuerySnapshotInternal::Initialize(loader);
@@ -245,7 +245,6 @@ void FirestoreInternal::ReleaseClasses(App* app) {
   EventListenerInternal::Terminate(app);
   FieldValueInternal::Terminate(app);
   FirebaseFirestoreExceptionInternal::Terminate(app);
-  ListenerRegistrationInternal::Terminate(app);
   TransactionInternal::Terminate(app);
   Wrapper::Terminate(app);
 }
@@ -402,7 +401,7 @@ ListenerRegistration FirestoreInternal::AddSnapshotsInSyncListener(
 
   if (!env.ok() || !java_registration) return {};
   return ListenerRegistration(new ListenerRegistrationInternal(
-      this, listener, passing_listener_ownership, java_registration.get()));
+      this, listener, passing_listener_ownership, java_registration));
 }
 
 #if defined(FIREBASE_USE_STD_FUNCTION)
diff --git a/firestore/src/android/listener_registration_android.cc b/firestore/src/android/listener_registration_android.cc
@@ -1,61 +1,68 @@
 #include "firestore/src/android/listener_registration_android.h"
 
 #include "app/src/assert.h"
-#include "app/src/util_android.h"
+#include "firestore/src/android/firestore_android.h"
 #include "firestore/src/include/firebase/firestore/listener_registration.h"
+#include "firestore/src/jni/env.h"
+#include "firestore/src/jni/loader.h"
 
 namespace firebase {
 namespace firestore {
+namespace {
 
-#define LISTENER_REGISTRATION_METHODS(X) X(Remove, "remove", "()V")
-METHOD_LOOKUP_DECLARATION(listener_registration, LISTENER_REGISTRATION_METHODS)
-METHOD_LOOKUP_DEFINITION(listener_registration,
-                         PROGUARD_KEEP_CLASS
-                         "com/google/firebase/firestore/ListenerRegistration",
-                         LISTENER_REGISTRATION_METHODS)
+using jni::Env;
+using jni::Object;
+
+constexpr char kClassName[] =
+    PROGUARD_KEEP_CLASS "com/google/firebase/firestore/ListenerRegistration";
+
+jni::Method<void> kRemove("remove", "()V");
+
+}  // namespace
+
+void ListenerRegistrationInternal::Initialize(jni::Loader& loader) {
+  loader.LoadClass(kClassName, kRemove);
+}
 
 ListenerRegistrationInternal::ListenerRegistrationInternal(
     FirestoreInternal* firestore,
     EventListener<DocumentSnapshot>* event_listener, bool owning_event_listener,
-    jobject listener_registration)
+    const Object& listener_registration)
     : firestore_(firestore),
-      listener_registration_(
-          firestore->app()->GetJNIEnv()->NewGlobalRef(listener_registration)),
+      listener_registration_(listener_registration),
       document_event_listener_(event_listener),
       owning_event_listener_(owning_event_listener) {
   FIREBASE_ASSERT(firestore != nullptr);
   FIREBASE_ASSERT(event_listener != nullptr);
-  FIREBASE_ASSERT(listener_registration != nullptr);
+  FIREBASE_ASSERT(listener_registration);
 
   firestore->RegisterListenerRegistration(this);
 }
 
 ListenerRegistrationInternal::ListenerRegistrationInternal(
     FirestoreInternal* firestore, EventListener<QuerySnapshot>* event_listener,
-    bool owning_event_listener, jobject listener_registration)
+    bool owning_event_listener, const Object& listener_registration)
     : firestore_(firestore),
-      listener_registration_(
-          firestore->app()->GetJNIEnv()->NewGlobalRef(listener_registration)),
+      listener_registration_(listener_registration),
       query_event_listener_(event_listener),
       owning_event_listener_(owning_event_listener) {
   FIREBASE_ASSERT(firestore != nullptr);
   FIREBASE_ASSERT(event_listener != nullptr);
-  FIREBASE_ASSERT(listener_registration != nullptr);
+  FIREBASE_ASSERT(listener_registration);
 
   firestore->RegisterListenerRegistration(this);
 }
 
 ListenerRegistrationInternal::ListenerRegistrationInternal(
     FirestoreInternal* firestore, EventListener<void>* event_listener,
-    bool owning_event_listener, jobject listener_registration)
+    bool owning_event_listener, const Object& listener_registration)
     : firestore_(firestore),
-      listener_registration_(
-          firestore->app()->GetJNIEnv()->NewGlobalRef(listener_registration)),
+      listener_registration_(listener_registration),
       void_event_listener_(event_listener),
       owning_event_listener_(owning_event_listener) {
   FIREBASE_ASSERT(firestore != nullptr);
   FIREBASE_ASSERT(event_listener != nullptr);
-  FIREBASE_ASSERT(listener_registration != nullptr);
+  FIREBASE_ASSERT(listener_registration);
 
   firestore->RegisterListenerRegistration(this);
 }
@@ -64,18 +71,14 @@ ListenerRegistrationInternal::ListenerRegistrationInternal(
 // FirestoreInternal will hold the lock and unregister all of them. So we do not
 // call UnregisterListenerRegistration explicitly here.
 ListenerRegistrationInternal::~ListenerRegistrationInternal() {
-  if (listener_registration_ == nullptr) {
+  if (!listener_registration_) {
     return;
   }
 
   // Remove listener and release java ListenerRegistration object.
-  JNIEnv* env = firestore_->app()->GetJNIEnv();
-  env->CallVoidMethod(
-      listener_registration_,
-      listener_registration::GetMethodId(listener_registration::kRemove));
-  env->DeleteGlobalRef(listener_registration_);
-  util::CheckAndClearJniExceptions(env);
-  listener_registration_ = nullptr;
+  Env env = GetEnv();
+  env.Call(listener_registration_, kRemove);
+  listener_registration_.clear();
 
   // de-allocate owning EventListener object.
   if (owning_event_listener_) {
@@ -85,21 +88,7 @@ ListenerRegistrationInternal::~ListenerRegistrationInternal() {
   }
 }
 
-/* static */
-bool ListenerRegistrationInternal::Initialize(App* app) {
-  JNIEnv* env = app->GetJNIEnv();
-  jobject activity = app->activity();
-  bool result = listener_registration::CacheMethodIds(env, activity);
-  util::CheckAndClearJniExceptions(env);
-  return result;
-}
-
-/* static */
-void ListenerRegistrationInternal::Terminate(App* app) {
-  JNIEnv* env = app->GetJNIEnv();
-  listener_registration::ReleaseClass(env);
-  util::CheckAndClearJniExceptions(env);
-}
+jni::Env ListenerRegistrationInternal::GetEnv() { return firestore_->GetEnv(); }
 
 }  // namespace firestore
 }  // namespace firebase
diff --git a/firestore/src/android/listener_registration_android.h b/firestore/src/android/listener_registration_android.h
@@ -1,11 +1,13 @@
 #ifndef FIREBASE_FIRESTORE_CLIENT_CPP_SRC_ANDROID_LISTENER_REGISTRATION_ANDROID_H_
 #define FIREBASE_FIRESTORE_CLIENT_CPP_SRC_ANDROID_LISTENER_REGISTRATION_ANDROID_H_
 
-#include <jni.h>
 #include "firestore/src/android/firestore_android.h"
 #include "firestore/src/include/firebase/firestore/document_snapshot.h"
 #include "firestore/src/include/firebase/firestore/event_listener.h"
 #include "firestore/src/include/firebase/firestore/query_snapshot.h"
+#include "firestore/src/jni/jni_fwd.h"
+#include "firestore/src/jni/object.h"
+#include "firestore/src/jni/ownership.h"
 
 namespace firebase {
 namespace firestore {
@@ -22,21 +24,23 @@ class ListenerRegistrationInternal {
  public:
   using ApiType = ListenerRegistration;
 
+  static void Initialize(jni::Loader& loader);
+
   // Global references will be created from the jobjects. The caller is
   // responsible for cleaning up any local references to jobjects after the
   // constructor returns.
   ListenerRegistrationInternal(FirestoreInternal* firestore,
                                EventListener<DocumentSnapshot>* event_listener,
                                bool owning_event_listener,
-                               jobject listener_registration);
+                               const jni::Object& listener_registration);
   ListenerRegistrationInternal(FirestoreInternal* firestore,
                                EventListener<QuerySnapshot>* event_listener,
                                bool owning_event_listener,
-                               jobject listener_registration);
+                               const jni::Object& listener_registration);
   ListenerRegistrationInternal(FirestoreInternal* firestore,
                                EventListener<void>* event_listener,
                                bool owning_event_listener,
-                               jobject listener_registration);
+                               const jni::Object& listener_registration);
 
   // Delete the default one to make the ownership more obvious i.e.
   // FirestoreInternal owns each instance and forbid anyone else to make copy.
@@ -57,13 +61,11 @@ class ListenerRegistrationInternal {
 
  private:
   friend class DocumentReferenceInternal;
-  friend class FirestoreInternal;
 
-  static bool Initialize(App* app);
-  static void Terminate(App* app);
+  jni::Env GetEnv();
 
   FirestoreInternal* firestore_ = nullptr;  // not owning
-  jobject listener_registration_ = nullptr;
+  jni::Global<jni::Object> listener_registration_;
 
   // May own it, see owning_event_listener_. If user pass in an EventListener
   // directly, then the registration does not own it. If user pass in a lambda,
diff --git a/firestore/src/android/query_android.cc b/firestore/src/android/query_android.cc
@@ -294,10 +294,8 @@ ListenerRegistration QueryInternal::AddSnapshotListener(
                java_metadata, java_listener);
 
   if (!env.ok()) return {};
-
   return ListenerRegistration(new ListenerRegistrationInternal(
-      firestore_, listener, passing_listener_ownership,
-      java_registration.get()));
+      firestore_, listener, passing_listener_ownership, java_registration));
 }
 
 Local<Array<Object>> QueryInternal::ConvertFieldValues(
diff --git a/firestore/src/jni/ownership.h b/firestore/src/jni/ownership.h
@@ -104,6 +104,13 @@ class Local : public T {
     return static_cast<jni_type>(result);
   }
 
+  void clear() {
+    if (env_ && T::object_) {
+      env_->DeleteLocalRef(T::object_);
+      T::object_ = nullptr;
+    }
+  }
+
   JNIEnv* env() const { return env_; }
 
  private:
@@ -226,6 +233,14 @@ class Global : public T {
     return static_cast<jni_type>(result);
   }
 
+  void clear() {
+    if (T::object_) {
+      JNIEnv* env = GetEnv();
+      env->DeleteGlobalRef(T::object_);
+      T::object_ = nullptr;
+    }
+  }
+
  private:
   JNIEnv* EnsureEnv(JNIEnv* other = nullptr) {
     if (other != nullptr) {

Original file line number	Diff line number	Diff line change
`@@ -176,8 +176,7 @@ ListenerRegistration DocumentReferenceInternal::AddSnapshotListener(`
`176`	`176`
`177`	`177`	`if (!env.ok() \|\| !java_registration) return {};`
`178`	`178`	`return ListenerRegistration(new ListenerRegistrationInternal(`
`179`		`- firestore_, listener, passing_listener_ownership,`
`180`		`- java_registration.get()));`
	`179`	`+ firestore_, listener, passing_listener_ownership, java_registration));`
`181`	`180`	`}`
`182`	`181`
`183`	`182`	`Class DocumentReferenceInternal::GetClass() { return Class(clazz); }`