Avoid considering readonly password elements in password form

In the sites with readonly password field, the page is likely
using a virtual keyboard and bypassing the password field value. Ex: m.epost.go.kr
Currently chrome has no way to see the actual password for such kind of forms, hence avoiding saving passwords with readonly attribute

BUG=475488
Committed: https://crrev.com/baf47c72055256f067c203547f9de60f64609186
Cr-Commit-Position: refs/heads/master@{#324620}

[ramya.v, 2015-04-10 05:05:15]

ramya.v@samsung.com changed reviewers:
+ vabr@chromium.org

[ramya.v, 2015-04-10 05:05:15]

PTAL!

Thanks

[vabr (Chromium), 2015-04-10 08:04:51]

Thank you for the patch and filed bug!

I agree that because the password field never contains the typed password, there
is no point in saving it (we save an unrelated string of the same length as the
password instead). Using the presence of "readonly" as an indicator that the
password field is useless sounds reasonable right now, as long as we properly
document the choice (see my comments below). If we were able to save the right
password, then I would suggest forcing autofill of the readonly field instead,
but that's sadly not the case.

In addition to the comments below, please also improve the CL title and
description. The title should say what change this CL makes (currently it says
what bug lead to creating the CL). The description should elaborate on the
title, making clear what the change consists of, why is it made, and how does it
fix the problems observed on the associated bug.

Thanks!
Vaclav

https://codereview.chromium.org/1072813002/diff/20001/chrome/test/data/passwo...
File chrome/test/data/password/password_form_with_password_readonly.html
(right):

https://codereview.chromium.org/1072813002/diff/20001/chrome/test/data/passwo...
chrome/test/data/password/password_form_with_password_readonly.html:1: <!--
Currently, git recognised this as branching off
chrome/test/data/password/signup_form.html, because the new file is 66% similar
to that one.

To avoid confusing git history, please use
git cl upload --similarity 70
(70 being randomly chosen as something bigger than 66) when you upload your next
patch set. That will force considering the new file as not being branched off of
other files.

https://codereview.chromium.org/1072813002/diff/20001/chrome/test/data/passwo...
chrome/test/data/password/password_form_with_password_readonly.html:2: Form with
password element readonly
nit: Reorder words and add a full stop:
Form with readonly password element.

https://codereview.chromium.org/1072813002/diff/20001/chrome/test/data/passwo...
chrome/test/data/password/password_form_with_password_readonly.html:8: <input
type="password" readonly="readonly" id="password_field" name="password_field">
nit: The readonly attribute does not require a value. I understand that this is
how it appears on the m.epost.go.kr page, but let's simplify the test by just
using readonly instead of readonly="readonly".

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

https://codereview.chromium.org/1072813002/diff/20001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:364: //
If Password field is readonly, there is no point in considering
I suggest the following rephrasing, to better capture the reasons for not
supporting those forms for now:

// If the password field is readonly, the page is likely using a virtual
keyboard and bypassing the password field value (see http://crbug.com/475488).
There is nothing Chrome can do to fill passwords for now.

https://codereview.chromium.org/1072813002/diff/20001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:367: if
(!password.isNull() && password.isReadOnly())
Instead of doing this check here, I suggest moving adding this to the condition
on line 257. That way, the read-only passwords will not even be considered for
being used, and in case there are other, modifiable, password fields, Chrome
will try to use them.

There is a caveat, though, in case the readonly password element is explicitly
marked as a password field by an autocomplete attribute. To handle those
correctly, I suggest dropping this block, and changing the check on line 257 to

if (input_element->isPasswordField() && (!input_element.isReadInly() ||
HasAutocompleteAttributeValue(*input_element, "current-password") ||
HasAutocompleteAttributeValue(*input_element, "new-password")) {
...
}

[ramya.v, 2015-04-10 10:40:24]

Thanks for the review comments. Updated as per comments. PTAL!

https://codereview.chromium.org/1072813002/diff/20001/chrome/test/data/passwo...
File chrome/test/data/password/password_form_with_password_readonly.html
(right):

https://codereview.chromium.org/1072813002/diff/20001/chrome/test/data/passwo...
chrome/test/data/password/password_form_with_password_readonly.html:1: <!--
On 2015/04/10 08:04:51, vabr (Chromium) wrote:
> Currently, git recognised this as branching off
> chrome/test/data/password/signup_form.html, because the new file is 66%
similar
> to that one.
> 
> To avoid confusing git history, please use
> git cl upload --similarity 70
> (70 being randomly chosen as something bigger than 66) when you upload your
next
> patch set. That will force considering the new file as not being branched off
of
> other files.

Done.

https://codereview.chromium.org/1072813002/diff/20001/chrome/test/data/passwo...
chrome/test/data/password/password_form_with_password_readonly.html:2: Form with
password element readonly
On 2015/04/10 08:04:51, vabr (Chromium) wrote:
> nit: Reorder words and add a full stop:
> Form with readonly password element.

Done.

https://codereview.chromium.org/1072813002/diff/20001/chrome/test/data/passwo...
chrome/test/data/password/password_form_with_password_readonly.html:8: <input
type="password" readonly="readonly" id="password_field" name="password_field">
On 2015/04/10 08:04:51, vabr (Chromium) wrote:
> nit: The readonly attribute does not require a value. I understand that this
is
> how it appears on the m.epost.go.kr page, but let's simplify the test by just
> using readonly instead of readonly="readonly".

Done.

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

https://codereview.chromium.org/1072813002/diff/20001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:364: //
If Password field is readonly, there is no point in considering
On 2015/04/10 08:04:51, vabr (Chromium) wrote:
> I suggest the following rephrasing, to better capture the reasons for not
> supporting those forms for now:
> 
> // If the password field is readonly, the page is likely using a virtual
> keyboard and bypassing the password field value (see http://crbug.com/475488).
> There is nothing Chrome can do to fill passwords for now.

Done.

https://codereview.chromium.org/1072813002/diff/20001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:367: if
(!password.isNull() && password.isReadOnly())
On 2015/04/10 08:04:51, vabr (Chromium) wrote:
> Instead of doing this check here, I suggest moving adding this to the
condition
> on line 257. That way, the read-only passwords will not even be considered for
> being used, and in case there are other, modifiable, password fields, Chrome
> will try to use them.
> 
> There is a caveat, though, in case the readonly password element is explicitly
> marked as a password field by an autocomplete attribute. To handle those
> correctly, I suggest dropping this block, and changing the check on line 257
to
> 
> if (input_element->isPasswordField() && (!input_element.isReadInly() ||
> HasAutocompleteAttributeValue(*input_element, "current-password") ||
> HasAutocompleteAttributeValue(*input_element, "new-password")) {
> ...
> }

Done.

[vabr (Chromium), 2015-04-10 10:45:39]

Thanks!
This LGTM with two nits (=feel free to land this once you address them).

Cheers,
Vaclav

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

https://codereview.chromium.org/1072813002/diff/40001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:258: //
keyboard and bypassing the password field value (see http://crbug.com/475488)
nit: Here the comment ran over the 80 characters per line limit. If you use
clang-format (you should), then the easiest way to sort this out is to keep all
comments one one line, and use "git cl format" before uploading. This way
clang-format wraps the comments fro you correctly. Please note that clang-format
does not join multiple comment lines before wrapping, so it is important to keep
the whole comment on one (long) line before doing the automatic formatting.

https://codereview.chromium.org/1072813002/diff/40001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:258: //
keyboard and bypassing the password field value (see http://crbug.com/475488)
nit: Also, a full-stop (".") is missing after the closing parenthesis.

[ramya.v, 2015-04-10 10:56:08]

The CQ bit was checked by ramya.v@samsung.com

[ramya.v, 2015-04-10 10:56:09]

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/1072813002/#ps60001
(title: "Formatted comment length to contain within 80 chars")

[commit-bot: I haz the power, 2015-04-10 10:56:40]

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

[commit-bot: I haz the power, 2015-04-10 12:16:21]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-10 12:17:08]

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

[ramya.v, 2015-04-13 03:01:44]

Landed patch after fixing the below nits.

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

https://codereview.chromium.org/1072813002/diff/40001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:258: //
keyboard and bypassing the password field value (see http://crbug.com/475488)
On 2015/04/10 10:45:38, vabr (Chromium) wrote:
> nit: Also, a full-stop (".") is missing after the closing parenthesis.

Done.

https://codereview.chromium.org/1072813002/diff/40001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:258: //
keyboard and bypassing the password field value (see http://crbug.com/475488)
On 2015/04/10 10:45:38, vabr (Chromium) wrote:
> nit: Also, a full-stop (".") is missing after the closing parenthesis.

Done.