[Autofill] Fill from the last digits when filling a phone number with a maximum length.

components/autofill/core/browser/autofill_field.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/autofill/core/browser/autofill_field.h"
 
 #include <stdint.h>
 
 #include "base/command_line.h"
 #include "base/i18n/string_search.h"
@@ skipping 513 matching lines @@
                                   FormFieldData* field_data) {
   AutofillType type = field.Type();
 
   // Don't fill if autocomplete=off is set on |field| on desktop for non credit
   // card related fields.
   if (!field.should_autocomplete && IsDesktopPlatform() &&
       (type.group() != CREDIT_CARD)) {
     return false;
   }
 
-  if (type.GetStorableType() == PHONE_HOME_NUMBER) {
+  if (type.GetStorableType() == PHONE_HOME_NUMBER ||
+      type.GetStorableType() == PHONE_HOME_WHOLE_NUMBER) {
     FillPhoneNumberField(field, value, field_data);
     return true;
   } else if (field_data->form_control_type == "select-one") {
     return FillSelectControl(type, value, app_locale, field_data);
   } else if (field_data->form_control_type == "month") {
     return FillMonthControl(value, field_data);
   } else if (type.GetStorableType() == ADDRESS_HOME_STREET_ADDRESS) {
     FillStreetAddress(value, address_language_code, field_data);
     return true;
   } else if (type.GetStorableType() == CREDIT_CARD_NUMBER) {
     FillCreditCardNumberField(field, value, field_data);
     return true;
   }
 
   field_data->value = value;
   return true;
 }
 
 base::string16 AutofillField::GetPhoneNumberValue(
     const AutofillField& field,
     const base::string16& number,
     const FormFieldData& field_data) {
   // Check to see if the size field matches the "prefix" or "suffix" size.
   // If so, return the appropriate substring.
-  if (number.length() !=
-          PhoneNumber::kPrefixLength + PhoneNumber::kSuffixLength) {
-    return number;
+  if (number.length() ==

[Mathieu, 2016/01/26 14:43:01]

In which cases will the phone number size be 7? I'm thinking almost never. This
appears to be well intentioned logic that may never work. Let's put a TODO to
investigate if libphonenumber can help here. It can be done in a subsequent CL
since you are just refactoring this block.

[sebsg, 2016/01/26 23:56:26]

Done.

+      PhoneNumber::kPrefixLength + PhoneNumber::kSuffixLength) {
+    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);
+    }
   }
 
-  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);
+  // Check to see if the max length of the field matches the "city and number"
+  // size. If |number| exceeds that size, cut the first part to provide a valid
+  // number for the field.
+  if (field_data.max_length == PhoneNumber::kCityAndNumberLength &&

[Mathieu, 2016/01/26 14:43:02]

Why should we check for max_length == 10? It feels like it could be applicable
to other situations such as european phone numbers.

[sebsg, 2016/01/26 23:56:26]

I thought it was safer that way, to only cut the front part when we are pretty
sure it's gonna help. However by looking at the test, I realize that the result
would be bad anyway if we cut the from. However the current way is pretty much
always wrong (cutting the back). We might event catch some nice use case in
Europe like you mentioned now. Thanks

+      number.length() > field_data.max_length) {
+    return number.substr(number.length() - field_data.max_length,
+                         field_data.max_length);
   }
 
   return number;
 }
 
 // static
 bool AutofillField::FindValueInSelectControl(const FormFieldData& field,
                                              const base::string16& value,
                                              size_t* index) {
   l10n::CaseInsensitiveCompare compare;
@@ skipping 12 matching lines @@
     if (compare.StringsEqual(value_stripped, option_contents)) {
       if (index)
         *index = i;
       return true;
     }
   }
   return false;
 }
 
 }  // namespace autofill