Close #7164: Add getSubscription to AutoPushFeature if one exists #7172
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7172 +/- ##
============================================
+ Coverage 77.26% 78.06% +0.80%
+ Complexity 5048 4782 -266
============================================
Files 675 625 -50
Lines 24705 22323 -2382
Branches 3641 3329 -312
============================================
- Hits 19088 17427 -1661
+ Misses 4111 3492 -619
+ Partials 1506 1404 -102 Continue to review full report at Codecov.
|
| fun getSubscription( | ||
| scope: String, | ||
| appServerKey: String? = null, | ||
| block: (AutoPushSubscription?) -> Unit |
There was a problem hiding this comment.
Consider this for a follow-up, if you want:
This API hides the fact that block may be called on a thread that's different from the caller's thread. What about just returning a Deferred<AutoPushSubscription?>, instead? The caller will need to manage the coroutines but at least it'll be obvious what's going on and doesn't leave a lot of room for error.
I made this mistake with the account manager callbacks, and the silent thread switching keeps biting us since it's just so easy to forget about when you're using the APIs.
I see that it's a broader pattern here, so I'll let you decide if this is something to be concerned about in practice. At the very least, I suggest documenting this within @param block docstring.
There was a problem hiding this comment.
That's a fair point that we should invoke the callback on the caller's thread. I'll have to spend some time to figure out how this would affect our integrations.
When we last tried exposing the async functions, it was a bit more complicated to complete the GeckoResult async call with the kotlin async call. We opted to follow the WebExtension learnings since it had a similar one-to-one mapping of APIs.
Now that we have these nested calls where we need to make async checks before another async call, we should re-visit that.
For that reason, I stuck with the current system and hid the containsSubscription call internally.
There was a problem hiding this comment.
We haven't (thankfully) run into issues with being on the wrong thread since consumers always need to be on a background thread to wait for messages, but that doesn't fix the problem with this design that you rightfully spotted.
| appServerKey: String? = null, | ||
| block: (AutoPushSubscription?) -> Unit | ||
| ) { | ||
| connection.ifInitialized { |
There was a problem hiding this comment.
So the block may never even get called?
There was a problem hiding this comment.
Yes, it could if the push native layer is not initialized, but we would have much bigger problems here if we can get this far in the call stack having not crashed yet.
This has come up before so maybe we call simplify this call by putting it within the Connection. It was meant to satisfy the push state along and remove the null-ability when needing to use the native API.
There was a problem hiding this comment.
Filed #7184 to track this work.
| @@ -154,6 +159,14 @@ internal class RustPushConnection( | |||
| return pushApi.unsubscribeAll() | |||
| } | |||
|
|
|||
| @GuardedBy("this") | |||
| override suspend fun containsSubscription(scope: PushScope): Boolean = synchronized(this) { | |||
| val pushApi = api | |||
There was a problem hiding this comment.
Why do you need a local variable? you already synchronzied the call to this, so as long as every access to api is also synchronzied it seems unnecessary?
| } else { | ||
| result.completeExceptionally(WebPushException("Retrieving subscription failed.")) | ||
| } | ||
| result.complete(subscription?.toGeckoSubscription()) |
There was a problem hiding this comment.
Due to connection.ifInitialized this may never get called, I think. That seems like a problem, if we never complete the result.
…one exists We added the ability to check if a subscription exists first to match the API requirement for WebPush support in GeckoView. We do this because the call to `getSubscription` on the engine side expects it to work as a check before creating a subscription, where-as our implementation used a "get or create" pattern instead. The fallout from this is that visiting sites that check for subscriptions were immediately given one without user permission.
…nally With the previous patch, we fixed a bug where we need to check if a subscription exists. This new (correct) behaviour broke our original implementation of how to handle the null case in the engine.
|
bors r=grigoryk |
|
Build succeeded: |
There are two parts to this fix:
First, we added the ability to check if a subscription exists first to match the
API requirement for WebPush support in GeckoView. We do this because the
call to
getSubscriptionon the engine side expects it to work as acheck before creating a subscription, where-as our implementation used a
"get or create" pattern instead.
The fallout from this is that visiting sites that check for
subscriptions were immediately given one without user permission.
Second, with the previous patch, we fixed a bug where we need to check if a
subscription exists. This new (correct) behaviour broke our original
implementation of how to handle the null case in the engine.
Integration PR for Reference Browser: mozilla-mobile/reference-browser#1208
Fenix PR will be a fast-follow.
Closes #7164
Pull Request checklist
After merge