Autofill - Fix Harry and David checkout: ignore role="presentation"

Autofill - Fix Harry and David checkout: ignore role="presentation"
fields.

BUG=438396
Committed: https://crrev.com/b78bada8e9cae05a78568c511defe2817eeb7e2b
Cr-Commit-Position: refs/heads/master@{#307337}

[Evan Stade, 2014-12-02 23:36:09]

estade@chromium.org changed reviewers:
+ brettw@chromium.org

[Evan Stade, 2014-12-02 23:36:10]

https://codereview.chromium.org/734983006/diff/20001/chrome/test/data/autofil...
File
chrome/test/data/autofill/heuristics/output/24_checkout_harryanddavid.com.out
(right):

https://codereview.chromium.org/734983006/diff/20001/chrome/test/data/autofil...
chrome/test/data/autofill/heuristics/output/24_checkout_harryanddavid.com.out:8:
UNKNOWN_TYPE |  | First Name* | Χ  | typeOfAddress_1-default
Everything you see here with value=X has role="presentation", and therefore
should be ignored. This fixes last name, zip code, address line 1, etc.

[brettw, 2014-12-03 21:36:24]

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

https://codereview.chromium.org/734983006/diff/40001/components/autofill/cont...
components/autofill/content/renderer/form_autofill_util.cc:975: if
(field->role.size() > kMaxDataLength) {
Looks like this file prefers no {} for single-line conditionals.

[brettw, 2014-12-03 21:36:33]

LGTM with previous nit

[Evan Stade, 2014-12-03 22:29:51]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2014-12-03 22:29:55]

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

https://codereview.chromium.org/734983006/diff/40001/components/autofill/cont...
components/autofill/content/renderer/form_autofill_util.cc:975: if
(field->role.size() > kMaxDataLength) {
On 2014/12/03 21:36:24, brettw wrote:
> Looks like this file prefers no {} for single-line conditionals.

ah yes, I think these curlies were here because of a debugging statement.
Removed.

[commit-bot: I haz the power, 2014-12-03 22:30:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/734983006/60001

[commit-bot: I haz the power, 2014-12-03 22:37:35]

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

[commit-bot: I haz the power, 2014-12-03 22:37:36]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Evan Stade, 2014-12-03 23:33:25]

estade@chromium.org changed reviewers:
+ jschuh@chromium.org

[Evan Stade, 2014-12-03 23:33:25]

+jschuh for components/autofill/content/common/autofill_messages.h

can you CQ if lgty?

[Evan Stade, 2014-12-05 22:37:23]

On 2014/12/03 23:33:25, Evan Stade wrote:
> +jschuh for components/autofill/content/common/autofill_messages.h
> 
> can you CQ if lgty?

ping jschuh

[Evan Stade, 2014-12-05 22:37:23]

On 2014/12/03 23:33:25, Evan Stade wrote:
> +jschuh for components/autofill/content/common/autofill_messages.h
> 
> can you CQ if lgty?

ping jschuh

[jschuh, 2014-12-05 23:03:04]

Sorry, apparently gmail decided I didn't need to see this. Quick question, does
the role have to be a string when it gets to the browser or can it be sebnt and
handled as an enum because it's restricted to a known set of values on the
browser side?

[Evan Stade, 2014-12-06 00:47:08]

On 2014/12/05 23:03:04, jschuh wrote:
> Sorry, apparently gmail decided I didn't need to see this. Quick question,
does
> the role have to be a string when it gets to the browser or can it be sebnt
and
> handled as an enum because it's restricted to a known set of values on the
> browser side?

Well the browser only handles a set of known values (currently "presentation"
and "not presentation") but we could potentially start using role for more
things. Pretty much the same story as form_control_type and
autocomplete_attribute.

[Ilya Sherman, 2014-12-06 00:53:33]

On 2014/12/06 00:47:08, Evan Stade wrote:
> On 2014/12/05 23:03:04, jschuh wrote:
> > Sorry, apparently gmail decided I didn't need to see this. Quick question,
> does
> > the role have to be a string when it gets to the browser or can it be sebnt
> and
> > handled as an enum because it's restricted to a known set of values on the
> > browser side?
> 
> Well the browser only handles a set of known values (currently "presentation"
> and "not presentation") but we could potentially start using role for more
> things. Pretty much the same story as form_control_type and
> autocomplete_attribute.

FWIW, I've long been meaning to convert some of the other strings that we pass
over IPC, like the form control type, to be enums instead -- if for no other
reason than that enum comparisons are less error-prone than string literal
comparisons.

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

On 2014/12/06 00:53:33, Ilya Sherman wrote:
> On 2014/12/06 00:47:08, Evan Stade wrote:
> > On 2014/12/05 23:03:04, jschuh wrote:
> > > Sorry, apparently gmail decided I didn't need to see this. Quick question,
> > does
> > > the role have to be a string when it gets to the browser or can it be
sebnt
> > and
> > > handled as an enum because it's restricted to a known set of values on the
> > > browser side?
> > 
> > Well the browser only handles a set of known values (currently
"presentation"
> > and "not presentation") but we could potentially start using role for more
> > things. Pretty much the same story as form_control_type and
> > autocomplete_attribute.
> 
> FWIW, I've long been meaning to convert some of the other strings that we pass
> over IPC, like the form control type, to be enums instead -- if for no other
> reason than that enum comparisons are less error-prone than string literal
> comparisons.

How do you convert from string to enum without a string literal comparison?

[jschuh, 2014-12-06 04:26:39]

You still need the comparison (or map or whatever) for the conversion, but my
concern is in passing the most constrained type over the IPC layer. Enums are
best because the IPC layer supports those directly. So, I'd much prefer handling
role that way if possible.

[Evan Stade, 2014-12-08 19:31:42]

ok, changed to enum. Seems form_control_type should also be an enum, although
that's a task for another day. autocomplete_attribute might be harder to switch
given that it can be composed of multiple tokens (e.g. "shipping given-name")

[jschuh, 2014-12-08 19:45:45]

thanks. lgtm for ipc security

[Evan Stade, 2014-12-08 19:53:35]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-12-08 19:54:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/734983006/80001

[commit-bot: I haz the power, 2014-12-08 21:10:48]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-12-08 21:11:30]

Patchset 5 (id:??) landed as
https://crrev.com/b78bada8e9cae05a78568c511defe2817eeb7e2b
Cr-Commit-Position: refs/heads/master@{#307337}