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

Issue 9625026: Save password without an associated username. (Closed)

Created:
8 years, 9 months ago by Yumikiyo Osanai
Modified:
4 years, 8 months ago
CC:
chromium-reviews, dhollowa+watch_chromium.org, darin-cc_chromium.org, dyu1, brettw-cc_chromium.org, Ilya Sherman
Visibility:
Public.

Description

Save password without an associated username. My change is as below (1) Changed PasswordFormManager::HasValidPasswordForm() to return "true" if there isn't username. (2) Changed FindFormInputElements() and PasswordAutofillManager::OnFillPasswordForm() to autofill if there isn't username. Contributed by yumios.art@gmail.com BUG=111705 TEST=Try to save the password in below sites and confirm to autofill the password and username(if username exist). And, execute tests in password_form_manager_unittest.cc

Patch Set 1 #

Total comments: 2

Patch Set 2 : Refine the way to solve the issue as commented #

Patch Set 3 : Modify the errors from Lint #

Total comments: 2

Patch Set 4 : Modify how basic_data stores password/username datas, and add tests #

Patch Set 5 : Delete a unintentional spaces #

Total comments: 8

Patch Set 6 : Remove redundant functions and enums from PasswordFormFillData #

Total comments: 14

Patch Set 7 : Refine the detail of the patch #

Total comments: 8

Patch Set 8 : Refine the detail of the patch #

Total comments: 6

Patch Set 9 : Refine the patch and add tests in password_form_manager.unittest.cc #

Patch Set 10 : Modify the patch about errors from lint #

Total comments: 8

Patch Set 11 : Refine the test codes #

Patch Set 12 : Modify some indents in the code #

Total comments: 50

Patch Set 13 : Refine the unit_test codes #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+288 lines, -119 lines) Patch
M chrome/browser/password_manager/password_form_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -2 lines 0 comments Download
M chrome/browser/password_manager/password_form_manager_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +200 lines, -96 lines 2 comments Download
M chrome/renderer/autofill/password_autofill_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +40 lines, -7 lines 0 comments Download
M chrome/renderer/autofill/password_autofill_manager_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +27 lines, -5 lines 0 comments Download
M webkit/forms/password_form_dom_manager.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +5 lines, -0 lines 0 comments Download
M webkit/forms/password_form_dom_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +15 lines, -9 lines 0 comments Download

Messages

Total messages: 35 (0 generated)
Ilya Sherman
https://chromiumcodereview.appspot.com/9625026/diff/1/chrome/renderer/autofill/password_autofill_manager.cc File chrome/renderer/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9625026/diff/1/chrome/renderer/autofill/password_autofill_manager.cc#newcode63 chrome/renderer/autofill/password_autofill_manager.cc:63: } This doesn't seem like the right change. We ...
8 years, 9 months ago (2012-03-08 23:48:49 UTC) #1
Yumikiyo Osanai
Thank you for reviewing my patch! I basically agree with your comment, and I think ...
8 years, 9 months ago (2012-03-09 12:18:45 UTC) #2
tim (not reviewing)
On 2012/03/09 12:18:45, Yumikiyo Osanai wrote: > Thank you for reviewing my patch! > > ...
8 years, 9 months ago (2012-03-14 05:02:33 UTC) #3
yumios.art_gmail.com
Thank you for replying about my comment! As you pointed out, it seems that the ...
8 years, 9 months ago (2012-03-18 17:52:15 UTC) #4
yumios.art_gmail.com
Dear Tim, Hi! I've succced to modify the code as I commented, and uploaded the ...
8 years, 9 months ago (2012-03-20 15:53:04 UTC) #5
Yumikiyo Osanai
https://chromiumcodereview.appspot.com/9625026/diff/1/chrome/renderer/autofill/password_autofill_manager.cc File chrome/renderer/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9625026/diff/1/chrome/renderer/autofill/password_autofill_manager.cc#newcode63 chrome/renderer/autofill/password_autofill_manager.cc:63: } On 2012/03/08 23:48:49, Ilya Sherman wrote: > This ...
8 years, 9 months ago (2012-03-20 15:53:36 UTC) #6
Ilya Sherman
https://chromiumcodereview.appspot.com/9625026/diff/10002/webkit/forms/password_form_dom_manager.h File webkit/forms/password_form_dom_manager.h (right): https://chromiumcodereview.appspot.com/9625026/diff/10002/webkit/forms/password_form_dom_manager.h#newcode38 webkit/forms/password_form_dom_manager.h:38: }; Hmm, I don't much like all this extra ...
8 years, 9 months ago (2012-03-20 23:39:43 UTC) #7
Yumikiyo Osanai
https://chromiumcodereview.appspot.com/9625026/diff/10002/webkit/forms/password_form_dom_manager.h File webkit/forms/password_form_dom_manager.h (right): https://chromiumcodereview.appspot.com/9625026/diff/10002/webkit/forms/password_form_dom_manager.h#newcode38 webkit/forms/password_form_dom_manager.h:38: }; I've changed to reverse the order of username ...
8 years, 9 months ago (2012-03-21 18:01:37 UTC) #8
Yumikiyo Osanai
Thank you for reviewing my patch! I've refined the patch and added a test:) If ...
8 years, 9 months ago (2012-03-21 18:05:21 UTC) #9
Ilya Sherman
https://chromiumcodereview.appspot.com/9625026/diff/20001/webkit/forms/password_form_dom_manager.cc File webkit/forms/password_form_dom_manager.cc (right): https://chromiumcodereview.appspot.com/9625026/diff/20001/webkit/forms/password_form_dom_manager.cc#newcode99 webkit/forms/password_form_dom_manager.cc:99: result->wait_for_username = wait_for_username_before_autofill; nit: Please move this to be ...
8 years, 9 months ago (2012-03-22 16:57:44 UTC) #10
Yumikiyo Osanai
Thank you for reviewing my patch! I basically agree with your comment. I've modified it ...
8 years, 9 months ago (2012-03-26 21:23:31 UTC) #11
Yumikiyo Osanai
https://chromiumcodereview.appspot.com/9625026/diff/20001/webkit/forms/password_form_dom_manager.cc File webkit/forms/password_form_dom_manager.cc (right): https://chromiumcodereview.appspot.com/9625026/diff/20001/webkit/forms/password_form_dom_manager.cc#newcode99 webkit/forms/password_form_dom_manager.cc:99: result->wait_for_username = wait_for_username_before_autofill; On 2012/03/22 16:57:44, Ilya Sherman wrote: ...
8 years, 9 months ago (2012-03-26 21:23:42 UTC) #12
Ilya Sherman
The general direction of this CL looks pretty good. Tim should probably take over reviewership ...
8 years, 9 months ago (2012-03-27 00:15:28 UTC) #13
Yumikiyo Osanai
Thank you for reviewing my patch! I've modified and uploaded the patch. > Tim should ...
8 years, 9 months ago (2012-03-27 22:59:28 UTC) #14
Ilya Sherman
https://chromiumcodereview.appspot.com/9625026/diff/24001/chrome/renderer/autofill/password_autofill_manager.cc File chrome/renderer/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9625026/diff/24001/chrome/renderer/autofill/password_autofill_manager.cc#newcode457 chrome/renderer/autofill/password_autofill_manager.cc:457: On 2012/03/27 22:59:28, Yumikiyo Osanai wrote: > On 2012/03/27 ...
8 years, 9 months ago (2012-03-27 23:28:57 UTC) #15
Yumikiyo Osanai
https://chromiumcodereview.appspot.com/9625026/diff/24001/chrome/renderer/autofill/password_autofill_manager.cc File chrome/renderer/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9625026/diff/24001/chrome/renderer/autofill/password_autofill_manager.cc#newcode457 chrome/renderer/autofill/password_autofill_manager.cc:457: On 2012/03/27 23:28:57, Ilya Sherman wrote: > On 2012/03/27 ...
8 years, 9 months ago (2012-03-28 00:11:00 UTC) #16
Ilya Sherman
Tim, could you take a look at this CL? Are you comfortable with the direction ...
8 years, 9 months ago (2012-03-28 00:21:54 UTC) #17
Yumikiyo Osanai
I've added tests in password_form_manager.unittest.cc and refine the patch :) And, sorry for being late ...
8 years, 9 months ago (2012-03-29 00:14:33 UTC) #18
Yumikiyo Osanai
Dear Tim and Ilya, Hi! It seems that the patch haven't reviewed yet... If it's ...
8 years, 8 months ago (2012-04-02 23:59:36 UTC) #19
tim (not reviewing)
http://codereview.chromium.org/9625026/diff/35002/chrome/browser/password_manager/password_form_manager_unittest.cc File chrome/browser/password_manager/password_form_manager_unittest.cc (right): http://codereview.chromium.org/9625026/diff/35002/chrome/browser/password_manager/password_form_manager_unittest.cc#newcode150 chrome/browser/password_manager/password_form_manager_unittest.cc:150: profile(), NULL, *observed_form2(), false); Why do we need observed_form2 ...
8 years, 8 months ago (2012-04-03 16:40:26 UTC) #20
Yumikiyo Osanai
Thanks for reviewing the patch! I've modified the code. Please review it. (Actually the patch ...
8 years, 8 months ago (2012-04-05 00:19:50 UTC) #21
Ilya Sherman
While looking into [ http://code.google.com/p/chromium/issues/detail?id=122050 ], I realized this might bring up a "Save password?" ...
8 years, 8 months ago (2012-04-05 22:20:36 UTC) #22
Yumikiyo Osanai
Thanks Ilya for revirwing and commenting. I've modified the patch where you pointed out and ...
8 years, 8 months ago (2012-04-06 00:17:34 UTC) #23
Ilya Sherman
I'll continue to defer to Tim for the primary review on this CL -- just ...
8 years, 8 months ago (2012-04-06 22:04:12 UTC) #24
Yumikiyo Osanai
I'm sorry that I took a bit emotional behavior... I think we should think what ...
8 years, 8 months ago (2012-04-07 10:12:46 UTC) #25
Yumikiyo Osanai
Hi! I've uploaded the pictures about step 5 below site :) http://code.google.com/p/chromium/issues/detail?id=111705 (At first, I've ...
8 years, 8 months ago (2012-04-07 11:08:25 UTC) #26
Yumikiyo Osanai
Oh! I took a typo in just before reply. > I think the risk of ...
8 years, 8 months ago (2012-04-07 14:11:39 UTC) #27
Yumikiyo Osanai
Dear Tim and Ilya Hi. I've been waiting for your review and LGTM... But you've ...
8 years, 8 months ago (2012-04-11 18:44:31 UTC) #28
tim (not reviewing)
On 2012/04/11 18:44:31, Yumikiyo Osanai wrote: > Dear Tim and Ilya > > Hi. > ...
8 years, 8 months ago (2012-04-13 04:50:03 UTC) #29
Yumikiyo Osanai
Thank you for reviewing the issue! > In the test comment, I was wondering if ...
8 years, 8 months ago (2012-04-16 00:33:09 UTC) #30
Ilya Sherman
http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_manager/password_form_manager_unittest.cc File chrome/browser/password_manager/password_form_manager_unittest.cc (right): http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_manager/password_form_manager_unittest.cc#newcode38 chrome/browser/password_manager/password_form_manager_unittest.cc:38: // Remove the username and regererate the profile. nit: ...
8 years, 8 months ago (2012-04-19 00:27:37 UTC) #31
Ilya Sherman
I meant to say: The wording suggestions from my previous reply are merely suggestions. Feel ...
8 years, 8 months ago (2012-04-19 00:28:29 UTC) #32
Yumikiyo Osanai
Thank you for reviewing and writing many valuable comment! I've fixed the code :-) Please ...
8 years, 8 months ago (2012-04-26 23:33:21 UTC) #33
Yumikiyo Osanai
On 2012/04/19 00:28:29, Ilya Sherman wrote: > I meant to say: The wording suggestions from ...
8 years, 8 months ago (2012-04-26 23:33:45 UTC) #34
Ilya Sherman
8 years, 8 months ago (2012-04-26 23:42:41 UTC) #35
It looks like you missed my comments on the non-test code files.  Please take a
look at those as well :)

http://codereview.chromium.org/9625026/diff/63001/chrome/browser/password_man...
File chrome/browser/password_manager/password_form_manager_unittest.cc (right):

http://codereview.chromium.org/9625026/diff/63001/chrome/browser/password_man...
chrome/browser/password_manager/password_form_manager_unittest.cc:41: // for
which there is no username.
nit: "no username" -> "no username field".

http://codereview.chromium.org/9625026/diff/63001/chrome/browser/password_man...
chrome/browser/password_manager/password_form_manager_unittest.cc:367: 
nit: Please remove this blank line (367)

Powered by Google App Engine
This is Rietveld 408576698