Autofill: Remove fax number completely.

Autofill: Remove fax number completely.

BUG=81846
TEST=*

R=isherman@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=101602

[Ilya Sherman, 2011-09-15 03:47:08]

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_manager.h (left):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_manager.h:221: // |interaction_timestamp|,
updates the cached timestamp.  The latter check is
nit: Please preserve two spaces after a sentence closing period in Autofill code
(throughout).

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_metrics.cc (left):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_metrics.cc:45: FAX,
Please leave this in and add a comment that it is deprecated.  For the sake of
analysis, it's important to preserve the numeric values of the UMA constants.

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_type.h (right):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_type.h:27: PHONE_HOME,
nit: If it's not too much extra work to fold into this CL, it would be nice to
rename "PHONE_HOME" to just "PHONE".

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/form_st...
File chrome/browser/autofill/form_structure.cc (right):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/form_st...
chrome/browser/autofill/form_structure.cc:946: if (current_type_group ==
AutofillType::PHONE_HOME)
nit: You can get rid of the |current_type_group| variable as well, and just
inline the call into the if-stmt condition.

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/form_st...
File chrome/browser/autofill/form_structure_unittest.cc (left):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/form_st...
chrome/browser/autofill/form_structure_unittest.cc:415:
EXPECT_EQ(PHONE_FAX_NUMBER, form_structure->field(2)->heuristic_type());
nit: This line should be updated to PHONE_HOME_NUMBER rather than being removed.

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/persona...
File chrome/browser/autofill/personal_data_manager.cc (right):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager.cc:107: field_type !=
PHONE_HOME_NUMBER) {
nit: I think this can now all fit on one line.

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/phone_f...
File chrome/browser/autofill/phone_field.h (right):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/phone_f...
chrome/browser/autofill/phone_field.h:44: // Some field names are different for
phone.
nit: This comment no longer makes sense.  I think all of these methods can be
inlined into GetRegExp().

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/phone_f...
chrome/browser/autofill/phone_field.h:85: // TODO(jhawkins): This is probably
not necessary anymore.
Indeed, I think you can nix it.

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/phone_n...
File chrome/browser/autofill/phone_number.h (right):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/phone_n...
chrome/browser/autofill/phone_number.h:57: AutofillFieldType
GetWholeNumberType() const;
Let's just get rid of these, as well as the whole HomePhoneNumber class.

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/phone_n...
chrome/browser/autofill/phone_number.h:80: AutofillType::FieldTypeGroup
phone_group_;
There's no reason to store the |phone_group_| here any more.

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/phone_n...
chrome/browser/autofill/phone_number.h:100: AutofillType::FieldTypeGroup
phone_group_;
There's no reason to store the |phone_group_| here any more.

http://codereview.chromium.org/7892048/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/autofill_edit_address_overlay.html (left):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/autofill_edit_address_overlay.html:102: </div>
Let's have Phone and Email be side-by-side, since there's enough space for them
now.

http://codereview.chromium.org/7892048/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/autofill_edit_address_overlay.js (right):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/autofill_edit_address_overlay.js:161: &&
$('email-list').items.length <= 1;
extra '&&'

http://codereview.chromium.org/7892048/diff/1/chrome/browser/sync/protocol/au...
File chrome/browser/sync/protocol/autofill_specifics.proto (left):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/sync/protocol/au...
chrome/browser/sync/protocol/autofill_specifics.proto:45: optional string
phone_fax_whole_number = 14;
I don't think you're allowed to remove fields from the protobuf.  Someone from
the Sync team should double-check me on that though.

http://codereview.chromium.org/7892048/diff/1/chrome/browser/sync/protocol/pr...
File chrome/browser/sync/protocol/proto_value_conversions.cc (left):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/sync/protocol/pr...
chrome/browser/sync/protocol/proto_value_conversions.cc:203:
SET_STR(phone_fax_whole_number);
(If you're not changing the protobuf, this should remain as well.)

http://codereview.chromium.org/7892048/diff/1/chrome/browser/webdata/autofill...
File chrome/browser/webdata/autofill_table.cc (right):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/webdata/autofill...
chrome/browser/webdata/autofill_table.cc:1567: const std::string
autofill_profiles_is_too_big =
nit: Should this variable be named 'autofill_profile_is_too_big'?  (as long as
you're changing nearby code)

http://codereview.chromium.org/7892048/diff/1/chrome/browser/webdata/autofill...
chrome/browser/webdata/autofill_table.cc:1942: // Add the other bits: names,
emails and phone.
nit: 'phone' -> 'phone numbers'.  Also, I kind of prefer keeping the Oxford
comma.

http://codereview.chromium.org/7892048/diff/1/chrome/browser/webdata/autofill...
File chrome/browser/webdata/autofill_table.h (right):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/webdata/autofill...
chrome/browser/webdata/autofill_table.h:92: //                      number.
I don't think we want to have a 'type' at all anymore.

[James Hawkins, 2011-09-16 03:23:27]

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_manager.h (left):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_manager.h:221: // |interaction_timestamp|,
updates the cached timestamp.  The latter check is
On 2011/09/15 03:47:08, Ilya Sherman wrote:
> nit: Please preserve two spaces after a sentence closing period in Autofill
code
> (throughout).

Done.

I could swear we made the style rule of one space, but it turns out it hasn't
been codified.

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_metrics.cc (left):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_metrics.cc:45: FAX,
On 2011/09/15 03:47:08, Ilya Sherman wrote:
> Please leave this in and add a comment that it is deprecated.  For the sake of
> analysis, it's important to preserve the numeric values of the UMA constants.

Done.

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_type.h (right):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_type.h:27: PHONE_HOME,
On 2011/09/15 03:47:08, Ilya Sherman wrote:
> nit: If it's not too much extra work to fold into this CL, it would be nice to
> rename "PHONE_HOME" to just "PHONE".

I grepped it, and that would be a huge addition :-/

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/form_st...
File chrome/browser/autofill/form_structure.cc (right):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/form_st...
chrome/browser/autofill/form_structure.cc:946: if (current_type_group ==
AutofillType::PHONE_HOME)
On 2011/09/15 03:47:08, Ilya Sherman wrote:
> nit: You can get rid of the |current_type_group| variable as well, and just
> inline the call into the if-stmt condition.

Done.

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/form_st...
File chrome/browser/autofill/form_structure_unittest.cc (left):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/form_st...
chrome/browser/autofill/form_structure_unittest.cc:415:
EXPECT_EQ(PHONE_FAX_NUMBER, form_structure->field(2)->heuristic_type());
On 2011/09/15 03:47:08, Ilya Sherman wrote:
> nit: This line should be updated to PHONE_HOME_NUMBER rather than being
removed.

Done.

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/persona...
File chrome/browser/autofill/personal_data_manager.cc (right):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager.cc:107: field_type !=
PHONE_HOME_NUMBER) {
On 2011/09/15 03:47:08, Ilya Sherman wrote:
> nit: I think this can now all fit on one line.

Done.

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/phone_f...
File chrome/browser/autofill/phone_field.h (right):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/phone_f...
chrome/browser/autofill/phone_field.h:44: // Some field names are different for
phone.
On 2011/09/15 03:47:08, Ilya Sherman wrote:
> nit: This comment no longer makes sense.  I think all of these methods can be
> inlined into GetRegExp().

Done.

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/phone_f...
chrome/browser/autofill/phone_field.h:85: // TODO(jhawkins): This is probably
not necessary anymore.
On 2011/09/15 03:47:08, Ilya Sherman wrote:
> Indeed, I think you can nix it.

Done.

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/phone_n...
File chrome/browser/autofill/phone_number.h (right):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/phone_n...
chrome/browser/autofill/phone_number.h:57: AutofillFieldType
GetWholeNumberType() const;
On 2011/09/15 03:47:08, Ilya Sherman wrote:
> Let's just get rid of these, as well as the whole HomePhoneNumber class.

Done.

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/phone_n...
chrome/browser/autofill/phone_number.h:80: AutofillType::FieldTypeGroup
phone_group_;
On 2011/09/15 03:47:08, Ilya Sherman wrote:
> There's no reason to store the |phone_group_| here any more.

Done.

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/phone_n...
chrome/browser/autofill/phone_number.h:100: AutofillType::FieldTypeGroup
phone_group_;
On 2011/09/15 03:47:08, Ilya Sherman wrote:
> There's no reason to store the |phone_group_| here any more.

Done.

http://codereview.chromium.org/7892048/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/autofill_edit_address_overlay.html (left):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/autofill_edit_address_overlay.html:102: </div>
On 2011/09/15 03:47:08, Ilya Sherman wrote:
> Let's have Phone and Email be side-by-side, since there's enough space for
them
> now.

I'd like to leave that for a followup CL. This is large enough as is.

http://codereview.chromium.org/7892048/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/autofill_edit_address_overlay.js (right):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/autofill_edit_address_overlay.js:161: &&
$('email-list').items.length <= 1;
On 2011/09/15 03:47:08, Ilya Sherman wrote:
> extra '&&'

Done.

http://codereview.chromium.org/7892048/diff/1/chrome/browser/sync/protocol/au...
File chrome/browser/sync/protocol/autofill_specifics.proto (left):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/sync/protocol/au...
chrome/browser/sync/protocol/autofill_specifics.proto:45: optional string
phone_fax_whole_number = 14;
On 2011/09/15 03:47:08, Ilya Sherman wrote:
> I don't think you're allowed to remove fields from the protobuf.  Someone from
> the Sync team should double-check me on that though.

I'll mark it deprecated and touch base with them after.

http://codereview.chromium.org/7892048/diff/1/chrome/browser/sync/protocol/pr...
File chrome/browser/sync/protocol/proto_value_conversions.cc (left):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/sync/protocol/pr...
chrome/browser/sync/protocol/proto_value_conversions.cc:203:
SET_STR(phone_fax_whole_number);
On 2011/09/15 03:47:08, Ilya Sherman wrote:
> (If you're not changing the protobuf, this should remain as well.)

Done.

http://codereview.chromium.org/7892048/diff/1/chrome/browser/webdata/autofill...
File chrome/browser/webdata/autofill_table.cc (right):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/webdata/autofill...
chrome/browser/webdata/autofill_table.cc:1567: const std::string
autofill_profiles_is_too_big =
On 2011/09/15 03:47:08, Ilya Sherman wrote:
> nit: Should this variable be named 'autofill_profile_is_too_big'?  (as long as
> you're changing nearby code)

I don't know, but if so, then the credit_cards line above would need to be
changed as well. Leaving off of this CL.

http://codereview.chromium.org/7892048/diff/1/chrome/browser/webdata/autofill...
chrome/browser/webdata/autofill_table.cc:1942: // Add the other bits: names,
emails and phone.
On 2011/09/15 03:47:08, Ilya Sherman wrote:
> nit: 'phone' -> 'phone numbers'.  Also, I kind of prefer keeping the Oxford
> comma.

It's optional and less economical, but whatevs.

http://codereview.chromium.org/7892048/diff/1/chrome/browser/webdata/autofill...
File chrome/browser/webdata/autofill_table.h (right):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/webdata/autofill...
chrome/browser/webdata/autofill_table.h:92: //                      number.
On 2011/09/15 03:47:08, Ilya Sherman wrote:
> I don't think we want to have a 'type' at all anymore.

This can be done in a followup CL (which was my plan).  I'll have to verify, but
I believe we'll need to do a migration if we remove a column.

[Ilya Sherman, 2011-09-16 03:57:08]

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_type.h (right):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_type.h:27: PHONE_HOME,
On 2011/09/16 03:23:28, James Hawkins wrote:
> On 2011/09/15 03:47:08, Ilya Sherman wrote:
> > nit: If it's not too much extra work to fold into this CL, it would be nice
to
> > rename "PHONE_HOME" to just "PHONE".
> 
> I grepped it, and that would be a huge addition :-/

Ok, don't worry about it then.

http://codereview.chromium.org/7892048/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/autofill_edit_address_overlay.html (left):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/autofill_edit_address_overlay.html:102: </div>
On 2011/09/16 03:23:28, James Hawkins wrote:
> On 2011/09/15 03:47:08, Ilya Sherman wrote:
> > Let's have Phone and Email be side-by-side, since there's enough space for
> them
> > now.
> 
> I'd like to leave that for a followup CL. This is large enough as is.

Ok, but in that case please add a TODO or file a bug for this.

http://codereview.chromium.org/7892048/diff/1/chrome/browser/webdata/autofill...
File chrome/browser/webdata/autofill_table.h (right):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/webdata/autofill...
chrome/browser/webdata/autofill_table.h:92: //                      number.
On 2011/09/16 03:23:28, James Hawkins wrote:
> On 2011/09/15 03:47:08, Ilya Sherman wrote:
> > I don't think we want to have a 'type' at all anymore.
> 
> This can be done in a followup CL (which was my plan).  I'll have to verify,
but
> I believe we'll need to do a migration if we remove a column.

Ok, but in that case please add a TODO or file a bug for removing the column.  I
think you're right that a migration is needed.

http://codereview.chromium.org/7892048/diff/12002/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_ie_toolbar_import_win.cc (right):

http://codereview.chromium.org/7892048/diff/12002/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_ie_toolbar_import_win.cc:136:
PhoneNumber::PhoneCombineHelper home(AutofillType::PHONE_HOME);
I'd expect this not to compile, since the constructor no longer takes a
parameter.

http://codereview.chromium.org/7892048/diff/12002/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_ie_toolbar_import_win.cc:151: // We need to
store phone data in the variables, before building the whole
nit: "the variables" -> "|home|", which should probably just be called "phone".

http://codereview.chromium.org/7892048/diff/12002/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/7892048/diff/12002/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:181: 'browser/autofill/home_phone_number.h',
Can we remove the home_phone_number.* files entirely?

http://codereview.chromium.org/7892048/diff/12002/chrome/test/data/autofill/m...
File chrome/test/data/autofill/merge/input/email.in (left):

http://codereview.chromium.org/7892048/diff/12002/chrome/test/data/autofill/m...
chrome/test/data/autofill/merge/input/email.in:168: PHONE_FAX_WHOLE_NUMBER:
j@x.com
You should remove this whole profile, so that email.out does not change.

[James Hawkins, 2011-09-16 04:23:21]

http://codereview.chromium.org/7892048/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/autofill_edit_address_overlay.html (left):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/autofill_edit_address_overlay.html:102: </div>
On 2011/09/16 03:57:09, Ilya Sherman wrote:
> On 2011/09/16 03:23:28, James Hawkins wrote:
> > On 2011/09/15 03:47:08, Ilya Sherman wrote:
> > > Let's have Phone and Email be side-by-side, since there's enough space for
> > them
> > > now.
> > 
> > I'd like to leave that for a followup CL. This is large enough as is.
> 
> Ok, but in that case please add a TODO or file a bug for this.

Done.

http://codereview.chromium.org/7892048/diff/1/chrome/browser/webdata/autofill...
File chrome/browser/webdata/autofill_table.h (right):

http://codereview.chromium.org/7892048/diff/1/chrome/browser/webdata/autofill...
chrome/browser/webdata/autofill_table.h:92: //                      number.
On 2011/09/16 03:57:09, Ilya Sherman wrote:
> On 2011/09/16 03:23:28, James Hawkins wrote:
> > On 2011/09/15 03:47:08, Ilya Sherman wrote:
> > > I don't think we want to have a 'type' at all anymore.
> > 
> > This can be done in a followup CL (which was my plan).  I'll have to verify,
> but
> > I believe we'll need to do a migration if we remove a column.
> 
> Ok, but in that case please add a TODO or file a bug for removing the column. 
I
> think you're right that a migration is needed.

Done.

http://codereview.chromium.org/7892048/diff/12002/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_ie_toolbar_import_win.cc (right):

http://codereview.chromium.org/7892048/diff/12002/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_ie_toolbar_import_win.cc:136:
PhoneNumber::PhoneCombineHelper home(AutofillType::PHONE_HOME);
On 2011/09/16 03:57:09, Ilya Sherman wrote:
> I'd expect this not to compile, since the constructor no longer takes a
> parameter.

Done.

http://codereview.chromium.org/7892048/diff/12002/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_ie_toolbar_import_win.cc:151: // We need to
store phone data in the variables, before building the whole
On 2011/09/16 03:57:09, Ilya Sherman wrote:
> nit: "the variables" -> "|home|", which should probably just be called
"phone".

Done.

http://codereview.chromium.org/7892048/diff/12002/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/7892048/diff/12002/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:181: 'browser/autofill/home_phone_number.h',
On 2011/09/16 03:57:09, Ilya Sherman wrote:
> Can we remove the home_phone_number.* files entirely?

Done.

http://codereview.chromium.org/7892048/diff/12002/chrome/test/data/autofill/m...
File chrome/test/data/autofill/merge/input/email.in (left):

http://codereview.chromium.org/7892048/diff/12002/chrome/test/data/autofill/m...
chrome/test/data/autofill/merge/input/email.in:168: PHONE_FAX_WHOLE_NUMBER:
j@x.com
On 2011/09/16 03:57:09, Ilya Sherman wrote:
> You should remove this whole profile, so that email.out does not change.

Done.

[Ilya Sherman, 2011-09-16 05:03:53]

LGTM!  (Assuming green trybots, of course.)

http://codereview.chromium.org/7892048/diff/14009/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_ie_toolbar_import_win.cc (right):

http://codereview.chromium.org/7892048/diff/14009/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_ie_toolbar_import_win.cc:151: // We need to
store phone data in the variables, before building the whole
nit: "the variables" -> "|phone|"

[James Hawkins, 2011-09-17 00:03:47]

http://codereview.chromium.org/7892048/diff/14009/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_ie_toolbar_import_win.cc (right):

http://codereview.chromium.org/7892048/diff/14009/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_ie_toolbar_import_win.cc:151: // We need to
store phone data in the variables, before building the whole
On 2011/09/16 05:03:53, Ilya Sherman wrote:
> nit: "the variables" -> "|phone|"

Done.