fix: trim comments + cleanup

Author: matinzd

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/activityresult/DeferredActivityResultLauncher.kt

@@ -16,15 +16,13 @@ import com.facebook.react.bridge.UiThreadUtil
 import com.facebook.react.common.ReactConstants
 
 /**
- * An [ActivityResultLauncher] handed out before the host Activity's `ActivityResultRegistry` is
- * available. It delegates to the real launcher once [bind] is called, and queues a single pending
- * [launch] issued while unbound, firing it on bind. [unbind] detaches it when the host Activity is
- * destroyed so that [ReactActivityResultCallerImpl] can rebind it against the next host's registry.
+ * An [ActivityResultLauncher] that may exist before any `ActivityResultRegistry` is available: it
+ * delegates to the real launcher once [bind] is called, queues a single [launch] issued while
+ * unbound (fired on bind), and can be [unbind]-ed and rebound against a new host's registry.
  *
- * [launch] and [unregister] are called off the UI thread but reach `@MainThread` registry methods,
- * so both hop. [delegate] and [pendingLaunch] are therefore UI-thread only and need no lock. Note
- * [launch] decides bound-vs-queue *inside* the hop: doing it before would let a concurrent [unbind]
- * strand the launch on a dead registry.
+ * [delegate] and [pendingLaunch] are only touched on the UI thread; [launch] and [unregister] get
+ * there via [onUiThread]. [launch] decides between delegating and queueing *on* the UI thread, so
+ * a concurrent [unbind] cannot leave it pointed at a dead registry.
  */
 internal class DeferredActivityResultLauncher<I>(
     private val key: String,
@@ -58,7 +56,7 @@ internal class DeferredActivityResultLauncher<I>(
   }
 
   override fun unregister() {
-    // Drop the registration first, so nothing rebinds this launcher while the hop is in flight.
+    // Drop the registration first so nothing rebinds this launcher in the meantime.
     onUnregister()
     onUiThread {
       delegate?.unregister()
@@ -68,8 +66,8 @@ internal class DeferredActivityResultLauncher<I>(
   }
 
   /**
-   * Attaches [launcher], obtained from [registry], and fires any launch queued while unbound.
-   * [registry] is remembered so [isBoundTo] can tell whether a later host is a different one.
+   * Attaches [launcher], obtained from [registry] (remembered for [isBoundTo]), and fires any
+   * queued launch.
    */
   fun bind(registry: ActivityResultRegistry, launcher: ActivityResultLauncher<I>) {
     UiThreadUtil.assertOnUiThread()
@@ -89,6 +87,6 @@ internal class DeferredActivityResultLauncher<I>(
     boundRegistry = null
   }
 
-  /** Whether this launcher is already bound to [registry] specifically -- not merely to something. */
+  /** Whether this launcher is bound to [registry] itself, not just to any registry. */
   fun isBoundTo(registry: ActivityResultRegistry): Boolean = boundRegistry === registry
 }

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/activityresult/ReactActivityResultCaller.kt

@@ -12,37 +12,25 @@ import androidx.activity.result.ActivityResultLauncher
 import androidx.activity.result.contract.ActivityResultContract
 
 /**
- * Lets a native module register an AndroidX [ActivityResultContract] against the host Activity's
- * `ActivityResultRegistry` and receive results, without any changes to the consumer's
- * `MainActivity`.
- *
- * The API deliberately mirrors `androidx.activity.ComponentActivity.registerForActivityResult`:
- * same method name, same [ActivityResultCallback] shape, same returned [ActivityResultLauncher]
- * type. Unlike an Activity, a caller obtained from a `ReactContext` may register at any time --
- * including before any Activity exists -- and the returned launcher binds lazily to the real
- * registry once the host resumes.
+ * Lets a native module register an AndroidX [ActivityResultContract] and receive results without
+ * any changes to the consumer's `MainActivity`. Mirrors
+ * `androidx.activity.ComponentActivity.registerForActivityResult`, except registration is legal at
+ * any time: the returned launcher binds to the real registry once a host Activity resumes.
  *
  * Every registration carries a key that must be unique within the `ReactContext` and stable across
- * process death -- after the process is killed mid-flow, AndroidX replays the restored result to
- * whichever registration reproduces the same key string. The default key is scoped to the caller
- * (`"<owner class>:<contract class>"`), which is what lets two unrelated libraries both register a
- * stock contract such as `ActivityResultContracts.GetContent` without colliding.
- *
- * A collision throws an [IllegalStateException] at registration time. With owner scoping this is
- * only reachable when a single owner registers the same contract class twice; the fix is the
- * overload that takes an extra `key`, which is appended to -- not substituted for -- the
- * owner-and-contract scope, so a poorly chosen key can never reintroduce a cross-library collision.
+ * process death (AndroidX replays a restored result to whichever registration reproduces the same
+ * key). The default key `"<owner class>:<contract class>"` lets unrelated libraries register the
+ * same stock contract without colliding; a collision throws [IllegalStateException] at
+ * registration time, and the keyed overload (which appends to that scope, not replaces it)
+ * resolves it.
  */
 internal interface ReactActivityResultCaller {
 
   /**
    * Registers [contract] under the key `"<owner class>:<contract class>"` and returns a launcher
-   * for it.
-   *
-   * [owner] should be a stable, long-lived object -- typically the native module itself. An
-   * anonymous object or a short-lived per-call helper yields a synthetic name such as
-   * `com.example.Foo$1`, which is fragile across builds and defeats re-association after process
-   * death.
+   * for it. [owner] should be a stable, long-lived object, typically the native module itself: an
+   * anonymous class's generated name can change between builds, which breaks result delivery
+   * after the process is killed and restored.
    *
    * @throws IllegalStateException if [owner] already registered this contract class
    */
@@ -53,12 +41,9 @@ internal interface ReactActivityResultCaller {
   ): ActivityResultLauncher<I>
 
   /**
-   * Registers [contract] under the key `"<owner class>:<contract class>:<key>"`. Use this when one
-   * owner needs several launchers of the same contract class.
-   *
-   * [key] only has to be unique among [owner]'s registrations of this contract class -- the
-   * owner-and-contract scope is still applied -- but it must be stable across process death, so
-   * derive it from a constant rather than from runtime state.
+   * Registers [contract] under the key `"<owner class>:<contract class>:<key>"`. Use this when
+   * one owner needs several launchers of the same contract class. [key] only has to be unique
+   * among those, but must stay the same across process restarts, so derive it from a constant.
    *
    * @throws IllegalStateException if [owner] already registered this contract class under [key]
    */

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/activityresult/ReactActivityResultCallerImpl.kt

@@ -20,11 +20,9 @@ import com.facebook.react.common.ReactConstants
 import java.util.concurrent.ConcurrentHashMap
 
 /**
- * Runs [block] on the UI thread, inline if already there.
- *
- * [ActivityResultRegistry] is `@MainThread` and its key tables are unsynchronized. Nothing enforces
- * that at runtime, so an off-thread call corrupts them silently rather than throwing -- and RN
- * registers on the JS thread and launches on the native-modules thread.
+ * Runs [block] on the UI thread, inline if already there. [ActivityResultRegistry] is `@MainThread`
+ * but not enforced at runtime: an off-thread call corrupts it silently, and RN calls in from the JS
+ * and native-modules threads.
  */
 internal fun onUiThread(block: () -> Unit) {
   if (UiThreadUtil.isOnUiThread()) block() else UiThreadUtil.runOnUiThread(block)
@@ -33,36 +31,25 @@ internal fun onUiThread(block: () -> Unit) {
 /**
  * Default [ReactActivityResultCaller], owned by a [ReactContext].
  *
- * Registrations are accepted at any time -- native modules are created lazily, typically well after
- * the host Activity has resumed -- and bound to the current Activity's [ActivityResultRegistry]
- * either immediately (when an Activity is already available) or on the next `onHostResume`.
- * Registrations outlive any single Activity: keys stay stable, so AndroidX can re-associate a result
- * that arrives after Activity recreation.
- *
- * ## Which registry a launcher is bound to
- *
- * Every `onHostResume` reconciles each launcher against the *current* registry, rebinding it if it
- * is attached to a different one. It deliberately does not stop at "already bound to something":
- * with multi-Activity navigation the new Activity resumes before the old one is destroyed, and
- * `ReactHostImpl.onHostDestroy(activity)` drops the old Activity's destroy entirely once
- * `currentActivity` has moved on. A launcher that only checked "am I bound?" would stay attached to
- * the previous Activity's dead registry -- leaking it, and misrouting anything launched from the new
- * screen.
+ * Registrations are accepted at any time and bound to the current Activity's
+ * [ActivityResultRegistry] immediately or on the next `onHostResume`. They outlive any single
+ * Activity: keys stay stable so AndroidX can re-associate a result after Activity recreation.
  *
- * ## Threading
+ * Every `onHostResume` checks each launcher against the *current* registry, not just "already
+ * bound to something": with multi-Activity navigation the new Activity resumes before the old one
+ * is destroyed (whose onHostDestroy is dropped once `currentActivity` moves on), so a bound-only
+ * check would leave launchers attached to the previous Activity's dead registry.
  *
- * [entries] is concurrent and reachable from any thread. Everything that touches
- * [ActivityResultRegistry] goes through [onUiThread].
- *
- * Registration itself stays on the caller's thread, so the launcher is returned immediately and a
- * duplicate key throws from the frame that caused it. Only the registry call is hopped.
+ * Threading: [entries] is concurrent and reachable from any thread; everything touching the
+ * registry goes through [onUiThread]. Registration stays on the caller's thread so the launcher
+ * returns immediately and a duplicate key throws at the causing frame. Only the registry call
+ * moves to the UI thread.
  */
 internal class ReactActivityResultCallerImpl(private val reactContext: ReactContext) :
     ReactActivityResultCaller, LifecycleEventListener {
 
   private class Entry<I, O>(
       val key: String,
-      val registrantDescription: String,
       private val contract: ActivityResultContract<I, O>,
       private val callback: ActivityResultCallback<O>,
       val launcher: DeferredActivityResultLauncher<I>,
@@ -73,8 +60,8 @@ internal class ReactActivityResultCallerImpl(private val reactContext: ReactCont
      */
     fun bindTo(registry: ActivityResultRegistry) {
       if (launcher.isBoundTo(registry)) return
-      // Release the previous host's registry first: it may already be dead, and leaving the
-      // callback registered there leaks that Activity and misroutes anything launched from it.
+      // Release any previous (possibly dead) registry first; staying registered there leaks its
+      // Activity and sends launches to the wrong one.
       launcher.unbind()
       launcher.bind(registry, registry.register(key, contract, callback))
     }
@@ -86,17 +73,13 @@ internal class ReactActivityResultCallerImpl(private val reactContext: ReactCont
     reactContext.addLifecycleEventListener(this)
   }
 
-  private fun getOwnerId(owner: Any): String = owner.javaClass.name
-
   override fun <I, O> registerForActivityResult(
       owner: Any,
       contract: ActivityResultContract<I, O>,
       callback: ActivityResultCallback<O>,
   ): ActivityResultLauncher<I> {
-    val id = getOwnerId(owner)
     return register(
-        key = "$id:${contract.javaClass.name}",
-        registrantDescription = id,
+        key = "${owner.javaClass.name}:${contract.javaClass.name}",
         collisionHint =
             "Register once and reuse the launcher, or pass a distinct key per launcher: " +
                 "registerForActivityResult(owner, \"someName\", contract, callback).",
@@ -110,28 +93,24 @@ internal class ReactActivityResultCallerImpl(private val reactContext: ReactCont
       contract: ActivityResultContract<I, O>,
       callback: ActivityResultCallback<O>,
   ): ActivityResultLauncher<I> {
-    val id = getOwnerId(owner)
     return register(
-        key = "$id:${contract.javaClass.name}:$key",
-        registrantDescription = id,
+        key = "${owner.javaClass.name}:${contract.javaClass.name}:$key",
         collisionHint = "Pass a key that is unique among this owner's launchers of this contract.",
         contract = contract,
         callback = callback)
   }
 
   private fun <I, O> register(
       key: String,
-      registrantDescription: String,
       collisionHint: String,
       contract: ActivityResultContract<I, O>,
       callback: ActivityResultCallback<O>,
   ): ActivityResultLauncher<I> {
     val launcher = DeferredActivityResultLauncher(key, contract) { entries.remove(key) }
-    val entry = Entry(key, registrantDescription, contract, callback, launcher)
-    entries.putIfAbsent(key, entry)?.let { existing ->
+    val entry = Entry(key, contract, callback, launcher)
+    if (entries.putIfAbsent(key, entry) != null) {
       throw IllegalStateException(
-          "${existing.registrantDescription} already registered a launcher for key '$key'. " +
-              collisionHint)
+          "A launcher is already registered for key '$key'. $collisionHint")
     }
     onUiThread { currentRegistry()?.let { registry -> entry.bindTo(registry) } }
     return launcher
@@ -145,9 +124,8 @@ internal class ReactActivityResultCallerImpl(private val reactContext: ReactCont
   override fun onHostPause(): Unit = Unit
 
   override fun onHostDestroy() = onUiThread {
-    // Detach from the dying registry but keep the registrations: they rebind against the next host's
-    // registry under the same keys on the next onHostResume, which is how AndroidX re-associates a
-    // result that outlives the Activity.
+    // Detach from the dying registry but keep the registrations: they rebind under the same keys
+    // on the next onHostResume, which is how AndroidX re-associates a surviving result.
     entries.values.forEach { it.launcher.unbind() }
   }