Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(337)

Issue 772253003: Create an autofill Suggestion class (Closed)

Created:
4 years, 9 months ago by brettw
Modified:
4 years, 9 months ago
Reviewers:
Evan Stade
CC:
chromium-reviews, benquan, browser-components-watch_chromium.org, jam, darin-cc_chromium.org, Dane Wallinga, dyu1, estade+watch_chromium.org, Ilya Sherman, rouslan+autofillwatch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Create an autofill Suggestion class. There should be no functionality change. Previously autofill results were passed through as a set of three or four vectors: std::vector<base::string16>* popup_values, std::vector<base::string16>* popup_labels, std::vector<base::string16>* popup_icons std::vector<int>* identifiers all of which have to stay in sync. This makes it impossible to add metadata, function calls are ugly, and the code is error prone. As part of wallet credit card autofill, we'll need to add more metadata here. This patch replaces this with a new autofill::Suggestion struct containing all of the information, and pipes this around to all the affected places. It also adds a new SuggestionBackendID struct to replace the old GUIDPair typedef which makes the code clearer. There are three types of IDs for autofill suggestions which is kind of confusing: a GUID created by the database which is actually a GUID + an integer (GUIDPair typedef in the old code), an integer ID used by the frontend and IPC, and an intermediate integer used by the autofill manager to create the frontend int ID. This patch adds the GUID and frontend integer ID to the new Suggestion class as the backend_id and frontend_id. It is kind of confusing to have two different types of IDs in the same class, and previously most code dealt with only one or the other. But I think the code is much cleaner not having to keep separate parallel lists of IDs all over the place. Unified the naming in the autofill popup controller. It previously used name/subtext and now that it uses the standard Suggestion struct, I changed them to value/label to match everything else. Added constants for the card types used by RequestAutocomplete Added some additional stuff to the CreditCard class for wallet sync. Add additional members to the autofill table for wallet card manipulation. These are currently unused. Committed: https://crrev.com/5f88815a2b0e55bca6c6ee72afa051e65855906d Cr-Commit-Position: refs/heads/master@{#308457}

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : Add CheckSuggestions wrappers #

Patch Set 5 : chrome compiles #

Patch Set 6 : browser tests compile #

Patch Set 7 : merge #

Patch Set 8 : merge #

Patch Set 9 : Add frontned ID to suggestion #

Patch Set 10 : #

Patch Set 11 : self review 1 #

Patch Set 12 : self review 2 #

Total comments: 7

Patch Set 13 : Mac compiles #

Patch Set 14 : Windows compile #

Patch Set 15 : Review comments except for Suggestions member rename #

Patch Set 16 : merge #

Patch Set 17 : Compile error fixed #

Patch Set 18 : Android WebView #

Patch Set 19 : unit tests work, still needs more #

Patch Set 20 : Autofill popup controller unit tests pass, bug fix #

Patch Set 21 : browser tests compile #

Patch Set 22 : Android AOSP #

Patch Set 23 : Android fix #

Patch Set 24 : More Android #

Patch Set 25 : Missing Android include #

Patch Set 26 : Another Android fix #

Patch Set 27 : Bug fix for dialog controller #

Patch Set 28 : merge #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1361 lines, -1454 lines) Patch
M android_webview/native/aw_autofill_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 3 chunks +6 lines, -11 lines 0 comments Download
M android_webview/native/aw_autofill_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 4 chunks +15 lines, -18 lines 0 comments Download
M chrome/browser/autofill/content_autofill_driver_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +2 lines, -5 lines 0 comments Download
M chrome/browser/ui/android/autofill/autofill_popup_view_android.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 2 chunks +14 lines, -13 lines 0 comments Download
M chrome/browser/ui/autofill/autofill_dialog_controller_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +8 lines, -7 lines 0 comments Download
M chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 6 chunks +30 lines, -37 lines 0 comments Download
M chrome/browser/ui/autofill/autofill_popup_controller.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +10 lines, -15 lines 0 comments Download
M chrome/browser/ui/autofill/autofill_popup_controller_impl.h View 1 2 3 4 5 6 7 8 9 10 4 chunks +16 lines, -23 lines 0 comments Download
M chrome/browser/ui/autofill/autofill_popup_controller_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 24 chunks +139 lines, -121 lines 0 comments Download
M chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 12 chunks +106 lines, -108 lines 0 comments Download
M chrome/browser/ui/autofill/chrome_autofill_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -4 lines 0 comments Download
M chrome/browser/ui/autofill/chrome_autofill_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +2 lines, -5 lines 0 comments Download
M chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm View 1 2 3 4 5 6 7 8 9 10 11 12 6 chunks +14 lines, -12 lines 0 comments Download
M chrome/browser/ui/views/autofill/autofill_popup_view_views.cc View 1 2 3 4 5 6 7 8 9 10 5 chunks +19 lines, -16 lines 0 comments Download
M components/autofill.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +5 lines, -0 lines 0 comments Download
M components/autofill/content/browser/wallet/instrument.cc View 2 chunks +7 lines, -6 lines 0 comments Download
M components/autofill/content/browser/wallet/wallet_items.cc View 1 2 3 4 5 2 chunks +8 lines, -7 lines 0 comments Download
M components/autofill/core/browser/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +5 lines, -0 lines 0 comments Download
M components/autofill/core/browser/autocomplete_history_manager.h View 1 2 3 4 5 6 7 8 4 chunks +7 lines, -10 lines 0 comments Download
M components/autofill/core/browser/autocomplete_history_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +11 lines, -28 lines 0 comments Download
M components/autofill/core/browser/autocomplete_history_manager_unittest.cc View 1 2 3 4 5 6 7 8 9 4 chunks +10 lines, -14 lines 0 comments Download
M components/autofill/core/browser/autofill_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +2 lines, -4 lines 0 comments Download
M components/autofill/core/browser/autofill_external_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +5 lines, -16 lines 0 comments Download
M components/autofill/core/browser/autofill_external_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 5 chunks +57 lines, -105 lines 0 comments Download
M components/autofill/core/browser/autofill_external_delegate_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 13 chunks +53 lines, -109 lines 0 comments Download
M components/autofill/core/browser/autofill_manager.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +22 lines, -25 lines 0 comments Download
M components/autofill/core/browser/autofill_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 11 chunks +98 lines, -133 lines 0 comments Download
M components/autofill/core/browser/autofill_manager_unittest.cc View 1 2 3 4 5 6 7 8 65 chunks +235 lines, -411 lines 0 comments Download
M components/autofill/core/browser/autofill_metrics_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -3 lines 0 comments Download
A components/autofill/core/browser/autofill_sync_constants.h View 1 chunk +19 lines, -0 lines 0 comments Download
A components/autofill/core/browser/autofill_sync_constants.cc View 1 chunk +19 lines, -0 lines 0 comments Download
M components/autofill/core/browser/credit_card.h View 3 chunks +10 lines, -0 lines 0 comments Download
M components/autofill/core/browser/credit_card.cc View 1 chunk +9 lines, -0 lines 0 comments Download
M components/autofill/core/browser/personal_data_manager.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +5 lines, -16 lines 0 comments Download
M components/autofill/core/browser/personal_data_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 5 chunks +34 lines, -51 lines 0 comments Download
M components/autofill/core/browser/personal_data_manager_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +21 lines, -48 lines 0 comments Download
A components/autofill/core/browser/suggestion.h View 1 2 3 4 5 6 7 8 1 chunk +60 lines, -0 lines 0 comments Download
A components/autofill/core/browser/suggestion.cc View 1 2 3 4 5 6 7 8 9 1 chunk +54 lines, -0 lines 0 comments Download
A components/autofill/core/browser/suggestion_test_helpers.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +76 lines, -0 lines 0 comments Download
M components/autofill/core/browser/test_autofill_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -4 lines 0 comments Download
M components/autofill/core/browser/test_autofill_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -4 lines 0 comments Download
M components/autofill/core/browser/test_autofill_external_delegate.cc View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -6 lines 0 comments Download
M components/autofill/core/browser/webdata/autofill_table.h View 2 chunks +15 lines, -3 lines 0 comments Download
M components/autofill/core/browser/webdata/autofill_table.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +81 lines, -11 lines 0 comments Download
M components/password_manager/core/browser/password_autofill_manager.cc View 1 2 3 4 5 6 7 8 6 chunks +20 lines, -24 lines 0 comments Download
M components/password_manager/core/browser/password_autofill_manager_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 7 chunks +24 lines, -21 lines 0 comments Download

Messages

Total messages: 39 (17 generated)
brettw
I haven't done the Mac-specific stuff yet, doing a compile now, so a few more ...
4 years, 9 months ago (2014-12-10 22:28:03 UTC) #2
Evan Stade
went through all non-test files. https://codereview.chromium.org/772253003/diff/210001/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/772253003/diff/210001/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc#newcode2136 chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2136: for (size_t i = ...
4 years, 9 months ago (2014-12-10 23:11:14 UTC) #3
brettw
I did everything else except the value/label rename which I'm not very excited about doing. ...
4 years, 9 months ago (2014-12-12 00:44:14 UTC) #4
Evan Stade
On 2014/12/12 00:44:14, brettw wrote: > I did everything else except the value/label rename which ...
4 years, 9 months ago (2014-12-12 00:46:19 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/772253003/300001
4 years, 9 months ago (2014-12-12 03:45:43 UTC) #7
commit-bot: I haz the power
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_dbg_recipe/builds/30231) android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tests_recipe/builds/36897)
4 years, 9 months ago (2014-12-12 04:02:27 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/772253003/320001
4 years, 9 months ago (2014-12-12 05:32:32 UTC) #11
commit-bot: I haz the power
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/103371) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/7114) linux_chromium_gn_rel ...
4 years, 9 months ago (2014-12-12 05:37:07 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/772253003/360001
4 years, 9 months ago (2014-12-12 19:37:02 UTC) #15
commit-bot: I haz the power
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/103716) android_chromium_gn_compile_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromium_gn_compile_dbg/builds/25611) android_chromium_gn_compile_rel ...
4 years, 9 months ago (2014-12-12 20:02:28 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/772253003/400001
4 years, 9 months ago (2014-12-12 21:51:07 UTC) #19
commit-bot: I haz the power
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_dbg_recipe/builds/30595) android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tests_recipe/builds/37269)
4 years, 9 months ago (2014-12-12 22:11:06 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/772253003/420001
4 years, 9 months ago (2014-12-12 22:24:24 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/772253003/440001
4 years, 9 months ago (2014-12-12 22:52:35 UTC) #25
commit-bot: I haz the power
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tests_recipe/builds/37317)
4 years, 9 months ago (2014-12-12 23:20:02 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/772253003/460001
4 years, 9 months ago (2014-12-13 00:48:48 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/772253003/480001
4 years, 9 months ago (2014-12-13 01:06:03 UTC) #31
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/14489)
4 years, 9 months ago (2014-12-13 02:50:16 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/772253003/500001
4 years, 9 months ago (2014-12-15 21:16:08 UTC) #35
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/772253003/510001
4 years, 9 months ago (2014-12-15 21:58:00 UTC) #37
commit-bot: I haz the power
Committed patchset #28 (id:510001)
4 years, 9 months ago (2014-12-15 23:32:07 UTC) #38
commit-bot: I haz the power
4 years, 9 months ago (2014-12-15 23:32:48 UTC) #39
Message was sent while issue was closed.
Patchset 28 (id:??) landed as
https://crrev.com/5f88815a2b0e55bca6c6ee72afa051e65855906d
Cr-Commit-Position: refs/heads/master@{#308457}

Powered by Google App Engine
This is Rietveld 408576698