Status Update
Comments
ma...@gmail.com <ma...@gmail.com> #2
I highly support this, this way every current FormGroup
subclass can define only the functions it needs (Everything besides Address
can get rid of the AutofillType
API and of the app_locale
parameters, many classes could also get rid of the VerificationStatus
API, etc.)
AutofillProfile
could then implement a GetDataForType(FieldType) -> absl::variant<Address, NameInfo, ...>
and provide the current API with visit
calls instead of polymorphism.
vi...@google.com <vi...@google.com>
ma...@gmail.com <ma...@gmail.com> #4
Re the discussion from
It's true that the polymorphism isn't used, but isn't desirable to have a consistent interface among data model classes regardless?
I agree an interface has some appeal as a tool to define a contract even if the polymorphism isn't needed. But I'd say in C++ it's not worth the cost that comes with inheritance. By cost I mean the general sketchiness, the code-search complexity, and very concretely that we can't default operator==()
safely (which is what led me to file this bug). In the case of the SingleFieldFormFiller
interface, removing an "unused" interface was I think positive in every way (
I'd rather use a concept
plus static_assert
to express that multiple classes define the same API, if we don't need run-time polymorphism.
FormGroup gives us a consistent way to read/write data from/to all Autofill objects.
In my view*, FormGroup
is only on the surface a "consistent way to read/write data from/to all Autofill objects". I think it fails at defining a clear contract that's uniformly met by the implementations because there are so many similarly named functions, some of which may be useless because they don't use some parameters. As a result, in my experience, I have to look at every implementation anyway to figure out what it does. And the implementation inheritance IMO makes this a lot annoying because I'm always afraid that I'm missing some override.
This is useful by itself, but also enables easy composition of FormGroups.
It looks to me like we can do the same easily with absl::variant
, but I haven't tried it.
*: Admittedly I haven't used FormGroup
that much yet, but I've found myself looking at it on code search a lot of times over the past couple of weeks.
vi...@google.com <vi...@google.com> #5
From the discussion in
If we've already gathered some attention around this important topic, I will create a design document to discuss possible solutions. I think we agree that the FormGroup class is not well designed, but we have different ideas about what to do with it.
Piotr, did you already have a chance to create such a doc?
Description
What
User experience
What type of Android issue is this?
Display or Rendering
What steps would let us observe this issue?
What was the effect of this issue on your device usage, such as lost time or work?
High
When
Time and frequency
Time when bug report was triggered: Feb 28, 2025 9:27 PM GMT+10:00
How often has this happened?
Once
Where
Component
Originating component: <not visible> (1684638)
Build and device data
- Build Number: google/cheetah_beta/cheetah:Baklava/BP22.250124.009/13034193:user/release-keys
(Note: It is the build when sending this report. For exact build reference, please see the attached bugreport.)
Debugging information
Google Play services
com.google.android.gms
Version 250733035 (25.07.33 (260400-728382230))
System App (Updated)
Android System WebView
com.google.android.webview
Version 694312133 (133.0.6943.121)
System App (Updated)
Network operator: Boost
SIM operator: Boost
Filed by Android Beta Feedback. Version (Updated): 2.46-betterbug.external_20241023_RC01 (DOGFOOD)https://developer.android.com/preview/feedback#feedback-app .
To learn more about our feedback process, please visit