Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
261993006:
Modified to allow to preserve two-word string in first-name and last-name in autofill profile. (Closed)
|
Created:
6 years, 7 months ago by Pritam Nikam Modified:
6 years, 6 months ago CC:
chromium-reviews, benquan, browser-components-watch_chromium.org, Dane Wallinga, dyu1, estade+watch_chromium.org, rouslan+autofillwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionModified to allow to preserve two-word string in first-name and last-name in autofill profile.
BUG=132332
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278058
Patch Set 1 #Patch Set 2 : Modified to allow to preserve two-word string in first-name and last-name in autofill profile. #Patch Set 3 : "Corrected parsing full_name_ to form first_, middle_ and last_ for autofill profile." #Patch Set 4 : #
Total comments: 10
Patch Set 5 : Incorporated review comments. #
Total comments: 22
Patch Set 6 : #
Total comments: 8
Patch Set 7 : #
Total comments: 36
Patch Set 8 : #
Total comments: 8
Patch Set 9 : Updated unit test cases to cover additional a check. #Patch Set 10 : Incorporated review comments and added unit tests. #
Total comments: 2
Patch Set 11 : #Patch Set 12 : Added case-insensativity handling for autofill nameinfo and unit-tests. #
Total comments: 4
Patch Set 13 : Added case-insensitivity handling and unit-tests. #
Total comments: 4
Patch Set 14 : Incorporated review comments and updated unit-tests. #
Total comments: 4
Patch Set 15 : Incorporated review comments and modified unit tests. #
Messages
Total messages: 50 (0 generated)
Please take a look for this patch.
Thanks for taking a look at this! However, this isn't the right approach. We *should* continue to tokenize the full name into its constituent parts. Otherwise, Chrome works less well in the common case. Instead, we should be more careful about how we add name variants. Specifically, we can check whether a NameInfo has redundant information in the full_name_ and first_, middle_, and last_ fields; or whether parsing the full_name_ gives different values than the first_, middle_, and last_ fields contain. For two NameInfo objects that have identical full names, we should always prefer to keep ones with more information, i.e. where parsing the full_name_ does not give the same results as are stored in first_, middle_, and last_.
On 2014/05/07 00:48:44, Ilya Sherman wrote:
> Thanks for taking a look at this! However, this isn't the right approach. We
> *should* continue to tokenize the full name into its constituent parts.
> Otherwise, Chrome works less well in the common case.
>
> Instead, we should be more careful about how we add name variants.
> Specifically, we can check whether a NameInfo has redundant information in the
> full_name_ and first_, middle_, and last_ fields; or whether parsing the
> full_name_ gives different values than the first_, middle_, and last_ fields
> contain. For two NameInfo objects that have identical full names, we should
> always prefer to keep ones with more information, i.e. where parsing the
> full_name_ does not give the same results as are stored in first_, middle_,
and
> last_.
Thanks for your inputs. I understand that we should not break existing behaviour
& keep tokenizing full_name so that comman cases shall work correctly, i.e.
first_token -> first_, last_token -> last_ & rest of the tokens to the middle_.
Moreover, cases where full_name_ can be break-down to first_, middle_ & last_
without any discrepancies.
Between, i'm bit suspicious on handling cases where:
A. composition of these fields first_, middle_ & last_ does not form an exact
match to full_name_, (for instance, a NameInfo profile with first_= "John",
middle_= "Q.", last_="Smith" & full_name_ = "John Quincy Smith" with all field
in a html-form). - shall these be treated as it is & shall we add bookkeeping
for full_name_ as well?
B. And from point you mentioned in your comments - "For two NameInfo objects
that have identical full names, we should always prefer to keep ones with more
information, i.e. where parsing the full_name_ does not give the same results as
are stored in first_, middle_, and last_" - here if i understand it correctly;
for instance; between NameInfo{"first/middle last1/last2"} &
NameInfo{"first/middle/last1 last2"}, we shall prefer second profile & discard
first?
On 2014/05/07 15:02:30, Pritam Nikam wrote:
> On 2014/05/07 00:48:44, Ilya Sherman wrote:
> > Thanks for taking a look at this! However, this isn't the right approach.
We
> > *should* continue to tokenize the full name into its constituent parts.
> > Otherwise, Chrome works less well in the common case.
> >
> > Instead, we should be more careful about how we add name variants.
> > Specifically, we can check whether a NameInfo has redundant information in
the
> > full_name_ and first_, middle_, and last_ fields; or whether parsing the
> > full_name_ gives different values than the first_, middle_, and last_ fields
> > contain. For two NameInfo objects that have identical full names, we should
> > always prefer to keep ones with more information, i.e. where parsing the
> > full_name_ does not give the same results as are stored in first_, middle_,
> and
> > last_.
>
> Thanks for your inputs. I understand that we should not break existing
behaviour
> & keep tokenizing full_name so that comman cases shall work correctly, i.e.
> first_token -> first_, last_token -> last_ & rest of the tokens to the
middle_.
> Moreover, cases where full_name_ can be break-down to first_, middle_ & last_
> without any discrepancies.
>
>
> Between, i'm bit suspicious on handling cases where:
> A. composition of these fields first_, middle_ & last_ does not form an exact
> match to full_name_, (for instance, a NameInfo profile with first_= "John",
> middle_= "Q.", last_="Smith" & full_name_ = "John Quincy Smith" with all field
> in a html-form). - shall these be treated as it is & shall we add bookkeeping
> for full_name_ as well?
You'll note that chrome://settings/autofill allows you to enter multiple names
associated with a single address. That's how we'd handle the case of a user who
sometimes goes by "John Q. Smith" and other times goes by "John Quincy Smith".
> B. And from point you mentioned in your comments - "For two NameInfo objects
> that have identical full names, we should always prefer to keep ones with more
> information, i.e. where parsing the full_name_ does not give the same results
as
> are stored in first_, middle_, and last_" - here if i understand it correctly;
> for instance; between NameInfo{"first/middle last1/last2"} &
> NameInfo{"first/middle/last1 last2"}, we shall prefer second profile & discard
> first?
Correct.
The CQ bit was checked by pritam.nikam@samsung.com
The CQ bit was unchecked by pritam.nikam@samsung.com
I've submitted a new patch for tokenizing ful_name_ to get first_, middle_ & last_ name. If first_/middle_/last_ are non-empty & available in full_name_ string then we fill rest of the fields on parsing the full name into tokens. But cases where any of these fields wont match in full name, we keep that field as it is and fill rest of the feilds by tokenizing full_name_. (I'm doubtful regarding these cases ???) For instance, first_->"first", middle_->empty, last_->empty & full_name_->"first middle last", on SetFullName(), we fill middle_ & last_ using the full_name_ tokens (i.e. middle_-> "middle" & last_ -> "last"). Same way, first_->"first", middle_->empty, last_->empty & full_name_->"middle last", on SetFullName(), we fill middle_ & last_ using the full_name_ tokens( i.e. first_-> "first", middle_-> "middle" & last_ -> "last"). I've updated unit-testcases to give an idea on implementation. Please take a look on this patch.
Thanks. The ContactInfo class isn't the right place to make this fix, however. Rather, the method that is doing the overwriting is here: [ https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... ].
Thanks Ilya for input. I've explored this code excerpt, please correct my understanding - autofill::AutofillProfile::OverwriteWithOrAddTo() helps to overwrite any existing profile with new legitimate or *valid learnable profile* (ref: autofill::PersonalDataManager::ImportFormData & autofill::PersonalDataManager::IsValidLearnableProfile()) and eventually writes record to autofill_profile_names DB-table as well as to autofill DB-table, in name-value pair format. However, cases where user doesn't fill all form fields it "doesn't consider" a profile as valid learnable profile, and in such cases it will not hit autofill::AutofillProfile::OverwriteWithOrAddTo() & does not write to to autofill_profile_names DB, instead it writes it to just autofill db-table, in name-value pair format. [for instance, here, http://jsfiddle.net/9T63L/ only firstname, lastname & fullname, and leave everything else empty] Few queries I'm having: A. when user punches-in last_name_ alone for a profile to spilt it to first_, middle_ & last_ I thought, we have to do it in NameInfo, and same thing goes with a profile first_, last_ & last_name_, (Current implementation also have it inside ContactInfo -> autofill:NameInfo::SetFullName()) B. Moreover, the decision, which profile to prefer between any 2 profiles having identical full names, can be done in autofill::AutofillProfile::OverwriteWithOrAddTo() for *valid learnable profiles* & for "not a learnable profile* at some other place, (may be inside autofill_webdata_backend_impl.cc?) not sure exact place to change. C. If a NameInfo profile having first_, last_ & middle_ strings *not matching* to full_name_ string, shall we over-write first_, last_ & middle_ on parsing full_name_ for that NameInfo object? (e.g. first_ -> "firstname1" middle_ -> "middlename1", last_->"lastname1" & full_name_->"first2 middle2 last2", so shall it be stays as a. first_ -> "firstname1" middle_ -> "middlename1", last_->"lastname1" OR modified to b. first_ -> "first2" middle_ -> "middle2", last_->"last2" ) ->> in patch set #3 i was trying this out only & it does not holds logic to prefer between 2 profiles, as i was not clear what shall be the common place to add this logic.
Added patch set #4 for autofill profiles with identical full names, prefer to keep ones with more information in profile. Please have a look at this patch. e.g. Prefer NameInfo(first_ -> "firstname", middle_-> "", last->"middlename lastname") over NameInfo(first_ -> "firstname", middle_-> "middlename", last->"lastname").
https://codereview.chromium.org/261993006/diff/80001/components/autofill/core... File components/autofill/core/browser/autofill_profile.cc (right): https://codereview.chromium.org/261993006/diff/80001/components/autofill/core... components/autofill/core/browser/autofill_profile.cc:368: NameInfo importedProfile = profiles[0]; Why pass a vector if you're only going to use the first item? https://codereview.chromium.org/261993006/diff/80001/components/autofill/core... components/autofill/core/browser/autofill_profile.cc:373: namelist.push_back(importedProfile); If the existing profile has 10 names, none of which match the new name, this will add the new name 10 times. (Or am I missing something?) https://codereview.chromium.org/261993006/diff/80001/components/autofill/core... components/autofill/core/browser/autofill_profile.cc:382: importedProfile.GetRawInfo(NAME_MIDDLE).length()) I was thinking of something more like: base::string16 full_name = prof.GetRawInfo(NAME_FULL); NameInfo automatically_parsed_name; automatically_parsed_name.SetRawInfo(NAME_FULL, full_name); bool is_automatically_parseable = (prof == automatically_parsed_name); except with better variable names. Conceptually, if serializing a NameInfo into a full name string and then reparsing it gives a different result from what you started with, then we should keep that version of the name. Otherwise, we shouldn't. https://codereview.chromium.org/261993006/diff/80001/components/autofill/core... components/autofill/core/browser/autofill_profile.cc:390: } There are many style guide violations in this code. Please read through the style guide and see if you can spot some of the issues. I'm not pointing specific issues out for now because there are still major structural changes needed. https://codereview.chromium.org/261993006/diff/80001/components/autofill/core... File components/autofill/core/browser/contact_info.cc (right): https://codereview.chromium.org/261993006/diff/80001/components/autofill/core... components/autofill/core/browser/contact_info.cc:117: void NameInfo::SetFullName(const base::string16& full) { You shouldn't need to make any changes to this method.
On 2014/05/13 06:42:08, Pritam Nikam wrote: > Few queries I'm having: > A. when user punches-in last_name_ alone for a profile to spilt it to first_, > middle_ & last_ I thought, we have to do it in NameInfo, and same thing goes > with a profile first_, last_ & last_name_, (Current implementation also have it > inside ContactInfo -> autofill:NameInfo::SetFullName()) Sorry, I don't understand the question. Could you please try to rephrase and clarify? > B. Moreover, the decision, which profile to prefer between any 2 profiles having > identical full names, can be done in > autofill::AutofillProfile::OverwriteWithOrAddTo() for *valid learnable profiles* > & for "not a learnable profile* at some other place, (may be inside > autofill_webdata_backend_impl.cc?) not sure exact place to change. We don't need any changes for data that doesn't correspond to a learnable profile. Only learnable profiles should be saved as Autofill profiles, and this bug is just about Autofill profiles. Single-field autocomplete already works as expected in terms of tokenizing names, i.e. it doesn't try to do any parsing of the name. > C. If a NameInfo profile having first_, last_ & middle_ strings *not matching* > to full_name_ string, shall we over-write first_, last_ & middle_ on parsing > full_name_ for that NameInfo object? > (e.g. first_ -> "firstname1" middle_ -> "middlename1", last_->"lastname1" & > full_name_->"first2 middle2 last2", > so shall it be stays as a. first_ -> "firstname1" middle_ -> "middlename1", > last_->"lastname1" > OR modified to b. first_ -> "first2" middle_ -> "middle2", last_->"last2" ) ->> > in patch set #3 i was trying this out only & it does not holds logic to prefer > between 2 profiles, as i was not clear what shall be the common place to add > this logic. It should not be possible for the parsed name fragment strings not to recombine to the full_name_ string.
Thanks for review. Incorporated review comments, patch #5 is uploaded for review. Please have a look. https://codereview.chromium.org/261993006/diff/80001/components/autofill/core... File components/autofill/core/browser/autofill_profile.cc (right): https://codereview.chromium.org/261993006/diff/80001/components/autofill/core... components/autofill/core/browser/autofill_profile.cc:368: NameInfo importedProfile = profiles[0]; On 2014/05/15 22:21:27, Ilya Sherman wrote: > Why pass a vector if you're only going to use the first item? Done. https://codereview.chromium.org/261993006/diff/80001/components/autofill/core... components/autofill/core/browser/autofill_profile.cc:373: namelist.push_back(importedProfile); On 2014/05/15 22:21:27, Ilya Sherman wrote: > If the existing profile has 10 names, none of which match the new name, this > will add the new name 10 times. (Or am I missing something?) Done. https://codereview.chromium.org/261993006/diff/80001/components/autofill/core... components/autofill/core/browser/autofill_profile.cc:382: importedProfile.GetRawInfo(NAME_MIDDLE).length()) On 2014/05/15 22:21:27, Ilya Sherman wrote: > I was thinking of something more like: > > base::string16 full_name = prof.GetRawInfo(NAME_FULL); > NameInfo automatically_parsed_name; > automatically_parsed_name.SetRawInfo(NAME_FULL, full_name); > bool is_automatically_parseable = (prof == automatically_parsed_name); > > except with better variable names. Conceptually, if serializing a NameInfo into > a full name string and then reparsing it gives a different result from what you > started with, then we should keep that version of the name. Otherwise, we > shouldn't. Done. https://codereview.chromium.org/261993006/diff/80001/components/autofill/core... components/autofill/core/browser/autofill_profile.cc:390: } On 2014/05/15 22:21:27, Ilya Sherman wrote: > There are many style guide violations in this code. Please read through the > style guide and see if you can spot some of the issues. I'm not pointing > specific issues out for now because there are still major structural changes > needed. Done. https://codereview.chromium.org/261993006/diff/80001/components/autofill/core... File components/autofill/core/browser/contact_info.cc (right): https://codereview.chromium.org/261993006/diff/80001/components/autofill/core... components/autofill/core/browser/contact_info.cc:117: void NameInfo::SetFullName(const base::string16& full) { On 2014/05/15 22:21:27, Ilya Sherman wrote: > You shouldn't need to make any changes to this method. Done.
Incorporated review comments in new patch #5. Please have a look.
Thanks. Apologies for my delay in reviewing this code -- I've been traveling, and hence fell behind on code reviews. https://codereview.chromium.org/261993006/diff/100001/components/autofill/cor... File components/autofill/core/browser/autofill_profile.cc (right): https://codereview.chromium.org/261993006/diff/100001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:521: void AutofillProfile::SetPreferredNameInfo( nit: Please name this something more like "OverwriteOrAppendNames()" https://codereview.chromium.org/261993006/diff/100001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:522: const std::vector<NameInfo>& profiles) { nit: "profiles" -> "names" https://codereview.chromium.org/261993006/diff/100001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:523: std::vector<NameInfo> append_list; nit: Please name this something more like "names_to_append" https://codereview.chromium.org/261993006/diff/100001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:525: std::vector<NameInfo>::const_iterator itr = profiles.begin(); nit: Please declare this within the scope of the for loop. https://codereview.chromium.org/261993006/diff/100001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:525: std::vector<NameInfo>::const_iterator itr = profiles.begin(); nit: Please name this "it", as that is the common name used for iterators in the Autofill code. https://codereview.chromium.org/261993006/diff/100001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:527: for (; itr != profiles.end(); ++itr) { Is it ever possible for the imported profile to contain more than one name? (I'm actually not sure offhand, but my guess is that it is not. I encourage you to check.) If it's not possible, then please remove the outer for loop, and instead include a DCHECK to verify that there is only a single element in the vector. https://codereview.chromium.org/261993006/diff/100001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:532: for (size_t index = 0; index < name_.size(); index++) { nit: "index++" -> "++index" https://codereview.chromium.org/261993006/diff/100001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:533: NameInfo prof = name_[index]; nit: Please avoid using abbreviations when naming variables. https://codereview.chromium.org/261993006/diff/100001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:550: append_list.push_back(imported_profile); This isn't quite right, as this variant of the name might exist further along in the list. It seems like what you ought to do instead is to break out of the for loop if the name is already found in it. It would be really good to write some tests for this code. This is a good opportunity to practice test-driven development [1]: by writing the tests earlier, you're more likely to think about possible tricky cases, and then have test coverage that can tell you whether or not you've addressed the tricky cases. [1] http://en.wikipedia.org/wiki/Test-driven_development https://codereview.chromium.org/261993006/diff/100001/components/autofill/cor... File components/autofill/core/browser/autofill_profile.h (right): https://codereview.chromium.org/261993006/diff/100001/components/autofill/cor... components/autofill/core/browser/autofill_profile.h:71: void SetPreferredNameInfo(const std::vector<NameInfo>& profiles); nit: This should have private visibility. https://codereview.chromium.org/261993006/diff/100001/components/autofill/cor... File components/autofill/core/browser/contact_info.cc (right): https://codereview.chromium.org/261993006/diff/100001/components/autofill/cor... components/autofill/core/browser/contact_info.cc:44: last_ == info.last_); nit: "return !(*this == info);"
Submitted new patch to incorporated review comments and added few test-cases to cover possible known scenarios. Please have a look on patch #6. https://codereview.chromium.org/261993006/diff/100001/components/autofill/cor... File components/autofill/core/browser/autofill_profile.cc (right): https://codereview.chromium.org/261993006/diff/100001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:521: void AutofillProfile::SetPreferredNameInfo( On 2014/05/29 07:38:03, Ilya Sherman wrote: > nit: Please name this something more like "OverwriteOrAppendNames()" Done. https://codereview.chromium.org/261993006/diff/100001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:522: const std::vector<NameInfo>& profiles) { On 2014/05/29 07:38:03, Ilya Sherman wrote: > nit: "profiles" -> "names" Done. https://codereview.chromium.org/261993006/diff/100001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:523: std::vector<NameInfo> append_list; On 2014/05/29 07:38:03, Ilya Sherman wrote: > nit: Please name this something more like "names_to_append" Function restructured, hence not needed. https://codereview.chromium.org/261993006/diff/100001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:525: std::vector<NameInfo>::const_iterator itr = profiles.begin(); On 2014/05/29 07:38:03, Ilya Sherman wrote: > nit: Please declare this within the scope of the for loop. Function restructured, hence not needed. https://codereview.chromium.org/261993006/diff/100001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:525: std::vector<NameInfo>::const_iterator itr = profiles.begin(); On 2014/05/29 07:38:03, Ilya Sherman wrote: > nit: Please name this "it", as that is the common name used for iterators in the > Autofill code. Done. https://codereview.chromium.org/261993006/diff/100001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:527: for (; itr != profiles.end(); ++itr) { On 2014/05/29 07:38:03, Ilya Sherman wrote: > Is it ever possible for the imported profile to contain more than one name? > (I'm actually not sure offhand, but my guess is that it is not. I encourage you > to check.) If it's not possible, then please remove the outer for loop, and > instead include a DCHECK to verify that there is only a single element in the > vector. Done. 1. HTML Form submission can have maximum 1 name in valid autofill profile. 2. "chrome://settings/autofill" can have multiple name entries but it never hit function. 3. Autofill syncable profile may have multiple name entries (?), Not able to explore https://codereview.chromium.org/261993006/diff/100001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:532: for (size_t index = 0; index < name_.size(); index++) { On 2014/05/29 07:38:03, Ilya Sherman wrote: > nit: "index++" -> "++index" Done. https://codereview.chromium.org/261993006/diff/100001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:533: NameInfo prof = name_[index]; On 2014/05/29 07:38:03, Ilya Sherman wrote: > nit: Please avoid using abbreviations when naming variables. Function restructured, hence not needed. https://codereview.chromium.org/261993006/diff/100001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:550: append_list.push_back(imported_profile); On 2014/05/29 07:38:03, Ilya Sherman wrote: > This isn't quite right, as this variant of the name might exist further along in > the list. It seems like what you ought to do instead is to break out of the for > loop if the name is already found in it. > > It would be really good to write some tests for this code. This is a good > opportunity to practice test-driven development [1]: by writing the tests > earlier, you're more likely to think about possible tricky cases, and then have > test coverage that can tell you whether or not you've addressed the tricky > cases. > > [1] http://en.wikipedia.org/wiki/Test-driven_development Done. Added test-cases to cover identical full names as well as unique names. Ref: components/autofill/core/browser/autofill_profile_unittest.cc https://codereview.chromium.org/261993006/diff/100001/components/autofill/cor... File components/autofill/core/browser/autofill_profile.h (right): https://codereview.chromium.org/261993006/diff/100001/components/autofill/cor... components/autofill/core/browser/autofill_profile.h:71: void SetPreferredNameInfo(const std::vector<NameInfo>& profiles); On 2014/05/29 07:38:03, Ilya Sherman wrote: > nit: This should have private visibility. To provide unit-test this need to have public visibility. API renamed to OverwriteOrAppendNames() & added to unit-test suit. https://codereview.chromium.org/261993006/diff/100001/components/autofill/cor... File components/autofill/core/browser/contact_info.cc (right): https://codereview.chromium.org/261993006/diff/100001/components/autofill/cor... components/autofill/core/browser/contact_info.cc:44: last_ == info.last_); On 2014/05/29 07:38:03, Ilya Sherman wrote: > nit: "return !(*this == info);" Done.
Incorporated review comments in new patch #6. Please have a look.
https://codereview.chromium.org/261993006/diff/120001/components/autofill/cor... File components/autofill/core/browser/autofill_profile.cc (right): https://codereview.chromium.org/261993006/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:524: DCHECK(names.size() == 1); Thanks for listing the cases from which this method is reachable -- I had forgotten that the Sync code also calls OverwriteWithOrAddTo. It is definitely possible for a synced profile to have multiple name variants in it, so this DCHECK is incorrect. I apologize for leading you astray. https://codereview.chromium.org/261993006/diff/120001/components/autofill/cor... File components/autofill/core/browser/autofill_profile.h (right): https://codereview.chromium.org/261993006/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_profile.h:151: void OverwriteOrAppendNames(const std::vector<NameInfo>& names); This should remain private, as it is an implementation detail. You should test the public method that calls this one. https://codereview.chromium.org/261993006/diff/120001/components/autofill/cor... File components/autofill/core/browser/autofill_profile_unittest.cc (right): https://codereview.chromium.org/261993006/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_profile_unittest.cc:915: "12345678910"); nit: Please only set the relevant fields, i.e. the name fields. https://codereview.chromium.org/261993006/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_profile_unittest.cc:965: } This test code is somewhat fragile and hard to follow, because the output of one part of the test becomes the input to the next part. I'd recommend using something like the following structure to improve both clarity and resilience of the test code: struct TestCase { std::string starting_names[][3]; std::string additional_names[][3]; std::string expected_result[][3]; } test_cases = { // Identical name. {{{"Marion", "Mitchell", "Morrison"}}, {{"Marion", "Mitchell", "Morrison"}}, {{"Marion", "Mitchell", "Morrison"}}} // A parse that has a two-word last name should take precedence over a parse // that assumes the two names are a middle and a last name. {{{"Marion", "Mitchell", "Morrison"}}, {{"Marion", "", "Mitchell Morrison"}}, {{"Marion", "", "Mitchell Morrison"}}} {{{"Marion", "", "Mitchell Morrison"}}, {{"Marion", "Mitchell", "Morrison"}}, {{"Marion", "", "Mitchell Morrison"}}} // A parse that has a two-word first name should take precedence over a parse // that assumes the two names are a first and a middle name. {{{"Marion", "Mitchell", "Morrison"}}, {{"Marion Mitchell", "", "Morrison"}}, {{"Marion Mitchell", "", "Morrison"}}} {{{"Marion Mitchell", "", "Morrison"}}, {{"Marion", "Mitchell", "Morrison"}}, {{"Marion Mitchell", "", "Morrison"}}} // ..., including test cases that have multiple names in the starting_names // and in the additional_names list. }; for (int i = 0; i < ARRAYSIZE_UNSAFE(test_cases); ++i) { SCOPED_TRACE(base::StringPrintf("i: %" PRIuS, i)); // Construct the starting_profile. // Construct the additional_profile. starting_profile.OverwriteWithOrAddTo(additional_profile); // Verify the test expectations. } (Of course, if you have an idea for an even better structure for the test, feel free to implement that. Your goal should be to make it easy to follow the test code and to add new test cases.)
Thanks Ilya! Incorporated review comments & submitted new patch #7 for review. Please have a look. https://codereview.chromium.org/261993006/diff/120001/components/autofill/cor... File components/autofill/core/browser/autofill_profile.cc (right): https://codereview.chromium.org/261993006/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:524: DCHECK(names.size() == 1); On 2014/05/31 00:34:41, Ilya Sherman wrote: > Thanks for listing the cases from which this method is reachable -- I had > forgotten that the Sync code also calls OverwriteWithOrAddTo. It is definitely > possible for a synced profile to have multiple name variants in it, so this > DCHECK is incorrect. I apologize for leading you astray. Done. https://codereview.chromium.org/261993006/diff/120001/components/autofill/cor... File components/autofill/core/browser/autofill_profile.h (right): https://codereview.chromium.org/261993006/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_profile.h:151: void OverwriteOrAppendNames(const std::vector<NameInfo>& names); On 2014/05/31 00:34:41, Ilya Sherman wrote: > This should remain private, as it is an implementation detail. You should test > the public method that calls this one. Done. https://codereview.chromium.org/261993006/diff/120001/components/autofill/cor... File components/autofill/core/browser/autofill_profile_unittest.cc (right): https://codereview.chromium.org/261993006/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_profile_unittest.cc:915: "12345678910"); On 2014/05/31 00:34:41, Ilya Sherman wrote: > nit: Please only set the relevant fields, i.e. the name fields. Done. https://codereview.chromium.org/261993006/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_profile_unittest.cc:965: } On 2014/05/31 00:34:41, Ilya Sherman wrote: > This test code is somewhat fragile and hard to follow, because the output of one > part of the test becomes the input to the next part. I'd recommend using > something like the following structure to improve both clarity and resilience of > the test code: > > struct TestCase { > std::string starting_names[][3]; > std::string additional_names[][3]; > std::string expected_result[][3]; > } test_cases = { > // Identical name. > {{{"Marion", "Mitchell", "Morrison"}}, > {{"Marion", "Mitchell", "Morrison"}}, > {{"Marion", "Mitchell", "Morrison"}}} > // A parse that has a two-word last name should take precedence over a parse > // that assumes the two names are a middle and a last name. > {{{"Marion", "Mitchell", "Morrison"}}, > {{"Marion", "", "Mitchell Morrison"}}, > {{"Marion", "", "Mitchell Morrison"}}} > {{{"Marion", "", "Mitchell Morrison"}}, > {{"Marion", "Mitchell", "Morrison"}}, > {{"Marion", "", "Mitchell Morrison"}}} > // A parse that has a two-word first name should take precedence over a parse > // that assumes the two names are a first and a middle name. > {{{"Marion", "Mitchell", "Morrison"}}, > {{"Marion Mitchell", "", "Morrison"}}, > {{"Marion Mitchell", "", "Morrison"}}} > {{{"Marion Mitchell", "", "Morrison"}}, > {{"Marion", "Mitchell", "Morrison"}}, > {{"Marion Mitchell", "", "Morrison"}}} > // ..., including test cases that have multiple names in the starting_names > // and in the additional_names list. > }; > > for (int i = 0; i < ARRAYSIZE_UNSAFE(test_cases); ++i) { > SCOPED_TRACE(base::StringPrintf("i: %" PRIuS, i)); > > // Construct the starting_profile. > // Construct the additional_profile. > starting_profile.OverwriteWithOrAddTo(additional_profile); > // Verify the test expectations. > } > > > (Of course, if you have an idea for an even better structure for the test, feel > free to implement that. Your goal should be to make it easy to follow the test > code and to add new test cases.) Done.
Thanks Ilya! Incorporated review comments & submitted new patch #7 for review. Please have a look.
Thanks! https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... File components/autofill/core/browser/autofill_profile.cc (right): https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:523: std::vector<NameInfo> name_list(name_); nit: Let's call this "result" or something. Otherwise, it's a little hard to keep track of the distinction between "names" and "name_list". Alternately, you could rename the variable currently named |names| to |new_names| instead or |imported_names| instead. https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:527: NameInfo imported_profile_name = *it; nit: Let's drop "profile", and call this |imported_name| or |new_name". https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:528: NameInfo profile_name_with_parsed_tokens; nit: Let's call this something more like |heuristically_parsed_name|. https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:530: bool identical_full_names = false; Let's simplify the logic a bit by replacing these two booleans with a single "bool should_append_{imported,new}_name = true;". https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:538: // results as are stored in first_, middle_, and last_. Here's a suggested rewording of this comment. Of course, feel free to further refine my wording :) "The imported name has the same full name string as one of the existing names for this profile. Because full names are _heuristically_ parsed into {first, middle, last} name components, it's possible that either the existing name or the imported name was misparsed. Prefer to keep the name whose {first, middle, last} components do not match those computed by the heuristic parse, as this more likely represents the correct, user-input parse of the name." https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:548: } With the changes above, the body of this loop would become something like NameInfo current_name = name_[index]; if (current_name == imported_profile_name) { should_append_{imported,new}_name = false; break; } base::string16 full_name = current_name.GetRawInfo(NAME_FULL); if (full_name == imported_profile_name.GetRawInfo(NAME_FULL)) { // Comment. NameInfo heuristically_parsed_name; heuristically_parsed_name.SetRawInfo(NAME_FULL, full_name); if (current_name == heuristically_parsed_name) { name_list[index] = imported_profile_name; should_append_{imported,new}_name = false; break; } } https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:552: // Append unique names to the list nit: Please end the sentence with a period. https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:555: imported_profile_name != profile_name_with_parsed_tokens)) { With the changes above, this if-stmt's condition would become simply "if (should_append_{imported,new}_name)". https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:558: } Optional nit: I'd leave a blank line after this one. https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:615: } nit: No need for curly braces. https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... File components/autofill/core/browser/autofill_profile.h (right): https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile.h:197: // otherwise append to this list. nit: Suggested rephrasing. Of course, feel free to further refine my wording :) """ Appends unique names from |names| onto the |name_| list, dropping duplicates. If a name in |names| has the same full name representation as a name in |name_|, keeps the variant that has more information (i.e. is not reconstructible via a heuristic parse of the full name string). """ https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... File components/autofill/core/browser/autofill_profile_unittest.cc (right): https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile_unittest.cc:909: } TestCase; nit: "typedef struct { ... } TestCase" -> "struct TestCase { ... }". https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile_unittest.cc:936: // take precedence over a parse that assumes the two names middle and nit: "the two names middle" -> "two middle names" https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile_unittest.cc:943: {"Arthur Ignatius", "", "Conan Doyle"}}}; Please also add test cases where the starting profile and/or the additional profile contains multiple names. https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile_unittest.cc:943: {"Arthur Ignatius", "", "Conan Doyle"}}}; Please also add a test case where the names do not have the same full name string, i.e. the list of merged names is longer than either of the incoming lists. https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile_unittest.cc:943: {"Arthur Ignatius", "", "Conan Doyle"}}}; Please also include a test case for merging {"Marion Mitchell", "", "Morrison"} and {"Marion", "", "Mitchell Morrison"}. https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile_unittest.cc:968: // test OverwriteOrAppendNames() via public helper interface nit: "Merge the names from the |additional_profile| into the |starting_profile|". https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile_unittest.cc:979: } nit: Please leave a blank line after this one.
Thanks Ilya! I have incorporated review comments and new patch is ready for review. Please have a look. https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... File components/autofill/core/browser/autofill_profile.cc (right): https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:523: std::vector<NameInfo> name_list(name_); On 2014/06/03 00:25:32, Ilya Sherman wrote: > nit: Let's call this "result" or something. Otherwise, it's a little hard to > keep track of the distinction between "names" and "name_list". > > Alternately, you could rename the variable currently named |names| to > |new_names| instead or |imported_names| instead. Done. name_list -> result https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:527: NameInfo imported_profile_name = *it; On 2014/06/03 00:25:32, Ilya Sherman wrote: > nit: Let's drop "profile", and call this |imported_name| or |new_name". Done. https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:528: NameInfo profile_name_with_parsed_tokens; On 2014/06/03 00:25:32, Ilya Sherman wrote: > nit: Let's call this something more like |heuristically_parsed_name|. Done. https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:530: bool identical_full_names = false; On 2014/06/03 00:25:32, Ilya Sherman wrote: > Let's simplify the logic a bit by replacing these two booleans with a single > "bool should_append_{imported,new}_name = true;". Done. https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:538: // results as are stored in first_, middle_, and last_. On 2014/06/03 00:25:32, Ilya Sherman wrote: > Here's a suggested rewording of this comment. Of course, feel free to further > refine my wording :) > > "The imported name has the same full name string as one of the existing names > for this profile. Because full names are _heuristically_ parsed into {first, > middle, last} name components, it's possible that either the existing name or > the imported name was misparsed. Prefer to keep the name whose {first, middle, > last} components do not match those computed by the heuristic parse, as this > more likely represents the correct, user-input parse of the name." Done. https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:548: } On 2014/06/03 00:25:32, Ilya Sherman wrote: > With the changes above, the body of this loop would become something like > > NameInfo current_name = name_[index]; > if (current_name == imported_profile_name) { > should_append_{imported,new}_name = false; > break; > } > > base::string16 full_name = current_name.GetRawInfo(NAME_FULL); > if (full_name == imported_profile_name.GetRawInfo(NAME_FULL)) { > // Comment. > NameInfo heuristically_parsed_name; > heuristically_parsed_name.SetRawInfo(NAME_FULL, full_name); > if (current_name == heuristically_parsed_name) { > name_list[index] = imported_profile_name; > should_append_{imported,new}_name = false; > break; > } > } > Done. https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:552: // Append unique names to the list On 2014/06/03 00:25:32, Ilya Sherman wrote: > nit: Please end the sentence with a period. Done. https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:555: imported_profile_name != profile_name_with_parsed_tokens)) { On 2014/06/03 00:25:32, Ilya Sherman wrote: > With the changes above, this if-stmt's condition would become simply "if > (should_append_{imported,new}_name)". Done. https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:558: } On 2014/06/03 00:25:32, Ilya Sherman wrote: > Optional nit: I'd leave a blank line after this one. Done. In my opinion,below check is still required to avoid appending heuristically parsed names. (imported_profile_name != profile_name_with_parsed_tokens) Something like: if (should_append_imported_name && (imported_name != heuristically_parsed_name)) results.push_back(imported_name); https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:615: } On 2014/06/03 00:25:32, Ilya Sherman wrote: > nit: No need for curly braces. Done. https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... File components/autofill/core/browser/autofill_profile.h (right): https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile.h:197: // otherwise append to this list. On 2014/06/03 00:25:32, Ilya Sherman wrote: > nit: Suggested rephrasing. Of course, feel free to further refine my wording :) > > """ > Appends unique names from |names| onto the |name_| list, dropping duplicates. > If a name in |names| has the same full name representation as a name in |name_|, > keeps the variant that has more information (i.e. is not reconstructible via a > heuristic parse of the full name string). > """ Done. https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... File components/autofill/core/browser/autofill_profile_unittest.cc (right): https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile_unittest.cc:909: } TestCase; On 2014/06/03 00:25:32, Ilya Sherman wrote: > nit: "typedef struct { ... } TestCase" -> "struct TestCase { ... }". Done. https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile_unittest.cc:936: // take precedence over a parse that assumes the two names middle and On 2014/06/03 00:25:32, Ilya Sherman wrote: > nit: "the two names middle" -> "two middle names" Done. https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile_unittest.cc:943: {"Arthur Ignatius", "", "Conan Doyle"}}}; On 2014/06/03 00:25:32, Ilya Sherman wrote: > Please also add test cases where the starting profile and/or the additional > profile contains multiple names. Done. Added in end of the test case. Please have a look. https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile_unittest.cc:943: {"Arthur Ignatius", "", "Conan Doyle"}}}; On 2014/06/03 00:25:32, Ilya Sherman wrote: > Please also include a test case for merging {"Marion Mitchell", "", "Morrison"} > and {"Marion", "", "Mitchell Morrison"}. Done. Added in end of the test case. Please have a look. https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile_unittest.cc:943: {"Arthur Ignatius", "", "Conan Doyle"}}}; On 2014/06/03 00:25:32, Ilya Sherman wrote: > Please also add a test case where the names do not have the same full name > string, i.e. the list of merged names is longer than either of the incoming > lists. Done. Added in end of the test case. Please have a look. https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile_unittest.cc:968: // test OverwriteOrAppendNames() via public helper interface On 2014/06/03 00:25:32, Ilya Sherman wrote: > nit: "Merge the names from the |additional_profile| into the > |starting_profile|". Done. https://codereview.chromium.org/261993006/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_profile_unittest.cc:979: } On 2014/06/03 00:25:32, Ilya Sherman wrote: > nit: Please leave a blank line after this one. Done.
Thanks Ilya! I have incorporated review comments and new patch (#8) is ready for review. Please have a look.
Thanks, Pritam! This is getting really close :) https://codereview.chromium.org/261993006/diff/200001/components/autofill/cor... File components/autofill/core/browser/autofill_profile.cc (right): https://codereview.chromium.org/261993006/diff/200001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:559: (imported_name != heuristically_parsed_name)) What is the test case that would fail if this line were missing? (Moreover, if you decide that it is not needed, please consider whether there is a test case that you could add that would *fail* if this line were present.) https://codereview.chromium.org/261993006/diff/200001/components/autofill/cor... File components/autofill/core/browser/autofill_profile_unittest.cc (right): https://codereview.chromium.org/261993006/diff/200001/components/autofill/cor... components/autofill/core/browser/autofill_profile_unittest.cc:1058: } Please include these in the list of cases at the top of the test, by using multi-dimensional arrays as I previously suggested.
Thanks Ilya! I've tried to answer to the comments you mentioned. Sorry, but i believe, I'm missing something here. Please have a look at my comments. https://codereview.chromium.org/261993006/diff/200001/components/autofill/cor... File components/autofill/core/browser/autofill_profile.cc (right): https://codereview.chromium.org/261993006/diff/200001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:559: (imported_name != heuristically_parsed_name)) If this (imported_name != heuristically_parsed_name) check is missing then few of our test-cases will going to fail. e.g. // considering same test-case structure struct TestCase { std::string starting_names[3]; std::string additional_names[3]; std::string expected_result[3]; }; Below test cases will fail, as avoid this check will try to append additional names (from imported profile) to the |name_| list even if full names are _heuristically_ parsed into {first, middle, last} name components, and matching to full name for one of the exiting names. 1. {{"Marion Mitchell", "", "Morrison"}, {"Marion", "Mitchell", "Morrison"}, {"Marion Mitchell", "", "Morrison"}}, 2. {{"Marion", "", "Mitchell Morrison"}, {"Marion", "Mitchell", "Morrison"}, {"Marion", "", "Mitchell Morrison"}}, 3. {{"Arthur Ignatius", "", "Conan Doyle"}, {"Arthur", "Ignatius Conan", "Doyle"}, {"Arthur Ignatius", "", "Conan Doyle"}}, etc... hope I'm not confusing you. https://codereview.chromium.org/261993006/diff/200001/components/autofill/cor... File components/autofill/core/browser/autofill_profile_unittest.cc (right): https://codereview.chromium.org/261993006/diff/200001/components/autofill/cor... components/autofill/core/browser/autofill_profile_unittest.cc:1058: } As i understand adding multiple-name in starting & additional name in a single statically defined array won't be possible. And instead I have to define these structure case-by-case basis and keep a separate array of test-cases. Likewise, starting_names - 2, additional_names - 2, expected_result - 2/3/4 struct TestCase { std::string starting_names[2][3]; std::string additional_names[2][3]; std::string expected_result[4][3]; <-- 2 or 3 or 4 }; starting_names - 1, additional_names - 2, expected_result - 2/3 struct TestCase { std::string starting_names[3]; std::string additional_names[2][3]; std::string expected_result[3][3]; <-- 2 or 3 }; Dynamically it will be possible, but then we have add logic to populate test-cases on case-by-case basis, isn't it? or didn't comprehend it correctly. Please correct my understanding.
Thanks Ilya! I've tried to answer to the comments you mentioned. Sorry, but i believe, I'm missing something here. Please have a look at my comments. I've uploaded new patch (#9) for review to cover this (imported_name != heuristically_parsed_name) condition.
https://codereview.chromium.org/261993006/diff/200001/components/autofill/cor... File components/autofill/core/browser/autofill_profile.cc (right): https://codereview.chromium.org/261993006/diff/200001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:559: (imported_name != heuristically_parsed_name)) On 2014/06/04 06:54:44, Pritam Nikam wrote: > If this (imported_name != heuristically_parsed_name) check is missing then few > of our test-cases will going to fail. > > e.g. > > // considering same test-case structure > struct TestCase { > std::string starting_names[3]; > std::string additional_names[3]; > std::string expected_result[3]; > }; > > Below test cases will fail, as avoid this check will try to append additional > names (from imported profile) to the |name_| list even if full names are > _heuristically_ parsed into {first, middle, last} name components, and matching > to full name for one of the exiting names. > > 1. {{"Marion Mitchell", "", "Morrison"}, > {"Marion", "Mitchell", "Morrison"}, > {"Marion Mitchell", "", "Morrison"}}, > > 2. {{"Marion", "", "Mitchell Morrison"}, > {"Marion", "Mitchell", "Morrison"}, > {"Marion", "", "Mitchell Morrison"}}, > > 3. {{"Arthur Ignatius", "", "Conan Doyle"}, > {"Arthur", "Ignatius Conan", "Doyle"}, > {"Arthur Ignatius", "", "Conan Doyle"}}, > etc... > > hope I'm not confusing you. Sorry, you are of course correct that this check is needed in some cases -- I was thinking that heuristically_parsed_name would always be computed, even when the full names did not match, which would cause problems. I'd recommend moving this condition into the the code that handles identical full names. So, specifically, I'd move line 528 to be immediately above line 548, and then immediately below that include if (imported_name == heuristically_parsed_name) { should_append_imported_name = false; break; } The advantage of this structure is that it groups all of the code that deals with the case where the full names match. https://codereview.chromium.org/261993006/diff/200001/components/autofill/cor... File components/autofill/core/browser/autofill_profile_unittest.cc (right): https://codereview.chromium.org/261993006/diff/200001/components/autofill/cor... components/autofill/core/browser/autofill_profile_unittest.cc:1058: } On 2014/06/04 06:54:44, Pritam Nikam wrote: > As i understand adding multiple-name in starting & additional name in a single > statically defined array won't be possible. And instead I have to define these > structure case-by-case basis and keep a separate array of test-cases. > > Likewise, starting_names - 2, additional_names - 2, expected_result - 2/3/4 > struct TestCase { > std::string starting_names[2][3]; > std::string additional_names[2][3]; > std::string expected_result[4][3]; <-- 2 or 3 or 4 > }; > > starting_names - 1, additional_names - 2, expected_result - 2/3 > struct TestCase { > std::string starting_names[3]; > std::string additional_names[2][3]; > std::string expected_result[3][3]; <-- 2 or 3 > }; > > Dynamically it will be possible, but then we have add logic to populate > test-cases on case-by-case basis, isn't it? or didn't comprehend it correctly. > Please correct my understanding. Hmm, it looks like you're right that there's no way to do this prior to C++11, which we can't use yet in Chromium code. Bummer! However, we can still use std::vector for a similar effect; it's just a little clunkier. Specifically, you could do something like this: struct NameParts { NameParts(const std::string& first, const std::string& middle, const std::string& last) : first(first), middle(middle), last(last) {} std::string first; std::string middle; std::string last; }; struct TestCase { TestCase(const NameParts& starting_name, const NameParts& additional_name, const NameParts& expected_result) : starting_names(std::vector<NameParts>(1, starting_name)), additional_names(std::vector<NameParts>(1, additional_name)), expected_result(std::vector<NameParts>(1, expected_result)) {} TestCase(const std::vector<NameParts>& starting_names, const std::vector<NameParts>& additional_names, const std::vector<NameParts>& expected_result) : starting_names(starting_names), additional_names(additional_names), expected_result(expected_result) {} std::vector<NameParts> starting_names; std::vector<NameParts> additional_names; std::vector<NameParts> expected_result; }; std::vector<TestCase> test_cases; // Identical name. test_cases.push_back(TestCase(NameParts("Marion", "Mitchell", "Morrison"), NameParts("Marion", "Mitchell", "Morrison"), NameParts("Marion", "Mitchell", "Morrison"))); // A parse that has a two-word last name should take precedence over a // parse that assumes the two names are a middle and a last name. test_cases.push_back(TestCase(NameParts("Marion", "Mitchell", "Morrison"), NameParts("Marion", "", "Mitchell Morrison"), NameParts("Marion", "", "Mitchell Morrison"))); test_cases.push_back(TestCase(NameParts("Marion", "", "Mitchell Morrison"), NameParts("Marion", "Mitchell", "Morrison"), NameParts("Marion", "", "Mitchell Morrison")));
Thanks Ilya! Incorporated review comments and new patch set #10 is ready for review. Please have a look. https://codereview.chromium.org/261993006/diff/200001/components/autofill/cor... File components/autofill/core/browser/autofill_profile.cc (right): https://codereview.chromium.org/261993006/diff/200001/components/autofill/cor... components/autofill/core/browser/autofill_profile.cc:559: (imported_name != heuristically_parsed_name)) On 2014/06/04 22:38:27, Ilya Sherman wrote: > On 2014/06/04 06:54:44, Pritam Nikam wrote: > > If this (imported_name != heuristically_parsed_name) check is missing then few > > of our test-cases will going to fail. > > > > e.g. > > > > // considering same test-case structure > > struct TestCase { > > std::string starting_names[3]; > > std::string additional_names[3]; > > std::string expected_result[3]; > > }; > > > > Below test cases will fail, as avoid this check will try to append additional > > names (from imported profile) to the |name_| list even if full names are > > _heuristically_ parsed into {first, middle, last} name components, and > matching > > to full name for one of the exiting names. > > > > 1. {{"Marion Mitchell", "", "Morrison"}, > > {"Marion", "Mitchell", "Morrison"}, > > {"Marion Mitchell", "", "Morrison"}}, > > > > 2. {{"Marion", "", "Mitchell Morrison"}, > > {"Marion", "Mitchell", "Morrison"}, > > {"Marion", "", "Mitchell Morrison"}}, > > > > 3. {{"Arthur Ignatius", "", "Conan Doyle"}, > > {"Arthur", "Ignatius Conan", "Doyle"}, > > {"Arthur Ignatius", "", "Conan Doyle"}}, > > etc... > > > > hope I'm not confusing you. > > Sorry, you are of course correct that this check is needed in some cases -- I > was thinking that heuristically_parsed_name would always be computed, even when > the full names did not match, which would cause problems. I'd recommend moving > this condition into the the code that handles identical full names. So, > specifically, I'd move line 528 to be immediately above line 548, and then > immediately below that include > > if (imported_name == heuristically_parsed_name) { > should_append_imported_name = false; > break; > } > > The advantage of this structure is that it groups all of the code that deals > with the case where the full names match. Done. https://codereview.chromium.org/261993006/diff/200001/components/autofill/cor... File components/autofill/core/browser/autofill_profile_unittest.cc (right): https://codereview.chromium.org/261993006/diff/200001/components/autofill/cor... components/autofill/core/browser/autofill_profile_unittest.cc:1058: } On 2014/06/04 22:38:27, Ilya Sherman wrote: > On 2014/06/04 06:54:44, Pritam Nikam wrote: > > As i understand adding multiple-name in starting & additional name in a single > > statically defined array won't be possible. And instead I have to define these > > structure case-by-case basis and keep a separate array of test-cases. > > > > Likewise, starting_names - 2, additional_names - 2, expected_result - 2/3/4 > > struct TestCase { > > std::string starting_names[2][3]; > > std::string additional_names[2][3]; > > std::string expected_result[4][3]; <-- 2 or 3 or 4 > > }; > > > > starting_names - 1, additional_names - 2, expected_result - 2/3 > > struct TestCase { > > std::string starting_names[3]; > > std::string additional_names[2][3]; > > std::string expected_result[3][3]; <-- 2 or 3 > > }; > > > > Dynamically it will be possible, but then we have add logic to populate > > test-cases on case-by-case basis, isn't it? or didn't comprehend it correctly. > > Please correct my understanding. > > Hmm, it looks like you're right that there's no way to do this prior to C++11, > which we can't use yet in Chromium code. Bummer! > > However, we can still use std::vector for a similar effect; it's just a little > clunkier. Specifically, you could do something like this: > > struct NameParts { > NameParts(const std::string& first, > const std::string& middle, > const std::string& last) > : first(first), middle(middle), last(last) {} > > std::string first; > std::string middle; > std::string last; > }; > struct TestCase { > TestCase(const NameParts& starting_name, > const NameParts& additional_name, > const NameParts& expected_result) > : starting_names(std::vector<NameParts>(1, starting_name)), > additional_names(std::vector<NameParts>(1, additional_name)), > expected_result(std::vector<NameParts>(1, expected_result)) {} > TestCase(const std::vector<NameParts>& starting_names, > const std::vector<NameParts>& additional_names, > const std::vector<NameParts>& expected_result) > : starting_names(starting_names), > additional_names(additional_names), > expected_result(expected_result) {} > > std::vector<NameParts> starting_names; > std::vector<NameParts> additional_names; > std::vector<NameParts> expected_result; > }; > > std::vector<TestCase> test_cases; > > // Identical name. > test_cases.push_back(TestCase(NameParts("Marion", "Mitchell", "Morrison"), > NameParts("Marion", "Mitchell", "Morrison"), > NameParts("Marion", "Mitchell", "Morrison"))); > > // A parse that has a two-word last name should take precedence over a > // parse that assumes the two names are a middle and a last name. > test_cases.push_back(TestCase(NameParts("Marion", "Mitchell", "Morrison"), > NameParts("Marion", "", "Mitchell Morrison"), > NameParts("Marion", "", "Mitchell Morrison"))); > test_cases.push_back(TestCase(NameParts("Marion", "", "Mitchell Morrison"), > NameParts("Marion", "Mitchell", "Morrison"), > NameParts("Marion", "", "Mitchell Morrison"))); Done.
Thanks Ilya! Incorporated review comments and new patch set #10 is ready for review. Please have a look.
LGTM % a final nit. Thanks, Pritam! https://codereview.chromium.org/261993006/diff/250001/components/autofill/cor... File components/autofill/core/browser/autofill_profile_unittest.cc (right): https://codereview.chromium.org/261993006/diff/250001/components/autofill/cor... components/autofill/core/browser/autofill_profile_unittest.cc:1005: // Cases where merging 2 profiles with same fullnames but nit: "fullnames" -> "full names"
Thanks Ilya! I've updated the comments in new patch (#11). Please have a look. https://codereview.chromium.org/261993006/diff/250001/components/autofill/cor... File components/autofill/core/browser/autofill_profile_unittest.cc (right): https://codereview.chromium.org/261993006/diff/250001/components/autofill/cor... components/autofill/core/browser/autofill_profile_unittest.cc:1005: // Cases where merging 2 profiles with same fullnames but On 2014/06/09 22:11:52, Ilya Sherman wrote: > nit: "fullnames" -> "full names" Done.
Thanks Ilya! I've updated the comments in new patch (#11). Please have a look.
The CQ bit was checked by pritam.nikam@samsung.com
The CQ bit was unchecked by pritam.nikam@samsung.com
The CQ bit was checked by pritam.nikam@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pritam.nikam@samsung.com/261993006/270001
The CQ bit was unchecked by pritam.nikam@samsung.com
Hi Ilya, 2 test-cases were failing with this patch as case-insensateness was not handled. I've added a new patch (#12) with this handling along with new unit-test to verify. with my local components_unittests builds all unit-tests are getting pass. Please have a look.
https://codereview.chromium.org/261993006/diff/290001/components/autofill/cor... File components/autofill/core/browser/autofill_profile_unittest.cc (right): https://codereview.chromium.org/261993006/diff/290001/components/autofill/cor... components/autofill/core/browser/autofill_profile_unittest.cc:1008: // Cases where merging 2 profiles with same fullnames but nit: This should still be "full names" rather than "fullnames" ;) https://codereview.chromium.org/261993006/diff/290001/components/autofill/cor... File components/autofill/core/browser/contact_info.cc (right): https://codereview.chromium.org/261993006/diff/290001/components/autofill/cor... components/autofill/core/browser/contact_info.cc:40: StringToLowerASCII(last_) == StringToLowerASCII(info.last_)); operator== should be case-sensitive. If you want a case-insensitive comparison function, you should give it a name, like EqualsIgnoreCase() or CaseInsensitiveCompare().
Thanks Ilya! I've added a function named EqualsIgnoreCase() to handle case-insensitivity. Please have a look. https://codereview.chromium.org/261993006/diff/290001/components/autofill/cor... File components/autofill/core/browser/autofill_profile_unittest.cc (right): https://codereview.chromium.org/261993006/diff/290001/components/autofill/cor... components/autofill/core/browser/autofill_profile_unittest.cc:1008: // Cases where merging 2 profiles with same fullnames but On 2014/06/12 22:35:11, Ilya Sherman wrote: > nit: This should still be "full names" rather than "fullnames" ;) Done. https://codereview.chromium.org/261993006/diff/290001/components/autofill/cor... File components/autofill/core/browser/contact_info.cc (right): https://codereview.chromium.org/261993006/diff/290001/components/autofill/cor... components/autofill/core/browser/contact_info.cc:40: StringToLowerASCII(last_) == StringToLowerASCII(info.last_)); On 2014/06/12 22:35:11, Ilya Sherman wrote: > operator== should be case-sensitive. If you want a case-insensitive comparison > function, you should give it a name, like EqualsIgnoreCase() or > CaseInsensitiveCompare(). Done.
https://codereview.chromium.org/261993006/diff/310001/components/autofill/cor... File components/autofill/core/browser/contact_info.h (right): https://codereview.chromium.org/261993006/diff/310001/components/autofill/cor... components/autofill/core/browser/contact_info.h:26: bool operator!=(const NameInfo& info); Are these operators still needed, now that you've added EqualsIgnoreCase? https://codereview.chromium.org/261993006/diff/310001/components/autofill/cor... components/autofill/core/browser/contact_info.h:27: bool EqualsIgnoreCase(const NameInfo& info); nit: Please leave a blank line before this method declaration, and add a brief comment describing what it does.
Thanks Ilya for your inputs. Incorporated review comments with new patch & uploaded for review. Please have a look. https://codereview.chromium.org/261993006/diff/310001/components/autofill/cor... File components/autofill/core/browser/contact_info.h (right): https://codereview.chromium.org/261993006/diff/310001/components/autofill/cor... components/autofill/core/browser/contact_info.h:26: bool operator!=(const NameInfo& info); On 2014/06/13 21:13:49, Ilya Sherman wrote: > Are these operators still needed, now that you've added EqualsIgnoreCase? Done. Removed both operators == & !=, as these are not being used now. https://codereview.chromium.org/261993006/diff/310001/components/autofill/cor... components/autofill/core/browser/contact_info.h:27: bool EqualsIgnoreCase(const NameInfo& info); On 2014/06/13 21:13:49, Ilya Sherman wrote: > nit: Please leave a blank line before this method declaration, and add a brief > comment describing what it does. Done.
Thanks, Pritam. LGTM with the following comments addressed: https://codereview.chromium.org/261993006/diff/330001/components/autofill/cor... File components/autofill/core/browser/contact_info.h (right): https://codereview.chromium.org/261993006/diff/330001/components/autofill/cor... components/autofill/core/browser/contact_info.h:26: // Compares |NameInfo| objects for |frist_|, |middle_| and |last_| names, nit: "frist" -> "first" https://codereview.chromium.org/261993006/diff/330001/components/autofill/cor... File components/autofill/core/browser/contact_info_unittest.cc (right): https://codereview.chromium.org/261993006/diff/330001/components/autofill/cor... components/autofill/core/browser/contact_info_unittest.cc:118: EXPECT_TRUE(a.EqualsIgnoreCase(b)); Please add a test case (or multiple test cases) where the comparison fails. Currently, if EqualsIgnoreCase() were implemented as "return true;", the test would still pass.
Thanks Ilya. I've uploaded new patch after incorporating review comments. Please have a look. https://codereview.chromium.org/261993006/diff/330001/components/autofill/cor... File components/autofill/core/browser/contact_info.h (right): https://codereview.chromium.org/261993006/diff/330001/components/autofill/cor... components/autofill/core/browser/contact_info.h:26: // Compares |NameInfo| objects for |frist_|, |middle_| and |last_| names, On 2014/06/16 20:28:18, Ilya Sherman wrote: > nit: "frist" -> "first" Done. https://codereview.chromium.org/261993006/diff/330001/components/autofill/cor... File components/autofill/core/browser/contact_info_unittest.cc (right): https://codereview.chromium.org/261993006/diff/330001/components/autofill/cor... components/autofill/core/browser/contact_info_unittest.cc:118: EXPECT_TRUE(a.EqualsIgnoreCase(b)); On 2014/06/16 20:28:18, Ilya Sherman wrote: > Please add a test case (or multiple test cases) where the comparison fails. > Currently, if EqualsIgnoreCase() were implemented as "return true;", the test > would still pass. Done.
The CQ bit was checked by pritam.nikam@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pritam.nikam@samsung.com/261993006/350001
Bot failed for : android_dbg_triggered_tests Failed slave_steps failed androidwebview_instrumentation_tests. C 139.708s Main CRASH (1 tests): [org.chromium.android_webview.test.ClientOnPageFinishedTest#testOnPageFinishedCalledAfterError] These does not seems to be related to my changes. Are these existing failures in current chromoium android bot?
Message was sent while issue was closed.
Change committed as 278058
Message was sent while issue was closed.
On 2014/06/18 13:58:36, Pritam Nikam wrote: > Bot failed for : android_dbg_triggered_tests > > Failed slave_steps failed androidwebview_instrumentation_tests. > C 139.708s Main CRASH (1 tests): > [org.chromium.android_webview.test.ClientOnPageFinishedTest#testOnPageFinishedCalledAfterError] > > These does not seems to be related to my changes. > Are these existing failures in current chromoium android bot? Yeah, unfortunately, it's normal to see some flaky failures on a few bots. As you can see, though, the infrastructure will often work around these issues by retrying failed runs.
Message was sent while issue was closed.
On 2014/06/18 17:04:24, Ilya Sherman wrote: > On 2014/06/18 13:58:36, Pritam Nikam wrote: > > Bot failed for : android_dbg_triggered_tests > > > > Failed slave_steps failed androidwebview_instrumentation_tests. > > C 139.708s Main CRASH (1 tests): > > > [org.chromium.android_webview.test.ClientOnPageFinishedTest#testOnPageFinishedCalledAfterError] > > > > These does not seems to be related to my changes. > > Are these existing failures in current chromoium android bot? > > Yeah, unfortunately, it's normal to see some flaky failures on a few bots. As > you can see, though, the infrastructure will often work around these issues by > retrying failed runs. Thanks Ilya! It was a great learning. Many thanks for your guidance & forbearance. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
