autocomplete: support address-line3, address-level{1,2,3}

autocomplete: support filling address-line3, address-level{1,2,3}

Deprecate region and locality, log a message on the console when they're encountered.

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

[Evan Stade, 2014-06-06 20:58:45]

How do I update the autofill server with the new type?

[Ilya Sherman, 2014-06-06 23:24:07]

WDYT of getting initial reviews from Dan and/or Rouslan, and then final passes
from me, with the goal of adding more Autofill OWNERS over time?

On 2014/06/06 20:58:45, Evan Stade wrote:
> How do I update the autofill server with the new type?

For this particular field, it's nontrivial, because address line 3 is a more
noisy field than others.  But, I've emailed you a starting point for looking at
the server code.  I'd recommend implementing the changes involved with the
server in a separate CL.  Note that you'll likely also need to update
autofill_metrics.cc whenever you add new server field types.

[Evan Stade, 2014-06-09 17:17:01]

+groby for chrome/browser/ui

On 2014/06/06 23:24:07, Ilya Sherman wrote:
> WDYT of getting initial reviews from Dan and/or Rouslan, and then final passes
> from me, with the goal of adding more Autofill OWNERS over time?

I don't think anyone should be an owner without writing a substantial amount of
code in the affected directory.

I will add a separate reviewer for ui/ so you only have to look in
components/autofill.

[Ilya Sherman, 2014-06-10 00:40:41]

https://codereview.chromium.org/322453003/diff/40001/components/autofill/cont...
File components/autofill/content/renderer/form_cache.cc (right):

https://codereview.chromium.org/322453003/diff/40001/components/autofill/cont...
components/autofill/content/renderer/form_cache.cc:85: // We log an error
message for deprecated attributes, but only the first time
nit: "We log" -> "Log"

https://codereview.chromium.org/322453003/diff/40001/components/autofill/cont...
components/autofill/content/renderer/form_cache.cc:106: static const char*
deprecated[] = { "region", "locality" };
nit: "const char*" -> "const char* const"

https://codereview.chromium.org/322453003/diff/40001/components/autofill/cont...
components/autofill/content/renderer/form_cache.cc:110:
WebConsoleMessage::LevelLog,
Wouldn't LevelWarning or LevelError be more appropriate?

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

https://codereview.chromium.org/322453003/diff/40001/components/autofill/core...
components/autofill/core/browser/address.cc:104: break;
Please add test coverage, including calling SetInfo() with an address that has
lines 1 and 3, but not 2.  (Should that even be legal?)

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

https://codereview.chromium.org/322453003/diff/40001/components/autofill/core...
components/autofill/core/browser/autofill_type.cc:160: case
HTML_TYPE_ADDRESS_LEVEL3:
What about level 4?

[Ilya Sherman, 2014-06-10 00:47:58]

In addition to my comments on the diffs, please keep in mind that you'll need to
update autofill_metrics.cc.  

On 2014/06/09 17:17:01, Evan Stade wrote:
> On 2014/06/06 23:24:07, Ilya Sherman wrote:
> > WDYT of getting initial reviews from Dan and/or Rouslan, and then final
passes
> > from me, with the goal of adding more Autofill OWNERS over time?
> 
> I don't think anyone should be an owner without writing a substantial amount
of
> code in the affected directory.

In that case, please encourage others to pick up bugs that involve making
changes to the core Autofill directories.  I think it's still reasonable to ask
for preliminary code reviews as well -- these are, IMO, a great way to learn the
workings of a new part of the codebase.

I don't think that it's a good situation, long-term, to have only a single OWNER
who is actively working on the code.  If the core Autofill code were more in
maintenance mode, then it would be fine to rely primarily on maintainers for
code reviews.  But since the code is still being fairly routinely developed,
it's a good idea to train up more owners.

[Evan Stade, 2014-06-10 01:41:20]

On 2014/06/10 00:47:58, Ilya Sherman wrote:
> In addition to my comments on the diffs, please keep in mind that you'll need
to
> update autofill_metrics.cc.  
> 
> On 2014/06/09 17:17:01, Evan Stade wrote:
> > On 2014/06/06 23:24:07, Ilya Sherman wrote:
> > > WDYT of getting initial reviews from Dan and/or Rouslan, and then final
> passes
> > > from me, with the goal of adding more Autofill OWNERS over time?
> > 
> > I don't think anyone should be an owner without writing a substantial amount
> of
> > code in the affected directory.
> 
> In that case, please encourage others to pick up bugs that involve making
> changes to the core Autofill directories.  I think it's still reasonable to
ask
> for preliminary code reviews as well -- these are, IMO, a great way to learn
the
> workings of a new part of the codebase.
> 
> I don't think that it's a good situation, long-term, to have only a single
OWNER
> who is actively working on the code.  If the core Autofill code were more in
> maintenance mode, then it would be fine to rely primarily on maintainers for
> code reviews.  But since the code is still being fairly routinely developed,
> it's a good idea to train up more owners.

In theory, I agree with this, but as a practical matter, I don't think minting
OWNERs is a strong deciding factor for what people work on. Given that the
development is closer to trickle than flood at the moment, I think only one
active OWNER is ok, though not ideal.

I do sympathize with the situation of being asked for reviews after changing
projects, because I've been doing webui reviews for years after stopping active
development on it. But I'm not aware of a good solution to this general problem.

[groby-ooo-7-16, 2014-06-10 20:20:32]

c/b/ui LGTM

[Evan Stade, 2014-06-12 01:49:47]

https://codereview.chromium.org/322453003/diff/40001/components/autofill/cont...
File components/autofill/content/renderer/form_cache.cc (right):

https://codereview.chromium.org/322453003/diff/40001/components/autofill/cont...
components/autofill/content/renderer/form_cache.cc:85: // We log an error
message for deprecated attributes, but only the first time
On 2014/06/10 00:40:40, Ilya Sherman wrote:
> nit: "We log" -> "Log"

Done.

https://codereview.chromium.org/322453003/diff/40001/components/autofill/cont...
components/autofill/content/renderer/form_cache.cc:106: static const char*
deprecated[] = { "region", "locality" };
On 2014/06/10 00:40:40, Ilya Sherman wrote:
> nit: "const char*" -> "const char* const"

Done.

https://codereview.chromium.org/322453003/diff/40001/components/autofill/cont...
components/autofill/content/renderer/form_cache.cc:110:
WebConsoleMessage::LevelLog,
On 2014/06/10 00:40:40, Ilya Sherman wrote:
> Wouldn't LevelWarning or LevelError be more appropriate?

yea, I think warning is correct

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

https://codereview.chromium.org/322453003/diff/40001/components/autofill/core...
components/autofill/core/browser/address.cc:104: break;
On 2014/06/10 00:40:40, Ilya Sherman wrote:
> Please add test coverage, including calling SetInfo() with an address that has
> lines 1 and 3, but not 2.  (Should that even be legal?)

Done.

Should it be legal? I dunno. It appears it's legal to have line 2 with no line
1, and line 1 and 3 with no 2 makes about as much sense to me.

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

https://codereview.chromium.org/322453003/diff/40001/components/autofill/core...
components/autofill/core/browser/autofill_type.cc:160: case
HTML_TYPE_ADDRESS_LEVEL3:
On 2014/06/10 00:40:40, Ilya Sherman wrote:
> What about level 4?

We can't actually fill that in yet, so adding it wouldn't do much.

https://codereview.chromium.org/322453003/diff/80001/components/autofill/core...
File components/autofill/core/browser/autofill_metrics.cc (right):

https://codereview.chromium.org/322453003/diff/80001/components/autofill/core...
components/autofill/core/browser/autofill_metrics.cc:37: ADDRESS_LINE_3,
is it correct to put this at the end or can it go by its brethren?

[Ilya Sherman, 2014-06-12 22:37:55]

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

https://codereview.chromium.org/322453003/diff/80001/components/autofill/core...
components/autofill/core/browser/address_unittest.cc:211: 
nit: Spurious newline.

https://codereview.chromium.org/322453003/diff/80001/components/autofill/core...
File components/autofill/core/browser/autofill_metrics.cc (right):

https://codereview.chromium.org/322453003/diff/80001/components/autofill/core...
components/autofill/core/browser/autofill_metrics.cc:37: ADDRESS_LINE_3,
On 2014/06/12 01:49:47, Evan Stade wrote:
> is it correct to put this at the end or can it go by its brethren?

This is correct.  Alternately, we could rename the metric and reorder the fields
so that line 3 is in the middle.

Note that when making changes to metrics, you should also update histograms.xml.

[Evan Stade, 2014-06-13 01:49:43]

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

https://codereview.chromium.org/322453003/diff/80001/components/autofill/core...
components/autofill/core/browser/address_unittest.cc:211: 
On 2014/06/12 22:37:54, Ilya Sherman wrote:
> nit: Spurious newline.

baleted

https://codereview.chromium.org/322453003/diff/80001/components/autofill/core...
File components/autofill/core/browser/autofill_metrics.cc (right):

https://codereview.chromium.org/322453003/diff/80001/components/autofill/core...
components/autofill/core/browser/autofill_metrics.cc:37: ADDRESS_LINE_3,
On 2014/06/12 22:37:54, Ilya Sherman wrote:
> On 2014/06/12 01:49:47, Evan Stade wrote:
> > is it correct to put this at the end or can it go by its brethren?
> 
> This is correct.  Alternately, we could rename the metric and reorder the
fields
> so that line 3 is in the middle.

wouldn't that make it harder to compare across chrome versions?

> 
> Note that when making changes to metrics, you should also update
histograms.xml.

Done.

[Ilya Sherman, 2014-06-13 21:50:09]

LGTM, thanks.

https://codereview.chromium.org/322453003/diff/80001/components/autofill/core...
File components/autofill/core/browser/autofill_metrics.cc (right):

https://codereview.chromium.org/322453003/diff/80001/components/autofill/core...
components/autofill/core/browser/autofill_metrics.cc:37: ADDRESS_LINE_3,
On 2014/06/13 01:49:43, Evan Stade wrote:
> On 2014/06/12 22:37:54, Ilya Sherman wrote:
> > On 2014/06/12 01:49:47, Evan Stade wrote:
> > > is it correct to put this at the end or can it go by its brethren?
> > 
> > This is correct.  Alternately, we could rename the metric and reorder the
> fields
> > so that line 3 is in the middle.
> 
> wouldn't that make it harder to compare across chrome versions?

Aye, I think adding it at the end, as you've done, is the better option.

[Evan Stade, 2014-06-13 22:18:42]

thx for the review.

[Evan Stade, 2014-06-13 22:18:47]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-06-13 22:24:21]

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

[commit-bot: I haz the power, 2014-06-14 04:51:23]

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

[commit-bot: I haz the power, 2014-06-14 04:51:24]

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

[Evan Stade, 2014-06-16 19:09:26]

Ilya, could you take a look at chrome/test/data/autofill/* as well? Thanks.

[Ilya Sherman, 2014-06-16 21:11:37]

Test changes LGTM.  Thanks, Evan :)

[Ilya Sherman, 2014-06-16 21:11:43]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-06-16 21:12:59]

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

[commit-bot: I haz the power, 2014-06-17 03:29:21]

Change committed as 277645