Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1868003003:
Preserving first/middle/last names when an Autofill profile is submitted (Closed)
|
Created:
4 years, 8 months ago by tmartino Modified:
4 years, 8 months ago CC:
bondd+autofillwatch_chromium.org, chromium-reviews, dbeam+watch-options_chromium.org, estade+watch_chromium.org, jdonnelly+autofillwatch_chromium.org, michaelpg+watch-options_chromium.org, rouslan+autofill_chromium.org, vabr+watchlistautofill_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPreserving first/middle/last names when an Autofill profile is submitted
via the dialog.
BUG=593391
Committed: https://crrev.com/2706044fbb030e69675bd0ff28c0b332bd486c44
Cr-Commit-Position: refs/heads/master@{#388255}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Addressing mathp refactoring suggestions #Patch Set 3 : Cleaning up refactor cruft #
Total comments: 5
Patch Set 4 : Addressing further mathp nits #
Total comments: 2
Patch Set 5 : Moving constants inside method #
Total comments: 6
Patch Set 6 : Addressing stevenjb nits #
Messages
Total messages: 23 (9 generated)
tmartino@chromium.org changed reviewers: + mathp@chromium.org
https://codereview.chromium.org/1868003003/diff/1/chrome/browser/ui/webui/opt... File chrome/browser/ui/webui/options/autofill_options_handler.h (right): https://codereview.chromium.org/1868003003/diff/1/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/autofill_options_handler.h:109: // |prior_profile|. Does this run into i18n issues? If so, is there a library I should defer to?
Hi Tommy, some questions but overall looks good https://codereview.chromium.org/1868003003/diff/1/chrome/browser/ui/webui/opt... File chrome/browser/ui/webui/options/autofill_options_handler.cc (right): https://codereview.chromium.org/1868003003/diff/1/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/autofill_options_handler.cc:211: AutofillOptionsHandler::AutofillOptionsHandler() : personal_data_(NULL) {} nit: NULL -> nullptr everywhere in this file. https://codereview.chromium.org/1868003003/diff/1/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/autofill_options_handler.cc:485: profile.SetInfo(AutofillType(autofill::NAME_FULL), value, The origin of this bug is here. When profile.SetInfo is called, it ends up calling NameInfo::SetFullName[1], which splits the name very unintelligently. Note that at this point though, |value| is the proper full name as passed to the editor and back. I believe to make it a little less confusing, we should pass |value| as full name instead of |profile| to ShouldTransferNameComponents, and move this logic here. Also, if the name was unedited, I'm not sure we should call profile.SetInfo(NAME_FULL) since as we see it has bad behavior. Perhaps profile.SetRawInfo(NAME_FULL, value) since we know it hasn't changed from last time? [1] https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... https://codereview.chromium.org/1868003003/diff/1/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/autofill_options_handler.cc:536: autofill::NAME_MIDDLE_INITIAL, curious: I'm not familiar with middle initial. In your test, is it always filled alongside NAME_MIDDLE (i.e. the first letter is taken)? https://codereview.chromium.org/1868003003/diff/1/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/autofill_options_handler.cc:645: bool AutofillOptionsHandler::ShouldTransferNameComponents( How about renaming this to ProfileExpressesFullName[1] and integrating it into autofill_data_util as a static function? [1] Note: you may pick a better name :) https://codereview.chromium.org/1868003003/diff/1/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/autofill_options_handler.cc:651: base::string16 first_last = StringConcatenationHelper( should probably return early if the comparison is true to avoid extra work. if (!full_name.compare(first_last)) return true; etc. https://codereview.chromium.org/1868003003/diff/1/chrome/browser/ui/webui/opt... File chrome/browser/ui/webui/options/autofill_options_handler.h (right): https://codereview.chromium.org/1868003003/diff/1/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/autofill_options_handler.h:120: autofill::AutofillProfile* prior_profile_; initialize it to nullptr in the constructor
https://codereview.chromium.org/1868003003/diff/1/chrome/browser/ui/webui/opt... File chrome/browser/ui/webui/options/autofill_options_handler.cc (right): https://codereview.chromium.org/1868003003/diff/1/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/autofill_options_handler.cc:211: AutofillOptionsHandler::AutofillOptionsHandler() : personal_data_(NULL) {} On 2016/04/11 15:01:21, Mathieu Perreault wrote: > nit: NULL -> nullptr > > everywhere in this file. Done. https://codereview.chromium.org/1868003003/diff/1/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/autofill_options_handler.cc:485: profile.SetInfo(AutofillType(autofill::NAME_FULL), value, On 2016/04/11 15:01:21, Mathieu Perreault wrote: > The origin of this bug is here. When profile.SetInfo is called, it ends up > calling NameInfo::SetFullName[1], which splits the name very unintelligently. > Note that at this point though, |value| is the proper full name as passed to the > editor and back. I believe to make it a little less confusing, we should pass > |value| as full name instead of |profile| to ShouldTransferNameComponents, and > move this logic here. > > Also, if the name was unedited, I'm not sure we should call > profile.SetInfo(NAME_FULL) since as we see it has bad behavior. Perhaps > profile.SetRawInfo(NAME_FULL, value) since we know it hasn't changed from last > time? > > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... With the new flow I don't think it should be necessary to check if it's changed. SetInfo is only called if ProfileMatchesFullName is false, and that either means we have weird, crufty old data, or the user actually did change something. WDYT? https://codereview.chromium.org/1868003003/diff/1/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/autofill_options_handler.cc:536: autofill::NAME_MIDDLE_INITIAL, On 2016/04/11 15:01:21, Mathieu Perreault wrote: > curious: I'm not familiar with middle initial. In your test, is it always filled > alongside NAME_MIDDLE (i.e. the first letter is taken)? It appears that it's not stored anywhere: https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... I removed this setter call, but kept the references in ProfileMatchesFullName. https://codereview.chromium.org/1868003003/diff/1/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/autofill_options_handler.cc:645: bool AutofillOptionsHandler::ShouldTransferNameComponents( On 2016/04/11 15:01:21, Mathieu Perreault wrote: > How about renaming this to ProfileExpressesFullName[1] and integrating it into > autofill_data_util as a static function? > > [1] Note: you may pick a better name :) Done. https://codereview.chromium.org/1868003003/diff/1/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/autofill_options_handler.cc:651: base::string16 first_last = StringConcatenationHelper( On 2016/04/11 15:01:21, Mathieu Perreault wrote: > should probably return early if the comparison is true to avoid extra work. > > if (!full_name.compare(first_last)) > return true; > > etc. Done. https://codereview.chromium.org/1868003003/diff/1/chrome/browser/ui/webui/opt... File chrome/browser/ui/webui/options/autofill_options_handler.h (right): https://codereview.chromium.org/1868003003/diff/1/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/autofill_options_handler.h:120: autofill::AutofillProfile* prior_profile_; On 2016/04/11 at 15:01:21, Mathieu Perreault wrote: > initialize it to nullptr in the constructor Done. Also changed personal_data_ to use nullptr on the same line, for consistency.
Hi Tommy, just a few outstanding things and we're good to go. https://codereview.chromium.org/1868003003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/autofill_options_handler.cc (right): https://codereview.chromium.org/1868003003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/autofill_options_handler.cc:485: // information when they match the full name given. This is because it may nit: line wrapping got weird here! https://codereview.chromium.org/1868003003/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_data_util.cc (right): https://codereview.chromium.org/1868003003/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_data_util.cc:145: StringConcatenationHelper(profile.GetRawInfo(autofill::NAME_FIRST), Not sure this StringConcatenationHelper is helping. static const base::string16 kSpace(" "); static const base::string16 kPeriodSpace(". "); ... candidate = profile.GetRawInfo(autofill::NAME_FIRST) + kSpace + profile.GetRawInfo(autofill::NAME_MIDDLE); is clearer IMO https://codereview.chromium.org/1868003003/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_data_util.h (right): https://codereview.chromium.org/1868003003/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_data_util.h:28: // |profile|. nit: line wrap
https://codereview.chromium.org/1868003003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/autofill_options_handler.cc (right): https://codereview.chromium.org/1868003003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/autofill_options_handler.cc:485: // information when they match the full name given. This is because it may On 2016/04/13 at 18:12:24, Mathieu Perreault wrote: > nit: line wrapping got weird here! git cl format is not as smart as I thought it was. Fixed. https://codereview.chromium.org/1868003003/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_data_util.cc (right): https://codereview.chromium.org/1868003003/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_data_util.cc:145: StringConcatenationHelper(profile.GetRawInfo(autofill::NAME_FIRST), On 2016/04/13 at 18:12:24, Mathieu Perreault wrote: > Not sure this StringConcatenationHelper is helping. > > static const base::string16 kSpace(" "); > static const base::string16 kPeriodSpace(". "); > ... > candidate = profile.GetRawInfo(autofill::NAME_FIRST) + kSpace + profile.GetRawInfo(autofill::NAME_MIDDLE); > > is clearer IMO Agreed.
lgtm with nit Thanks! https://codereview.chromium.org/1868003003/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_data_util.cc (right): https://codereview.chromium.org/1868003003/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_data_util.cc:34: const base::string16 kSpace = base::ASCIIToUTF16(" "); nit: can you move this into ProfileMatchesFullName?
tmartino@chromium.org changed reviewers: + stevenjb@chromium.org
+stevenjb
Looks like dbeam and estade are both OOO for a few days, and you're next on the
list. Could you take a look for OWNERS on autofill_options_handler.{h, cc}?
https://codereview.chromium.org/1868003003/diff/60001/components/autofill/cor...
File components/autofill/core/browser/autofill_data_util.cc (right):
https://codereview.chromium.org/1868003003/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_data_util.cc:34: const base::string16
kSpace = base::ASCIIToUTF16(" ");
On 2016/04/15 at 16:59:40, Mathieu Perreault wrote:
> nit: can you move this into ProfileMatchesFullName?
Done.
lgtm with nits https://codereview.chromium.org/1868003003/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/autofill_options_handler.cc (right): https://codereview.chromium.org/1868003003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/autofill_options_handler.cc:103: : components_language_code); components_language_code ? components_language_code : ¬_used https://codereview.chromium.org/1868003003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/autofill_options_handler.cc:109: : components_language_code); ditto https://codereview.chromium.org/1868003003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/autofill_options_handler.cc:486: // name--e.g., when the last name contains a space, the first word would be nit: s/--/, /
https://codereview.chromium.org/1868003003/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/autofill_options_handler.cc (right): https://codereview.chromium.org/1868003003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/autofill_options_handler.cc:103: : components_language_code); On 2016/04/18 at 20:42:35, stevenjb wrote: > components_language_code ? components_language_code : ¬_used Done. https://codereview.chromium.org/1868003003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/autofill_options_handler.cc:109: : components_language_code); On 2016/04/18 at 20:42:35, stevenjb wrote: > ditto Done. https://codereview.chromium.org/1868003003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/autofill_options_handler.cc:486: // name--e.g., when the last name contains a space, the first word would be On 2016/04/18 at 20:42:35, stevenjb wrote: > nit: s/--/, / Done.
The CQ bit was checked by tmartino@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/1868003003/#ps100001 (title: "Addressing stevenjb nits")
The CQ bit was unchecked by tmartino@chromium.org
The CQ bit was checked by tmartino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1868003003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1868003003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tmartino@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1868003003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1868003003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Preserving first/middle/last names when an Autofill profile is submitted via the dialog. BUG=593391 ========== to ========== Preserving first/middle/last names when an Autofill profile is submitted via the dialog. BUG=593391 Committed: https://crrev.com/2706044fbb030e69675bd0ff28c0b332bd486c44 Cr-Commit-Position: refs/heads/master@{#388255} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/2706044fbb030e69675bd0ff28c0b332bd486c44 Cr-Commit-Position: refs/heads/master@{#388255} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
