Implement JNI files to use MessageOperationsUtilities functionality on Android in CommNotificationHandler.java
Details
No test plan. Child differential will use those changes and thus will provide opportunities for testing.
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
native/android/app/src/cpp/MessageOperationsUtilitiesJNIHelper.cpp | ||
---|---|---|
9 ↗ | (On Diff #12886) | Does this contain only a single message or multiple? |
native/android/app/src/cpp/MessageOperationsUtilitiesJNIHelper.cpp | ||
---|---|---|
6 ↗ | (On Diff #12886) | I agree that storeMessageInfo is probably better. The only thing I am confused abut is that D3941 that is already accepted defined this method as storeNotification. I think it is better for JNI classes to use the same method names as corresponding C++ methods. Should I then rename this method across entire diff stack? This question and concern also applies to your comment below since it probably asks to rename variable that is also already used in accepted diffs. |
9 ↗ | (On Diff #12886) | Argument facebook::jni::JString rawMessageInfoString is expected to carry string under 'messageInfos' key from notification JSON object. All notifications I examined in debugger carried list of JSON objects under 'messageInfos' key, but they were always one-element lists. Still I assumed it is better to implement notification parsing functionality in such a way that we assume it may contain multiple messages and iterate over this array. Therefore it would probably be better to rename this argument to rawMessageInfosString. |
12 ↗ | (On Diff #12886) | I will copy and paste an answer to similar question in corresponding iOS diff D4063
|
Regarding the renaming, it should be introduced in all the affected diffs, even if they are accepted. And I agree, the naming should be consistent.