Processing USERNAME reply from Autofill server in Password Manager

Processing USERNAME reply from Autofill server in Password Manager

In this CL it's implemented overwriting username element in parsing of password forms when Autofill server informs which field is username. This will help to fix problems when local heuristics for finding username fails.

It works in the following way:
1. Autofill server reply is propagated to PasswordManager::ProcessAutofillPredictions, where all predictions that don't contain USERNAME or USERNAME_AND_EMAIL_ADDRESS fields are filtered out.
2. Predictions with USERNAME are propageted to PasswordAutofillAgent, where they are saved (in method PasswordAutofillAgent::OnAutofillDataReceived)
3. During password form parsing we use saved predictions to choose correct username (in password_form_conversion_utils.cc in function FindPredictedUsernameElement)
4. It's allowed for forms that were parsed with Autofill predictions to be different from form stored PendingLoginManagers in username_element (in PasswordFormManager::DoesManage) otherwise parsed with autofill data forms would not be accepted.

BUG=467460
Committed: https://crrev.com/89bccb1e2750638fb2fa3684aeb4074c02ff817e
Cr-Commit-Position: refs/heads/master@{#323280}

[dvadym, 2015-03-23 15:32:51]

dvadym@chromium.org changed reviewers:
+ gcasto@chromium.org

[dvadym, 2015-03-23 15:32:53]

Hi Garret,

 Could you please review this CL?

Best regards,
Vadym

[vabr (Chromium), 2015-03-23 16:48:22]

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

[vabr (Chromium), 2015-03-23 16:48:23]

An initial round of nits. :)

I'll have one more pass when the tests are ready, but looks good so far.

Cheers,
Vaclav

https://codereview.chromium.org/1028163002/diff/80001/components/autofill/con...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/1028163002/diff/80001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:114:
WebVector<WebFormControlElement>& control_elements,
No non-const references in arguments. Please use pointers.

https://codereview.chromium.org/1028163002/diff/80001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:123:
&form_data, NULL))
nit: nullptr

https://codereview.chromium.org/1028163002/diff/80001/components/autofill/con...
File components/autofill/content/renderer/password_form_conversion_utils.h
(right):

https://codereview.chromium.org/1028163002/diff/80001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.h:35: const
std::vector<autofill::FormDataPredictions>* form_predictions);
nit: Please add a comment about the new argument.

https://codereview.chromium.org/1028163002/diff/80001/components/password_man...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/1028163002/diff/80001/components/password_man...
components/password_manager/core/browser/password_manager.cc:727: // Leave only
forms that contain fields that useful for password manager.
typo: that useful -> that are useful

https://codereview.chromium.org/1028163002/diff/80001/components/password_man...
File components/password_manager/core/browser/password_manager_driver.h (right):

https://codereview.chromium.org/1028163002/diff/80001/components/password_man...
components/password_manager/core/browser/password_manager_driver.h:49: virtual
void AutofillDataReceived(
This seems non-essential, i.e., if this method is not implemented, the rest of
the password manager will still work. It should have a trivial implementation
instead (and you can remove the one from the Stub* class.

[dvadym, 2015-03-23 17:20:45]

Thanks Vaclav!

https://codereview.chromium.org/1028163002/diff/80001/components/autofill/con...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/1028163002/diff/80001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:114:
WebVector<WebFormControlElement>& control_elements,
On 2015/03/23 16:48:23, vabr (Chromium) wrote:
> No non-const references in arguments. Please use pointers.

Done.

https://codereview.chromium.org/1028163002/diff/80001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:123:
&form_data, NULL))
On 2015/03/23 16:48:23, vabr (Chromium) wrote:
> nit: nullptr

Done.

https://codereview.chromium.org/1028163002/diff/80001/components/autofill/con...
File components/autofill/content/renderer/password_form_conversion_utils.h
(right):

https://codereview.chromium.org/1028163002/diff/80001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.h:35: const
std::vector<autofill::FormDataPredictions>* form_predictions);
On 2015/03/23 16:48:23, vabr (Chromium) wrote:
> nit: Please add a comment about the new argument.

Added comment
// |form_predictions| is Autofill server response, if present it's used for
// overwriting default username element selection.

https://codereview.chromium.org/1028163002/diff/80001/components/password_man...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/1028163002/diff/80001/components/password_man...
components/password_manager/core/browser/password_manager.cc:727: // Leave only
forms that contain fields that useful for password manager.
On 2015/03/23 16:48:23, vabr (Chromium) wrote:
> typo: that useful -> that are useful

Done.

https://codereview.chromium.org/1028163002/diff/80001/components/password_man...
File components/password_manager/core/browser/password_manager_driver.h (right):

https://codereview.chromium.org/1028163002/diff/80001/components/password_man...
components/password_manager/core/browser/password_manager_driver.h:49: virtual
void AutofillDataReceived(
On 2015/03/23 16:48:23, vabr (Chromium) wrote:
> This seems non-essential, i.e., if this method is not implemented, the rest of
> the password manager will still work. It should have a trivial implementation
> instead (and you can remove the one from the Stub* class.

Thanks, it makes sense.

[Garrett Casto, 2015-03-24 01:31:12]

Generally looks good. Most of the comments are style or wordsmithing related.

https://codereview.chromium.org/1028163002/diff/120001/components/autofill/co...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/1028163002/diff/120001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:117:
return;
Nit: Generally we give a line of whitespace after early returns like this.

https://codereview.chromium.org/1028163002/diff/120001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:120:
form, WebFormControlElement(), REQUIRE_AUTOCOMPLETE,
Why REQUIRE_AUTOCOMPLETE and these particular ExtractMask options? Per
AutofillAgent::QueryAutofillSuggestions(), it's possible to get data from forms
even if certain elements have autocomplete="off", so I don't think that we want
to give ourselves this extra requirement. As for the extact mask, we don't
explicitly look at the value or option values, and none of these values affect
SameFormAs(), so I don't think that their extraction is necessary.

https://codereview.chromium.org/1028163002/diff/120001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:124:
return;
Nit: Newline.

https://codereview.chromium.org/1028163002/diff/120001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:127:
form_data.user_submitted = false;
It's a little strange to me that user_submitted is considered form identifying.
It seems similar to the value of the field to me. We may want to ask whoever
reviews this from the Autofill side why this is.

https://codereview.chromium.org/1028163002/diff/120001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:136:
return;
Nit: Newline.

https://codereview.chromium.org/1028163002/diff/120001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:138:
ExtractAutofillableElementsFromSet(*control_elements, REQUIRE_NONE);
What do you think about changing this so that instead of the browser sending a
vector of FormDataPredictions, it sends a map of FormData->FormFieldData for
each form that has a username set. That means that the following code would
instead be something like 

for element in control_element:
WebFormControlElementToFormField(element, &form_field)
if form_field.SameFieldAs(username_field):
...

The advantages are we don't send superfluous data to the renderer (and don't
have to iterate through it repeatedly), we don't have to duplicate checking the
server type (in case we change the logic there), and I think that it makes the
code slightly simpler. What do you think?

https://codereview.chromium.org/1028163002/diff/120001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:149:
WebFormControlElement& element = autofillable_elements[i];
Nit: Move the whitespace up a line. |element| logically goes with the if
statement following it, not the one proceeding it.

https://codereview.chromium.org/1028163002/diff/120001/components/autofill/co...
File components/autofill/core/browser/autofill_type.cc (right):

https://codereview.chromium.org/1028163002/diff/120001/components/autofill/co...
components/autofill/core/browser/autofill_type.cc:141: case
USERNAME_AND_EMAIL_ADDRESS:
The fact that this doesn't return EMAIL might break some things (e.g.
AutofillProfile). The principled way of dealing with this would probably be to
treat these enums as bit masks and "|" them together, but I'm not sure that we
really want to do that. Given that we currently don't care that this returns
USERNAME_FIELD for anything, I would just have this return EMAIL.

https://codereview.chromium.org/1028163002/diff/120001/components/autofill/co...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/1028163002/diff/120001/components/autofill/co...
components/autofill/core/common/password_form.h:241: // If true, this form was
parsed with using of Autofill predictions.
Should be either "with Autofill predictions" or "using Autofill predictions".

https://codereview.chromium.org/1028163002/diff/120001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1028163002/diff/120001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:164: //
username element can be different.
Nit: Should be either "allow the username element to be different" or "the
username element can be different"

[dvadym, 2015-03-24 16:38:50]

Thanks Garrett. I've made changes according to your comments. PTAL.

https://codereview.chromium.org/1028163002/diff/120001/components/autofill/co...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/1028163002/diff/120001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:117:
return;
On 2015/03/24 01:31:12, Garrett Casto wrote:
> Nit: Generally we give a line of whitespace after early returns like this.

Done.

https://codereview.chromium.org/1028163002/diff/120001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:120:
form, WebFormControlElement(), REQUIRE_AUTOCOMPLETE,
On 2015/03/24 01:31:12, Garrett Casto wrote:
> Why REQUIRE_AUTOCOMPLETE and these particular ExtractMask options? Per
> AutofillAgent::QueryAutofillSuggestions(), it's possible to get data from
forms
> even if certain elements have autocomplete="off", so I don't think that we
want
> to give ourselves this extra requirement. As for the extact mask, we don't
> explicitly look at the value or option values, and none of these values affect
> SameFormAs(), so I don't think that their extraction is necessary.

Thanks, it makes sense, I didn't dig into details and took this code from
another place. I changed REQUIRE_AUTOCOMPLETE -> REQUIRE_NONE and mask to
EXTRACT_NONE

https://codereview.chromium.org/1028163002/diff/120001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:124:
return;
On 2015/03/24 01:31:12, Garrett Casto wrote:
> Nit: Newline.

Done.

https://codereview.chromium.org/1028163002/diff/120001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:127:
form_data.user_submitted = false;
On 2015/03/24 01:31:12, Garrett Casto wrote:
> It's a little strange to me that user_submitted is considered form
identifying.
> It seems similar to the value of the field to me. We may want to ask whoever
> reviews this from the Autofill side why this is.

Yeah, for me it also seems strange. Anyway we need someone to review changes
from Autofill side and I'll ask him to review this issue.

https://codereview.chromium.org/1028163002/diff/120001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:136:
return;
On 2015/03/24 01:31:12, Garrett Casto wrote:
> Nit: Newline.

Done.

https://codereview.chromium.org/1028163002/diff/120001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:138:
ExtractAutofillableElementsFromSet(*control_elements, REQUIRE_NONE);
On 2015/03/24 01:31:12, Garrett Casto wrote:
> What do you think about changing this so that instead of the browser sending a
> vector of FormDataPredictions, it sends a map of FormData->FormFieldData for
> each form that has a username set. That means that the following code would
> instead be something like 
> 
> for element in control_element:
> WebFormControlElementToFormField(element, &form_field)
> if form_field.SameFieldAs(username_field):
> ...
> 
> The advantages are we don't send superfluous data to the renderer (and don't
> have to iterate through it repeatedly), we don't have to duplicate checking
the
> server type (in case we change the logic there), and I think that it makes the
> code slightly simpler. What do you think?

Yeah, I also think it's more clear. I'm surprised why map are rarely used in
such situations in Chrome, usually for by vector element is used. I've changed
to map<>.

https://codereview.chromium.org/1028163002/diff/120001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:149:
WebFormControlElement& element = autofillable_elements[i];
On 2015/03/24 01:31:12, Garrett Casto wrote:
> Nit: Move the whitespace up a line. |element| logically goes with the if
> statement following it, not the one proceeding it.

Done.

https://codereview.chromium.org/1028163002/diff/120001/components/autofill/co...
File components/autofill/core/browser/autofill_type.cc (right):

https://codereview.chromium.org/1028163002/diff/120001/components/autofill/co...
components/autofill/core/browser/autofill_type.cc:141: case
USERNAME_AND_EMAIL_ADDRESS:
On 2015/03/24 01:31:12, Garrett Casto wrote:
> The fact that this doesn't return EMAIL might break some things (e.g.
> AutofillProfile). The principled way of dealing with this would probably be to
> treat these enums as bit masks and "|" them together, but I'm not sure that we
> really want to do that. Given that we currently don't care that this returns
> USERNAME_FIELD for anything, I would just have this return EMAIL.

Ok, USERNAME_AND_EMAIL_ADDRESS -> EMAIL group

https://codereview.chromium.org/1028163002/diff/120001/components/autofill/co...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/1028163002/diff/120001/components/autofill/co...
components/autofill/core/common/password_form.h:241: // If true, this form was
parsed with using of Autofill predictions.
On 2015/03/24 01:31:12, Garrett Casto wrote:
> Should be either "with Autofill predictions" or "using Autofill predictions".

Done.

https://codereview.chromium.org/1028163002/diff/120001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1028163002/diff/120001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:164: //
username element can be different.
On 2015/03/24 01:31:12, Garrett Casto wrote:
> Nit: Should be either "allow the username element to be different" or "the
> username element can be different"

Done.

[Garrett Casto, 2015-03-24 23:31:50]

LGTM

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
File components/autofill/content/renderer/password_autofill_agent.h (right):

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
components/autofill/content/renderer/password_autofill_agent.h:181:
FindingUsernameWithAutofillPredictions);
Don't friend here. Instead just send a message in the test as the browser would.
See SimulateOnFillPasswordForm() for an example.

https://codereview.chromium.org/1028163002/diff/160001/components/password_ma...
File
components/password_manager/content/browser/content_password_manager_driver.cc
(right):

https://codereview.chromium.org/1028163002/diff/160001/components/password_ma...
components/password_manager/content/browser/content_password_manager_driver.cc:9:
#include "components/autofill/core/common/form_data_predictions.h"
Don't think that this is necessary anymore.

[vabr (Chromium), 2015-03-25 10:12:34]

Hi Vadym.
LGTM with a collection of mostly nits. :)

For the OWNERS check, I recommend adding
mkwst@ for the IPC parts, and
isherman@ for the non-password autofill.

Cheers,
Vaclav

https://codereview.chromium.org/1028163002/diff/160001/chrome/renderer/autofi...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):

https://codereview.chromium.org/1028163002/diff/160001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1874: // Tests
that in a password form with 2 text input elements the last before
nit: I found that sentence a bit difficult to parse. What about:
// Test that the last plain text field before a password field is chosen as a
username, in a form with 2 plain text fields without username predictions.

https://codereview.chromium.org/1028163002/diff/160001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1889: // Observe
that the PasswordAutofillAgent parse email as username.
grammar: parse -> parses
But, actually, I suggest replacing "parse email" with "identifies the second
field (e-mail)".

https://codereview.chromium.org/1028163002/diff/160001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1893: // Tests
that in a password form with 2 text input elements it's chosen as
The sentence has broken grammar and is hard to understand.
// Tests that username predictions are followed when identifying the username in
a password form with two plain text fields.

https://codereview.chromium.org/1028163002/diff/160001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1919: // Observe
that the PasswordAutofillAgent parse email as username.
Similarly to line 1893, I suggest:
// Observe that the PasswordAutofillAgent identifies the first field as
username.

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
components/autofill/content/common/autofill_messages.h:41:
IPC_STRUCT_TRAITS_END()
Please revert the indenting changes.

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
components/autofill/content/common/autofill_messages.h:106: typedef
std::map<autofill::FormData, autofill::FormFieldData>
Indent these 2 lines 2 spaces to the left and add a blank line after line 107.

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
components/autofill/content/common/autofill_messages.h:202: // which contains
information about username fields.
"information about username fields" is maybe too vague. Could you add one
sentence explaining the meaning of the map? Something like:
|predictions| map forms to their username fields.

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
components/autofill/content/common/autofill_messages.h:203:
IPC_MESSAGE_ROUTED1(AutofillMsg_AutofillPasswordDataReceived,
optional nit: AutofillPasswordDataReceived speaks about passwords, while the
comment above speaks about usernames. What about AutofillUsernameDataReceived?

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:6: 
Here you should #include <vector>.

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:113:
const std::map<autofill::FormData, autofill::FormFieldData>*
I suggest changing this to a const reference, and skip the call completely, if
the pointer to predictions is null.
That sounds like a better practice for performance, and will make the body of
this function shorter and clearer.

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:135:
NOTREACHED();
Did you mean
DCHECK_EQ(autofillable_elements.size(), form_data.fields.size())
?

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:239:
WebInputElement predicted_username_element;
For performance reasons, I would also recommend putting the prediction check
before the for loop on lines 169-237, and making sure that if the username is
set already, we skip any parts of code trying to guess it.

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:240:
FindPredictedUsernameElement(form, form_predictions, &control_elements,
Following up on my comment on line 113, I suggest only calling this if
form_predictions are not null.

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
File components/autofill/content/renderer/password_form_conversion_utils.h
(right):

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.h:9:
#include <vector>
Seems unused.

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
File components/autofill/core/common/password_form.cc (right):

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
components/autofill/core/common/password_form.cc:63: skip_zero_click ==
form.skip_zero_click;
Please add a test for parsed_using_autofill_predictions.

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
components/autofill/core/common/password_form.cc:101: << " skip_next_zero_click:
" << form.skip_zero_click;
Please add parsed_using_autofill_predictions (however, beware of
https://codereview.chromium.org/1022823002/ being almost ready to land, so the
logging might change).

https://codereview.chromium.org/1028163002/diff/160001/components/password_ma...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/1028163002/diff/160001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:729: for (auto form
: forms) {
TL;DR autofill::FormStructure* is short enough that replacing it with auto
actually harms readability.

More detail: In general, the trade-off between auto and a full type is:
auto helps avoid German-style words in type names
precise type helps to understand what the iterating variable is

Because autofill::FormStructure* is far from German-style :), the benefits of
auto are too small.

https://codereview.chromium.org/1028163002/diff/160001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:730: for (auto it =
form->begin(); it != form->end(); ++it) {
Here I would also vote against auto, although the direct type name is much more
German-style than above.
for (std::vector<AutofillField*>::const_iterator field = form->begin(); field !=
form->end(); ++field) {...}

The reason is that FormStructure is not a container, so it is at all not clear
what begin() and end() do on it. Also please note changing |it| to a more
meaningful |field|, so that it is clear that the iterator represents a field.

https://codereview.chromium.org/1028163002/diff/160001/components/password_ma...
File components/password_manager/core/browser/password_manager_driver.h (right):

https://codereview.chromium.org/1028163002/diff/160001/components/password_ma...
components/password_manager/core/browser/password_manager_driver.h:49: //
Notifies the driver that account creation |forms| were found.
I'm a bit puzzled about this comment. Is this not about username selection
rather than account creation?

https://codereview.chromium.org/1028163002/diff/160001/components/password_ma...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/1028163002/diff/160001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1040:
TEST_F(PasswordManagerTest, FormSubmittedChangedWithAutofillResponse) {
The test name is a bit unclear, and the comment below does not help (it says
nothing about changing usernames based on username predictions).

https://codereview.chromium.org/1028163002/diff/160001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1051: //
We simulate that |username_element| is changed due to autofill response.
The comment does not completely explain what is being done.
What about:
// Simulate that based on autofill server username prediction, the username of
the form changed from the default candidate ("Email") to something else. Set the
parsed_using_autofill_predictions bit to true to make sure that choice of
username is trusted further in the code.

https://codereview.chromium.org/1028163002/diff/160001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1070:
ASSERT_TRUE(form_to_save.get());
nit: You can drop ".get()".

https://codereview.chromium.org/1028163002/diff/160001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1071:
EXPECT_CALL(*store_, AddLogin(FormMatches(form)));
At this point, it is still not clear to me what this test tests. If not for
parsed_using_autofill_predictions being true, would this fail? Maybe this
warrants an explanation in a code comment.

[Mike West, 2015-03-25 10:24:55]

mkwst@chromium.org changed reviewers:
+ mkwst@chromium.org

[Mike West, 2015-03-25 10:24:57]

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
components/autofill/content/common/autofill_messages.h:41:
IPC_STRUCT_TRAITS_END()
On 2015/03/25 at 10:12:33, vabr (Chromium) wrote:
> Please revert the indenting changes.

+1. This makes it quite difficult to review the IPC changes.

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
components/autofill/content/renderer/password_autofill_agent.cc:1212: void
PasswordAutofillAgent::OnAutofillDataReceived(
Nit: This should be named in line what whatever you end up naming the message.
That is, `OnAutofillPasswordDataReceived` at the moment.

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
components/autofill/content/renderer/password_autofill_agent.cc:1214:
form_predictions_ = predictions;
Is there any sanity checking you can do here? How much data do you reasonably
expect to receive?

[dvadym, 2015-03-25 16:34:08]

Thanks Garrett, Vaclav and Mike!
 I've addressed all your comments in PatchSet 10.

https://codereview.chromium.org/1028163002/diff/160001/chrome/renderer/autofi...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):

https://codereview.chromium.org/1028163002/diff/160001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1874: // Tests
that in a password form with 2 text input elements the last before
On 2015/03/25 10:12:33, vabr (Chromium) wrote:
> nit: I found that sentence a bit difficult to parse. What about:
> // Test that the last plain text field before a password field is chosen as a
> username, in a form with 2 plain text fields without username predictions.

Done.

https://codereview.chromium.org/1028163002/diff/160001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1889: // Observe
that the PasswordAutofillAgent parse email as username.
On 2015/03/25 10:12:33, vabr (Chromium) wrote:
> grammar: parse -> parses
> But, actually, I suggest replacing "parse email" with "identifies the second
> field (e-mail)".

Done.

https://codereview.chromium.org/1028163002/diff/160001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1893: // Tests
that in a password form with 2 text input elements it's chosen as
On 2015/03/25 10:12:33, vabr (Chromium) wrote:
> The sentence has broken grammar and is hard to understand.
> // Tests that username predictions are followed when identifying the username
in
> a password form with two plain text fields.

Done.

https://codereview.chromium.org/1028163002/diff/160001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1919: // Observe
that the PasswordAutofillAgent parse email as username.
On 2015/03/25 10:12:33, vabr (Chromium) wrote:
> Similarly to line 1893, I suggest:
> // Observe that the PasswordAutofillAgent identifies the first field as
> username.

Done.

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
components/autofill/content/common/autofill_messages.h:41:
IPC_STRUCT_TRAITS_END()
On 2015/03/25 10:24:56, Mike West wrote:
> On 2015/03/25 at 10:12:33, vabr (Chromium) wrote:
> > Please revert the indenting changes.
> 
> +1. This makes it quite difficult to review the IPC changes.

Sorry, I didn't realize that indenting was changed, fixed.

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
components/autofill/content/common/autofill_messages.h:202: // which contains
information about username fields.
On 2015/03/25 10:12:33, vabr (Chromium) wrote:
> "information about username fields" is maybe too vague. Could you add one
> sentence explaining the meaning of the map? Something like:
> |predictions| map forms to their username fields.

Done.

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
components/autofill/content/common/autofill_messages.h:203:
IPC_MESSAGE_ROUTED1(AutofillMsg_AutofillPasswordDataReceived,
On 2015/03/25 10:12:33, vabr (Chromium) wrote:
> optional nit: AutofillPasswordDataReceived speaks about passwords, while the
> comment above speaks about usernames. What about AutofillUsernameDataReceived?

Done.

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
components/autofill/content/renderer/password_autofill_agent.cc:1212: void
PasswordAutofillAgent::OnAutofillDataReceived(
On 2015/03/25 10:24:57, Mike West wrote:
> Nit: This should be named in line what whatever you end up naming the message.
> That is, `OnAutofillPasswordDataReceived` at the moment.

Done.

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
components/autofill/content/renderer/password_autofill_agent.cc:1214:
form_predictions_ = predictions;
On 2015/03/25 10:24:57, Mike West wrote:
> Is there any sanity checking you can do here? How much data do you reasonably
> expect to receive?

I don't expect anything in code. In case where it should work I suppose that
forms corresponding to FormData in |predictions| are present on the current
page. But it may not be true, for example when JavaScript changes <form> element
on the page. But in such cases it will simply not work.

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
File components/autofill/content/renderer/password_autofill_agent.h (right):

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
components/autofill/content/renderer/password_autofill_agent.h:181:
FindingUsernameWithAutofillPredictions);
On 2015/03/24 23:31:50, Garrett Casto wrote:
> Don't friend here. Instead just send a message in the test as the browser
would.
> See SimulateOnFillPasswordForm() for an example.

Done.

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:113:
const std::map<autofill::FormData, autofill::FormFieldData>*
On 2015/03/25 10:12:33, vabr (Chromium) wrote:
> I suggest changing this to a const reference, and skip the call completely, if
> the pointer to predictions is null.
> That sounds like a better practice for performance, and will make the body of
> this function shorter and clearer.

Done.

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:135:
NOTREACHED();
On 2015/03/25 10:12:33, vabr (Chromium) wrote:
> Did you mean
> DCHECK_EQ(autofillable_elements.size(), form_data.fields.size())
> ?

Thanks, I didn't know about DCHECK_EQ

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:239:
WebInputElement predicted_username_element;
On 2015/03/25 10:12:33, vabr (Chromium) wrote:
> For performance reasons, I would also recommend putting the prediction check
> before the for loop on lines 169-237, and making sure that if the username is
> set already, we skip any parts of code trying to guess it.

After offline discussing, it was decided to leave as now, namely sever side
prediction has priority over all other means to guess username (including
autocomplete='username').

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:240:
FindPredictedUsernameElement(form, form_predictions, &control_elements,
On 2015/03/25 10:12:33, vabr (Chromium) wrote:
> Following up on my comment on line 113, I suggest only calling this if
> form_predictions are not null.

Done.

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
File components/autofill/content/renderer/password_form_conversion_utils.h
(right):

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.h:9:
#include <vector>
On 2015/03/25 10:12:33, vabr (Chromium) wrote:
> Seems unused.

Done.

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
File components/autofill/core/common/password_form.cc (right):

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
components/autofill/core/common/password_form.cc:63: skip_zero_click ==
form.skip_zero_click;
On 2015/03/25 10:12:33, vabr (Chromium) wrote:
> Please add a test for parsed_using_autofill_predictions.

Done.

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
components/autofill/core/common/password_form.cc:101: << " skip_next_zero_click:
" << form.skip_zero_click;
On 2015/03/25 10:12:33, vabr (Chromium) wrote:
> Please add parsed_using_autofill_predictions (however, beware of
> https://codereview.chromium.org/1022823002/ being almost ready to land, so the
> logging might change).

Done.

https://codereview.chromium.org/1028163002/diff/160001/components/password_ma...
File
components/password_manager/content/browser/content_password_manager_driver.cc
(right):

https://codereview.chromium.org/1028163002/diff/160001/components/password_ma...
components/password_manager/content/browser/content_password_manager_driver.cc:9:
#include "components/autofill/core/common/form_data_predictions.h"
On 2015/03/24 23:31:50, Garrett Casto wrote:
> Don't think that this is necessary anymore.

Done.

https://codereview.chromium.org/1028163002/diff/160001/components/password_ma...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/1028163002/diff/160001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:729: for (auto form
: forms) {
On 2015/03/25 10:12:34, vabr (Chromium) wrote:
> TL;DR autofill::FormStructure* is short enough that replacing it with auto
> actually harms readability.
> 
> More detail: In general, the trade-off between auto and a full type is:
> auto helps avoid German-style words in type names
> precise type helps to understand what the iterating variable is
> 
> Because autofill::FormStructure* is far from German-style :), the benefits of
> auto are too small.

Done.

https://codereview.chromium.org/1028163002/diff/160001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:730: for (auto it =
form->begin(); it != form->end(); ++it) {
On 2015/03/25 10:12:34, vabr (Chromium) wrote:
> Here I would also vote against auto, although the direct type name is much
more
> German-style than above.
> for (std::vector<AutofillField*>::const_iterator field = form->begin(); field
!=
> form->end(); ++field) {...}
> 
> The reason is that FormStructure is not a container, so it is at all not clear
> what begin() and end() do on it. Also please note changing |it| to a more
> meaningful |field|, so that it is clear that the iterator represents a field.

Done.

https://codereview.chromium.org/1028163002/diff/160001/components/password_ma...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/1028163002/diff/160001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1040:
TEST_F(PasswordManagerTest, FormSubmittedChangedWithAutofillResponse) {
On 2015/03/25 10:12:34, vabr (Chromium) wrote:
> The test name is a bit unclear, and the comment below does not help (it says
> nothing about changing usernames based on username predictions).

Comment was fixed to 
  // Test that observing that a newly submitted form differs from provisionally
  // saved one due to autoflll predictions caused another field to be identified
as username.

https://codereview.chromium.org/1028163002/diff/160001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1051: //
We simulate that |username_element| is changed due to autofill response.
On 2015/03/25 10:12:34, vabr (Chromium) wrote:
> The comment does not completely explain what is being done.
> What about:
> // Simulate that based on autofill server username prediction, the username of
> the form changed from the default candidate ("Email") to something else. Set
the
> parsed_using_autofill_predictions bit to true to make sure that choice of
> username is trusted further in the code.

Done.

https://codereview.chromium.org/1028163002/diff/160001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1070:
ASSERT_TRUE(form_to_save.get());
On 2015/03/25 10:12:34, vabr (Chromium) wrote:
> nit: You can drop ".get()".

Done.

https://codereview.chromium.org/1028163002/diff/160001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1071:
EXPECT_CALL(*store_, AddLogin(FormMatches(form)));
On 2015/03/25 10:12:34, vabr (Chromium) wrote:
> At this point, it is still not clear to me what this test tests. If not for
> parsed_using_autofill_predictions being true, would this fail? Maybe this
> warrants an explanation in a code comment.

I think that other added comments explain this. Namely that the form will be
rejected if this bit is not set

[dvadym, 2015-03-25 17:15:19]

dvadym@chromium.org changed reviewers:
+ isherman@chromium.org

[dvadym, 2015-03-25 17:15:20]

Hi Ilya,

 Could you please review changes in this CL that correspond to changes in
autofill code. These changes are about transfering messages from Autofill server
to PasswordManager and then to PasswordAutofillAgent and back. They are in files
  components/autofill/core/browser/*
  components/autofill/content/common/*
  components/autofill/content/browser/*
  chrome/browser/ui/autofill/chrome_autofill_client.[h|cc]
  
Best regards,
Vadym

[Ilya Sherman, 2015-03-26 00:30:04]

I'm not super happy about the round-trip through the renderer that's being used
here.  Is it much harder to keep the logic constrained to the browser process?

https://codereview.chromium.org/1028163002/diff/180001/chrome/renderer/autofi...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):

https://codereview.chromium.org/1028163002/diff/180001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:281:
blink::WebInputElement GetInputElementByID(const char* id) {
nit: Please pass either a StringPiece or a "const std::string&"

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
components/autofill/content/common/autofill_messages.h:106:
FormDataFieldDataMap;
I'm really not thrilled about defining a typedef in an IPC messages file. 
Please inline this type name.

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
components/autofill/content/common/autofill_messages.h:205: FormDataFieldDataMap
/* predictions */)
Why is an IPC message needed?  It seems like this information should be managed
by the browser process.  The renderer process should not be involved.  What am I
missing?

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
File components/autofill/content/renderer/password_autofill_agent.h (right):

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
components/autofill/content/renderer/password_autofill_agent.h:106:
std::map<autofill::FormData, autofill::FormFieldData> form_predictions_;
Please move this to be declared where the other member vars are declared.

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:116:
WebInputElement& predicted_username_element) {
You should never pass a value by non-const reference.  Please use a pointer
here.

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
File components/autofill/content/renderer/password_form_conversion_utils.h
(right):

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.h:38:
form_predictions);
Please pass this by const-reference.

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
File components/autofill/core/browser/autofill_metrics.cc (right):

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
components/autofill/core/browser/autofill_metrics.cc:38: GROUP_USERNAME,
Please update histograms.xml for this change.

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
File components/autofill/core/browser/field_types.h (right):

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
components/autofill/core/browser/field_types.h:238: USERNAME_FIELD,
nit: "Field" seems redundant.

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
File components/autofill/core/common/password_form.cc (right):

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
components/autofill/core/common/password_form.cc:104: << "
parsed_using_autofill_predictions:"
nit: Please add a space after the colon.

[vabr (Chromium), 2015-03-26 09:43:55]

Hi Vadym,
Some more comments under way. :)

Thanks!
Vaclav

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:6: 
On 2015/03/25 10:12:33, vabr (Chromium) wrote:
> Here you should #include <vector>.

Ping.

https://codereview.chromium.org/1028163002/diff/160001/components/password_ma...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/1028163002/diff/160001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1071:
EXPECT_CALL(*store_, AddLogin(FormMatches(form)));
On 2015/03/25 16:34:08, dvadym wrote:
> On 2015/03/25 10:12:34, vabr (Chromium) wrote:
> > At this point, it is still not clear to me what this test tests. If not for
> > parsed_using_autofill_predictions being true, would this fail? Maybe this
> > warrants an explanation in a code comment.
> 
> I think that other added comments explain this. Namely that the form will be
> rejected if this bit is not set

Acknowledged.

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:239: //
Now server predictions has more priority then autocomplete='username'. It
An attempt at making this comment shorter and clearer:
// Let server predictions override the selection of the username field. This
allows instant adjusting without changing Chromium code.

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
File components/autofill/content/renderer/password_form_conversion_utils.h
(right):

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.h:38:
form_predictions);
On 2015/03/26 00:30:04, Ilya Sherman wrote:
> Please pass this by const-reference.

Ilya -- I believe the pointer was used to allow specifying no predictions easily
by nullptr (see, e.g.,
components/autofill/content/renderer/password_generation_agent.cc). Do you
object to that?

https://codereview.chromium.org/1028163002/diff/180001/components/password_ma...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/1028163002/diff/180001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1041: //
Test that observing that a newly submitted form differs from provisionally
I still struggle to understand, partly because the sentence spans 3 lines. This
seems to tell me that observing a difference in the forms should influence
username identification. But I doubt that is true, and it is not what is being
tested.

This verifies that if the provisionally saved and observed forms differ in the
choice of the username, the saving still succeeds, as long as the changed form
is marked "parsed using autofill predictions".

What about using the above explanation?

Also, why would the server-side predictions apply to the provisionally saved,
but not the observed form?

https://codereview.chromium.org/1028163002/diff/180001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1057: //
will rejected as not equal to provionally saved one.
will rejected -> will be rejected
provionally -> the provisionally

https://codereview.chromium.org/1028163002/diff/180001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1057: //
will rejected as not equal to provionally saved one.
Aren't we telling the password manager, that |form| is _the provisionally saved
one_ on line 1061? How can it differ from itself? Did you mean "the observed
one"?

[dvadym, 2015-03-26 12:25:08]

Thanks Ilya!
Sorry, maybe my comment was a little bit confusing (about sending data back to
the manager).
I've added only sending data from browser to renderer. Then using this data
renderer (in PasswordAutofillAgent) can do additional processing after
submission of form. And then renderer sends the same amount of data as before
this CL (namely PasswordForm).
About the question why we need to send data to renderer and make additional
processing in it. I'm also not very happy with it, but it's how Password storing
functionality work, namely:
 in renderer: PasswordAutofillAgent parses html forms (for this
CreatePasswordForm from password_form_conversion_utils.cc is called), and as
result PasswordForm is created, that contains information about password and
username element and no html.
 in browser: PasswordManager doesn't have html form, it has only PasswordForm
where values for password and username are already set. And using autofill
heuristics we can't change anything, since we don't have value of fields that is
username according to predictions.
 In my implementation I've changed parsing of forms in renderer in order to
create PasswordForm with correct username.
 
Maybe it would be better to make more processing in browser, for this we need to
send more information about html form renderer. But it seems to be quite big
refactoring and it's not clear if it's improve password manager.

[dvadym, 2015-03-26 12:29:13]

Thanks Ilya and Vaclav!

 I've addressed your comments in PatchSet 12, PTAL.

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/1028163002/diff/160001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:6: 
On 2015/03/25 10:12:33, vabr (Chromium) wrote:
> Here you should #include <vector>.

Done.

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
components/autofill/content/common/autofill_messages.h:106:
FormDataFieldDataMap;
On 2015/03/26 00:30:04, Ilya Sherman wrote:
> I'm really not thrilled about defining a typedef in an IPC messages file. 
> Please inline this type name.

I can't do it directly, because comma after FormData is considered as comma
between arguments in macros IPC_MESSAGE_ROUTED1, that causes compilation errors.
I didn't find other cases how to do with map. Can you advice what to do with map
args?

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
components/autofill/content/common/autofill_messages.h:205: FormDataFieldDataMap
/* predictions */)
On 2015/03/26 00:30:04, Ilya Sherman wrote:
> Why is an IPC message needed?  It seems like this information should be
managed
> by the browser process.  The renderer process should not be involved.  What am
I
> missing?

I've tried to explain it in separate message.

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
File components/autofill/content/renderer/password_autofill_agent.h (right):

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
components/autofill/content/renderer/password_autofill_agent.h:106:
std::map<autofill::FormData, autofill::FormFieldData> form_predictions_;
On 2015/03/26 00:30:04, Ilya Sherman wrote:
> Please move this to be declared where the other member vars are declared.

Done.

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:116:
WebInputElement& predicted_username_element) {
On 2015/03/26 00:30:04, Ilya Sherman wrote:
> You should never pass a value by non-const reference.  Please use a pointer
> here.

Done.

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:239: //
Now server predictions has more priority then autocomplete='username'. It
On 2015/03/26 09:43:55, vabr (Chromium) wrote:
> An attempt at making this comment shorter and clearer:
> // Let server predictions override the selection of the username field. This
> allows instant adjusting without changing Chromium code.

Done.

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
File components/autofill/core/browser/autofill_metrics.cc (right):

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
components/autofill/core/browser/autofill_metrics.cc:38: GROUP_USERNAME,
On 2015/03/26 00:30:04, Ilya Sherman wrote:
> Please update histograms.xml for this change.

I've added in one place in histograms.xml about usernames, is it enough?

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
File components/autofill/core/browser/field_types.h (right):

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
components/autofill/core/browser/field_types.h:238: USERNAME_FIELD,
On 2015/03/26 00:30:04, Ilya Sherman wrote:
> nit: "Field" seems redundant.

I did it to be consistent with PASSWORD_FIELD. And after changing to USERNAME, I
realized why I can't name it USERNAME: in enum ServerFieldType it's already
defined element USERNAME. So it seems USERNAME_FIELD is ok name for this item.
What do you think?

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
File components/autofill/core/common/password_form.cc (right):

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
components/autofill/core/common/password_form.cc:104: << "
parsed_using_autofill_predictions:"
On 2015/03/26 00:30:04, Ilya Sherman wrote:
> nit: Please add a space after the colon.

Done.

https://codereview.chromium.org/1028163002/diff/180001/components/password_ma...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/1028163002/diff/180001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1041: //
Test that observing that a newly submitted form differs from provisionally
On 2015/03/26 09:43:55, vabr (Chromium) wrote:
> I still struggle to understand, partly because the sentence spans 3 lines.
This
> seems to tell me that observing a difference in the forms should influence
> username identification. But I doubt that is true, and it is not what is being
> tested.
> 
> This verifies that if the provisionally saved and observed forms differ in the
> choice of the username, the saving still succeeds, as long as the changed form
> is marked "parsed using autofill predictions".
> 
> What about using the above explanation?
> 
> Also, why would the server-side predictions apply to the provisionally saved,
> but not the observed form?

Thanks, comment for test changed to yours.

https://codereview.chromium.org/1028163002/diff/180001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1057: //
will rejected as not equal to provionally saved one.
On 2015/03/26 09:43:55, vabr (Chromium) wrote:
> will rejected -> will be rejected
> provionally -> the provisionally

Done.

https://codereview.chromium.org/1028163002/diff/180001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1057: //
will rejected as not equal to provionally saved one.
On 2015/03/26 09:43:55, vabr (Chromium) wrote:
> Aren't we telling the password manager, that |form| is _the provisionally
saved
> one_ on line 1061? How can it differ from itself? Did you mean "the observed
> one"?

Added comment 
  // Note that during initial parsing we don't have yet autofill server
predictions, that's why observed form and submitted form may be different.

[vabr (Chromium), 2015-03-26 12:49:46]

LGTM with some minor nits.
Cheers,
Vaclav

https://codereview.chromium.org/1028163002/diff/220001/components/password_ma...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/1028163002/diff/220001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1063: //
will rejected as not equal to the observed one. Note that during
typo: rejected -> be rejected

https://codereview.chromium.org/1028163002/diff/220001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1064: //
initial parsing we don't have yet autofill server predictions, that's why
nit: move "yet" three words to the right

[dvadym, 2015-03-26 13:38:27]

Thanks Vaclav, fixed (PatchSet 13)

https://codereview.chromium.org/1028163002/diff/220001/components/password_ma...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/1028163002/diff/220001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1063: //
will rejected as not equal to the observed one. Note that during
On 2015/03/26 12:49:46, vabr (Chromium) wrote:
> typo: rejected -> be rejected

Done.

https://codereview.chromium.org/1028163002/diff/220001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1064: //
initial parsing we don't have yet autofill server predictions, that's why
On 2015/03/26 12:49:46, vabr (Chromium) wrote:
> nit: move "yet" three words to the right

Done.

[dvadym, 2015-03-27 18:49:01]

https://codereview.chromium.org/1028163002/diff/180001/chrome/renderer/autofi...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):

https://codereview.chromium.org/1028163002/diff/180001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:281:
blink::WebInputElement GetInputElementByID(const char* id) {
On 2015/03/26 00:30:04, Ilya Sherman wrote:
> nit: Please pass either a StringPiece or a "const std::string&"

Done.

[Ilya Sherman, 2015-03-31 00:45:04]

I guess LGTM (% nits), but it would be really nice to reduce the amount of work
that the renderer does.  IMO, ideally, the renderer should do minimal work in
order to pass needed information up to the browser process, and the meaty stuff
should happen there.

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
File components/autofill/core/browser/autofill_metrics.cc (right):

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
components/autofill/core/browser/autofill_metrics.cc:38: GROUP_USERNAME,
On 2015/03/26 12:29:13, dvadym wrote:
> On 2015/03/26 00:30:04, Ilya Sherman wrote:
> > Please update histograms.xml for this change.
> 
> I've added in one place in histograms.xml about usernames, is it enough?

Yes, thanks :)

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
File components/autofill/core/browser/field_types.h (right):

https://codereview.chromium.org/1028163002/diff/180001/components/autofill/co...
components/autofill/core/browser/field_types.h:238: USERNAME_FIELD,
On 2015/03/26 12:29:13, dvadym wrote:
> On 2015/03/26 00:30:04, Ilya Sherman wrote:
> > nit: "Field" seems redundant.
> 
> I did it to be consistent with PASSWORD_FIELD. And after changing to USERNAME,
I
> realized why I can't name it USERNAME: in enum ServerFieldType it's already
> defined element USERNAME. So it seems USERNAME_FIELD is ok name for this item.
> What do you think?

Ah, fair enough.  Okay :)

https://codereview.chromium.org/1028163002/diff/260001/components/autofill/co...
File components/autofill/content/renderer/password_autofill_agent.h (right):

https://codereview.chromium.org/1028163002/diff/260001/components/autofill/co...
components/autofill/content/renderer/password_autofill_agent.h:284: // Contains
information which field is username for forms on the page.
nit: "Contains server predictions for which field is the username field ..."

https://codereview.chromium.org/1028163002/diff/260001/components/autofill/co...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/1028163002/diff/260001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:210:
*toWebInputElement(&autofillable_elements[i]);
Note that toWebInputElement() can return a null pointer.  I think it's probably
wise to have an explicit null check here.

https://codereview.chromium.org/1028163002/diff/260001/components/autofill/co...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/1028163002/diff/260001/components/autofill/co...
components/autofill/core/common/password_form.h:260: bool
parsed_using_autofill_predictions;
nit: I'd name this with a "was_" prefix, i.e.
"was_parsed_using_autofill_predictions"

[Ilya Sherman, 2015-03-31 00:45:08]

I guess LGTM (% nits), but it would be really nice to reduce the amount of work
that the renderer does.  IMO, ideally, the renderer should do minimal work in
order to pass needed information up to the browser process, and the meaty stuff
should happen there.

[dvadym, 2015-03-31 09:54:03]

Thanks Ilya!
 I've addressed all your comments in PatchSet 15.

 Yeah, it would be nice to move more work to Browser.

https://codereview.chromium.org/1028163002/diff/260001/components/autofill/co...
File components/autofill/content/renderer/password_autofill_agent.h (right):

https://codereview.chromium.org/1028163002/diff/260001/components/autofill/co...
components/autofill/content/renderer/password_autofill_agent.h:284: // Contains
information which field is username for forms on the page.
On 2015/03/31 00:45:04, Ilya Sherman wrote:
> nit: "Contains server predictions for which field is the username field ..."

Done.

https://codereview.chromium.org/1028163002/diff/260001/components/autofill/co...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/1028163002/diff/260001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils.cc:210:
*toWebInputElement(&autofillable_elements[i]);
On 2015/03/31 00:45:04, Ilya Sherman wrote:
> Note that toWebInputElement() can return a null pointer.  I think it's
probably
> wise to have an explicit null check here.

Thanks, it makes sense. Done

https://codereview.chromium.org/1028163002/diff/260001/components/autofill/co...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/1028163002/diff/260001/components/autofill/co...
components/autofill/core/common/password_form.h:260: bool
parsed_using_autofill_predictions;
On 2015/03/31 00:45:04, Ilya Sherman wrote:
> nit: I'd name this with a "was_" prefix, i.e.
> "was_parsed_using_autofill_predictions"

Done.

[Mike West, 2015-03-31 10:18:35]

IPC LGTM % nits.

https://codereview.chromium.org/1028163002/diff/280001/components/autofill/co...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/1028163002/diff/280001/components/autofill/co...
components/autofill/content/common/autofill_messages.h:105: typedef
std::map<autofill::FormData, autofill::FormFieldData>
Nit: I think we now prefer `using FormDataFieldDataMap = std::map<>;` to
`typedef` (https://chromium-cpp.appspot.com/).

Skimming through other `_messages.h` files, typdefing maps is somewhat common.
See `extension_messages.h` for instance... As discussed, please add a comment
here explaining the reason for the definition (It would also be reasonable to
replicate the definition in PasswordAutofillAgent so you can use the same name
in both places).

[dvadym, 2015-03-31 11:37:26]

Thanks Mike,

 I've addressed your comments in PatchSet 16

https://codereview.chromium.org/1028163002/diff/280001/components/autofill/co...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/1028163002/diff/280001/components/autofill/co...
components/autofill/content/common/autofill_messages.h:105: typedef
std::map<autofill::FormData, autofill::FormFieldData>
On 2015/03/31 10:18:34, Mike West wrote:
> Nit: I think we now prefer `using FormDataFieldDataMap = std::map<>;` to
> `typedef` (https://chromium-cpp.appspot.com/).
> 
> Skimming through other `_messages.h` files, typdefing maps is somewhat common.
> See `extension_messages.h` for instance... As discussed, please add a comment
> here explaining the reason for the definition (It would also be reasonable to
> replicate the definition in PasswordAutofillAgent so you can use the same name
> in both places).

Changed to using.
I've added comment
// IPC_MESSAGE macros fail on the std::map, when expanding. We need to define
// a type to avoid that.

[dvadym, 2015-04-01 08:37:16]

The CQ bit was checked by dvadym@chromium.org

[dvadym, 2015-04-01 08:37:17]

The patchset sent to the CQ was uploaded after l-g-t-m from gcasto@chromium.org,
vabr@chromium.org, isherman@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1028163002/#ps300001
(title: "Addressed reviewer comments")

[commit-bot: I haz the power, 2015-04-01 08:37:31]

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

[commit-bot: I haz the power, 2015-04-01 08:46:48]

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

[commit-bot: I haz the power, 2015-04-01 08:46:50]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[dvadym, 2015-04-01 09:56:24]

The CQ bit was checked by dvadym@chromium.org to run a CQ dry run

[dvadym, 2015-04-01 09:56:25]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
isherman@chromium.org, gcasto@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1028163002/#ps340001
(title: "Fix compilation errors for Android and IOS")

[commit-bot: I haz the power, 2015-04-01 09:57:19]

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

[commit-bot: I haz the power, 2015-04-01 10:07:21]

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

[commit-bot: I haz the power, 2015-04-01 10:07:24]

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

[dvadym, 2015-04-01 10:38:01]

The CQ bit was checked by dvadym@chromium.org to run a CQ dry run

[dvadym, 2015-04-01 10:38:02]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
isherman@chromium.org, gcasto@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1028163002/#ps360001
(title: "Fix mac enum compilation error")

[commit-bot: I haz the power, 2015-04-01 10:38:26]

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

[commit-bot: I haz the power, 2015-04-01 10:50:13]

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

[commit-bot: I haz the power, 2015-04-01 10:50:15]

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

[dvadym, 2015-04-01 11:35:25]

dvadym@chromium.org changed reviewers:
+ dconnelly@chromium.org

[dvadym, 2015-04-01 11:35:27]

Hi Daniel,

 Could you please review IOS changes:
  components/autofill/ios/browser/autofill_driver_ios.h
  components/autofill/ios/browser/autofill_driver_ios.mm ?

Best regards,
Vadym

[dvadym, 2015-04-01 11:45:39]

dvadym@chromium.org changed reviewers:
+ boliu@chromium.org

[dvadym, 2015-04-01 11:45:40]

Hi Bo,

Could you please review Android changes:
  android_webview/native/aw_autofill_client.h
  android_webview/native/aw_autofill_client.cc ?
  
Best regards,
Vadym

[dconnelly, 2015-04-01 14:36:54]

This is not going to work on iOS. Please file a new iOS bug, assign it to me,
link this CL and the design doc, and mark which release we need this for.

ios autofill driver change LGTM

[boliu, 2015-04-01 15:32:09]

android_webview rs lgtm

[dvadym, 2015-04-01 16:03:52]

The CQ bit was checked by dvadym@chromium.org

[commit-bot: I haz the power, 2015-04-01 16:04:15]

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

[commit-bot: I haz the power, 2015-04-01 16:15:20]

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

[commit-bot: I haz the power, 2015-04-01 16:15:23]

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

[dvadym, 2015-04-01 16:31:05]

The CQ bit was checked by dvadym@chromium.org

[dvadym, 2015-04-01 16:31:06]

The patchset sent to the CQ was uploaded after l-g-t-m from gcasto@chromium.org,
boliu@chromium.org, mkwst@chromium.org, vabr@chromium.org,
isherman@chromium.org, dconnelly@chromium.org
Link to the patchset: https://codereview.chromium.org/1028163002/#ps380001
(title: "Rebase")

[commit-bot: I haz the power, 2015-04-01 16:31:41]

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

[commit-bot: I haz the power, 2015-04-01 17:44:27]

Committed patchset #20 (id:380001)

[commit-bot: I haz the power, 2015-04-01 17:45:04]

Patchset 20 (id:??) landed as
https://crrev.com/89bccb1e2750638fb2fa3684aeb4074c02ff817e
Cr-Commit-Position: refs/heads/master@{#323280}

[stgao, 2015-04-03 00:00:33]

stgao@chromium.org changed reviewers:
+ stgao@chromium.org

[stgao, 2015-04-03 00:00:34]

FYI: it seems this CL is related to quite a few compile failures in
chromium.lkgr.

https://build.chromium.org/p/chromium.lkgr/builders/Win%20ASan%20Release/buil...
https://build.chromium.org/p/chromium.lkgr/builders/Win%20Asan%20Release%20Pr...
https://build.chromium.org/p/chromium.lkgr/builders/ASan%20Release%20(32-bit%...