Autofill Service provider - showing suggestions on other apps - internal version #5561
Conversation
cmonfortep
changed the title
cmonfortep
changed the title
cmonfortep
force-pushed
the
feature/cristian/autofill/autofill_provider_poc
branch
from
1814778 to
3a16022
Compare
This stack of pull requests is managed by Graphite. Learn more about stacking. |
cmonfortep
force-pushed
the
feature/cristian/autofill/autofill_provider_poc
branch
from
3a16022 to
8647864
Compare
| override fun parseStructure(structure: AssistStructure): MutableList<AutofillRootNode> { | ||
| val autofillRootNodes = mutableListOf<AutofillRootNode>() | ||
| val windowNodeCount = structure.windowNodeCount | ||
| Timber.i("DDGAutofillService windowNodeCount: $windowNodeCount") |
There was a problem hiding this comment.
nit: will comment on the logging just the once, but this comment applies to all logs added in this PR
- are all the logs added going to be too noisy to keep around as is, especially at the
infolevel? - maybe worth doing a pass seeing if they should be kept and/or downgraded to
verbose
There was a problem hiding this comment.
I like the proposal of keeping the debug ones as verbose.
I'm gonna do some clean up, but some of them are really helpful when understanding heuristics results.
...fill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/service/RealAutofillService.kt
Show resolved
Hide resolved
| import timber.log.Timber | ||
|
|
||
| @InjectWith(scope = ServiceScope::class) | ||
| class RealAutofillService : AutofillService() { |
There was a problem hiding this comment.
I think we want some global error handling around the autofill service parsing, and in here is the likely place i think. going on the model that any errors happening in autofill service should not result in our app crashing.
e.g., top level runCatching inside onFillRequest ?
There was a problem hiding this comment.
I will add some runCatching now, but intention is to send an error pixel if it crashes. That will be included when we implement pixels.
| return PendingIntent | ||
| .getActivity( | ||
| context, | ||
| Random.nextInt(), |
There was a problem hiding this comment.
can you add a comment to explain why Random.nextInt() is needed here
| val domain = credential.nonEmptyDomain() | ||
| val domainTitle = credential.nonEmptyDomainTitle() | ||
|
|
||
| val title = listOfNotNull(userName, domainTitle, domain).first() |
There was a problem hiding this comment.
.first() will crash if the list is empty. not sure if that’s possible at this point or not, or if the caller of this function prevents it. but would be best if we didn’t rely on the caller to avoid a crash. maybe use firstOrNull and handle the null case in here?
cmonfortep
force-pushed
the
feature/cristian/autofill/autofill_provider_poc
branch
from
cb79246 to
12bf984
Compare
Task/Issue URL: https://app.asana.com/0/1149059203486286/1209279712661815/f
Description
Scope:
Create autofill Service (only internal)
Parse structure
Apply heuristics
Extract packageId or Domain
Show suggestions
autofill on suggestion clicked
Show "Search in DDG" suggestion (only UI)
Steps to test this PR
The easiest way to test the logic here is to do it using #5508
And use https://app.asana.com/0/1149059203486286/1209279712661812/f for test scenarios
UI changes