Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(296)

Issue 1072813002: Avoid considering readonly password elements in password form (Closed)

Created:
5 years, 8 months ago by ramya.v
Modified:
5 years, 8 months ago
Reviewers:
vabr (Chromium)
CC:
chromium-reviews, mlamouri+watch-content_chromium.org, browser-components-watch_chromium.org, jam, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, estade+watch_chromium.org, gcasto+watchlist_chromium.org, rouslan+autofillwatch_chromium.org, mkwst+watchlist-passwords_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Patch Set 2 : Updating test #

Total comments: 10

Patch Set 3 : Updated as per review comments #

Total comments: 4

Patch Set 4 : Formatted comment length to contain within 80 chars #

Unified diffs Side-by-side diffs Delta from patch set Stats (+50 lines, -1 line) Patch
M chrome/browser/password_manager/password_manager_browsertest.cc View 1 1 chunk +18 lines, -0 lines 0 comments Download
A chrome/test/data/password/password_form_with_password_readonly.html View 1 2 1 chunk +24 lines, -0 lines 0 comments Download
M components/autofill/content/renderer/password_form_conversion_utils.cc View 1 2 3 1 chunk +8 lines, -1 line 0 comments Download

Messages

Total messages: 11 (3 generated)
ramya.v
PTAL! Thanks
5 years, 8 months ago (2015-04-10 05:05:15 UTC) #2
vabr (Chromium)
Thank you for the patch and filed bug! I agree that because the password field ...
5 years, 8 months ago (2015-04-10 08:04:51 UTC) #3
ramya.v
Thanks for the review comments. Updated as per comments. PTAL! https://codereview.chromium.org/1072813002/diff/20001/chrome/test/data/password/password_form_with_password_readonly.html File chrome/test/data/password/password_form_with_password_readonly.html (right): https://codereview.chromium.org/1072813002/diff/20001/chrome/test/data/password/password_form_with_password_readonly.html#newcode1 ...
5 years, 8 months ago (2015-04-10 10:40:24 UTC) #4
vabr (Chromium)
Thanks! This LGTM with two nits (=feel free to land this once you address them). ...
5 years, 8 months ago (2015-04-10 10:45:39 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1072813002/60001
5 years, 8 months ago (2015-04-10 10:56:40 UTC) #8
commit-bot: I haz the power
Committed patchset #4 (id:60001)
5 years, 8 months ago (2015-04-10 12:16:21 UTC) #9
commit-bot: I haz the power
Patchset 4 (id:??) landed as https://crrev.com/baf47c72055256f067c203547f9de60f64609186 Cr-Commit-Position: refs/heads/master@{#324620}
5 years, 8 months ago (2015-04-10 12:17:08 UTC) #10
ramya.v
5 years, 8 months ago (2015-04-13 03:01:44 UTC) #11
Message was sent while issue was closed.
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.

Powered by Google App Engine
This is Rietveld 408576698