Add FFI descriptors (WIP) by jrconlin · Pull Request #683 · mozilla/application-services

jrconlin · 2019-02-15T23:26:05Z

No description provided.

thomcc

Just happened across this, and will note some issues I see here that will save you time or are subtle.

Haven't looked at the part where you actually call things from kotlin, i suspect a lot of this is based on how we were doing things on the kotlin side before we switched to using handles for almost everything.

jonalmeida

I realise this is still a WIP, but also consider my comments more as questions from a limited understanding on the internals of push. 😸

jonalmeida · 2019-02-21T00:38:19Z

+ sender_id: String,
+ server_host: String?="push.service.mozilla.org",
+ socket_protocol: String?="https",
+ bridge_type:String?="fcm",


For the bridge_type, what are the valid types? Maybe it's easier with some typed class?

I'll look into adding a typed class for this, but I'm definitely a Kotlin noob. I'll see if there's anything like an enumeration that could be used.

jonalmeida · 2019-02-21T00:44:10Z

+ socket_protocol: String?="https",
+ bridge_type:String?="fcm",
+ encryption_key: String? = null) : PushAPI, AutoCloseable {
+ private var conn: RawPushConnection?


From my limited understanding, I believe this is a long-lived socket connection. If so, is there an automatic retry?

It's not quite a long lived connection, at least not on mobile.
Mobile will be using whatever the native messaging service offers. There are some calls that need to be made to the server, and those will be handled by the push connection. We're also going to wrap these inside of a "PushManager" which will handle more things like decryption and managing the individual connections.
As far as you should be concerned, there should be a fairly small set of functions that you'll need to call. You will have to keep some state, though, for things like which ChannelID goes to what callback, but ideally, we'll try to keep as much of that out of your hair as possible.

jonalmeida · 2019-02-21T00:46:42Z

-open class UrlParseFailed(msg: String): PushException(msg)
-open class InvalidPlaceInfo(msg: String): PushException(msg)
-open class PushConnectionBusy(msg: String): PushException(msg)
+ * @return Map of ChannelID: Endpoint, be sure to notify apps registered to given


Would it be expected that the android component calls this check before performing any action first?

Not necessarily. This check can be a bit expensive, since it fetches content from the server and iterates through known connections. It's more a general "sanity check" on the known data state. Calling it once a day should be sufficient.
In the past we've had a few occasions where a client has gotten out of sync with the server. This has resulted in the subscription provider never knowing that a given subscription is no longer valid and sending effectively useless data to the Push Service. This function provides a way to rectify that situation in the future.

thomcc

A lot of the FFI code is busted, due to using handles on the Rust side, but a PointerType subclass on the Kotlin side.

I suspect you can leverage ffi_support for making a bunch of the ffi rust code simpler too.

thomcc

Unsure about the package naming. I thought the only one that wasn't a domain name was the actual namespace in the code. @ncalexan: Ping?

You also need to add yourself to the megazords.

add to the cargo.toml: (for example https://github.com/mozilla/application-services/blob/master/megazords/reference-browser/Cargo.toml)
add to the lib.rs (for example https://github.com/mozilla/application-services/blob/master/megazords/reference-browser/src/lib.rs#L8)
Add to the megazord definition list https://github.com/mozilla/application-services/blob/master/gradle-plugin/src/main/kotlin/mozilla/appservices/AppServicesExtension.kt#L38
bump https://github.com/mozilla/application-services/blob/master/gradle-plugin/build.gradle#L3 (and after a release goes out with these changes, we need to talk to @ncalexan to bump the plugin version)

Or those steps can be done in another PR.

pjenvey · 2019-03-05T23:47:09Z

I still think this should be done in the same way the other FFIs do it but it's not a huge deal.

thanks Thom, we'll revisit this one after merging this

jrconlin · 2019-03-05T23:54:57Z

doing the auto-squash might be a bit problematic due to age and intervening commits.

If preferred, I'm fine doing a manual squash to reduce the total number of commits.

thomcc

Looks good, but you need to update megazords/fenix/android/src/main/java/mozilla/appservices/FenixMegazord.java, megazords/fenix/src/lib.rs, I think

This patch adds: * Kotlin FFI integration with unit tests * simplifies decryption to accept raw header values * adds new storage system Closes#693, #544, #543, #542

ncalexan · 2019-03-20T17:19:47Z

Unsure about the package naming. I thought the only one that wasn't a domain name was the actual namespace in the code. @ncalexan: Ping?

You also need to add yourself to the megazords.

1. add to the cargo.toml: (for example https://github.com/mozilla/application-services/blob/master/megazords/reference-browser/Cargo.toml)
2. add to the lib.rs (for example https://github.com/mozilla/application-services/blob/master/megazords/reference-browser/src/lib.rs#L8)
3. Add to the megazord definition list https://github.com/mozilla/application-services/blob/master/gradle-plugin/src/main/kotlin/mozilla/appservices/AppServicesExtension.kt#L38
4. bump https://github.com/mozilla/application-services/blob/master/gradle-plugin/build.gradle#L3 (and after a release goes out with these changes, we need to talk to @ncalexan to bump the plugin version)

Or those steps can be done in another PR.

Sorry, this got lost in my github firehose. I published an updated version of the plugin for Grisha's use at the end of last week.

rfk · 2019-03-20T21:17:20Z

You also need to add yourself to the megazords.

Do we have a doc where JR should have been able to find this step (and its sub-steps)?

thomcc · 2019-03-20T21:18:51Z

We don't (it's on my TODO) but I did describe the rough steps #683 (review)

jrconlin requested a review from pjenvey February 15, 2019 23:26

thomcc suggested changes Feb 17, 2019
View reviewed changes

mhammond added the in progress label Feb 19, 2019

thomcc reviewed Feb 20, 2019
View reviewed changes

Comment threadcomponents/push/ffi/src/lib.rs Outdated

jonalmeida reviewed Feb 21, 2019
View reviewed changes

jrconlin force-pushed the push-ffi branch 2 times, most recently from 1f4a193 to 9c188aaCompare February 26, 2019 00:50

jrconlin requested review from jonalmeida and thomcc February 26, 2019 19:28

thomcc suggested changes Feb 26, 2019
View reviewed changes

thomcc reviewed Feb 26, 2019
View reviewed changes

Comment threadcomponents/push/storage/src/db.rs Outdated
Comment threadcomponents/push/storage/src/db.rs Outdated

thomcc reviewed Feb 26, 2019
View reviewed changes

Comment threadcomponents/push/ffi/src/lib.rs Outdated

thomcc reviewed Feb 26, 2019
View reviewed changes

Comment threadcomponents/push/ffi/src/lib.rs Outdated
Comment threadcomponents/push/ffi/src/lib.rs Outdated
Comment threadcomponents/push/ffi/src/lib.rs Outdated
Comment threadsettings.gradle

ncalexan reviewed Feb 28, 2019
View reviewed changes

Comment threadcomponents/push/android/build.gradle

jrconlin force-pushed the push-ffi branch from e9c6eed to ee5f63eCompare March 2, 2019 00:57

jdragojevic added the push label Mar 4, 2019

jrconlin force-pushed the push-ffi branch from d49e917 to 960592dCompare March 4, 2019 22:56

jrconlin self-assigned this Mar 4, 2019

jrconlin requested a review from thomcc March 4, 2019 23:46

pjenvey reviewed Mar 5, 2019
View reviewed changes

Comment threadcomponents/push/subscriber/src/lib.rs Outdated
Comment threadcomponents/push/android/src/main/java/mozilla/appservices/push/PushManager.kt Outdated

thomcc reviewed Mar 5, 2019
View reviewed changes

jrconlin requested review from ncalexan, pjenvey and thomcc March 5, 2019 23:53

thomcc approved these changes Mar 5, 2019
View reviewed changes

feat: Add FFI Descriptors and integrate Push Component into Kotlin
26352eb
This patch adds: * Kotlin FFI integration with unit tests * simplifies decryption to accept raw header values * adds new storage system Closes#693, #544, #543, #542

jrconlin force-pushed the push-ffi branch from 7e922fc to 26352ebCompare March 6, 2019 00:35

pjenvey approved these changes Mar 6, 2019
View reviewed changes

jrconlin merged commit 2685d00 into masterMar 6, 2019

jrconlin deleted the push-ffi branch March 7, 2019 00:11

thomcc mentioned this pull request Mar 14, 2019
Fix syntax error in gradle plugin #775
Merged

Uh oh!

Conversation

jrconlin commented Feb 15, 2019

Uh oh!

thomcc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonalmeida left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonalmeidaFeb 21, 2019

Choose a reason for hiding this comment

Uh oh!

jrconlinFeb 25, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonalmeidaFeb 21, 2019

Choose a reason for hiding this comment

Uh oh!

jrconlinFeb 25, 2019

Choose a reason for hiding this comment

Uh oh!

jonalmeidaFeb 21, 2019

Choose a reason for hiding this comment

Uh oh!

jrconlinFeb 25, 2019

Choose a reason for hiding this comment

Uh oh!

thomcc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thomcc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pjenvey commented Mar 5, 2019

Uh oh!

jrconlin commented Mar 5, 2019

Uh oh!

thomcc left a comment

Choose a reason for hiding this comment

Uh oh!

ncalexan commented Mar 20, 2019

Uh oh!

rfk commented Mar 20, 2019

Uh oh!