[Autofill] OnLoadedServerPredictions correctly supplies FormStructure to ParseQueryResponse.

[Autofill] OnLoadedServerPredictions correctly supplies FormStructure to ParseQueryResponse.

Previous change was not correct and lacked sufficient testing. Now testing with two forms to test for correct vector order.

BUG=555025
TEST=AutofillManagerTest

Committed: https://crrev.com/945cd05f1fe6c217b8bc83f161ebcbb9c1e748ab
Cr-Commit-Position: refs/heads/master@{#361155}

[Mathieu, 2015-11-23 16:20:48]

Description was changed from

==========
[Autofill] OnLoadedServerPredictions correctly supplies FormStructure to
ParseQueryResponse.

Previous change was not correct and lacked sufficient testing. Now testing with
two forms to test for correct vector order.

BUG=555025
TEST=AutofillManagerTest
==========

to

==========
[Autofill] OnLoadedServerPredictions correctly supplies FormStructure to
ParseQueryResponse.

Previous change was not correct and lacked sufficient testing. Now testing with
two forms to test for correct vector order.

BUG=555025
TEST=AutofillManagerTest
==========

[Mathieu, 2015-11-23 16:20:49]

mathp@chromium.org changed reviewers:
+ vabr@chromium.org

[Mathieu, 2015-11-23 16:21:16]

Hi!

Please take a look. My last change was unfortunately wrong for multi-form pages.

[vabr (Chromium), 2015-11-23 16:59:25]

Thanks, the added tests and the change LGTM, with comments.

I'm not sure I fully understand the issue, though. Was the problem, that the
field types are not grouped by forms, and thus having |queried_forms| in wrong
order broke assigning fields to forms by offsets?

Cheers,
Vaclav

https://codereview.chromium.org/1472723003/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/1472723003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:748: // |form_signatures|.
We invert both lists because most recent forms are at
Just curious -- was this meant to improve performance? Is it still worth it with
the std::reverse at the end, and the added complexity?

https://codereview.chromium.org/1472723003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:758: found_signature =
true;
Could you just execute "break;" here and remove |found_signature|? Or did you
mean to use |form_signature| after the for loop to test for success?

[Mathieu, 2015-11-23 17:27:07]

Thanks!

https://codereview.chromium.org/1472723003/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/1472723003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:748: // |form_signatures|.
We invert both lists because most recent forms are at
On 2015/11/23 16:59:25, vabr (Chromium) wrote:
> Just curious -- was this meant to improve performance? Is it still worth it
with
> the std::reverse at the end, and the added complexity?

Yes it's meant to improve performance. With <form>-less Autofill a new form
structure is created at every field added. This pool of forms can then get quite
large. I think complexity is not that bad and replicated elsewhere in this file.

https://codereview.chromium.org/1472723003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:758: found_signature =
true;
On 2015/11/23 16:59:25, vabr (Chromium) wrote:
> Could you just execute "break;" here and remove |found_signature|? Or did you
> mean to use |form_signature| after the for loop to test for success?

Of course, I did this too quickly, brain fart.

[Mathieu, 2015-11-23 17:27:51]

On 2015/11/23 17:27:07, Mathieu Perreault wrote:
> Thanks!
> 
>
https://codereview.chromium.org/1472723003/diff/1/components/autofill/core/br...
> File components/autofill/core/browser/autofill_manager.cc (right):
> 
>
https://codereview.chromium.org/1472723003/diff/1/components/autofill/core/br...
> components/autofill/core/browser/autofill_manager.cc:748: //
|form_signatures|.
> We invert both lists because most recent forms are at
> On 2015/11/23 16:59:25, vabr (Chromium) wrote:
> > Just curious -- was this meant to improve performance? Is it still worth it
> with
> > the std::reverse at the end, and the added complexity?
> 
> Yes it's meant to improve performance. With <form>-less Autofill a new form
> structure is created at every field added. This pool of forms can then get
quite
> large. I think complexity is not that bad and replicated elsewhere in this
file.
> 
>
https://codereview.chromium.org/1472723003/diff/1/components/autofill/core/br...
> components/autofill/core/browser/autofill_manager.cc:758: found_signature =
> true;
> On 2015/11/23 16:59:25, vabr (Chromium) wrote:
> > Could you just execute "break;" here and remove |found_signature|? Or did
you
> > mean to use |form_signature| after the for loop to test for success?
> 
> Of course, I did this too quickly, brain fart.

Exactly, if you look at the test and the expected server responses, we don't
group the forms in the response. It's obviously brittle, and we're hoping to
address this when moving to proto.

[Mathieu, 2015-11-23 17:27:59]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2015-11-23 17:27:59]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/1472723003/#ps20001
(title: "addressed comments")

[commit-bot: I haz the power, 2015-11-23 17:28:57]

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

[vabr (Chromium), 2015-11-23 17:47:07]

LGTM, thanks!

https://codereview.chromium.org/1472723003/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/1472723003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:748: // |form_signatures|.
We invert both lists because most recent forms are at
On 2015/11/23 17:27:07, Mathieu Perreault wrote:
> On 2015/11/23 16:59:25, vabr (Chromium) wrote:
> > Just curious -- was this meant to improve performance? Is it still worth it
> with
> > the std::reverse at the end, and the added complexity?
> 
> Yes it's meant to improve performance. With <form>-less Autofill a new form
> structure is created at every field added. This pool of forms can then get
quite
> large. I think complexity is not that bad and replicated elsewhere in this
file.

Acknowledged.

[vabr (Chromium), 2015-11-23 17:47:27]

On 2015/11/23 17:27:51, Mathieu Perreault wrote:
> On 2015/11/23 17:27:07, Mathieu Perreault wrote:
> > Thanks!
> > 
> >
>
https://codereview.chromium.org/1472723003/diff/1/components/autofill/core/br...
> > File components/autofill/core/browser/autofill_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/1472723003/diff/1/components/autofill/core/br...
> > components/autofill/core/browser/autofill_manager.cc:748: //
> |form_signatures|.
> > We invert both lists because most recent forms are at
> > On 2015/11/23 16:59:25, vabr (Chromium) wrote:
> > > Just curious -- was this meant to improve performance? Is it still worth
it
> > with
> > > the std::reverse at the end, and the added complexity?
> > 
> > Yes it's meant to improve performance. With <form>-less Autofill a new form
> > structure is created at every field added. This pool of forms can then get
> quite
> > large. I think complexity is not that bad and replicated elsewhere in this
> file.
> > 
> >
>
https://codereview.chromium.org/1472723003/diff/1/components/autofill/core/br...
> > components/autofill/core/browser/autofill_manager.cc:758: found_signature =
> > true;
> > On 2015/11/23 16:59:25, vabr (Chromium) wrote:
> > > Could you just execute "break;" here and remove |found_signature|? Or did
> you
> > > mean to use |form_signature| after the for loop to test for success?
> > 
> > Of course, I did this too quickly, brain fart.
> 
> Exactly, if you look at the test and the expected server responses, we don't
> group the forms in the response. It's obviously brittle, and we're hoping to
> address this when moving to proto.

SGTM, thank you for the confirmation!

[commit-bot: I haz the power, 2015-11-23 19:28:24]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-11-23 19:29:23]

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