|
|
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 Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionSave 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
Messages
Total messages: 35 (0 generated)
https://chromiumcodereview.appspot.com/9625026/diff/1/chrome/renderer/autofil... File chrome/renderer/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9625026/diff/1/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_manager.cc:63: } This doesn't seem like the right change. We shouldn't ignore form elements that we failed to find. Rather, we shouldn't look for username elements when we don't need them. I think you're going to want to update webkit::forms::PasswordFormDomManager::InitFillData() to not add the username element if the password autofill data includes only a password.
Thank you for reviewing my patch! I basically agree with your comment, and I think we have to improve the quality of the code as below. 1. Change the webkit::forms::PasswordFormFillData class structure. According to comments and member variable in PasswordFormFillData, it seems to presume that username always exists. It's a flaw of the structure of the class, because other webkit password classes handle the username as an optional element. (According to the comment of webkit::form::PasswordForm::username_value) 2. Change the way to use the PasswordFormFillData. we use some magic numbers to get and set the password and username. For example, we presume the username is form_data.basic_data.fields[0].name is the username, and form_data.basic_data.fields[1].name is the password. we should eliminate the magic numbers to improve the quality of code. I'll change the above things and upload the patch. - Yumi (Yumikiyo Osanai)
On 2012/03/09 12:18:45, Yumikiyo Osanai wrote: > Thank you for reviewing my patch! > > I basically agree with your comment, and I think we have to improve the quality > of the code as below. > > 1. Change the webkit::forms::PasswordFormFillData class structure. According > to comments and member variable in PasswordFormFillData, it seems to presume > that username always exists. It's a flaw of the structure of the class, > because other webkit password classes handle the username as an optional > element. (According to the comment of > webkit::form::PasswordForm::username_value) > > 2. Change the way to use the PasswordFormFillData. we use some magic numbers > to get and set the password and username. For example, we presume the username > is form_data.basic_data.fields[0].name is the username, and > form_data.basic_data.fields[1].name is the password. we should eliminate the > magic numbers to improve the quality of code. > > I'll change the above things and upload the patch. > > - Yumi (Yumikiyo Osanai) I'm a bit concerned because a lot of the password manager code relies on the presence of a username. It's optional at the webkit layer, true... in fact, I wrote that comment in PasswordForm years ago :) Although it's important for more passwords code than I see being considered here. For example, take a look at PasswordFormManager's use of best_matches_. The username value is the primary index. At the very least we should add tests to the password manager suite to cover empty username cases.
Thank you for replying about my comment! As you pointed out, it seems that the password managers use usernames as if they always exist. So, I'll change the code carefully not to make side effects, and I'll add some tests for the case there isn't any username. > For example, take a look at PasswordFormManager's use of best_matches_. > The username value is the primary index. Thank you for letting me know about it! I'm not aware that best_matches_ use the username value as a primary key. I've debugged about it and I think it doesn't occur any side effects. In the case there isn't any username, the username value is just empty string, so that I succeed to autofill the password without crushing. (Because LoginDatabase::AddLogin() and UpdateLogin() set username as empty string, which is default value of string16) But I should add a comment that username is empty string if there isn't any usernames, and I should be carefully about the possibility of the side effects when the username doesn't exist. > It's optional at the webkit layer, true... in fact, > I wrote that comment in PasswordForm years ago :) Wow! It's amazing. And I'm very happy to talk with many spacialists! I'm very happy to join the chromium community because I've met many great and kind engineers, and got ideas, techniques, and perspectives about programming. These experience gave me the inspiration and the motivation to do programming! BTW, I'm sorry to reply to your response... I'm busy with my work last week. Now, I have time to take this issue, I hope I upload the patch within several days. - Yumi
Dear Tim, Hi! I've succced to modify the code as I commented, and uploaded the patch :) Please review the patch. For now, I haven't added tests yet, because I think I should confirm that my changes are on the right track before I make tests. After I finish to modify the chromium code, I'll add unit tests :) - Yumi (Yumikiyo Osanai)
https://chromiumcodereview.appspot.com/9625026/diff/1/chrome/renderer/autofil... File chrome/renderer/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9625026/diff/1/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_manager.cc:63: } On 2012/03/08 23:48:49, Ilya Sherman wrote: > This doesn't seem like the right change. We shouldn't ignore form elements that > we failed to find. Rather, we shouldn't look for username elements when we > don't need them. I think you're going to want to update > webkit::forms::PasswordFormDomManager::InitFillData() to not add the username > element if the password autofill data includes only a password. Done.
https://chromiumcodereview.appspot.com/9625026/diff/10002/webkit/forms/passwo... File webkit/forms/password_form_dom_manager.h (right): https://chromiumcodereview.appspot.com/9625026/diff/10002/webkit/forms/passwo... webkit/forms/password_form_dom_manager.h:38: }; Hmm, I don't much like all this extra complexity in the struct. Why not just set an empty username if there is no username for this form? Or, you could reverse the order of the username and password fields so that the password field comes first. Then, if basic_data.fields.size() is 1 there's only a password, and no username.
https://chromiumcodereview.appspot.com/9625026/diff/10002/webkit/forms/passwo... File webkit/forms/password_form_dom_manager.h (right): https://chromiumcodereview.appspot.com/9625026/diff/10002/webkit/forms/passwo... webkit/forms/password_form_dom_manager.h:38: }; I've changed to reverse the order of username and password.
Thank you for reviewing my patch! I've refined the patch and added a test:) If you agree with the patch, please give me LGTM. - Yumi
https://chromiumcodereview.appspot.com/9625026/diff/20001/webkit/forms/passwo... File webkit/forms/password_form_dom_manager.cc (right): https://chromiumcodereview.appspot.com/9625026/diff/20001/webkit/forms/passwo... webkit/forms/password_form_dom_manager.cc:99: result->wait_for_username = wait_for_username_before_autofill; nit: Please move this to be immediately after line 90, or -- if it's necessary to adjust based on whether there's a username field -- to after you've determined whether there is a username field. https://chromiumcodereview.appspot.com/9625026/diff/20001/webkit/forms/passwo... webkit/forms/password_form_dom_manager.cc:102: // we suppose that there isn't any username field and don't set the username. Do we really need to check both the username_element and the username_value, or is just checking one sufficient? https://chromiumcodereview.appspot.com/9625026/diff/20001/webkit/forms/passwo... File webkit/forms/password_form_dom_manager.h (right): https://chromiumcodereview.appspot.com/9625026/diff/20001/webkit/forms/passwo... webkit/forms/password_form_dom_manager.h:37: }; There should be no need to add this enum -- you can determine whether there is a username field based on the size of basic_data.fields. https://chromiumcodereview.appspot.com/9625026/diff/20001/webkit/forms/passwo... webkit/forms/password_form_dom_manager.h:59: DataType GetDataType() const; There should be no need to add these methods. This is a struct, so it should have a trivial interface. It might be helpful to have the two convenience methods const FormField& GetUsernameField(const PasswordFormFillData& fill_data); const FormField& GetPasswordField(const PasswordFormFillData& fill_data); defined in an anonymous namespace in password_autofill_manager.cc, though.
Thank you for reviewing my patch! I basically agree with your comment. I've modified it and upload the refined patch. Please review it :) - Yumi
https://chromiumcodereview.appspot.com/9625026/diff/20001/webkit/forms/passwo... File webkit/forms/password_form_dom_manager.cc (right): https://chromiumcodereview.appspot.com/9625026/diff/20001/webkit/forms/passwo... 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: > nit: Please move this to be immediately after line 90, or -- if it's necessary > to adjust based on whether there's a username field -- to after you've > determined whether there is a username field. Done. I've moved it immediately after line 90. https://chromiumcodereview.appspot.com/9625026/diff/20001/webkit/forms/passwo... webkit/forms/password_form_dom_manager.cc:102: // we suppose that there isn't any username field and don't set the username. On 2012/03/22 16:57:44, Ilya Sherman wrote: > Do we really need to check both the username_element and the username_value, or > is just checking one sufficient? As you said, It doesn't need to check both of those elements. I've chenge to check the username_element only. https://chromiumcodereview.appspot.com/9625026/diff/20001/webkit/forms/passwo... File webkit/forms/password_form_dom_manager.h (right): https://chromiumcodereview.appspot.com/9625026/diff/20001/webkit/forms/passwo... webkit/forms/password_form_dom_manager.h:37: }; On 2012/03/22 16:57:44, Ilya Sherman wrote: > There should be no need to add this enum -- you can determine whether there is a > username field based on the size of basic_data.fields. Umm... You are right, it seems to be it's a bit verbose. I've changed the code as you advised. https://chromiumcodereview.appspot.com/9625026/diff/20001/webkit/forms/passwo... webkit/forms/password_form_dom_manager.h:59: DataType GetDataType() const; On 2012/03/22 16:57:44, Ilya Sherman wrote: > There should be no need to add these methods. This is a struct, so it should > have a trivial interface. It might be helpful to have the two convenience > methods > > const FormField& GetUsernameField(const PasswordFormFillData& fill_data); > const FormField& GetPasswordField(const PasswordFormFillData& fill_data); > > defined in an anonymous namespace in password_autofill_manager.cc, though. I agree with your idea. I remove these member methods, and add two functions in anonymous namespace in password_autofill_manager.cc.
The general direction of this CL looks pretty good. Tim should probably take over reviewership pretty soon, as he knows the password manager code much better than I do. https://chromiumcodereview.appspot.com/9625026/diff/24001/chrome/renderer/aut... File chrome/renderer/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9625026/diff/24001/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:211: } nit: I would write this method simply as DCHECK_GE(fill_data.basic_data.fields.size(), 2U); return fill_data.basic_data.fields[1]; https://chromiumcodereview.appspot.com/9625026/diff/24001/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:226: return fill_data.basic_data.fields[0]; nit: I would write this method simply as DCHECK(!fill_data.basic_data.fields.empty()); return fill_data.basic_data.fields[0]; https://chromiumcodereview.appspot.com/9625026/diff/24001/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:457: nit: There's no need to do anything else on this iteration if wait_for_username is true. I would add an if-stmt here along the lines of if (!HasUsernameField(form_data)) continue; (where HasUsernameField() is a simple helper function that returns """ form_data.basic_data.fields.size() == 2"""), and reduce indentation for the rest of the code (i.e. lines 465--478 in this file in this patch set). https://chromiumcodereview.appspot.com/9625026/diff/24001/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:491: if (StartsWith(GetUsernameField(fill_data).value, input, false)) nit: Please use "&&" rather than nested if-stmts https://chromiumcodereview.appspot.com/9625026/diff/24001/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:543: return false; nit: No need for curly braces, since this if-stmt's body is just one line. https://chromiumcodereview.appspot.com/9625026/diff/24001/webkit/forms/passwo... File webkit/forms/password_form_dom_manager.cc (right): https://chromiumcodereview.appspot.com/9625026/diff/24001/webkit/forms/passwo... webkit/forms/password_form_dom_manager.cc:57: if (form_on_page.username_element != none_username) { nit: This would be a little clearer if written as """ if (!form_on_page.username_element.empty()) { ... } """
Thank you for reviewing my patch! I've modified and uploaded the patch. > Tim should probably take over reviewership pretty soon, > as he knows the password manager code much better than I do. Wow! I'm looking forward to hearing from Tim. And, thank you very much for reviewing the patch many times and helping me. I keep trying to contribute to chromium, so I hope we'll meet other topic and make chromium more fantastic! > Ilya https://chromiumcodereview.appspot.com/9625026/diff/24001/chrome/renderer/aut... File chrome/renderer/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9625026/diff/24001/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:211: } On 2012/03/27 00:15:28, Ilya Sherman wrote: > nit: I would write this method simply as > > DCHECK_GE(fill_data.basic_data.fields.size(), 2U); > return fill_data.basic_data.fields[1]; Thanks! I didn't know the DCHECK_** macro. It seems very useful. I'll use DCHECK_EQ macro in this function. For now, if fill_data.basic_data.fields.size() > 2, it's probably wrong data. https://chromiumcodereview.appspot.com/9625026/diff/24001/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:226: return fill_data.basic_data.fields[0]; On 2012/03/27 00:15:28, Ilya Sherman wrote: > nit: I would write this method simply as > > DCHECK(!fill_data.basic_data.fields.empty()); > return fill_data.basic_data.fields[0]; Done. https://chromiumcodereview.appspot.com/9625026/diff/24001/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:457: On 2012/03/27 00:15:28, Ilya Sherman wrote: > nit: There's no need to do anything else on this iteration if wait_for_username > is true. I would add an if-stmt here along the lines of > > if (!HasUsernameField(form_data)) > continue; > > (where HasUsernameField() is a simple helper function that returns """ > form_data.basic_data.fields.size() == 2"""), and reduce indentation for the rest > of the code (i.e. lines 465--478 in this file in this patch set). I've defined the HasUsernameField() and changed to use the function. But I haven't changed anything about below your comment. > nit: There's no need to do anything else on this iteration if wait_for_username I'm a bit concerned that it changes the behavior of FillUserNameAndPassword() function. Because the original version of this function is same as my patch when wait_for_username is true. For now, I'm not confidence this behavior is false. https://chromiumcodereview.appspot.com/9625026/diff/24001/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:491: if (StartsWith(GetUsernameField(fill_data).value, input, false)) On 2012/03/27 00:15:28, Ilya Sherman wrote: > nit: Please use "&&" rather than nested if-stmts Done. https://chromiumcodereview.appspot.com/9625026/diff/24001/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:543: return false; On 2012/03/27 00:15:28, Ilya Sherman wrote: > nit: No need for curly braces, since this if-stmt's body is just one line. Done. And I've changed to use the HasUsernameField() to check there isn't any username. https://chromiumcodereview.appspot.com/9625026/diff/24001/webkit/forms/passwo... File webkit/forms/password_form_dom_manager.cc (right): https://chromiumcodereview.appspot.com/9625026/diff/24001/webkit/forms/passwo... webkit/forms/password_form_dom_manager.cc:57: if (form_on_page.username_element != none_username) { On 2012/03/27 00:15:28, Ilya Sherman wrote: > nit: This would be a little clearer if written as """ if > (!form_on_page.username_element.empty()) { ... } """ Done.
https://chromiumcodereview.appspot.com/9625026/diff/24001/chrome/renderer/aut... File chrome/renderer/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9625026/diff/24001/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:457: On 2012/03/27 22:59:28, Yumikiyo Osanai wrote: > On 2012/03/27 00:15:28, Ilya Sherman wrote: > > nit: There's no need to do anything else on this iteration if > wait_for_username > > is true. I would add an if-stmt here along the lines of > > > > if (!HasUsernameField(form_data)) > > continue; > > > > (where HasUsernameField() is a simple helper function that returns """ > > form_data.basic_data.fields.size() == 2"""), and reduce indentation for the > rest > > of the code (i.e. lines 465--478 in this file in this patch set). > > I've defined the HasUsernameField() and changed to use the function. > > But I haven't changed anything about below your comment. > > nit: There's no need to do anything else on this iteration if > wait_for_username > I'm a bit concerned that it changes the behavior of FillUserNameAndPassword() > function. Because the original version of this function is same as my patch when > wait_for_username is true. > For now, I'm not confidence this behavior is false. Sorry, I think my comment was a little unclear. What I meant was: There's no need to do anything else on this iteration if wait_for_username is true *and there is no username*. So, exactly the change you made is what I meant, except also moving the HasUsernameField() check to be immediately after the |wait_for_username| check. https://chromiumcodereview.appspot.com/9625026/diff/29001/chrome/renderer/aut... File chrome/renderer/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9625026/diff/29001/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:201: DCHECK_EQ(fill_data.basic_data.fields.size(), 2U); nit: This should be written as DCHECK_EQ(2U, fill_data.basic_data.fields.size()); -- the macro works backward to what most people expect, in terms of how it logs failures. Or, you could just write this as DCHECK(HasUsernameField(fill_data)); https://chromiumcodereview.appspot.com/9625026/diff/29001/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:453: continue; nit: You can move this above line 446 -- there's no need to get the password_element if we're not going to use it. https://chromiumcodereview.appspot.com/9625026/diff/29001/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:482: suggestions->push_back(GetUsernameField(fill_data).value); nit: This if-stmt should probably have curly braces, as the if-stmt condition is longer than a single line (even though the body is just one line).
https://chromiumcodereview.appspot.com/9625026/diff/24001/chrome/renderer/aut... File chrome/renderer/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9625026/diff/24001/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:457: On 2012/03/27 23:28:57, Ilya Sherman wrote: > On 2012/03/27 22:59:28, Yumikiyo Osanai wrote: > > On 2012/03/27 00:15:28, Ilya Sherman wrote: > > > nit: There's no need to do anything else on this iteration if > > wait_for_username > > > is true. I would add an if-stmt here along the lines of > > > > > > if (!HasUsernameField(form_data)) > > > continue; > > > > > > (where HasUsernameField() is a simple helper function that returns """ > > > form_data.basic_data.fields.size() == 2"""), and reduce indentation for the > > rest > > > of the code (i.e. lines 465--478 in this file in this patch set). > > > > I've defined the HasUsernameField() and changed to use the function. > > > > But I haven't changed anything about below your comment. > > > nit: There's no need to do anything else on this iteration if > > wait_for_username > > I'm a bit concerned that it changes the behavior of FillUserNameAndPassword() > > function. Because the original version of this function is same as my patch > when > > wait_for_username is true. > > For now, I'm not confidence this behavior is false. > > Sorry, I think my comment was a little unclear. What I meant was: There's no > need to do anything else on this iteration if wait_for_username is true *and > there is no username*. So, exactly the change you made is what I meant, except > also moving the HasUsernameField() check to be immediately after the > |wait_for_username| check. Thank you for writing the detail:) I've moved the HasUsernameField() check immediately after the wait_for_username check. https://chromiumcodereview.appspot.com/9625026/diff/29001/chrome/renderer/aut... File chrome/renderer/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9625026/diff/29001/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:201: DCHECK_EQ(fill_data.basic_data.fields.size(), 2U); On 2012/03/27 23:28:57, Ilya Sherman wrote: > nit: This should be written as DCHECK_EQ(2U, > fill_data.basic_data.fields.size()); -- the macro works backward to what most > people expect, in terms of how it logs failures. Or, you could just write this > as DCHECK(HasUsernameField(fill_data)); I agree with your idea. I've fixed to use "DCHECK(HasUsernameField(fill_data))". BTW, you also recommend to use "DCHECK_EQ(2U, fill_data.basic_data.fields.size());". But, I don't find the difference between my usage of DCHECK_EQ and yours. It seems to just reverses the argument. ( Bacause, my usage is "DCHECK_EQ(fill_data.basic_data.fields.size(), 2U);", and your idea is "DCHECK_EQ(2U, fill_data.basic_data.fields.size());") https://chromiumcodereview.appspot.com/9625026/diff/29001/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:453: continue; On 2012/03/27 23:28:57, Ilya Sherman wrote: > nit: You can move this above line 446 -- there's no need to get the > password_element if we're not going to use it. Oh, you are right! I fixed it. I misunderstood a bit what this function does. https://chromiumcodereview.appspot.com/9625026/diff/29001/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:482: suggestions->push_back(GetUsernameField(fill_data).value); On 2012/03/27 23:28:57, Ilya Sherman wrote: > nit: This if-stmt should probably have curly braces, as the if-stmt condition is > longer than a single line (even though the body is just one line). Done.
Tim, could you take a look at this CL? Are you comfortable with the direction of these changes? Yumi, you might want to add some additional tests for the chrome/browser/password_manager/password_form_manager code to verify that having an empty string for the username doesn't cause any problems, per Tim's earlier review comments. https://chromiumcodereview.appspot.com/9625026/diff/29001/chrome/renderer/aut... File chrome/renderer/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9625026/diff/29001/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:201: DCHECK_EQ(fill_data.basic_data.fields.size(), 2U); On 2012/03/28 00:11:00, Yumikiyo Osanai wrote: > On 2012/03/27 23:28:57, Ilya Sherman wrote: > > nit: This should be written as DCHECK_EQ(2U, > > fill_data.basic_data.fields.size()); -- the macro works backward to what most > > people expect, in terms of how it logs failures. Or, you could just write > this > > as DCHECK(HasUsernameField(fill_data)); > I agree with your idea. > I've fixed to use "DCHECK(HasUsernameField(fill_data))". > > BTW, you also recommend to use "DCHECK_EQ(2U, > fill_data.basic_data.fields.size());". > But, I don't find the difference between my usage of DCHECK_EQ and yours. It > seems to just reverses the argument. > ( Bacause, my usage is > "DCHECK_EQ(fill_data.basic_data.fields.size(), 2U);", > and your idea is > "DCHECK_EQ(2U, fill_data.basic_data.fields.size());") The difference is indeed just in the argument order, which affects what gets printed when the DCHECK_EQ fails. The failure message makes more sense if the constant (the "expected" value) is the first argument to the DCHECK_EQ macro rather than the second. If that's confusing, try writing a DCHECK_EQ which you know will fail, and see what gets printed when you write the arguments one way or the other :) https://chromiumcodereview.appspot.com/9625026/diff/33002/chrome/renderer/aut... File chrome/renderer/autofill/password_autofill_manager.cc (left): https://chromiumcodereview.appspot.com/9625026/diff/33002/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:439: login_to_password_info_.end()) nit: This indentation was correct previously (though it's also fine to indent the second line four more spaces than it was previously). https://chromiumcodereview.appspot.com/9625026/diff/33002/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:441: nit: Please preserve this line break. https://chromiumcodereview.appspot.com/9625026/diff/33002/chrome/renderer/aut... File chrome/renderer/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9625026/diff/33002/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:458: form_elements->input_elements[GetUsernameField(form_data).name]; nit: I would avoid moving this block of code relative to the password_element initialization, both because it makes the diff a little clearer, and because the comment, as it is currently written, describes this as happening first (and it kind of makes sense to think of dealing with the username field prior to the password field, given the ordering of most login forms -- at least ones for English websites :)
I've added tests in password_form_manager.unittest.cc and refine the patch :) And, sorry for being late to upload the patch than I expected. I encountered the build error about unit_test same as below issue... I took some hours to find this issue :'-( http://code.google.com/p/chromium/issues/detail?id=61845 https://chromiumcodereview.appspot.com/9625026/diff/29001/chrome/renderer/aut... File chrome/renderer/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9625026/diff/29001/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:201: DCHECK_EQ(fill_data.basic_data.fields.size(), 2U); On 2012/03/28 00:21:54, Ilya Sherman wrote: > On 2012/03/28 00:11:00, Yumikiyo Osanai wrote: > > On 2012/03/27 23:28:57, Ilya Sherman wrote: > > > nit: This should be written as DCHECK_EQ(2U, > > > fill_data.basic_data.fields.size()); -- the macro works backward to what > most > > > people expect, in terms of how it logs failures. Or, you could just write > > this > > > as DCHECK(HasUsernameField(fill_data)); > > I agree with your idea. > > I've fixed to use "DCHECK(HasUsernameField(fill_data))". > > > > BTW, you also recommend to use "DCHECK_EQ(2U, > > fill_data.basic_data.fields.size());". > > But, I don't find the difference between my usage of DCHECK_EQ and yours. It > > seems to just reverses the argument. > > ( Bacause, my usage is > > "DCHECK_EQ(fill_data.basic_data.fields.size(), 2U);", > > and your idea is > > "DCHECK_EQ(2U, fill_data.basic_data.fields.size());") > > The difference is indeed just in the argument order, which affects what gets > printed when the DCHECK_EQ fails. The failure message makes more sense if the > constant (the "expected" value) is the first argument to the DCHECK_EQ macro > rather than the second. If that's confusing, try writing a DCHECK_EQ which you > know will fail, and see what gets printed when you write the arguments one way > or the other :) Thank you for letting me know the details! I've tried to see the output, and I understand how the macro outputs :) https://chromiumcodereview.appspot.com/9625026/diff/33002/chrome/renderer/aut... File chrome/renderer/autofill/password_autofill_manager.cc (left): https://chromiumcodereview.appspot.com/9625026/diff/33002/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:439: login_to_password_info_.end()) On 2012/03/28 00:21:54, Ilya Sherman wrote: > nit: This indentation was correct previously (though it's also fine to indent > the second line four more spaces than it was previously). Done. https://chromiumcodereview.appspot.com/9625026/diff/33002/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:441: On 2012/03/28 00:21:54, Ilya Sherman wrote: > nit: Please preserve this line break. Done. https://chromiumcodereview.appspot.com/9625026/diff/33002/chrome/renderer/aut... File chrome/renderer/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9625026/diff/33002/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:458: form_elements->input_elements[GetUsernameField(form_data).name]; On 2012/03/28 00:21:54, Ilya Sherman wrote: > nit: I would avoid moving this block of code relative to the password_element > initialization, both because it makes the diff a little clearer, and because the > comment, as it is currently written, describes this as happening first (and it > kind of makes sense to think of dealing with the username field prior to the > password field, given the ordering of most login forms -- at least ones for > English websites :) Oh! I understand it. It means we'd like to set the elements as the ordering of most login forms. I'm agree with your idea.
Dear Tim and Ilya, Hi! It seems that the patch haven't reviewed yet... If it's not too much trouble, could you review the patch? If you haven't enough time to review it, please let me know. - Yumi
http://codereview.chromium.org/9625026/diff/35002/chrome/browser/password_man... File chrome/browser/password_manager/password_form_manager_unittest.cc (right): http://codereview.chromium.org/9625026/diff/35002/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:150: profile(), NULL, *observed_form2(), false); Why do we need observed_form2 It seems tests use either version, so it should be a way to set this up so that we don't need the extra members or Profile in the test class, and instead do an extra configuration step in the tests that want '2'. Also, this appears to be mostly copy/paste from the test above. Can you move the similar code into a function (perhaps parameterized by your form1/2 variables) that is called once with a non-empty username form and once with an empty username? This will prevent the tests from diverging in the future. http://codereview.chromium.org/9625026/diff/35002/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:397: } What happens if - a user saves a password for a form with no username element on foo.com - the user visits foo.com/a, which has a password form *with* [and next, without] a username element.
Thanks for reviewing the patch! I've modified the code. Please review it. (Actually the patch isn't perfect, bacause I have a question about a part of comment you wrote. Please look the comments and let me know the detail. https://chromiumcodereview.appspot.com/9625026/diff/35002/chrome/browser/pass... File chrome/browser/password_manager/password_form_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/9625026/diff/35002/chrome/browser/pass... chrome/browser/password_manager/password_form_manager_unittest.cc:150: profile(), NULL, *observed_form2(), false); I basically agree with your idea. I've removed the '2' members and added SetUpForNoUsernameTests() function. And, I've took refactoring the code to delete copy/paste tests as possible. On 2012/04/03 16:40:27, timsteele wrote: > Why do we need observed_form2 It seems tests use either version, so it should be > a way to set this up so that we don't need the extra members or Profile in the > test class, and instead do an extra configuration step in the tests that want > '2'. > > Also, this appears to be mostly copy/paste from the test above. Can you move > the similar code into a function (perhaps parameterized by your form1/2 > variables) that is called once with a non-empty username form and once with an > empty username? This will prevent the tests from diverging in the future. https://chromiumcodereview.appspot.com/9625026/diff/35002/chrome/browser/pass... chrome/browser/password_manager/password_form_manager_unittest.cc:397: } Umm... I don't understand what you want to test... Please write the detail. And I've tried to create the test as PasswordFormManagerTest(), but I haven't found the way to express "visit foo.com/a". I know manager->ProvisionallySave() can save the password, but I haven't found the funciton that offers "visit a URL". Please see the TEST_F(PasswordFormManagerTest, TestVisitOtherForm), and if you have any idea, please let me know. On 2012/04/03 16:40:27, timsteele wrote: > What happens if > - a user saves a password for a form with no username element on http://foo.com > - the user visits foo.com/a, which has a password form *with* [and next, > without] a username element.
While looking into [ http://code.google.com/p/chromium/issues/detail?id=122050 ], I realized this might bring up a "Save password?" prompt that would save a username-less password when re-authenticating to google.com -- e.g. with the following STR: 1) Go to http://www.google.com/accounts 2) Sign in if necessary 3) Click Security 4) Click "Authorize sites" 5) Get prompted to re-enter your passphrase. At step 5, there is no username field, as the username is pre-populated (and not marked up as a form field). It seems wrong to offer to save a username-less password for that page... https://chromiumcodereview.appspot.com/9625026/diff/35002/chrome/renderer/aut... File chrome/renderer/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9625026/diff/35002/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:468: form_elements->input_elements[GetUsernameField(form_data).name]; nit: The indentation here was previously correct -- lines that have to be wrapped because they're longer than 80-columns should be indented by 4 spaces (to emphasize the wrapping). https://chromiumcodereview.appspot.com/9625026/diff/35002/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:473: form_elements->input_elements[GetPasswordField(form_data).name]; nit: Ditto.
Thanks Ilya for revirwing and commenting. I've modified the patch where you pointed out and upload. >At step 5, there is no username field, as the username ispre-populated (and notmarked up as a form field). >It seems wrong to offer to save a username-less password for that page... Thank you for pointing out the situation I didn't realize too. uum... honestry, I haven't tried yet because My mobile phone is broken now and I haven't done with 2-step verification... But I think the prompt probably comes out as you said. And I think it's very difficult to solve if it comes out. Because we can't find the site has changed to username-less login style or the site semantically autofill the username. Althongh it may occur, I believe we should solve " Save password without an associated username." at fitst. It seems that we this patch to solve this issue, and this issue is duplicated by other many issues. I think it's better to solve this issue, and then we try to next issue(Ilya pointed out). I believe it's a "done is better than perfect" situation, as Zuckerberg said. - Yumi https://chromiumcodereview.appspot.com/9625026/diff/35002/chrome/renderer/aut... File chrome/renderer/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9625026/diff/35002/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:468: form_elements->input_elements[GetUsernameField(form_data).name]; On 2012/04/05 22:20:36, Ilya Sherman wrote: > nit: The indentation here was previously correct -- lines that have to be > wrapped because they're longer than 80-columns should be indented by 4 spaces > (to emphasize the wrapping). Done. https://chromiumcodereview.appspot.com/9625026/diff/35002/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:473: form_elements->input_elements[GetPasswordField(form_data).name]; On 2012/04/05 22:20:36, Ilya Sherman wrote: > nit: Ditto. Done.
I'll continue to defer to Tim for the primary review on this CL -- just thought that case was worth bringing up so that we knew what might break with this CL, and could better evaluate the benefits & risks.
I'm sorry that I took a bit emotional behavior... I think we should think what browsers should perform. After I sent the yesterday's reply, I've fixed my phone and I've tried the case you wrote. Unfortunately, the popup to save password comes out at step 5. (I'll upload the pictures about it later.) I agree with Ilya in the point that we don't want to autofill in the webpage which want to re-authentication. But, I think the web-browsers should save password in without an associated username element. In this case, I think that the way to re-auth in http://www.google.com/accounts is imperfection if the site wants to comfirm the person is identical to the owner of the account with using to re-type the password, because, as you know, the most of web-browsers save passwords. I believe that the function to save passwords is very useful for many users to prevent us from re-typing the passwords and usernames. But, the "autofill the password and username" has the same issue, because if other person use our browser and goes to http://www.google.com/accounts, this browser autofills my username and password. So, the function named "autofill" has the nature of increasing risks by nature... But, of course, we should minimize the risks. I think the risk of autofilling the username and password is less than autofilling the password to re-auth in http://www.google.com/accounts . Because we already fill the password and username in the re-auth case, while we fill the first time in the first login case. I guess that this is the essence of this issue. In this case, http://www.google.com/accounts should other way to reduce risks if they really want to reduce the risk.(Or, they persuade the users not to use firefox... which save the password without the username...) But, I think we should debate how we should do. As Ilya said, we should evaluate the benefits & risks. If the risk is too big, we should take the workaround that we don't save password without username. I'd like to hear many response about this issue. I look forward to hearing from Ilya and Tim, and other many people. I hope we will take the best way :) - Yumi On 2012/04/06 22:04:12, Ilya Sherman wrote: > I'll continue to defer to Tim for the primary review on this CL -- just thought > that case was worth bringing up so that we knew what might break with this CL, > and could better evaluate the benefits & risks.
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 tried to upload pictures in https://chromiumcodereview.appspot.com/9625026/ , but I haven't found the way to upload to this site...) - Yumi
Oh! I took a typo in just before reply. > I think the risk of autofilling the username and password is less than > autofilling the password to re-auth in http://www.google.com/accounts . It's typo. below text is correct. I think the risk of autofilling the username and password is "more" than autofilling the password to re-auth in http://www.google.com/accounts Because we already fill the password and username in the re-auth case, while we fill the first time in the first login case. I guess that this is the essence of this issue. In this case, http://www.google.com/accounts should other way to reduce risks if they really want to reduce the risk.(Or, they persuade the users not to use firefox... which save the password without the username...)
Dear Tim and Ilya Hi. I've been waiting for your review and LGTM... But you've not reviewed yet and even replied... Even, I've thought about the risk which is pointed out by Ilya and submitted my opinion. But you've not replied yet. I guess you've been very busy and have no time to deal with this patch and issue. If you so, please introduce me to the person who can review it. Or please give me the right to review and commit it myself(In shout, please give me the right to be committer.). We've took already 1 month about this patch. I think It's toooo late. I understand some problems take many times because I'm developing linux kernel in my work. But What I concerned is that you haven't enough time to look this issue or you're not interested in this issue.(If you're not interested in this issue, you shouldn't look about that kinds of issue. Other person who is interested in this issue should review it.) I'm just contribute to chromium, and want to find with communicating many people. But you are not responding, especially Tim. Please give me a response as soon as possible. - Yumi
On 2012/04/11 18:44:31, Yumikiyo Osanai wrote: > Dear Tim and Ilya > > Hi. > I've been waiting for your review and LGTM... > But you've not reviewed yet and even replied... > > Even, I've thought about the risk which is pointed out by Ilya and submitted my > opinion. > But you've not replied yet. > > I guess you've been very busy and have no time to deal with this patch and > issue. > If you so, please introduce me to the person who can review it. > Or please give me the right to review and commit it myself(In shout, please give > me the right to be committer.). > > We've took already 1 month about this patch. I think It's toooo late. > I understand some problems take many times because I'm developing linux kernel > in my work. But What I concerned is that you haven't enough time to look this > issue or you're not interested in this issue.(If you're not interested in this > issue, you shouldn't look about that kinds of issue. Other person who is > interested in this issue should review it.) > > I'm just contribute to chromium, and want to find with communicating many > people. But you are not responding, especially Tim. Please give me a response as > soon as possible. > > > - Yumi Sorry for the slow responses. In the test comment, I was wondering if the matching behavior would suggest a password that was entered on a form with a username element *but* with an empty username, for a form that does not have a username field. Ilya's case is more interesting though. My worry is that any time we show a 'Save Password' infobar when we really shouldn't, it gets in the way and it lowers the user's trust of the feature. And it's an explicit design goal to avoid that, which is why for example we try to not prompt to save a password if we detect that your password was invalid. However, you could argue that in that case, the password field should have autocomplete=off. So I'm torn, but suspect that it's probably OK... Ilya, should we check with Tyler? I'm totally comfortable with Ilya reviewing this by the way (probably a better idea anyway given my low availability this week!) I don't think I can spot any more fundamental issues by inspection so we may just have to try it if we're okay with the tradeoffs.
Thank you for reviewing the issue! > In the test comment, I was wondering if the matching behavior would > suggest a password that was entered on a form with a username element > *but* with an empty username, for a form that does not have a username field. I'm agree with your idea, and it's the same behavior as my patch already does :) > Ilya's case is more interesting though. My worry is that any time > we show a 'Save Password' infobar when we really shouldn't, > it gets in the way and it lowers the user's trust of the feature. I'm agree with this idea too. I'd like to refine the code that we don't show a 'Save Password' infobar when the password value is same as the value we already stored. But, I haven't understood the mechanism how 'Save Password' show yet... If you know about the mechanism, please tell me :) (I found that the SavePasswordInfoBarDelegate expresses the 'Save Password' infober, but I haven't found when (and what function) we show the infobar, and what is the conditions of showing the infobar...) > However, you could argue that in that case, the password field > should have autocomplete=off. If the google account webpage creators don't want to autofill a password, I think they should add the attribute autocomplete=off in their html. - Yumi
http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... File chrome/browser/password_manager/password_form_manager_unittest.cc (right): http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:38: // Remove the username and regererate the profile. nit: "regererate" -> "regenerate" http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:39: // This function change the observed_form_ and saved_match_. nit: "change" -> "changes" http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:41: // that there is no username. nit: "that there is no username" -> "for which there is no username field" http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:89: GetPendingCredentials(manager)->origin.spec()); nit: Please indent to the parenthesis after "EQ" http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:91: GetPendingCredentials(manager)->signon_realm); nit: Please indent to the parenthesis after "EQ" http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:94: GetPendingCredentials(manager)->password_value); nit: Please indent to the parenthesis after "EQ" http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:96: GetPendingCredentials(manager)->username_value); nit: Please indent to the parenthesis after "EQ" http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:97: return; nit: This return statement is redundant; please remove it. http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:100: void EmptyAction() { nit: Perhaps "TestEmptyAction()"? Method names typically should have verbs. http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:102: profile(), NULL, *observed_form(), false)); nit: Please preserve the previous indentation, i.e. indent this four extra spaces rather than two. http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:115: GetPendingCredentials(manager.get())->action); nit: Please indent to the parenthesis after "EQ" http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:118: void ValidForms() { nit: Perhaps "TestValidForms()"? Method names typically should have verbs. http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:153: void ValidFormsBasic() { nit: Perhaps "TestValidFormsBasic()"? Method names typically should have verbs. http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:199: profile(), NULL, *observed_form(), false); nit: Can this be allocated on the stack, rather than on the heap, so that we don't need to |delete| it later? http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:239: profile(), NULL, *observed_form(), false); nit: Can this be allocated on the stack, rather than on the heap, so that we don't need to |delete| it later? http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:260: // If there is no username, we supporse it's an update. nit: "supporse" -> "assume" http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:265: saved_match()->origin.spec()); nit: Please preserve the parameter order that was used before, i.e. "saved_match()->origin.spec()" should be the first parameter. http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:267: saved_match()->signon_realm); nit: Please preserve the parameter order that was used before, i.e. "saved_match()->signon_realm" should be the first parameter. http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:367: profile(), NULL, *observed_form(), false); nit: It looks like this will leak. Can it be allocated on the stack rather than on the heap? If not, you should probably use a scoped_ptr<> http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:377: // *with* a username element. Should there be code below this comment? http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:380: // *without* a username element. Should there be code below this comment? http://codereview.chromium.org/9625026/diff/50001/chrome/renderer/autofill/pa... File chrome/renderer/autofill/password_autofill_manager.cc (right): http://codereview.chromium.org/9625026/diff/50001/chrome/renderer/autofill/pa... chrome/renderer/autofill/password_autofill_manager.cc:198: // Get whether fill_data has a username or not. nit: Wording suggestion: """Returns true iff the |fill_data| includes a username field.""" http://codereview.chromium.org/9625026/diff/50001/chrome/renderer/autofill/pa... chrome/renderer/autofill/password_autofill_manager.cc:203: // Get the username field from fill_data. nit: Wording suggestion: "Returns the username field from the |fill_data|. The caller is responsible for checking that this field exists prior to calling this function." http://codereview.chromium.org/9625026/diff/50001/chrome/renderer/autofill/pa... chrome/renderer/autofill/password_autofill_manager.cc:204: // If it hasn't any username, calls NOTREACHED(). nit: Please omit this line -- it's redundant with the code (and is indeed a bit out of sync, since the code now instead has a DCHECK()). http://codereview.chromium.org/9625026/diff/50001/chrome/renderer/autofill/pa... chrome/renderer/autofill/password_autofill_manager.cc:211: // Get the password field from fill_data. nit: Wording suggestion: "Returns the password field from |fill_data|." http://codereview.chromium.org/9625026/diff/50001/chrome/renderer/autofill/pa... chrome/renderer/autofill/password_autofill_manager.cc:469: // If there isn't any username, go to next iteration. nit: This comment is redundant with the code. Instead, I'd suggest something like "If the form does not have a username field, it doesn't make sense to wait for the username to be filled." http://codereview.chromium.org/9625026/diff/50001/chrome/renderer/autofill/pa... chrome/renderer/autofill/password_autofill_manager.cc:513: // There are both a password and a username. nit: Please move this comment into the if-stmt, right above line 516 in the current patch set, since it only describes what happens if the if-stmt's condition passes. http://codereview.chromium.org/9625026/diff/50001/chrome/renderer/autofill/pa... chrome/renderer/autofill/password_autofill_manager.cc:583: // that the username form and the possword form exist. nit: Wording suggestion: """If there is no username field, there are no suggestions that make sense to show in the popup.""" http://codereview.chromium.org/9625026/diff/50001/chrome/renderer/autofill/pa... File chrome/renderer/autofill/password_autofill_manager_browsertest.cc (right): http://codereview.chromium.org/9625026/diff/50001/chrome/renderer/autofill/pa... chrome/renderer/autofill/password_autofill_manager_browsertest.cc:161: } nit: Since this function is used only in a single test, and the code is really short, please inline the code in the test function. This helps someone reading the code more quickly understand how the test works. http://codereview.chromium.org/9625026/diff/50001/chrome/renderer/autofill/pa... chrome/renderer/autofill/password_autofill_manager_browsertest.cc:175: PasswordFormFillData fill_data_only_password_; nit: Since this is used only in a single test, please define this locally in that test. This helps someone reading the code more quickly understand how the test works. http://codereview.chromium.org/9625026/diff/50001/webkit/forms/password_form_... File webkit/forms/password_form_dom_manager.cc (right): http://codereview.chromium.org/9625026/diff/50001/webkit/forms/password_form_... webkit/forms/password_form_dom_manager.cc:55: // we suppose that there isn't any username field and don't set the username. nit: Please wrap to 80-col, so that the first line is not shorter than the second one. For the wording, perhaps "The |username_element| should be empty if and only if the form does not include a username field." http://codereview.chromium.org/9625026/diff/50001/webkit/forms/password_form_... File webkit/forms/password_form_dom_manager.h (right): http://codereview.chromium.org/9625026/diff/50001/webkit/forms/password_form_... webkit/forms/password_form_dom_manager.h:37: // Otherwise, we supporse that it has wrong datas. nit: Wording suggestion: """basic_data.fields[0] corresponds to the password field, which is always present. basic_data.fields[1] corresponds to the username field, if there is one. So, basic_data.size() will be 2 for forms containing both a username and a password field; and 1 for forms containing only a password field."""
I meant to say: The wording suggestions from my previous reply are merely suggestions. Feel free to incorporate them where you think they're clearer, tweak them to make them better, or keep the existing wording if you think it's clearer as is.
Thank you for reviewing and writing many valuable comment! I've fixed the code :-) Please review it. http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... File chrome/browser/password_manager/password_form_manager_unittest.cc (right): http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:38: // Remove the username and regererate the profile. On 2012/04/19 00:27:37, Ilya Sherman wrote: > nit: "regererate" -> "regenerate" Done. http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:39: // This function change the observed_form_ and saved_match_. On 2012/04/19 00:27:37, Ilya Sherman wrote: > nit: "change" -> "changes" Done. http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:89: GetPendingCredentials(manager)->origin.spec()); On 2012/04/19 00:27:37, Ilya Sherman wrote: > nit: Please indent to the parenthesis after "EQ" Done. http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:91: GetPendingCredentials(manager)->signon_realm); On 2012/04/19 00:27:37, Ilya Sherman wrote: > nit: Please indent to the parenthesis after "EQ" Done. http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:94: GetPendingCredentials(manager)->password_value); On 2012/04/19 00:27:37, Ilya Sherman wrote: > nit: Please indent to the parenthesis after "EQ" Done. http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:96: GetPendingCredentials(manager)->username_value); On 2012/04/19 00:27:37, Ilya Sherman wrote: > nit: Please indent to the parenthesis after "EQ" Done. http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:97: return; On 2012/04/19 00:27:37, Ilya Sherman wrote: > nit: This return statement is redundant; please remove it. Done. http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:100: void EmptyAction() { On 2012/04/19 00:27:37, Ilya Sherman wrote: > nit: Perhaps "TestEmptyAction()"? Method names typically should have verbs. Done. http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:102: profile(), NULL, *observed_form(), false)); On 2012/04/19 00:27:37, Ilya Sherman wrote: > nit: Please preserve the previous indentation, i.e. indent this four extra > spaces rather than two. Done. http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:115: GetPendingCredentials(manager.get())->action); On 2012/04/19 00:27:37, Ilya Sherman wrote: > nit: Please indent to the parenthesis after "EQ" Done. http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:118: void ValidForms() { On 2012/04/19 00:27:37, Ilya Sherman wrote: > nit: Perhaps "TestValidForms()"? Method names typically should have verbs. Done. http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:153: void ValidFormsBasic() { On 2012/04/19 00:27:37, Ilya Sherman wrote: > nit: Perhaps "TestValidFormsBasic()"? Method names typically should have verbs. Done. http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:199: profile(), NULL, *observed_form(), false); On 2012/04/19 00:27:37, Ilya Sherman wrote: > nit: Can this be allocated on the stack, rather than on the heap, so that we > don't need to |delete| it later? Oh! You are right. I've fixed to allocate the instance on the stack. http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:260: // If there is no username, we supporse it's an update. On 2012/04/19 00:27:37, Ilya Sherman wrote: > nit: "supporse" -> "assume" Done. http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:265: saved_match()->origin.spec()); On 2012/04/19 00:27:37, Ilya Sherman wrote: > nit: Please preserve the parameter order that was used before, i.e. > "saved_match()->origin.spec()" should be the first parameter. Done. http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:267: saved_match()->signon_realm); On 2012/04/19 00:27:37, Ilya Sherman wrote: > nit: Please preserve the parameter order that was used before, i.e. > "saved_match()->signon_realm" should be the first parameter. Done. http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:377: // *with* a username element. On 2012/04/19 00:27:37, Ilya Sherman wrote: > Should there be code below this comment? Oh, Thank you for pointing out it. I forgot to delete the comment. What I'd like to test in this area is already does in TestNewLogin(). http://codereview.chromium.org/9625026/diff/50001/chrome/browser/password_man... chrome/browser/password_manager/password_form_manager_unittest.cc:380: // *without* a username element. On 2012/04/19 00:27:37, Ilya Sherman wrote: > Should there be code below this comment? Oh, I'll remove this comment. What I'd like to test in this area is already does in TestNewLoginAndUpdateWithoutUsername.
On 2012/04/19 00:28:29, Ilya Sherman wrote: > I meant to say: The wording suggestions from my previous reply are merely > suggestions. Feel free to incorporate them where you think they're clearer, > tweak them to make them better, or keep the existing wording if you think it's > clearer as is. Thank you! I'm very encouraged with your comment. I'm going to try to solve the issue you pointed out after committing the patch. (I agree with your suggestion in the point we shouldn't show the "save password" popup. But I'm not find a solution to solve it. So, I think it's better that we commit this patch anyway, and we try to solve next issue.) So, I'm going to submit the "save password" popup issue to bag tracker after I commit this patch. - Yumi
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) |