SingleFieldFormFiller causes unnecessary complexity [374266445]

Fixed

Bug

Status Update

No update yet.

Description

sc...@google.com

created issue #1

Oct 18, 2024 10:36AM

The SingleFieldFormFiller interface is implemented by four classes, but it's used nowhere:

The implementing classes are:

AutocompleteHistoryManager
MerchantPromoCodeManager
IbanManager
SingleFieldFormFillRouter

The need to implement the interface leads to unnecessary complexity:

Unused FormStructure* and AutofillField* parameters in some cases. Generally, these pointers are dangerous (e.g., issue 374250918), so even unused ones add complexity.
Empty functions, such as SingleFieldFormFillRouter::OnWillSubmitFormWithFields().
The OnSuggestionsReturnedCallback is a RepeatingCallback (even though it's called at most once) because the router needs to call multiple functions with the same signature.

Comments

ap...@google.com <ap...@google.com> #2Oct 19, 2024 12:05AM

Project: chromium/src
Branch: main
Author: Christoph Schwering <schwering@google.com>
Link: https://chromium-review.googlesource.com/5938289

[Autofill] Delete SingleFieldFormFiller and unused parameters

Expand for full commit details

[Autofill] Delete SingleFieldFormFiller and unused parameters

There's no reference to SingleFieldFormFiller outside of the
base-clauses of the deriving classes.

Since the interface complicates implementing and using the fillers
(see the bug), this CL removes the interface.

In the derived classes, it removes unused unused `FormStructure*`
and `AutofillField*` parameters that were previously required to
implement the interface.

It also turns the remaining `FormStructure*` and `AutofillField*`
parameters in `IbanManager` and `MerchantPromoCodeManager` into
references and moves the null-checks to the
`SingleFieldFormFillerRouter`.

The CL has have no side-effects.

Bug: 374266445
Change-Id: I114fe494f8decb8ff7c5a09532e04e92507d77b6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5938289
Reviewed-by: Jan Keitel <jkeitel@google.com>
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Commit-Queue: Christoph Schwering <schwering@google.com>
Cr-Commit-Position: refs/heads/main@{#1370903}

Files:

M chrome/browser/autofill/autocomplete_browsertest.cc
M components/autofill/core/browser/BUILD.gn
M components/autofill/core/browser/autocomplete_history_manager.cc
M components/autofill/core/browser/autocomplete_history_manager.h
M components/autofill/core/browser/autocomplete_history_manager_unittest.cc
M components/autofill/core/browser/browser_autofill_manager_unittest.cc
M components/autofill/core/browser/merchant_promo_code_manager.cc
M components/autofill/core/browser/merchant_promo_code_manager.h
M components/autofill/core/browser/merchant_promo_code_manager_unittest.cc
M components/autofill/core/browser/mock_autocomplete_history_manager.h
M components/autofill/core/browser/mock_merchant_promo_code_manager.h
M components/autofill/core/browser/mock_single_field_form_fill_router.h
M components/autofill/core/browser/payments/iban_manager.cc
M components/autofill/core/browser/payments/iban_manager.h
M components/autofill/core/browser/payments/iban_manager_unittest.cc
M components/autofill/core/browser/payments/test/mock_iban_manager.h
M components/autofill/core/browser/single_field_form_fill_router.cc
M components/autofill/core/browser/single_field_form_fill_router.h
M components/autofill/core/browser/single_field_form_fill_router_unittest.cc
D components/autofill/core/browser/single_field_form_filler.cc
D components/autofill/core/browser/single_field_form_filler.h

Hash: fa0bf09352a9961f8266070e5d969ae38b3af48b
Date: Sat Oct 19 00:04:00 2024

ap...@google.com <ap...@google.com> #3Oct 21, 2024 11:47AM

Project: chromium/src
Branch: main
Author: Christoph Schwering <schwering@google.com>
Link: https://chromium-review.googlesource.com/5938883

[Autofill] Make OnSuggestionsReturnedCallback a OnceCallback

Expand for full commit details

[Autofill] Make OnSuggestionsReturnedCallback a OnceCallback

This CL turns OnSuggestionsReturnedCallback into a OnceCallback to
correctly convey to the caller that the callback is called at most
one time.

It had been a RepeatingCallback because all single-field form fillers
implemented the same interface and therefore took the callback by
value.

Since they're not bound by that interface anymore, this CL changes
the parameters in the fillers to mutable references.

The CL has have no side-effects.

Bug: 374266445
Change-Id: I1acb87fff43e488a51be03fd6d908858892c1af1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5938883
Reviewed-by: Jan Keitel <jkeitel@google.com>
Auto-Submit: Christoph Schwering <schwering@google.com>
Commit-Queue: Christoph Schwering <schwering@google.com>
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1371300}

Files:

M chrome/browser/autofill/autocomplete_browsertest.cc
M components/autofill/core/browser/autocomplete_history_manager.cc
M components/autofill/core/browser/autocomplete_history_manager.h
M components/autofill/core/browser/autocomplete_history_manager_unittest.cc
M components/autofill/core/browser/merchant_promo_code_manager.cc
M components/autofill/core/browser/merchant_promo_code_manager.h
M components/autofill/core/browser/merchant_promo_code_manager_unittest.cc
M components/autofill/core/browser/mock_autocomplete_history_manager.h
M components/autofill/core/browser/mock_merchant_promo_code_manager.h
M components/autofill/core/browser/payments/iban_manager.cc
M components/autofill/core/browser/payments/iban_manager.h
M components/autofill/core/browser/payments/iban_manager_unittest.cc
M components/autofill/core/browser/payments/test/mock_iban_manager.h
M components/autofill/core/browser/single_field_form_fill_router.cc
M components/autofill/core/browser/single_field_form_fill_router.h

Hash: 916c7d8255333525bee7f9069cd6b6373a72b7ce
Date: Mon Oct 21 11:46:11 2024

ap...@google.com <ap...@google.com> #4Oct 21, 2024 11:49AM

Project: chromium/src
Branch: main
Author: Christoph Schwering <schwering@google.com>
Link: https://chromium-review.googlesource.com/5942363

[Autofill] Remove empty functions from single-field fillers

Expand for full commit details

[Autofill] Remove empty functions from single-field fillers

This CL removes the empty member functions that existed because
single-field fillers used to implement the same interface.

The CL has have no side-effects.

Bug: 374266445
Change-Id: Id3633806cfa3649034ff81e7162ffc74093c3a95
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5942363
Reviewed-by: Jan Keitel <jkeitel@google.com>
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Commit-Queue: Christoph Schwering <schwering@google.com>
Cr-Commit-Position: refs/heads/main@{#1371301}

Files:

M components/autofill/core/browser/autocomplete_history_manager.h
M components/autofill/core/browser/merchant_promo_code_manager.cc
M components/autofill/core/browser/merchant_promo_code_manager.h
M components/autofill/core/browser/mock_merchant_promo_code_manager.h
M components/autofill/core/browser/mock_single_field_form_fill_router.h
M components/autofill/core/browser/payments/iban_manager.h
M components/autofill/core/browser/payments/test/mock_iban_manager.h
M components/autofill/core/browser/single_field_form_fill_router.cc
M components/autofill/core/browser/single_field_form_fill_router.h
M components/autofill/core/browser/single_field_form_fill_router_unittest.cc

Hash: b5b041ef59455ff367df9bbfd74d2562ed820ca5
Date: Mon Oct 21 11:46:22 2024

ap...@google.com <ap...@google.com> #5Oct 21, 2024 09:51PM

Project: chromium/src
Branch: main
Author: Christoph Schwering <schwering@google.com>
Link: https://chromium-review.googlesource.com/5943128

[Autofill] Simplify AutocompleteHistoryManager request callbacks

Expand for full commit details

[Autofill] Simplify AutocompleteHistoryManager request callbacks

This CL replaces a std::map of base::RepeatingCallbacks with a simple
switch statement and direct function calls.

The CL has have no side-effects.

Bug: 374266445
Change-Id: I15307a654a7dc38fbcd54315f185eb093352b7a8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5943128
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Reviewed-by: Jan Keitel <jkeitel@google.com>
Commit-Queue: Christoph Schwering <schwering@google.com>
Cr-Commit-Position: refs/heads/main@{#1371674}

Files:

M components/autofill/core/browser/autocomplete_history_manager.cc
M components/autofill/core/browser/autocomplete_history_manager.h
M components/autofill/core/browser/mock_autocomplete_history_manager.cc

Hash: 7951eba3c66d25be5491dadbe2553dad3e9bbbd0
Date: Mon Oct 21 21:47:54 2024

ap...@google.com <ap...@google.com> #6Oct 21, 2024 10:12PM

Project: chromium/src
Branch: main
Author: Christoph Schwering <schwering@google.com>
Link: https://chromium-review.googlesource.com/5942364

[Autofill] Remove "Form" from SingleFieldFormFillRouter

Expand for full commit details

[Autofill] Remove "Form" from SingleFieldFormFillRouter

The CL renames SingleFieldFormFillRouter to SingleFieldFillRouter.

The CL has have no side-effects.

Bug: 374266445
Change-Id: I6d364123da5a56a25ae966433bf357d65e585e6a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5942364
Reviewed-by: Jan Keitel <jkeitel@google.com>
Commit-Queue: Christoph Schwering <schwering@google.com>
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1371689}

Files:

M chrome/browser/autofill/autocomplete_browsertest.cc
M components/autofill/core/browser/BUILD.gn
M components/autofill/core/browser/autocomplete_history_manager.cc
M components/autofill/core/browser/autocomplete_history_manager.h
M components/autofill/core/browser/autocomplete_history_manager_unittest.cc
M components/autofill/core/browser/autofill_external_delegate_unittest.cc
M components/autofill/core/browser/browser_autofill_manager.cc
M components/autofill/core/browser/browser_autofill_manager.h
M components/autofill/core/browser/browser_autofill_manager_test_api.h
M components/autofill/core/browser/browser_autofill_manager_unittest.cc
M components/autofill/core/browser/merchant_promo_code_manager.cc
M components/autofill/core/browser/merchant_promo_code_manager.h
M components/autofill/core/browser/merchant_promo_code_manager_unittest.cc
M components/autofill/core/browser/mock_autocomplete_history_manager.h
M components/autofill/core/browser/mock_merchant_promo_code_manager.h
A components/autofill/core/browser/mock_single_field_fill_router.cc
A components/autofill/core/browser/mock_single_field_fill_router.h
D components/autofill/core/browser/mock_single_field_form_fill_router.cc
D components/autofill/core/browser/mock_single_field_form_fill_router.h
M components/autofill/core/browser/payments/iban_manager.cc
M components/autofill/core/browser/payments/iban_manager.h
M components/autofill/core/browser/payments/iban_manager_unittest.cc
M components/autofill/core/browser/payments/test/mock_iban_manager.h
M components/autofill/core/browser/single_field_fill_router.cc
M components/autofill/core/browser/single_field_fill_router.h
M components/autofill/core/browser/single_field_fill_router_unittest.cc
M components/autofill/core/browser/test_browser_autofill_manager.cc

Hash: b46591a20c1123496acf1d59519c539c80f82aa7
Date: Mon Oct 21 22:08:46 2024

sc...@google.com <sc...@google.com> Oct 24, 2024 03:18PM

Marked as fixed.

ap...@google.com <ap...@google.com> #7Nov 12, 2024 11:25AM

Project: chromium/src
Branch: main
Author: Christoph Schwering <schwering@google.com>
Link: https://chromium-review.googlesource.com/6013478

[Autofill] Remove "Form" in "SingleFieldForm"

Expand for full commit details

[Autofill] Remove "Form" in "SingleFieldForm" 
 
There's no class called `*SingleFieldForm*` anymore: 
- crrev.com/c/5938289 deleted `SingleFieldFormFiller` 
- crrev.com/c/5942364 deleted the `Form` in `SingleFieldFormFillRouter` 
 
This CL deletes the `Form` also in function and enum constant names. 
 
The CL was created with ``sed -i 's/SingleFieldForm/SingleField/g'`. 
 
Bug: 374266445 
Change-Id: I671158e5cdeaa25dfef2824c44f7defb2f7f9cb6 
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6013478 
Reviewed-by: Norge Vizcay <vizcay@google.com> 
Commit-Queue: Christoph Schwering <schwering@google.com> 
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com> 
Cr-Commit-Position: refs/heads/main@{#1381688}

Files:

M components/autofill/README.md
M components/autofill/core/browser/autofill_feedback_data.cc
M components/autofill/core/browser/autofill_feedback_data_unittest.cc
M components/autofill/core/browser/browser_autofill_manager.cc
M components/autofill/core/browser/browser_autofill_manager.h
M components/autofill/core/browser/browser_autofill_manager_unittest.cc
M components/autofill/core/browser/form_parsing/form_field_parser.cc
M components/autofill/core/browser/form_parsing/form_field_parser.h
M components/autofill/core/browser/form_parsing/form_field_parser_unittest.cc
M components/autofill/core/browser/form_structure.cc
M components/autofill/core/browser/form_structure.h
M components/autofill/core/browser/form_structure_fuzzer.cc
M components/autofill/core/browser/form_structure_unittest.cc
M components/autofill/core/browser/metrics/form_events/form_event_logger_base.cc
M components/autofill/core/browser/metrics/log_event.h

Hash: faa7fc866a305c4383f2d6a8b823fb870b996879
Date: Tue Nov 12 11:21:39 2024

IssueTracker

This issue is a part of the Chromium tracker.