Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
442403002:
Adjust displayed phone number for prefix/suffix case. (Closed)
|
Created:
6 years, 4 months ago by ziran.sun Modified:
6 years, 3 months ago Reviewers:
Ilya Sherman 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 Project:
chromium Visibility:
Public. |
DescriptionAdjust displayed phone number for prefix/suffix case.
R=isherman@chromium.org
BUG=84200
Committed: https://crrev.com/3c26fc7bc6413a5343f6f2cf2009b2eb8e07f347
Cr-Commit-Position: refs/heads/master@{#296162}
Patch Set 1 #Patch Set 2 : Add test code. #Patch Set 3 : Remove duplicate code. #
Total comments: 23
Patch Set 4 : Upload code as per review comments. #
Total comments: 6
Patch Set 5 : Update code as per review comments. #
Total comments: 1
Patch Set 6 : Rebase #Patch Set 7 : Remove duplicate documentation. #
Messages
Total messages: 15 (1 generated)
Please review. Thanks!
Does this allow us to remove the similar logic in autofill_field.cc? https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...
On 2014/08/12 20:23:32, Ilya Sherman wrote: > Does this allow us to remove the similar logic in autofill_field.cc? > https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... I had been thinking about it but wasn't sure what's the best way to do it. I have the feeling that the exceptional cases handled in AutofillField::FillFormField() should all be considered in AutofillManager::OnQueryFormFieldAutofill as well. For example, taking a multiple-line address example, if we double click the input field for first line address, we'll see the full address in the suggestion popup. I wonder if it's okay that we introduce a new function in autofill_field.cc to extract correct value for these exceptional cases. AutofillManager::OnQueryFormFieldAutofill should be able to call this function. I guess there will be some changes on the existing functions in that file. Please let me have your thoughts and if it makes sense to look at it in a separate CL. Thanks!
On 2014/08/14 14:19:45, ziran.sun wrote: > On 2014/08/12 20:23:32, Ilya Sherman wrote: > > Does this allow us to remove the similar logic in autofill_field.cc? > > > https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... > > I had been thinking about it but wasn't sure what's the best way to do it. > > I have the feeling that the exceptional cases handled in > AutofillField::FillFormField() should all be considered in > AutofillManager::OnQueryFormFieldAutofill as well. For example, taking a > multiple-line address example, if we double click the input field for first line > address, we'll see the full address in the suggestion popup. > > I wonder if it's okay that we introduce a new function in autofill_field.cc to > extract correct value for these exceptional cases. > AutofillManager::OnQueryFormFieldAutofill should be able to call this function. > I guess there will be some changes on the existing functions in that file. > > Please let me have your thoughts and if it makes sense to look at it in a > separate CL. > > Thanks! Yes, I think something like that makes sense -- thanks. We should definitely not duplicate the code in the meantime, so it might be a good idea to expand this CL to cover that more general work. (Apologies for the delay in my reply -- the end of last week was super busy for me.)
On 2014/08/18 20:58:16, Ilya Sherman wrote: > On 2014/08/14 14:19:45, ziran.sun wrote: > > On 2014/08/12 20:23:32, Ilya Sherman wrote: > > > Does this allow us to remove the similar logic in autofill_field.cc? > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... > > > > I had been thinking about it but wasn't sure what's the best way to do it. > > > > I have the feeling that the exceptional cases handled in > > AutofillField::FillFormField() should all be considered in > > AutofillManager::OnQueryFormFieldAutofill as well. For example, taking a > > multiple-line address example, if we double click the input field for first > line > > address, we'll see the full address in the suggestion popup. > > > > I wonder if it's okay that we introduce a new function in autofill_field.cc to > > extract correct value for these exceptional cases. > > AutofillManager::OnQueryFormFieldAutofill should be able to call this > function. > > I guess there will be some changes on the existing functions in that file. > > > > Please let me have your thoughts and if it makes sense to look at it in a > > separate CL. > > > > Thanks! > > Yes, I think something like that makes sense -- thanks. We should definitely > not duplicate the code in the meantime, so it might be a good idea to expand > this CL to cover that more general work. > > (Apologies for the delay in my reply -- the end of last week was super busy for > me.) After a further check on the exceptional cases handled on AutofillField::FillFormField(), I'm denying my initial thoughts. For the four exceptional cases (phone number, <select>, <month> and address), since we do not support initiating autofill popup from <select> and <month> field, this has ruled these two cases out. For address, I noticed that from the autofill settings (chrome://settings/autofill) we are filling address in one field, rather than filling in multiple single lines as before (such as in chrome 34.0.1847.116), I'm actually not sure what's the expected UI should be now. In addition, it handles exceptional case for "ADDRESS_HOME_STREET_ADDRESS", and the issue I report earlier is for case "ADDRESS_HOME_LINE1" or"ADDRESS_HOME_LINE2" instead. This has left me with phone number case only. Thus, I only updated the code for phone number case as per your initial comments. Hope it's okay.
Update code as per review comments. Please review. Thanks!
https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... File components/autofill/core/browser/autofill_field.cc (right): https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... components/autofill/core/browser/autofill_field.cc:286: field_data->value = value; nit: You can shorten this to AutofillField::GetPhoneNumberValue( field, number, field_data, &field_data->value); https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... components/autofill/core/browser/autofill_field.cc:490: // substract the value accordingly. nit: Let's update this to something like "Check to see if the size field matches the "prefix" or "suffix" sizes. If so, return the appropriate substring." https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... components/autofill/core/browser/autofill_field.cc:493: PhoneNumber::kPrefixLength + PhoneNumber::kSuffixLength) { nit: Please indent this line by four more spaces. https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... File components/autofill/core/browser/autofill_field.h (right): https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... components/autofill/core/browser/autofill_field.h:78: static void GetPhoneNumberValue(const AutofillField& field, Please document this method. https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... components/autofill/core/browser/autofill_field.h:78: static void GetPhoneNumberValue(const AutofillField& field, Can this method return the value, rather than writing it into an outparam? https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... components/autofill/core/browser/autofill_field.h:80: FormFieldData* field_data, Can this be passed by const-reference rather than as a pointer? https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:528: } Can all of this be done in the method that assigns the labels to begin with? If not, please at least define a new local function called something like FixUpPhoneNumberLabels(), and move the code into that. https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... components/autofill/core/browser/autofill_manager_unittest.cc:1478: TEST_F(AutofillManagerTest, GetProfileSuggestionsForPrefixSuffixPhone) { nit: "PrefixSuffixPhone" -> "PhonePrefixOrSuffix" https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... components/autofill/core/browser/autofill_manager_unittest.cc:1487: const char* label; nit: Can this be "const char* const"? Ditto for the other two. https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... components/autofill/core/browser/autofill_manager_unittest.cc:1526: ASCIIToUTF16("1")}; Why are the labels "1", rather than the values that will be filled?
Updated code as per review comments. Please review. Thanks! https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... File components/autofill/core/browser/autofill_field.cc (right): https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... components/autofill/core/browser/autofill_field.cc:286: field_data->value = value; On 2014/09/03 00:55:49, Ilya Sherman wrote: > nit: You can shorten this to > > AutofillField::GetPhoneNumberValue( > field, number, field_data, &field_data->value); Done. https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... components/autofill/core/browser/autofill_field.cc:490: // substract the value accordingly. On 2014/09/03 00:55:49, Ilya Sherman wrote: > nit: Let's update this to something like "Check to see if the size field matches > the "prefix" or "suffix" sizes. If so, return the appropriate substring." Done. https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... components/autofill/core/browser/autofill_field.cc:493: PhoneNumber::kPrefixLength + PhoneNumber::kSuffixLength) { On 2014/09/03 00:55:49, Ilya Sherman wrote: > nit: Please indent this line by four more spaces. Done. https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... File components/autofill/core/browser/autofill_field.h (right): https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... components/autofill/core/browser/autofill_field.h:78: static void GetPhoneNumberValue(const AutofillField& field, On 2014/09/03 00:55:49, Ilya Sherman wrote: > Can this method return the value, rather than writing it into an outparam? Done. https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... components/autofill/core/browser/autofill_field.h:78: static void GetPhoneNumberValue(const AutofillField& field, On 2014/09/03 00:55:49, Ilya Sherman wrote: > Please document this method. Done. https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... components/autofill/core/browser/autofill_field.h:80: FormFieldData* field_data, On 2014/09/03 00:55:49, Ilya Sherman wrote: > Can this be passed by const-reference rather than as a pointer? Done. https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:528: } On 2014/09/03 00:55:49, Ilya Sherman wrote: > Can all of this be done in the method that assigns the labels to begin with? If > not, please at least define a new local function called something like > FixUpPhoneNumberLabels(), and move the code into that. I'm not sure this change is related to phone number labels, it is rather on the preview values. Fixes on displayed label will be introduced in patch for bug 78674 (http://code.google.com/p/chromium/issues/detail?id=78674). Do please let me know if I misunderstood anything though :). https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... components/autofill/core/browser/autofill_manager_unittest.cc:1478: TEST_F(AutofillManagerTest, GetProfileSuggestionsForPrefixSuffixPhone) { On 2014/09/03 00:55:49, Ilya Sherman wrote: > nit: "PrefixSuffixPhone" -> "PhonePrefixOrSuffix" Done. https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... components/autofill/core/browser/autofill_manager_unittest.cc:1487: const char* label; On 2014/09/03 00:55:49, Ilya Sherman wrote: > nit: Can this be "const char* const"? Ditto for the other two. Done. https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... components/autofill/core/browser/autofill_manager_unittest.cc:1526: ASCIIToUTF16("1")}; On 2014/09/03 00:55:49, Ilya Sherman wrote: > Why are the labels "1", rather than the values that will be filled? From my understanding, distinguishing label does not have to be the value that will filled in and normally it is not. For phone numbers that are divided into countrycode-areacode-prefix-suffix-extension, the labels don't seem being helpful as you pointed out in - https://code.google.com/p/chromium/issues/detail?id=78674 I'm looking at above bug and hope to solve label issue in this bug. This part of test code and other relevant part of test code in this test file will be updated once I have the solution for bug 78674 in. Hope it's okay.
https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:528: } On 2014/09/04 13:10:25, ziran.sun wrote: > On 2014/09/03 00:55:49, Ilya Sherman wrote: > > Can all of this be done in the method that assigns the labels to begin with? > If > > not, please at least define a new local function called something like > > FixUpPhoneNumberLabels(), and move the code into that. > > I'm not sure this change is related to phone number labels, it is rather on the > preview values. Fixes on displayed label will be introduced in patch for bug > 78674 (http://code.google.com/p/chromium/issues/detail?id=78674). Do please let > me know if I misunderstood anything though :). > Sorry, you're right that I got labels and values switched in my head. Apologies for my confusion. However, the general idea of this suggestion still stands: Please move this code into AutofillManager::GetProfileSuggestions() or PersonalDataManager::GetProfileSuggestions(), rather than having it appear in this higher-level method. https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... components/autofill/core/browser/autofill_manager_unittest.cc:1526: ASCIIToUTF16("1")}; On 2014/09/04 13:10:25, ziran.sun wrote: > On 2014/09/03 00:55:49, Ilya Sherman wrote: > > Why are the labels "1", rather than the values that will be filled? > > From my understanding, distinguishing label does not have to be the value that > will filled in and normally it is not. > > For phone numbers that are divided into > countrycode-areacode-prefix-suffix-extension, the labels don't seem being > helpful as you pointed out in - > https://code.google.com/p/chromium/issues/detail?id=78674 > > I'm looking at above bug and hope to solve label issue in this bug. This part of > test code and other relevant part of test code in this test file will be updated > once I have the solution for bug 78674 in. Hope it's okay. Yes, you're right. Sorry for my confusion. https://codereview.chromium.org/442403002/diff/60001/components/autofill/core... File components/autofill/core/browser/autofill_field.cc (right): https://codereview.chromium.org/442403002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_field.cc:487: // Check to see if the size field matches the "prefix" or "suffix" sizes. nit: "sizes" -> "size" (my bad for the typo in my previous comment) https://codereview.chromium.org/442403002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_field.cc:502: return value; nit: This method can be made a little bit cleaner by using early return statements, and thereby reducing the level of indentation: if (number.length() != PhoneNumber::kPrefixLength + PhoneNumber::kSuffixLength) { return number; } if (field.phone_part() == AutofillField::PHONE_PREFIX || field_data.max_length == PhoneNumber::kPrefixLength) { return number.substr(PhoneNumber::kPrefixOffset, PhoneNumber::kPrefixLength); } if (field.phone_part() == AutofillField::PHONE_SUFFIX || field_data.max_length == PhoneNumber::kSuffixLength) { return number.substr(PhoneNumber::kSuffixOffset, PhoneNumber::kSuffixLength); } return number; https://codereview.chromium.org/442403002/diff/60001/components/autofill/core... File components/autofill/core/browser/autofill_field.h (right): https://codereview.chromium.org/442403002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_field.h:79: // appropriate substring of |number| if it's for phone prefix or suffix field. nit: Suggested rephrasing; feel free to further improve: "Returns the phone number value that should be filled into the given |field|. The returned value might be |number|, or could possibly be a prefix or suffix of |number| if that's appropriate for the field."
Sorry for the late reply. I was away last week. Code has been updated as per your review comments. Please review. Thanks! https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/442403002/diff/40001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:528: } On 2014/09/11 05:32:14, Ilya Sherman wrote: > On 2014/09/04 13:10:25, ziran.sun wrote: > > On 2014/09/03 00:55:49, Ilya Sherman wrote: > > > Can all of this be done in the method that assigns the labels to begin with? > > > If > > > not, please at least define a new local function called something like > > > FixUpPhoneNumberLabels(), and move the code into that. > > > > I'm not sure this change is related to phone number labels, it is rather on > the > > preview values. Fixes on displayed label will be introduced in patch for bug > > 78674 (http://code.google.com/p/chromium/issues/detail?id=78674). Do please > let > > me know if I misunderstood anything though :). > > > > Sorry, you're right that I got labels and values switched in my head. Apologies > for my confusion. > > However, the general idea of this suggestion still stands: Please move this code > into AutofillManager::GetProfileSuggestions() or > PersonalDataManager::GetProfileSuggestions(), rather than having it appear in > this higher-level method. Done. https://codereview.chromium.org/442403002/diff/60001/components/autofill/core... File components/autofill/core/browser/autofill_field.cc (right): https://codereview.chromium.org/442403002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_field.cc:487: // Check to see if the size field matches the "prefix" or "suffix" sizes. On 2014/09/11 05:32:14, Ilya Sherman wrote: > nit: "sizes" -> "size" (my bad for the typo in my previous comment) Done. https://codereview.chromium.org/442403002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_field.cc:502: return value; On 2014/09/11 05:32:14, Ilya Sherman wrote: > nit: This method can be made a little bit cleaner by using early return > statements, and thereby reducing the level of indentation: > > > if (number.length() != > PhoneNumber::kPrefixLength + PhoneNumber::kSuffixLength) { > return number; > } > > if (field.phone_part() == AutofillField::PHONE_PREFIX || > field_data.max_length == PhoneNumber::kPrefixLength) { > return > number.substr(PhoneNumber::kPrefixOffset, PhoneNumber::kPrefixLength); > } > > if (field.phone_part() == AutofillField::PHONE_SUFFIX || > field_data.max_length == PhoneNumber::kSuffixLength) { > return > number.substr(PhoneNumber::kSuffixOffset, PhoneNumber::kSuffixLength); > } > > return number; Done. https://codereview.chromium.org/442403002/diff/60001/components/autofill/core... File components/autofill/core/browser/autofill_field.h (right): https://codereview.chromium.org/442403002/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_field.h:79: // appropriate substring of |number| if it's for phone prefix or suffix field. On 2014/09/11 05:32:14, Ilya Sherman wrote: > nit: Suggested rephrasing; feel free to further improve: "Returns the phone > number value that should be filled into the given |field|. The returned value > might be |number|, or could possibly be a prefix or suffix of |number| if that's > appropriate for the field." Done.
LGTM. Thanks, Ziran :) https://codereview.chromium.org/442403002/diff/80001/components/autofill/core... File components/autofill/core/browser/autofill_field.cc (right): https://codereview.chromium.org/442403002/diff/80001/components/autofill/core... components/autofill/core/browser/autofill_field.cc:483: // if that's appropriate for the field. nit: No need to duplicate the comment from the header here.
The CQ bit was checked by ziran.sun@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/442403002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as 98113782fdbaa381992d20b5bf3dda63a0671f07
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/3c26fc7bc6413a5343f6f2cf2009b2eb8e07f347 Cr-Commit-Position: refs/heads/master@{#296162} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
