autofill names - dont parse when calling SetRawInfo(FULL_NAME)

autofill names - dont parse when calling SetRawInfo(FULL_NAME)

BUG=384640
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282712

[Ilya Sherman, 2014-06-24 07:21:37]

Thanks, Evan.

https://codereview.chromium.org/347183005/diff/20001/components/autofill/core...
File components/autofill/core/browser/autofill_profile.cc (right):

https://codereview.chromium.org/347183005/diff/20001/components/autofill/core...
components/autofill/core/browser/autofill_profile.cc:162: // collapses to full
name, area code collapses to full phone, etc.
Please update this comment.

https://codereview.chromium.org/347183005/diff/20001/components/autofill/core...
components/autofill/core/browser/autofill_profile.cc:582: // first name, last
name).
This comment needs more updating.

https://codereview.chromium.org/347183005/diff/20001/components/autofill/core...
components/autofill/core/browser/autofill_profile.cc:583:
CollapseCompoundFieldTypes(&field_types);
I'm concerned that treating first, middle, last, and full names as separate
lists will lead to incorrect behavior when merging names that partially match. 
For example, suppose I start with a profile that has the name "John Smith", and
then try to merge in a profile that has the name "Johnny Smith".  I think this
code will arrive at a new profile that has two first names, and only one last
name, which isn't a valid state.  Am I missing something that would prevent this
issue?

https://codereview.chromium.org/347183005/diff/20001/components/autofill/core...
File components/autofill/core/browser/personal_data_manager_unittest.cc (right):

https://codereview.chromium.org/347183005/diff/20001/components/autofill/core...
components/autofill/core/browser/personal_data_manager_unittest.cc:1100:
std::vector<base::string16> first_values, last_values, values;
nit: Perhaps "first_names", "last_names", and "emails"?

[Evan Stade, 2014-06-27 00:06:19]

https://codereview.chromium.org/347183005/diff/20001/components/autofill/core...
File components/autofill/core/browser/autofill_profile.cc (right):

https://codereview.chromium.org/347183005/diff/20001/components/autofill/core...
components/autofill/core/browser/autofill_profile.cc:162: // collapses to full
name, area code collapses to full phone, etc.
On 2014/06/24 07:21:37, Ilya Sherman wrote:
> Please update this comment.

reverted the change

https://codereview.chromium.org/347183005/diff/20001/components/autofill/core...
components/autofill/core/browser/autofill_profile.cc:582: // first name, last
name).
On 2014/06/24 07:21:37, Ilya Sherman wrote:
> This comment needs more updating.

reverted

https://codereview.chromium.org/347183005/diff/20001/components/autofill/core...
components/autofill/core/browser/autofill_profile.cc:583:
CollapseCompoundFieldTypes(&field_types);
On 2014/06/24 07:21:37, Ilya Sherman wrote:
> I'm concerned that treating first, middle, last, and full names as separate
> lists will lead to incorrect behavior when merging names that partially match.

> For example, suppose I start with a profile that has the name "John Smith",
and
> then try to merge in a profile that has the name "Johnny Smith".  I think this
> code will arrive at a new profile that has two first names, and only one last
> name, which isn't a valid state.  Am I missing something that would prevent
this
> issue?

Quite right, I didn't look at this closely enough and I sent the CL out for
review before looking at the unit test results. Sorry. The CL should be a bit
more fleshed out now.

I modified the loop below to try to improve its readability (for example, it was
pretty hard to see that if group == NAME, the previous 30 lines of code are
basically ignored).

[Ilya Sherman, 2014-06-27 06:25:08]

Note: It's easier for me to review your CL if you split off the rebase-induced
changes into a separate patch set.  That way, it's easy to diff between patch
sets to see what's changed, without having changes that landed as parts of other
CLs show up in the diff.

https://codereview.chromium.org/347183005/diff/40001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/347183005/diff/40001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:206:
g_browser_process->GetApplicationLocale());
Note that SetInfo() can override user-specified formatting, e.g. for phone
numbers.  Is that desirable?

https://codereview.chromium.org/347183005/diff/40001/components/autofill/core...
File components/autofill/core/browser/autofill_profile.cc (right):

https://codereview.chromium.org/347183005/diff/40001/components/autofill/core...
components/autofill/core/browser/autofill_profile.cc:193: : public
std::unary_function<const base::string16&, bool> {
FWIW, it looks like std::unary_function is deprecated in C++11, so it's not
clear to me whether there's much benefit to moving existing code over to use it.
 (I've never previously encountered it before, so I'm not familiar with the
details.)

https://codereview.chromium.org/347183005/diff/40001/components/autofill/core...
components/autofill/core/browser/autofill_profile.cc:606: continue;
Optional nit: I prefer to only use "continue" stmts when doing no work other
than skipping over an iteration of the loop.  In this case, I think an if/else
ladder would be clearer.

https://codereview.chromium.org/347183005/diff/40001/components/autofill/core...
components/autofill/core/browser/autofill_profile.cc:652: SetRawMultiInfo(*iter,
existing_values);
It seems like this ought to be outside the inner for loop.

https://codereview.chromium.org/347183005/diff/40001/components/autofill/core...
File components/autofill/core/browser/contact_info.cc (right):

https://codereview.chromium.org/347183005/diff/40001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:165: middle_.size() ==
info.middle_.size() &&
Why is this check necessary?  How can the strings be equal without being the
same size?  If somehow that's possible, why is it only middle names that need
special treatment?

https://codereview.chromium.org/347183005/diff/40001/components/autofill/core...
File components/autofill/core/browser/contact_info.h (right):

https://codereview.chromium.org/347183005/diff/40001/components/autofill/core...
components/autofill/core/browser/contact_info.h:26: // Compares |NameInfo|
objects for |first_|, |middle_| and |last_| names,
nit: "first_" and "last_" should be updated to "given_" and "family_" in this
comment.

https://codereview.chromium.org/347183005/diff/40001/components/autofill/core...
components/autofill/core/browser/contact_info.h:28: bool
ParsedBitsAreEqual(const NameInfo& info);
nit: "bits" makes me think of binary digits.  Perhaps "components" or "names" or
something like that?

[Evan Stade, 2014-06-28 01:01:35]

https://codereview.chromium.org/347183005/diff/40001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/347183005/diff/40001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:206:
g_browser_process->GetApplicationLocale());
On 2014/06/27 06:25:08, Ilya Sherman wrote:
> Note that SetInfo() can override user-specified formatting, e.g. for phone
> numbers.  Is that desirable?

When is SetInfo meant to be called? Isn't it for data that a user has typed,
whereas SetRawInfo is more like "copy without modifying"?

It seems like PhoneNumber is broken if we want to retain user-specified
formatting, but SetInfo overrides that formatting. I just tested with web
autofill, and we don't retain user formatting there because PDM uses SetInfo. We
do retain formatting for data entered in chrome://settings/autofill. I'm not
sure if we want these to differ, but I think this UI is more like a web form
than the settings dialog (which is only for power users).

https://codereview.chromium.org/347183005/diff/40001/components/autofill/core...
File components/autofill/core/browser/autofill_profile.cc (right):

https://codereview.chromium.org/347183005/diff/40001/components/autofill/core...
components/autofill/core/browser/autofill_profile.cc:193: : public
std::unary_function<const base::string16&, bool> {
On 2014/06/27 06:25:08, Ilya Sherman wrote:
> FWIW, it looks like std::unary_function is deprecated in C++11, so it's not
> clear to me whether there's much benefit to moving existing code over to use
it.
>  (I've never previously encountered it before, so I'm not familiar with the
> details.)

thanks for reminding me to remove this. It was only there as a part of a
slightly different approach that I originally attempted.

https://codereview.chromium.org/347183005/diff/40001/components/autofill/core...
components/autofill/core/browser/autofill_profile.cc:606: continue;
On 2014/06/27 06:25:08, Ilya Sherman wrote:
> Optional nit: I prefer to only use "continue" stmts when doing no work other
> than skipping over an iteration of the loop.  In this case, I think an if/else
> ladder would be clearer.

I'll pass on this nit, as I prefer less indentation for the meat of the loop.

https://codereview.chromium.org/347183005/diff/40001/components/autofill/core...
components/autofill/core/browser/autofill_profile.cc:652: SetRawMultiInfo(*iter,
existing_values);
On 2014/06/27 06:25:08, Ilya Sherman wrote:
> It seems like this ought to be outside the inner for loop.

good catch. This is the unfortunate kind of bug a test probably could never
catch, because the end state of *iter is correct.

https://codereview.chromium.org/347183005/diff/40001/components/autofill/core...
File components/autofill/core/browser/contact_info.cc (right):

https://codereview.chromium.org/347183005/diff/40001/components/autofill/core...
components/autofill/core/browser/contact_info.cc:165: middle_.size() ==
info.middle_.size() &&
On 2014/06/27 06:25:08, Ilya Sherman wrote:
> Why is this check necessary?  How can the strings be equal without being the
> same size?  If somehow that's possible, why is it only middle names that need
> special treatment?

this is totally not necessary. It was just me being paranoid  while I was
debugging a test failure, and then being forgetful when it came time to remove
it.

https://codereview.chromium.org/347183005/diff/40001/components/autofill/core...
File components/autofill/core/browser/contact_info.h (right):

https://codereview.chromium.org/347183005/diff/40001/components/autofill/core...
components/autofill/core/browser/contact_info.h:26: // Compares |NameInfo|
objects for |first_|, |middle_| and |last_| names,
On 2014/06/27 06:25:08, Ilya Sherman wrote:
> nit: "first_" and "last_" should be updated to "given_" and "family_" in this
> comment.

Done.

https://codereview.chromium.org/347183005/diff/40001/components/autofill/core...
components/autofill/core/browser/contact_info.h:28: bool
ParsedBitsAreEqual(const NameInfo& info);
On 2014/06/27 06:25:08, Ilya Sherman wrote:
> nit: "bits" makes me think of binary digits.  Perhaps "components" or "names"
or
> something like that?

Done.

https://codereview.chromium.org/347183005/diff/60001/components/autofill/core...
File components/autofill/core/browser/address.cc (left):

https://codereview.chromium.org/347183005/diff/60001/components/autofill/core...
components/autofill/core/browser/address.cc:179: if (storable_type ==
ADDRESS_HOME_STREET_ADDRESS && !value.empty() &&
this behavior was causing problems when I was trying to call SetInfo for data
entered via requestAutocomplete. I decided to delete this code because I don't
really agree with the logic behind it. "123 Main St, Apt 7" is a perfectly way
to type an address and doesn't need to be broken up into multiple address lines.

[Ilya Sherman, 2014-07-01 02:55:17]

LGTM, thanks.

https://codereview.chromium.org/347183005/diff/40001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/347183005/diff/40001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:206:
g_browser_process->GetApplicationLocale());
On 2014/06/28 01:01:35, Evan Stade wrote:
> On 2014/06/27 06:25:08, Ilya Sherman wrote:
> > Note that SetInfo() can override user-specified formatting, e.g. for phone
> > numbers.  Is that desirable?
> 
> When is SetInfo meant to be called? Isn't it for data that a user has typed,
> whereas SetRawInfo is more like "copy without modifying"?

SetInfo() is meant to be called on data that's coming from a webpage, where we
don't have control of the form.  It aggressively normalizes, to compensate for
that lack of control.

Either SetInfo() or SetRawInfo() is reasonable to call for input into our own
UIs.  However, for anything where we want to preserve user formatting -- which
historically has been at least phone numbers and credit card numbers -- we
should use SetRawInfo().

> It seems like PhoneNumber is broken if we want to retain user-specified
> formatting, but SetInfo overrides that formatting. I just tested with web
> autofill, and we don't retain user formatting there because PDM uses SetInfo.
We
> do retain formatting for data entered in chrome://settings/autofill. I'm not
> sure if we want these to differ, but I think this UI is more like a web form
> than the settings dialog (which is only for power users).

These do differ intentionally, as I described above.  If you think this UI is
not one where we want to preserve user formatting, I think that's a little
strange, but I won't push back too hard.

https://codereview.chromium.org/347183005/diff/60001/components/autofill/core...
File components/autofill/core/browser/address_unittest.cc (right):

https://codereview.chromium.org/347183005/diff/60001/components/autofill/core...
components/autofill/core/browser/address_unittest.cc:299:
EXPECT_FALSE(address.GetRawInfo(ADDRESS_HOME_LINE1).empty());
IMO it's good to start with all lines non-empty, and then end up with all lines
empty.  That provides slightly more test coverage.

[Evan Stade, 2014-07-01 23:18:52]

https://codereview.chromium.org/347183005/diff/40001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/347183005/diff/40001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:206:
g_browser_process->GetApplicationLocale());
On 2014/07/01 02:55:17, Ilya Sherman wrote:
> On 2014/06/28 01:01:35, Evan Stade wrote:
> > On 2014/06/27 06:25:08, Ilya Sherman wrote:
> > > Note that SetInfo() can override user-specified formatting, e.g. for phone
> > > numbers.  Is that desirable?
> > 
> > When is SetInfo meant to be called? Isn't it for data that a user has typed,
> > whereas SetRawInfo is more like "copy without modifying"?
> 
> SetInfo() is meant to be called on data that's coming from a webpage, where we
> don't have control of the form.  It aggressively normalizes, to compensate for
> that lack of control.
> 
> Either SetInfo() or SetRawInfo() is reasonable to call for input into our own
> UIs.  However, for anything where we want to preserve user formatting -- which
> historically has been at least phone numbers and credit card numbers -- we
> should use SetRawInfo().
> 
> > It seems like PhoneNumber is broken if we want to retain user-specified
> > formatting, but SetInfo overrides that formatting. I just tested with web
> > autofill, and we don't retain user formatting there because PDM uses
SetInfo.
> We
> > do retain formatting for data entered in chrome://settings/autofill. I'm not
> > sure if we want these to differ, but I think this UI is more like a web form
> > than the settings dialog (which is only for power users).
> 
> These do differ intentionally, as I described above.  If you think this UI is
> not one where we want to preserve user formatting, I think that's a little
> strange, but I won't push back too hard.

I'm actually OK with preserving user formatting (when there is any ---
3105558293 was probably typed that way because it's faster and not because the
user thinks it's the prettiest format), but I also want consistency. It's
inconsistent to call SetInfo for one type and SetRawInfo for another, or to call
SetInfo for one UI and SetRawInfo for another. I'm not sure what the "lack of
control" of web forms actually translates to in a concrete sense, specifically
with the phone number example. Why would a user type differently into <input
type="text"> than this dialog?

https://codereview.chromium.org/347183005/diff/60001/components/autofill/core...
File components/autofill/core/browser/address_unittest.cc (right):

https://codereview.chromium.org/347183005/diff/60001/components/autofill/core...
components/autofill/core/browser/address_unittest.cc:299:
EXPECT_FALSE(address.GetRawInfo(ADDRESS_HOME_LINE1).empty());
On 2014/07/01 02:55:17, Ilya Sherman wrote:
> IMO it's good to start with all lines non-empty, and then end up with all
lines
> empty.  That provides slightly more test coverage.

fair enough, fixed

[Ilya Sherman, 2014-07-02 00:02:15]

https://codereview.chromium.org/347183005/diff/40001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/347183005/diff/40001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:206:
g_browser_process->GetApplicationLocale());
On 2014/07/01 23:18:52, Evan Stade wrote:
> On 2014/07/01 02:55:17, Ilya Sherman wrote:
> > On 2014/06/28 01:01:35, Evan Stade wrote:
> > > On 2014/06/27 06:25:08, Ilya Sherman wrote:
> > > > Note that SetInfo() can override user-specified formatting, e.g. for
phone
> > > > numbers.  Is that desirable?
> > > 
> > > When is SetInfo meant to be called? Isn't it for data that a user has
typed,
> > > whereas SetRawInfo is more like "copy without modifying"?
> > 
> > SetInfo() is meant to be called on data that's coming from a webpage, where
we
> > don't have control of the form.  It aggressively normalizes, to compensate
for
> > that lack of control.
> > 
> > Either SetInfo() or SetRawInfo() is reasonable to call for input into our
own
> > UIs.  However, for anything where we want to preserve user formatting --
which
> > historically has been at least phone numbers and credit card numbers -- we
> > should use SetRawInfo().
> > 
> > > It seems like PhoneNumber is broken if we want to retain user-specified
> > > formatting, but SetInfo overrides that formatting. I just tested with web
> > > autofill, and we don't retain user formatting there because PDM uses
> SetInfo.
> > We
> > > do retain formatting for data entered in chrome://settings/autofill. I'm
not
> > > sure if we want these to differ, but I think this UI is more like a web
form
> > > than the settings dialog (which is only for power users).
> > 
> > These do differ intentionally, as I described above.  If you think this UI
is
> > not one where we want to preserve user formatting, I think that's a little
> > strange, but I won't push back too hard.
> 
> I'm actually OK with preserving user formatting (when there is any ---
> 3105558293 was probably typed that way because it's faster and not because the
> user thinks it's the prettiest format), but I also want consistency. It's
> inconsistent to call SetInfo for one type and SetRawInfo for another, or to
call
> SetInfo for one UI and SetRawInfo for another. I'm not sure what the "lack of
> control" of web forms actually translates to in a concrete sense, specifically
> with the phone number example. Why would a user type differently into <input
> type="text"> than this dialog?

There are plenty of forms out there that impose their own formatting, overriding
anything that the user entered.  We only see the end state, so it's hard to
distinguish the formatting the user wanted from the formatting the site wanted.

[Evan Stade, 2014-07-07 16:20:52]

https://codereview.chromium.org/347183005/diff/40001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/347183005/diff/40001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:206:
g_browser_process->GetApplicationLocale());
On 2014/07/02 00:02:15, Ilya Sherman wrote:
> On 2014/07/01 23:18:52, Evan Stade wrote:
> > On 2014/07/01 02:55:17, Ilya Sherman wrote:
> > > On 2014/06/28 01:01:35, Evan Stade wrote:
> > > > On 2014/06/27 06:25:08, Ilya Sherman wrote:
> > > > > Note that SetInfo() can override user-specified formatting, e.g. for
> phone
> > > > > numbers.  Is that desirable?
> > > > 
> > > > When is SetInfo meant to be called? Isn't it for data that a user has
> typed,
> > > > whereas SetRawInfo is more like "copy without modifying"?
> > > 
> > > SetInfo() is meant to be called on data that's coming from a webpage,
where
> we
> > > don't have control of the form.  It aggressively normalizes, to compensate
> for
> > > that lack of control.
> > > 
> > > Either SetInfo() or SetRawInfo() is reasonable to call for input into our
> own
> > > UIs.  However, for anything where we want to preserve user formatting --
> which
> > > historically has been at least phone numbers and credit card numbers -- we
> > > should use SetRawInfo().
> > > 
> > > > It seems like PhoneNumber is broken if we want to retain user-specified
> > > > formatting, but SetInfo overrides that formatting. I just tested with
web
> > > > autofill, and we don't retain user formatting there because PDM uses
> > SetInfo.
> > > We
> > > > do retain formatting for data entered in chrome://settings/autofill. I'm
> not
> > > > sure if we want these to differ, but I think this UI is more like a web
> form
> > > > than the settings dialog (which is only for power users).
> > > 
> > > These do differ intentionally, as I described above.  If you think this UI
> is
> > > not one where we want to preserve user formatting, I think that's a little
> > > strange, but I won't push back too hard.
> > 
> > I'm actually OK with preserving user formatting (when there is any ---
> > 3105558293 was probably typed that way because it's faster and not because
the
> > user thinks it's the prettiest format), but I also want consistency. It's
> > inconsistent to call SetInfo for one type and SetRawInfo for another, or to
> call
> > SetInfo for one UI and SetRawInfo for another. I'm not sure what the "lack
of
> > control" of web forms actually translates to in a concrete sense,
specifically
> > with the phone number example. Why would a user type differently into <input
> > type="text"> than this dialog?
> 
> There are plenty of forms out there that impose their own formatting,
overriding
> anything that the user entered.  We only see the end state, so it's hard to
> distinguish the formatting the user wanted from the formatting the site
wanted.

I see. And instead we impose the formatting that Chrome wants, even if it's
clobbering user-supplied formatting.

Let's consider 2 users, user A and user B, and two sites, site X and site Y.
User A cares about their formatting, user B does not. Site X has a freeform
input, site Y imposes formatting.

User A goes to site X. User provides formatting. Chrome overrides formatting.
User A is sad, unless she happens to like Chrome's formatting.
User A goes to site Y. Site imposes formatting. Chrome overrides formatting.
User A is sad, unless she happens to like Chrome's formatting.
(if smart enough, in these two cases, user A can go to
chrome://settings/autofill to fix it)

User B goes to site X. Chrome imposes formatting. User B is happy.
User B goes to site Y. Site imposes formatting. Chrome overrides formatting.
User B is happy.

So in the current world, user A is always sad, user B always happy. If Chrome
didn't override site formatting, user A would at least be happy on site X,
although we still couldn't fix site Y for them.

[Ilya Sherman, 2014-07-08 00:35:10]

https://codereview.chromium.org/347183005/diff/40001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/347183005/diff/40001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:206:
g_browser_process->GetApplicationLocale());
On 2014/07/07 16:20:52, Evan Stade wrote:
> On 2014/07/02 00:02:15, Ilya Sherman wrote:
> > On 2014/07/01 23:18:52, Evan Stade wrote:
> > > On 2014/07/01 02:55:17, Ilya Sherman wrote:
> > > > On 2014/06/28 01:01:35, Evan Stade wrote:
> > > > > On 2014/06/27 06:25:08, Ilya Sherman wrote:
> > > > > > Note that SetInfo() can override user-specified formatting, e.g. for
> > phone
> > > > > > numbers.  Is that desirable?
> > > > > 
> > > > > When is SetInfo meant to be called? Isn't it for data that a user has
> > typed,
> > > > > whereas SetRawInfo is more like "copy without modifying"?
> > > > 
> > > > SetInfo() is meant to be called on data that's coming from a webpage,
> where
> > we
> > > > don't have control of the form.  It aggressively normalizes, to
compensate
> > for
> > > > that lack of control.
> > > > 
> > > > Either SetInfo() or SetRawInfo() is reasonable to call for input into
our
> > own
> > > > UIs.  However, for anything where we want to preserve user formatting --
> > which
> > > > historically has been at least phone numbers and credit card numbers --
we
> > > > should use SetRawInfo().
> > > > 
> > > > > It seems like PhoneNumber is broken if we want to retain
user-specified
> > > > > formatting, but SetInfo overrides that formatting. I just tested with
> web
> > > > > autofill, and we don't retain user formatting there because PDM uses
> > > SetInfo.
> > > > We
> > > > > do retain formatting for data entered in chrome://settings/autofill.
I'm
> > not
> > > > > sure if we want these to differ, but I think this UI is more like a
web
> > form
> > > > > than the settings dialog (which is only for power users).
> > > > 
> > > > These do differ intentionally, as I described above.  If you think this
UI
> > is
> > > > not one where we want to preserve user formatting, I think that's a
little
> > > > strange, but I won't push back too hard.
> > > 
> > > I'm actually OK with preserving user formatting (when there is any ---
> > > 3105558293 was probably typed that way because it's faster and not because
> the
> > > user thinks it's the prettiest format), but I also want consistency. It's
> > > inconsistent to call SetInfo for one type and SetRawInfo for another, or
to
> > call
> > > SetInfo for one UI and SetRawInfo for another. I'm not sure what the "lack
> of
> > > control" of web forms actually translates to in a concrete sense,
> specifically
> > > with the phone number example. Why would a user type differently into
<input
> > > type="text"> than this dialog?
> > 
> > There are plenty of forms out there that impose their own formatting,
> overriding
> > anything that the user entered.  We only see the end state, so it's hard to
> > distinguish the formatting the user wanted from the formatting the site
> wanted.
> 
> I see. And instead we impose the formatting that Chrome wants, even if it's
> clobbering user-supplied formatting.
> 
> Let's consider 2 users, user A and user B, and two sites, site X and site Y.
> User A cares about their formatting, user B does not. Site X has a freeform
> input, site Y imposes formatting.
> 
> User A goes to site X. User provides formatting. Chrome overrides formatting.
> User A is sad, unless she happens to like Chrome's formatting.
> User A goes to site Y. Site imposes formatting. Chrome overrides formatting.
> User A is sad, unless she happens to like Chrome's formatting.
> (if smart enough, in these two cases, user A can go to
> chrome://settings/autofill to fix it)
> 
> User B goes to site X. Chrome imposes formatting. User B is happy.
> User B goes to site Y. Site imposes formatting. Chrome overrides formatting.
> User B is happy.
> 
> So in the current world, user A is always sad, user B always happy. If Chrome
> didn't override site formatting, user A would at least be happy on site X,
> although we still couldn't fix site Y for them.

Well, the most common case in the US is that the phone number is split across
multiple fields.  Hence, the most common case is that we add a pretty format
where there isn't one already.  I suppose we could change SetInfo() to only
write a pretty format if the incoming format is just a digit string.

[Evan Stade, 2014-07-08 01:28:12]

On 2014/07/08 00:35:10, Ilya Sherman wrote:
>
https://codereview.chromium.org/347183005/diff/40001/chrome/browser/ui/autofi...
> File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):
> 
>
https://codereview.chromium.org/347183005/diff/40001/chrome/browser/ui/autofi...
> chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:206:
> g_browser_process->GetApplicationLocale());
> On 2014/07/07 16:20:52, Evan Stade wrote:
> > On 2014/07/02 00:02:15, Ilya Sherman wrote:
> > > On 2014/07/01 23:18:52, Evan Stade wrote:
> > > > On 2014/07/01 02:55:17, Ilya Sherman wrote:
> > > > > On 2014/06/28 01:01:35, Evan Stade wrote:
> > > > > > On 2014/06/27 06:25:08, Ilya Sherman wrote:
> > > > > > > Note that SetInfo() can override user-specified formatting, e.g.
for
> > > phone
> > > > > > > numbers.  Is that desirable?
> > > > > > 
> > > > > > When is SetInfo meant to be called? Isn't it for data that a user
has
> > > typed,
> > > > > > whereas SetRawInfo is more like "copy without modifying"?
> > > > > 
> > > > > SetInfo() is meant to be called on data that's coming from a webpage,
> > where
> > > we
> > > > > don't have control of the form.  It aggressively normalizes, to
> compensate
> > > for
> > > > > that lack of control.
> > > > > 
> > > > > Either SetInfo() or SetRawInfo() is reasonable to call for input into
> our
> > > own
> > > > > UIs.  However, for anything where we want to preserve user formatting
--
> > > which
> > > > > historically has been at least phone numbers and credit card numbers
--
> we
> > > > > should use SetRawInfo().
> > > > > 
> > > > > > It seems like PhoneNumber is broken if we want to retain
> user-specified
> > > > > > formatting, but SetInfo overrides that formatting. I just tested
with
> > web
> > > > > > autofill, and we don't retain user formatting there because PDM uses
> > > > SetInfo.
> > > > > We
> > > > > > do retain formatting for data entered in chrome://settings/autofill.
> I'm
> > > not
> > > > > > sure if we want these to differ, but I think this UI is more like a
> web
> > > form
> > > > > > than the settings dialog (which is only for power users).
> > > > > 
> > > > > These do differ intentionally, as I described above.  If you think
this
> UI
> > > is
> > > > > not one where we want to preserve user formatting, I think that's a
> little
> > > > > strange, but I won't push back too hard.
> > > > 
> > > > I'm actually OK with preserving user formatting (when there is any ---
> > > > 3105558293 was probably typed that way because it's faster and not
because
> > the
> > > > user thinks it's the prettiest format), but I also want consistency.
It's
> > > > inconsistent to call SetInfo for one type and SetRawInfo for another, or
> to
> > > call
> > > > SetInfo for one UI and SetRawInfo for another. I'm not sure what the
"lack
> > of
> > > > control" of web forms actually translates to in a concrete sense,
> > specifically
> > > > with the phone number example. Why would a user type differently into
> <input
> > > > type="text"> than this dialog?
> > > 
> > > There are plenty of forms out there that impose their own formatting,
> > overriding
> > > anything that the user entered.  We only see the end state, so it's hard
to
> > > distinguish the formatting the user wanted from the formatting the site
> > wanted.
> > 
> > I see. And instead we impose the formatting that Chrome wants, even if it's
> > clobbering user-supplied formatting.
> > 
> > Let's consider 2 users, user A and user B, and two sites, site X and site Y.
> > User A cares about their formatting, user B does not. Site X has a freeform
> > input, site Y imposes formatting.
> > 
> > User A goes to site X. User provides formatting. Chrome overrides
formatting.
> > User A is sad, unless she happens to like Chrome's formatting.
> > User A goes to site Y. Site imposes formatting. Chrome overrides formatting.
> > User A is sad, unless she happens to like Chrome's formatting.
> > (if smart enough, in these two cases, user A can go to
> > chrome://settings/autofill to fix it)
> > 
> > User B goes to site X. Chrome imposes formatting. User B is happy.
> > User B goes to site Y. Site imposes formatting. Chrome overrides formatting.
> > User B is happy.
> > 
> > So in the current world, user A is always sad, user B always happy. If
Chrome
> > didn't override site formatting, user A would at least be happy on site X,
> > although we still couldn't fix site Y for them.
> 
> Well, the most common case in the US is that the phone number is split across
> multiple fields.  Hence, the most common case is that we add a pretty format
> where there isn't one already.  I suppose we could change SetInfo() to only
> write a pretty format if the incoming format is just a digit string.

yes, I think that makes the most sense.

[Evan Stade, 2014-07-08 21:13:23]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-07-08 21:14:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/estade@chromium.org/347183005/80001

[commit-bot: I haz the power, 2014-07-08 22:40:04]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-07-08 23:05:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-08 23:05:09]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[Evan Stade, 2014-07-09 23:20:31]

+aruslan for android changes

[aruslan, 2014-07-09 23:49:57]

It fails to compile because AutofillProfile::SetInfo accepts the locale argument
in addition to the two accepted by SetRawInfo.
(See ADDRESS_HOME_COUNTRY as an example.)

Please try android_clang_dbg and android_dbg once fixed to see if it passes the
tests.
Otherwise, android lgtm.

[Evan Stade, 2014-07-10 17:53:03]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-07-10 17:55:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/estade@chromium.org/347183005/120001

[commit-bot: I haz the power, 2014-07-10 21:12:45]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[aruslan, 2014-07-10 21:17:28]

Looks like SetInfo attempts to normalize the phone number or something (as
opposed to SetRawInfo).
If this is an intended behavior, please fix the test.
I'm surprised that only one test failed, though.

[FAIL]
org.chromium.chrome.browser.autofill.PersonalDataManagerTest#testMultilineStreetAddress:
C  106.692s Main  junit.framework.ComparisonFailure: expected:<50[.71.]53> but
was:<50[ 71 ]53>

http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...

[Evan Stade, 2014-07-11 18:19:41]

On 2014/07/10 21:17:28, aruslan wrote:
> Looks like SetInfo attempts to normalize the phone number or something (as
> opposed to SetRawInfo).
> If this is an intended behavior, please fix the test.
> I'm surprised that only one test failed, though.
> 
> [FAIL]
>
org.chromium.chrome.browser.autofill.PersonalDataManagerTest#testMultilineStreetAddress:
> C  106.692s Main  junit.framework.ComparisonFailure: expected:<50[.71.]53> but
> was:<50[ 71 ]53>
> 
>
http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...

yea, it's intended for now, although it will be revisited in
https://codereview.chromium.org/374053007/

[Evan Stade, 2014-07-11 18:19:46]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-07-11 18:20:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/estade@chromium.org/347183005/130001

[commit-bot: I haz the power, 2014-07-11 19:32:35]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-11 22:07:17]

Change committed as 282712