Page MenuHomePhabricator

Implement JNI files to use MessageOperationUtilities in CommNotificationsHandler on Android
ClosedPublic

Authored by marcin on May 17 2022, 7:18 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 2, 10:33 AM
Unknown Object (File)
Oct 17 2024, 4:03 AM
Unknown Object (File)
Oct 7 2024, 7:10 PM
Unknown Object (File)
Oct 7 2024, 7:10 PM
Unknown Object (File)
Oct 7 2024, 7:10 PM
Unknown Object (File)
Oct 7 2024, 7:10 PM
Unknown Object (File)
Oct 7 2024, 7:10 PM
Unknown Object (File)
Oct 7 2024, 7:10 PM

Details

Summary

Implement JNI files to use MessageOperationsUtilities functionality on Android in CommNotificationHandler.java

Test Plan

No test plan. Child differential will use those changes and thus will provide opportunities for testing.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.May 17 2022, 7:21 AM
Harbormaster failed remote builds in B9219: Diff 12811!
tomek requested changes to this revision.May 18 2022, 9:51 AM
tomek added inline comments.
native/android/app/src/cpp/MessageOperationsUtilitiesJNIHelper.cpp
6 ↗(On Diff #12886)

We're storing a message and not a notification

12 ↗(On Diff #12886)

Is it a good idea to initialize on every notification? Or is calling this multiple times a no-op?

This revision now requires changes to proceed.May 18 2022, 9:51 AM
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

comm::SQLiteQueryExecutor::initialize(sqliteFilePathStdStr) is implemented with std::call_once semantics, so multiple call to initialization method do not perform unnecessary operations. On the other hand I understand it might be confusing. Probably we could extend SQLiteQueryExecutor API to check if it is already initialized or add a flag in NotificationService. What do you think?

Re-requesting review to clarify the scope of changes that need to be introduced.

tomek requested changes to this revision.May 20 2022, 1:10 AM

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.

This revision now requires changes to proceed.May 20 2022, 1:10 AM

Use new method name: storeMessageInfos

This revision is now accepted and ready to land.May 23 2022, 10:06 AM