Fix of password autofilling on hidden passwords forms.

Fix of password autofilling on hidden passwords forms.

On CL https://codereview.chromium.org/1408423003/ it was implemented that hidden password fields are not taken into consideration. But it breaks sites on which password form is hidden initially and when the user clicks sign-in the form appear. The best solution seems to be to consider hidden passwords fields but visible password field should have priority

BUG=551461
Committed: https://crrev.com/59e5da4aef356c4ece1cb3a3e4320c633eb99b2a
Cr-Commit-Position: refs/heads/master@{#358078}

[dvadym, 2015-11-05 14:11:43]

dvadym@chromium.org changed reviewers:
+ vabr@google.com

[dvadym, 2015-11-05 14:11:44]

Hi Vaclav,

 Could you please review this CL?

Regards,
Vadym

[dvadym, 2015-11-05 15:07:19]

Description was changed from

==========
Fix for proccessing of hidden fields

BUG=551461
==========

to

==========
Fix of password autofilling on hidden passwords forms.

On CL https://codereview.chromium.org/1408423003/ it was implemented that hidden
password fields are not taken into consideration. But it breaks sites on which
password form is hidden initially and when the user clicks sign-in the form
appear. The best solution seems to be to consider hidden passwords fields but
visible password field should have priority

BUG=551461
==========

[vabr (Chromium), 2015-11-05 15:20:24]

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

[vabr (Chromium), 2015-11-05 15:20:25]

LGTM with comments.

Cheers,
Vaclav

https://codereview.chromium.org/1428283002/diff/40001/chrome/browser/password...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/1428283002/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2649: // Test
whether the password form which is loaded as hidden is autofilled
Please mention the difference against
AutofillSuggetionsForProblematicPasswordForm in the comment, and move this test
just after AutofillSuggetionsForProblematicPasswordForm.

https://codereview.chromium.org/1428283002/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2650: //
correctly. This shows very often situation when in order to sign-in the user
nit: shows -> happens; situation -> in situations

https://codereview.chromium.org/1428283002/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2651: // clicks
sign-in and a hidden passsword form becomes visible.
nit: "sign-in" -> "a sign-in button"

https://codereview.chromium.org/1428283002/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2671: // Now,
navigate to the password form having ambiguous Ids for username and
Are the IDs ambiguous? I thought there were hidden_password_form_username and
hidden_password_form_password.

https://codereview.chromium.org/1428283002/diff/40001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/1428283002/diff/40001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:201: if
(!form_util::IsWebNodeVisible((*result)[field_name])) {
Is this code path tested? The added test does not seem to involve ambiguous or
empty field names.

[dvadym, 2015-11-05 16:18:26]

Thanks Vaclav review. I've addressed your comments PTAL.

https://codereview.chromium.org/1428283002/diff/40001/chrome/browser/password...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/1428283002/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2649: // Test
whether the password form which is loaded as hidden is autofilled
On 2015/11/05 15:20:24, vabr (Chromium) wrote:
> Please mention the difference against
> AutofillSuggetionsForProblematicPasswordForm in the comment, and move this
test
> just after AutofillSuggetionsForProblematicPasswordForm.

Done.

https://codereview.chromium.org/1428283002/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2650: //
correctly. This shows very often situation when in order to sign-in the user
On 2015/11/05 15:20:24, vabr (Chromium) wrote:
> nit: shows -> happens; situation -> in situations

Done.

https://codereview.chromium.org/1428283002/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2651: // clicks
sign-in and a hidden passsword form becomes visible.
On 2015/11/05 15:20:24, vabr (Chromium) wrote:
> nit: "sign-in" -> "a sign-in button"

Done.

https://codereview.chromium.org/1428283002/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2671: // Now,
navigate to the password form having ambiguous Ids for username and
On 2015/11/05 15:20:24, vabr (Chromium) wrote:
> Are the IDs ambiguous? I thought there were hidden_password_form_username and
> hidden_password_form_password.

Thanks, no, it was copy paste of copy paste :). I've fixed also a comment in
another test

https://codereview.chromium.org/1428283002/diff/40001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/1428283002/diff/40001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:201: if
(!form_util::IsWebNodeVisible((*result)[field_name])) {
On 2015/11/05 15:20:25, vabr (Chromium) wrote:
> Is this code path tested? The added test does not seem to involve ambiguous or
> empty field names.

It's tested in previously added test
AutofillSuggetionsForProblematicAmbiguousPasswordForm line 2604 in
password_manager_browsertest.cc, namely there is password form with 2 password
fields without id/name, the first of them is hidden.

[vabr (Chromium), 2015-11-05 16:29:46]

Thanks!
LGTM with a nit.
Cheers,
Vaclav

https://codereview.chromium.org/1428283002/diff/40001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/1428283002/diff/40001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:201: if
(!form_util::IsWebNodeVisible((*result)[field_name])) {
On 2015/11/05 16:18:26, dvadym wrote:
> On 2015/11/05 15:20:25, vabr (Chromium) wrote:
> > Is this code path tested? The added test does not seem to involve ambiguous
or
> > empty field names.
> 
> It's tested in previously added test
> AutofillSuggetionsForProblematicAmbiguousPasswordForm line 2604 in
> password_manager_browsertest.cc, namely there is password form with 2 password
> fields without id/name, the first of them is hidden.

Acknowledged.

https://codereview.chromium.org/1428283002/diff/80001/chrome/browser/password...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/1428283002/diff/80001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2558: // This
test has difference from AutofillSuggetionsForProblematicPasswordForm
nit: has difference ... that -> differs ... in that

[dvadym, 2015-11-05 16:51:28]

Thanks Vaclav!

https://codereview.chromium.org/1428283002/diff/80001/chrome/browser/password...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/1428283002/diff/80001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2558: // This
test has difference from AutofillSuggetionsForProblematicPasswordForm
On 2015/11/05 16:29:46, vabr (Chromium) wrote:
> nit: has difference ... that -> differs ... in that

Done.

[vabr (Chromium), 2015-11-05 16:55:45]

lgtm

[dvadym, 2015-11-05 17:02:30]

The CQ bit was checked by dvadym@chromium.org

[commit-bot: I haz the power, 2015-11-05 17:04:17]

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

[commit-bot: I haz the power, 2015-11-05 17:59:33]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-11-05 18:00:11]

Patchset 6 (id:??) landed as
https://crrev.com/59e5da4aef356c4ece1cb3a3e4320c633eb99b2a
Cr-Commit-Position: refs/heads/master@{#358078}