Status Update
Comments
bu...@chromium.org <bu...@chromium.org> #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.
pe...@google.com <pe...@google.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.
bu...@google.com <bu...@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
Steps to reproduce the problem
The issue can be seen in test image #1 vs #2 athttps://gregbenzphotography.com/hdr-photos/hdr-gain-maps-vs-tone-mapping/
#1 (a JPG gain map) shows correct color and matches the source image.
#2 (an HDR AVIF, no gain map) shows a color shift from magenta towards yellow in the sky, color shift in the water, etc.
These images should appear nearly identical (and do in software such as Photoshop), but they do not match in Chrome.
View the image on a system with 4 stops of HDR headroom so as to allow full rendering (ie no tone mapping, as deviations in rendering would be expected for a gain map vs tone map when the display lacks sufficient headroom).
Tested with MacOS 15.3.1 using an Apple XDR monitor with brightness set low enough to ensure 4 stops of headroom. You may confirm you are set up for 4 stops of headroom via test #1:https://gregbenzphotography.com/hdr#tests
Problem Description
HDR AVIF shows a hue shift relative to the expected rendering.
It may appear somewhat modest to some observers, but is a significant color error which would affect color-critical work.
Summary
HDR AVIF shows unexpected color
Additional Data
Category: UI
Chrome Channel: Stable
Regression: N/A