Add ReactMarkerConstants.CONTENT_APPEARED support on Android #43620

Kudo · 2024-03-22T16:57:26Z

Summary:

Add the ReactMarkerConstants.CONTENT_APPEARED support on Android in bridgeless mode. This is an important marker for TTI measurement.

Changelog:

[ANDROID] [ADDED] - Add the ReactMarkerConstants.CONTENT_APPEARED support on Android in bridgeless mode.

Test Plan:

adding this on RNTesterActivity to see if the log is executed

ReactMarker.addListener { name, tag, instanceKey ->
  if (name == ReactMarkerConstants.CONTENT_APPEARED) {
    Log.i("XXX", "XXX")
  }
}

analysis-bot · 2024-03-22T17:28:34Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	20,248,988	-115,290
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	23,454,719	-114,804
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: 42f136d
Branch: main

Kudo · 2024-03-29T17:20:42Z

appreciated if @cortinico could take a look for this pr.

cortinico · 2024-04-02T11:13:11Z

cc @rubennorte

Kudo · 2024-04-11T07:19:20Z

rebase main to fix conflict

Kudo · 2024-04-24T16:47:28Z

much appreciated if @rubennorte could help to take a look for this pr

rubennorte · 2024-06-11T11:18:29Z

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java

-      if (mJSModuleName != null) {
-        ReactMarker.logMarker(ReactMarkerConstants.CONTENT_APPEARED, mJSModuleName, mRootViewTag);
-      }
+      String jsModuleName = getJSModuleName();


Sorry for the very late review.

This feels risky because getJSModuleName asserts, which could introduce new crashes. I understand you changed this so we can override it, but we should either try/catch here or create a new method that doesn't assert and use it instead (and maybe use it to access instead of mJSModuleName everywhere in this file).

thanks for having the comment. after this pr, mJSModuleName is only access from the getter.

i was thinking that assertion for getJSModuleName() could help us catching the issue as early as possible. if we believe that getJSModuleName() should always be non-nullable. otherwise, if getJSModuleName() could be nullable, i don't really like try-catch or introducing new method, having an alternative e6a722d change, please let me know which makes more sense to you.

could help us catching the issue as early as possible

I agree with this when adding a new API, but modifying an API to introduce this, if the previous behavior wasn't crashing, is not ideal.

Given that we already have some callers in the critical path (e.g.: startSurface) that were using the getter with the assert, I think it might be safe to continue using it, even for this use case.

alright, if that's more toward existing call paths, having try-catch makes more sense than introducing new method.
i've updated the pr to have try-catch. please check again if that makes sense to you. thanks!

Sorry I think you misunderstood. I meant that you can keep the implementation you had replacing all usages with getJSModuleName with the assert because we were already calling that when loading a new surface. If there were cases where it was defined, that should've thrown an error in those cases too. So replacing all with getJSModuleName and removing the null checks should be fine.

sorry i was easily mistaken. i've reset the branch to the previous version. hopes that is correct.

rubennorte · 2024-06-12T09:30:34Z

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactRoot.java

@@ -28,6 +28,7 @@ public interface ReactRoot {
  @Nullable
  Bundle getAppProperties();

+  @Nullable


We can remove this @Nullable annotation and all the new asserts on this diff

updated. does the diff look correct now?

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java

rubennorte · 2024-06-12T10:21:42Z

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java

@@ -632,7 +632,7 @@ public void setEventListener(@Nullable ReactRootViewEventListener eventListener)

  @Override
  public String getJSModuleName() {
-    return Assertions.assertNotNull(mJSModuleName);
+    return mJSModuleName;


I thought we were adding back the assertion. Can you please update this?

haha it looks like the original version is more correct. sorry i totally misunderstood what you said before.
updated and please let me know if there are any other feedback.

facebook-github-bot · 2024-06-12T10:33:04Z

@rubennorte has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

rubennorte

Thanks for adding support for this in the new architecture! I'm going to import this at Meta and test it internally before merging.

Kudo · 2024-06-12T10:43:47Z

much appreciated @rubennorte. sorry i spent you a while to address review feedback back-and-forth .

facebook-github-bot · 2024-06-13T11:01:45Z

@rubennorte merged this pull request in 5da9fdf.

github-actions · 2024-06-13T11:01:47Z

This pull request was successfully merged by @Kudo in 5da9fdf.

^{When will my fix make it into a release? | How to file a pick request?}

facebook-github-bot · 2024-06-13T14:23:26Z

This pull request has been reverted by 7e0f894.

Kudo · 2024-06-13T15:17:14Z

oh no i'm sorry if that cause some breaks in meta internally. let me know if there's anything i can help with.

rubennorte · 2024-06-13T17:30:00Z

oh no i'm sorry if that cause some breaks in meta internally. let me know if there's anything i can help with.

Hey, don't worry. It's just one CI job that didn't run correctly in the original PR. I'll re-commit it with the fix today/tomorrow.

Kudo · 2024-06-13T18:22:34Z

oh no i'm sorry if that cause some breaks in meta internally. let me know if there's anything i can help with.

Hey, don't worry. It's just one CI job that didn't run correctly in the original PR. I'll re-commit it with the fix today/tomorrow.

thanks! feel free to let me know if there's anything i can help.

…ts.CONTENT_APPEARED support on Android" Summary: Changelog: [ANDROID] [ADDED] - Add the ReactMarkerConstants.CONTENT_APPEARED support on Android in bridgeless mode. This re-applies #43620 which was reverted because a CI job started failing because we forgot to update `packages/react-native/ReactAndroid/api/ReactAndroid.api`. Reviewed By: cortinico Differential Revision: D58535868 fbshipit-source-id: 9eec33c5e798850a7434a6c391abf2fc3fc9d0a6

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. p: Expo Partner: Expo Partner labels Mar 22, 2024

Kudo marked this pull request as ready for review March 22, 2024 18:09

facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Mar 22, 2024

cortinico requested a review from rubennorte April 2, 2024 11:13

Kudo force-pushed the @kudo/bridgeless/CONTENT_APPEARED branch from b3e2a3e to 813c6b7 Compare April 11, 2024 07:19

Add ReactMarkerConstants.CONTENT_APPEARED support on Android

a237918

Kudo force-pushed the @kudo/bridgeless/CONTENT_APPEARED branch from 813c6b7 to a237918 Compare June 11, 2024 08:48

rubennorte reviewed Jun 11, 2024

View reviewed changes

assertion in caller

e6a722d

Kudo force-pushed the @kudo/bridgeless/CONTENT_APPEARED branch from 56862e0 to e6a722d Compare June 11, 2024 16:25

rubennorte reviewed Jun 12, 2024

View reviewed changes

Apply suggestions from code review

fdabe38

rubennorte reviewed Jun 12, 2024

View reviewed changes

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java Outdated Show resolved Hide resolved

rubennorte reviewed Jun 12, 2024

View reviewed changes

Apply suggestions from code review

9974f53

rubennorte approved these changes Jun 12, 2024

View reviewed changes

facebook-github-bot closed this in 5da9fdf Jun 13, 2024

facebook-github-bot added the Merged This PR has been merged. label Jun 13, 2024

facebook-github-bot added the Reverted label Jun 13, 2024

Kudo mentioned this pull request Jun 26, 2024

[0.74] Add ReactMarkerConstants.CONTENT_APPEARED support on Android reactwg/react-native-releases#337

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ReactMarkerConstants.CONTENT_APPEARED support on Android #43620

Add ReactMarkerConstants.CONTENT_APPEARED support on Android #43620

Kudo commented Mar 22, 2024

analysis-bot commented Mar 22, 2024 •

edited

Loading

Kudo commented Mar 29, 2024

cortinico commented Apr 2, 2024

Kudo commented Apr 11, 2024

Kudo commented Apr 24, 2024

rubennorte Jun 11, 2024 •

edited

Loading

Kudo Jun 11, 2024

rubennorte Jun 11, 2024

Kudo Jun 11, 2024

rubennorte Jun 11, 2024

Kudo Jun 11, 2024

rubennorte Jun 12, 2024

Kudo Jun 12, 2024

rubennorte Jun 12, 2024

Kudo Jun 12, 2024

facebook-github-bot commented Jun 12, 2024

rubennorte left a comment

Kudo commented Jun 12, 2024

facebook-github-bot commented Jun 13, 2024

github-actions bot commented Jun 13, 2024

facebook-github-bot commented Jun 13, 2024

Kudo commented Jun 13, 2024

rubennorte commented Jun 13, 2024

Kudo commented Jun 13, 2024

Add ReactMarkerConstants.CONTENT_APPEARED support on Android #43620

Add ReactMarkerConstants.CONTENT_APPEARED support on Android #43620

Conversation

Kudo commented Mar 22, 2024

Summary:

Changelog:

Test Plan:

analysis-bot commented Mar 22, 2024 • edited Loading

Kudo commented Mar 29, 2024

cortinico commented Apr 2, 2024

Kudo commented Apr 11, 2024

Kudo commented Apr 24, 2024

rubennorte Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jun 12, 2024

rubennorte left a comment

Choose a reason for hiding this comment

Kudo commented Jun 12, 2024

facebook-github-bot commented Jun 13, 2024

github-actions bot commented Jun 13, 2024

facebook-github-bot commented Jun 13, 2024

Kudo commented Jun 13, 2024

rubennorte commented Jun 13, 2024

Kudo commented Jun 13, 2024

analysis-bot commented Mar 22, 2024 •

edited

Loading

rubennorte Jun 11, 2024 •

edited

Loading