My idea to handle blob service query failure is that the notification containing blob_hash and encryption key will contain some very short text (to ensure it is deliverable via FCM) that will be displayed instead if we can't fetch the actual blob at the moment.

An alternative to handle network/blob service downtime would be to turn this query into a WorkRequest with exponential backoff retry policy: https://developer.android.com/guide/background/persistent/getting-started/define-work#retries_backoff

Why to make this call asynchronous?

On one hand FCM docs state that onMessageReceived is executed on separate thread - different than main thread: https://firebase.google.com/docs/reference/android/com/google/firebase/messaging/FirebaseMessagingService. On the other thread, I had done some search through FCM code and found out it is always the same thread - subsequent onMessageReceived calls are executed sequentially on dedicated background thread. Therefore I concluded that we shouldn't block displaying small notifications immediately on some large notification being downloaded.

To avoid background execution abuse I set time limit on each blob request to the value mentioned in the docs: https://firebase.google.com/docs/cloud-messaging/android/receive#backgrounded

But I think using WorkManager and exponential backoff policy for this case a bit of over-engineering.

• According to the docs, 20s is the maximum limit, but can be shorter depending on OS and other random stuff. Maybe we can decrease it to let's say 15s?
• Other than that, can we extract this number as a private static final constant? Along with this comment

My idea to handle blob service query failure is that the notification containing blob_hash and encryption key will contain some very short text (to ensure it is deliverable via FCM) that will be displayed instead if we can't fetch the actual blob at the moment.

This idea makes sense - I think we want notification to be displayed even if it's short instead of delaying its delivery until request succeeds (which may never happen).

As a user, I'd be more frustrated when received notif too late (or totally miss it) than if it was brief/vague but delivered quickly.

Makes sense to me

Do we have to create this object each time this function is called?

You are right @tomek . This if statement should be removed and badgeOnly always set to '0'. Real badge only notifs are handled in updateBadgeCount function in this file.

OkHttp library methods are throwing checked exception so throws clause is necessary.

This is probably out of the scope of this differential.

Are they declaring to throw Exception or a subtype? If only some subtypes, we can make this method declare throwing only the subtypes and not the Exception.

OkHttp methods that we use here [[ https://square.github.io/okhttp/#post-to-a-server | could throw IOException ]]. Out getAuthToken method could throw JSONException. Theoretically we can experience OutOfMemory error since we are downloading potentially large blob into memory. That said if we want to be more specific we would have to bring back the original code and remove the Exception.