[Password Manager] Ignore autofilling invisible password fields.

[Password Manager] Ignore autofilling invisible password fields.

At present, password manager does not distinguish invisible fields while
(auto)filled. Forms having multiple password fields with some marked as hidden,
autofill may fail.

With this patch password manager detects that the problematic password field is
invisible and ignores it.

BUG=543303
TEST=PasswordManagerBrowserTestBase
       .AutofillSuggetionsForProblematicPasswordForm
     PasswordManagerBrowserTestBase
       .AutofillSuggetionsForProblematicAmbiguousPasswordForm

Committed: https://crrev.com/e4f684a0db8e88d2df12e163f60064f94a0ff876
Cr-Commit-Position: refs/heads/master@{#355527}

[Pritam Nikam, 2015-10-19 13:10:55]

Description was changed from

==========
[Password Manager] Ignore autofilling invisible password fields.

At present, password manager does not distinguish invisible fields while
(auto)filled. Forms having multiple password fields with some marked as hidden,
autofill may fail.

With this patch password manager detects that the problematic password field is
invisible and ignores it.

BUG=543303
TEST=
==========

to

==========
[Password Manager] Ignore autofilling invisible password fields.

At present, password manager does not distinguish invisible fields while
(auto)filled. Forms having multiple password fields with some marked as hidden,
autofill may fail.

With this patch password manager detects that the problematic password field is
invisible and ignores it.

BUG=543303
TEST=PasswordManagerBrowserTestBase
       .AutofillSuggetionsForProblematicPasswordForm
==========

[Pritam Nikam, 2015-10-19 13:53:18]

Description was changed from

==========
[Password Manager] Ignore autofilling invisible password fields.

At present, password manager does not distinguish invisible fields while
(auto)filled. Forms having multiple password fields with some marked as hidden,
autofill may fail.

With this patch password manager detects that the problematic password field is
invisible and ignores it.

BUG=543303
TEST=PasswordManagerBrowserTestBase
       .AutofillSuggetionsForProblematicPasswordForm
==========

to

==========
[Password Manager] Ignore autofilling invisible password fields.

At present, password manager does not distinguish invisible fields while
(auto)filled. Forms having multiple password fields with some marked as hidden,
autofill may fail.

With this patch password manager detects that the problematic password field is
invisible and ignores it.

BUG=543303
TEST=PasswordManagerBrowserTestBase
       .AutofillSuggetionsForProblematicPasswordForm
     PasswordManagerBrowserTestBase
       .AutofillSuggetionsForProblematicAmbiguousPasswordForm
==========

[Pritam Nikam, 2015-10-19 14:12:30]

pritam.nikam@samsung.com changed reviewers:
+ dvadym@chromium.org, vabr@chromium.org

[Pritam Nikam, 2015-10-19 14:12:31]

Hi Vaclav & Vadym,

This patch is experimental & based on 2nd option mentioned here [1] i.e.
detecting that the problematic password field is invisible (hidden elements have
empty bounding box) and password manager ignores (auto)filling such fields.
Please glance once in your free time.

Thanks!

[1]. https://code.google.com/p/chromium/issues/detail?id=543303#c3

[vabr (Chromium), 2015-10-20 16:40:23]

Thanks, Pritam Nikam,

This LGTM once you address my comments.

Cheers,
Vaclav

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

https://codereview.chromium.org/1408423003/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2604: // in
ambiguous password form autofilled correctly.
typo: missing "is" or "gets" between "form" and "autofilled"

https://codereview.chromium.org/1408423003/diff/40001/chrome/test/data/passwo...
File chrome/test/data/password/ambiguous_password_form.html (right):

https://codereview.chromium.org/1408423003/diff/40001/chrome/test/data/passwo...
chrome/test/data/password/ambiguous_password_form.html:43: <label>Username:
</label><input type='text' placeholder='Username' />
Could you put the hidden password field between username and password? That
would make sure that Chrome does not work just because of first looking for the
username and then for the immediately following password.

https://codereview.chromium.org/1408423003/diff/40001/chrome/test/data/passwo...
File chrome/test/data/password/password_form.html (right):

https://codereview.chromium.org/1408423003/diff/40001/chrome/test/data/passwo...
chrome/test/data/password/password_form.html:92: <input type='password'
id="hidden_password" style="display:none;" autocomplete="off" />
Also here, please move the hidden field in between the two visible ones.

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

https://codereview.chromium.org/1408423003/diff/40001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:190:
IsInputElementHidden((blink::WebInputElement*)&input_element)) {
Please use IsWebNodeVisible. That will also get rid of the forbidden C-style
cast (do not ever use them in Chromium, use C++-style if necessary).

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

https://codereview.chromium.org/1408423003/diff/40001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.h:67: //
Returns true if |input_element| is hidden.
No need for this one, use IsWebNodeVisible from
components/autofill/content/renderer/form_autofill_util.h

[Pritam Nikam, 2015-10-22 09:00:08]

Thanks Vaclav for review. I've incorporated review comments in new patch set.
Please have a look.

Thanks!

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

https://codereview.chromium.org/1408423003/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2604: // in
ambiguous password form autofilled correctly.
On 2015/10/20 16:40:22, vabr (Chromium) wrote:
> typo: missing "is" or "gets" between "form" and "autofilled"

Done.

https://codereview.chromium.org/1408423003/diff/40001/chrome/test/data/passwo...
File chrome/test/data/password/ambiguous_password_form.html (right):

https://codereview.chromium.org/1408423003/diff/40001/chrome/test/data/passwo...
chrome/test/data/password/ambiguous_password_form.html:43: <label>Username:
</label><input type='text' placeholder='Username' />
On 2015/10/20 16:40:22, vabr (Chromium) wrote:
> Could you put the hidden password field between username and password? That
> would make sure that Chrome does not work just because of first looking for
the
> username and then for the immediately following password.

Done.

https://codereview.chromium.org/1408423003/diff/40001/chrome/test/data/passwo...
File chrome/test/data/password/password_form.html (right):

https://codereview.chromium.org/1408423003/diff/40001/chrome/test/data/passwo...
chrome/test/data/password/password_form.html:92: <input type='password'
id="hidden_password" style="display:none;" autocomplete="off" />
On 2015/10/20 16:40:22, vabr (Chromium) wrote:
> Also here, please move the hidden field in between the two visible ones.

Done.

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

https://codereview.chromium.org/1408423003/diff/40001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:190:
IsInputElementHidden((blink::WebInputElement*)&input_element)) {
On 2015/10/20 16:40:22, vabr (Chromium) wrote:
> Please use IsWebNodeVisible. That will also get rid of the forbidden C-style
> cast (do not ever use them in Chromium, use C++-style if necessary).

Done.

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

https://codereview.chromium.org/1408423003/diff/40001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.h:67: //
Returns true if |input_element| is hidden.
On 2015/10/20 16:40:23, vabr (Chromium) wrote:
> No need for this one, use IsWebNodeVisible from
> components/autofill/content/renderer/form_autofill_util.h

Done.

[Pritam Nikam, 2015-10-22 11:04:58]

The CQ bit was checked by pritam.nikam@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-10-22 11:05:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1408423003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1408423003/60001

[vabr (Chromium), 2015-10-22 11:24:10]

LGTM, thanks for addressing the comments!
Vaclav

[commit-bot: I haz the power, 2015-10-22 12:32:12]

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

[commit-bot: I haz the power, 2015-10-22 12:32:12]

Dry run: This issue passed the CQ dry run.

[Pritam Nikam, 2015-10-22 12:33:44]

On 2015/10/22 11:24:10, vabr (Chromium) wrote:
> LGTM, thanks for addressing the comments!
> Vaclav

Thanks Vaclav!
I'll go ahead with commit.

Thank!

[Pritam Nikam, 2015-10-22 12:33:46]

The CQ bit was checked by pritam.nikam@samsung.com

[commit-bot: I haz the power, 2015-10-22 12:33:53]

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

[commit-bot: I haz the power, 2015-10-22 12:37:41]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-22 12:38:43]

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