PIR: Implement resume emailConfirmation #6895
Conversation
pir/pir-impl/src/main/java/com/duckduckgo/pir/impl/common/PirRunStateHandler.kt
Show resolved
Hide resolved
pir/pir-impl/src/main/java/com/duckduckgo/pir/impl/store/PirRepository.kt
Outdated
Show resolved
Hide resolved
| runType: RunType, | ||
| ): Result<Unit> = Result.success(Unit) | ||
| ): Result<Unit> = withContext(dispatcherProvider.io()) { | ||
| // onJobStarted() |
There was a problem hiding this comment.
nit: should this be removed?
There was a problem hiding this comment.
I am actually still contemplating about it. 😅 I will remove for now. My problem here is to handle the scenario when the scan and email confirmation run together. We want to have the the callbacks running as long as one type of job is running. I need to think about that more. Will do in another task.
pir/pir-impl/src/main/java/com/duckduckgo/pir/impl/email/PirEmailConfirmation.kt
Outdated
Show resolved
Hide resolved
pir/pir-impl/src/main/java/com/duckduckgo/pir/impl/email/PirEmailConfirmation.kt
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| val script = pirCssScriptLoader.getScript() | ||
| maxWebViewCount = minOf(processedJobRecords.size, MAX_DETACHED_WEBVIEW_COUNT) |
There was a problem hiding this comment.
Q: what is the max detached webview count based on? Can there be an issue if for example this email confirmation is running and also scan or opt-out? In that case, theoretically there would be MAX_DETACHED_WEBVIEW_COUNT * 2 or * 3 created
There was a problem hiding this comment.
@landomen very good question actually. A bit of context, initially we set it to the total number of cores of the device since we were under the assumption that they will be fully parallel. However, after further investigations, we understood that they are not entirely parallel. Some interactions with the webview still has to happen and wait for the main thread(but async).
Because of this, we have been increasing and benchmarking the total number of webviews to see how much will be able to function relatively well. So far 20 has been an OK number - we will know once we get more users.
Now to the question of "Can there be an issue if for example this email confirmation is running and also scan or opt-out”, I don’t think it will be a big issue:
- Worst case it will be *2 (as scan/opt-outs don’t happen in parallel together).
- Realistically, only 4 brokers send links today so the chances of us maximizing the limit for email confirmation is low.
- The email confirmation webviews will be very short-lived. Majority (except for 1 broker) have simple steps for email confirmation - usually just expectation action after loading the links. Hence if we maxed it, it will just for a short time.
We can consider lessening it - but i think we could stick with the current numbers and see how it plays out in the wild. What do you think?
Task/Issue URL: https://app.asana.com/1/137249556945/project/481882893211075/task/1211491791085983?focus=true
Description
See attached task description.
Steps to test this PR
https://app.asana.com/1/137249556945/project/481882893211075/task/1211559446932897?focus=true