Autofill: Do not split sections on fields used for presentation roles.

Autofill: Do not split sections on fields used for presentation roles.

BUG=459132
Committed: https://crrev.com/517912ca0c66cc495529a9db707fc3d55ef18cb4
Cr-Commit-Position: refs/heads/master@{#317173}

[Lei Zhang, 2015-02-18 06:08:37]

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

[Evan Stade, 2015-02-18 22:51:26]

https://codereview.chromium.org/932023003/diff/1/components/autofill/core/bro...
File components/autofill/core/browser/form_structure.cc (right):

https://codereview.chromium.org/932023003/diff/1/components/autofill/core/bro...
components/autofill/core/browser/form_structure.cc:1218:
already_saw_current_type = false;
I don't think this is correct. We need to instead copy the PHONE_HOME clause
above for emails. A single form frequently asks for multiple emails.

[Lei Zhang, 2015-02-19 01:53:54]

On 2015/02/18 22:51:26, Evan Stade wrote:
>
https://codereview.chromium.org/932023003/diff/1/components/autofill/core/bro...
> File components/autofill/core/browser/form_structure.cc (right):
> 
>
https://codereview.chromium.org/932023003/diff/1/components/autofill/core/bro...
> components/autofill/core/browser/form_structure.cc:1218:
> already_saw_current_type = false;
> I don't think this is correct. We need to instead copy the PHONE_HOME clause
> above for emails. A single form frequently asks for multiple emails.

I tried that but it ends up grouping the "sign in" and "register new user"
sections on several registration / login pages. Make your suggested change
locally and see the test results. Some may consider that a regression.

We actually handle the confirmation email fields using the "adjacent fields"
heuristic instead. Perhaps what we want is to not set |previous_type| for fields
used for presentation.

[Evan Stade, 2015-02-19 17:03:45]

On 2015/02/19 01:53:54, Lei Zhang (OOO) wrote:
> On 2015/02/18 22:51:26, Evan Stade wrote:
> >
>
https://codereview.chromium.org/932023003/diff/1/components/autofill/core/bro...
> > File components/autofill/core/browser/form_structure.cc (right):
> > 
> >
>
https://codereview.chromium.org/932023003/diff/1/components/autofill/core/bro...
> > components/autofill/core/browser/form_structure.cc:1218:
> > already_saw_current_type = false;
> > I don't think this is correct. We need to instead copy the PHONE_HOME clause
> > above for emails. A single form frequently asks for multiple emails.
> 
> I tried that but it ends up grouping the "sign in" and "register new user"
> sections on several registration / login pages. Make your suggested change
> locally and see the test results. Some may consider that a regression.
> 
> We actually handle the confirmation email fields using the "adjacent fields"
> heuristic instead. Perhaps what we want is to not set |previous_type| for
fields
> used for presentation.

good investigation --- new change lgtm

[Evan Stade, 2015-02-19 21:22:40]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2015-02-19 21:25:02]

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

[commit-bot: I haz the power, 2015-02-19 23:44:23]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-02-19 23:44:52]

Patchset 2 (id:??) landed as
https://crrev.com/517912ca0c66cc495529a9db707fc3d55ef18cb4
Cr-Commit-Position: refs/heads/master@{#317173}