Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
369323003:
Implement autocomplete='current-password' and 'new-password'. (Closed)
|
Created:
6 years, 5 months ago by engedy Modified:
6 years, 5 months ago CC:
chromium-reviews, benquan, jam, browser-components-watch_chromium.org, darin-cc_chromium.org, Dane Wallinga, dyu1, rouslan+autofillwatch_chromium.org, estade+watch_chromium.org, gcasto+watchlist_chromium.org, Ilya Sherman, mkwst+watchlist_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionImplement autocomplete='current-password' and 'new-password'.
BUG=375333
# Depends on recently landed CL, ran try bots with manually specified base revison.
NOTRY=True
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282356
Patch Set 1 #Patch Set 2 : #
Total comments: 20
Patch Set 3 : Addressed comments from vabr@. #
Total comments: 2
Patch Set 4 : Fixed nits. #
Total comments: 4
Patch Set 5 : Addressed comments from gcasto@. #Patch Set 6 : Rebase/ #
Messages
Total messages: 12 (0 generated)
@Vaclav, @Garrett, please review. I think the main question here is how we want to handle partially marked-up forms. After discussing with Vaclav, I took a very conservative approach, as described in the code.
Thanks, Balázs. LGTM, with some comments. https://codereview.chromium.org/369323003/diff/20001/components/autofill/cont... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/369323003/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_form_conversion_utils.cc:48: // the element we are looking for. typo: the element -> as the element https://codereview.chromium.org/369323003/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_form_conversion_utils.cc:51: *current_password = *it; optional nit: I wonder if we should document that we rely on the two output arguments to be null. https://codereview.chromium.org/369323003/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_form_conversion_utils.cc:60: // We do this under the assumption they are not intentionally not marked, typo: remove the first "not" https://codereview.chromium.org/369323003/diff/20001/components/autofill/cont... File components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc (right): https://codereview.chromium.org/369323003/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc:42: // 2.) "", causing an empty attribute (i.e. autocomplete='') to be added. optional nit: Although in HTML, '' and "" is equivalent, maybe the comment should say the same as the code. Currently, the comment says autocomplete='', whereas the code produces autocomplete="". https://codereview.chromium.org/369323003/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc:46: std::string autocomplete_attribute(autocomplete != NULL ? optional nit: Usually the "!= NULL" is left out for pointer tests. If you find the result of dropping it confusing here, feel free to keep it. Personally, I would drop it. (If you decide to drop it, please also do it below.) https://codereview.chromium.org/369323003/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc:364: ASSERT_NE(static_cast<PasswordForm*>(NULL), password_form.get()); nit: ASSERT_TRUE(password_form) This is more readable than the current line, because it only contains the important stuff (assert, and the object). Usage of (scoped) pointers as Booleans is widespread enough in Chromium for this shortcut to make sense. Also, there is no added value in having ASSERT_NE output the value f the pointer in the failing case, because it will be "NULL vs. NULL". https://codereview.chromium.org/369323003/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc:366: EXPECT_EQ(base::UTF8ToUTF16("username1"), password_form->username_element); I'm a bit confused as for why this test is concerned with usernames at all. I suggest not to include usernames in the expectations at all, because it might hide the main purpose of the test. Alternatively, please add a comment why it is important to know how parsing of usernames went. (Does that influence how passwords are identified?) https://codereview.chromium.org/369323003/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc:376: ASSERT_EQ(1u, password_form->other_possible_usernames.size()); Why is this an assertion and all others are expectation? I could not find a canonical guide to choose between those two, but my understanding is that assertions are to make sure that the environment is sane, whereas expectations make sure that the tested code (not its dependencies) works as expected. So in this case I would suggest using EXPECT_EQ. It's OK to guard the next expectation with an if-condition, to make sure you don't go beyond the vector size, because if that condition fails, the expectation on this current line will already be failing.
https://codereview.chromium.org/369323003/diff/20001/components/autofill/cont... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/369323003/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_form_conversion_utils.cc:48: // the element we are looking for. On 2014/07/05 09:21:39, vabr (Chromium) wrote: > typo: the element -> as the element Done. I have actually removed this block of comments: it said the same that was already stated aboev. https://codereview.chromium.org/369323003/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_form_conversion_utils.cc:51: *current_password = *it; On 2014/07/05 09:21:39, vabr (Chromium) wrote: > optional nit: I wonder if we should document that we rely on the two output > arguments to be null. Done. https://codereview.chromium.org/369323003/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_form_conversion_utils.cc:60: // We do this under the assumption they are not intentionally not marked, On 2014/07/05 09:21:39, vabr (Chromium) wrote: > typo: remove the first "not" Done, also rephrased. https://codereview.chromium.org/369323003/diff/20001/components/autofill/cont... File components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc (right): https://codereview.chromium.org/369323003/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc:42: // 2.) "", causing an empty attribute (i.e. autocomplete='') to be added. On 2014/07/05 09:21:39, vabr (Chromium) wrote: > optional nit: Although in HTML, '' and "" is equivalent, maybe the comment > should say the same as the code. Currently, the comment says autocomplete='', > whereas the code produces autocomplete="". Done, replaced all 3 occurrences. https://codereview.chromium.org/369323003/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc:46: std::string autocomplete_attribute(autocomplete != NULL ? On 2014/07/05 09:21:40, vabr (Chromium) wrote: > optional nit: Usually the "!= NULL" is left out for pointer tests. If you find > the result of dropping it confusing here, feel free to keep it. Personally, I > would drop it. > (If you decide to drop it, please also do it below.) Done, replaced 2 occurrences. https://codereview.chromium.org/369323003/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc:364: ASSERT_NE(static_cast<PasswordForm*>(NULL), password_form.get()); On 2014/07/05 09:21:39, vabr (Chromium) wrote: > nit: > ASSERT_TRUE(password_form) > This is more readable than the current line, because it only contains the > important stuff (assert, and the object). > Usage of (scoped) pointers as Booleans is widespread enough in Chromium for this > shortcut to make sense. > Also, there is no added value in having ASSERT_NE output the value f the pointer > in the failing case, because it will be "NULL vs. NULL". I could not agree more. However, for consistency with the rest of the tests, let us keep this as is, just for a day or two. I am planning to file a follow-up CL to fix this and some other smells in this test file. https://codereview.chromium.org/369323003/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc:366: EXPECT_EQ(base::UTF8ToUTF16("username1"), password_form->username_element); On 2014/07/05 09:21:40, vabr (Chromium) wrote: > I'm a bit confused as for why this test is concerned with usernames at all. I > suggest not to include usernames in the expectations at all, because it might > hide the main purpose of the test. > > Alternatively, please add a comment why it is important to know how parsing of > usernames went. (Does that influence how passwords are identified?) Done. https://codereview.chromium.org/369323003/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc:376: ASSERT_EQ(1u, password_form->other_possible_usernames.size()); On 2014/07/05 09:21:40, vabr (Chromium) wrote: > Why is this an assertion and all others are expectation? > I could not find a canonical guide to choose between those two, but my > understanding is that assertions are to make sure that the environment is sane, > whereas expectations make sure that the tested code (not its dependencies) works > as expected. > So in this case I would suggest using EXPECT_EQ. It's OK to guard the next > expectation with an if-condition, to make sure you don't go beyond the vector > size, because if that condition fails, the expectation on this current line will > already be failing. I think it is common practice [1] in Chromium to use ASSERTs to short-circuit the test case and prevent a crash regardless of what is being asserted. Unless you feel strongly about this, I would prefer to keep it as is. [1] https://code.google.com/p/chromium/codesearch#search/&q=ASSERT_EQ%5C(.*%5C.si...
Still LGTM. Thanks, Vaclav https://codereview.chromium.org/369323003/diff/20001/components/autofill/cont... File components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc (right): https://codereview.chromium.org/369323003/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc:364: ASSERT_NE(static_cast<PasswordForm*>(NULL), password_form.get()); On 2014/07/07 08:00:39, engedy wrote: > On 2014/07/05 09:21:39, vabr (Chromium) wrote: > > nit: > > ASSERT_TRUE(password_form) > > This is more readable than the current line, because it only contains the > > important stuff (assert, and the object). > > Usage of (scoped) pointers as Booleans is widespread enough in Chromium for > this > > shortcut to make sense. > > Also, there is no added value in having ASSERT_NE output the value f the > pointer > > in the failing case, because it will be "NULL vs. NULL". > > I could not agree more. However, for consistency with the rest of the tests, let > us keep this as is, just for a day or two. > > I am planning to file a follow-up CL to fix this and some other smells in this > test file. OK, as soon as fixing this is scheduled soon and tracked, it's fine to keep this CL as is. https://codereview.chromium.org/369323003/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc:376: ASSERT_EQ(1u, password_form->other_possible_usernames.size()); On 2014/07/07 08:00:39, engedy wrote: > On 2014/07/05 09:21:40, vabr (Chromium) wrote: > > Why is this an assertion and all others are expectation? > > I could not find a canonical guide to choose between those two, but my > > understanding is that assertions are to make sure that the environment is > sane, > > whereas expectations make sure that the tested code (not its dependencies) > works > > as expected. > > So in this case I would suggest using EXPECT_EQ. It's OK to guard the next > > expectation with an if-condition, to make sure you don't go beyond the vector > > size, because if that condition fails, the expectation on this current line > will > > already be failing. > > I think it is common practice [1] in Chromium to use ASSERTs to short-circuit > the test case and prevent a crash regardless of what is being asserted. Unless > you feel strongly about this, I would prefer to keep it as is. > > [1] > https://code.google.com/p/chromium/codesearch#search/&q=ASSERT_EQ%5C(.*%5C.si... So here is one more for not using the ASSERT: when somebody modifies the test, and adds more checks at the end, which will not be affected by the property you assert on, those checks will not be done if the assert fails. It is generally useful to do as many checks as possible even in failing tests, to make diagnostics easier. Also, I'm not sure I buy your link [1] completely -- there are plenty of places where such asserts are used correctly in the sense I described in my comment. Often, though common, the code practises found in our tests are not the best to follow as well. This is just a suggestion, though. I'm not forcing this change on you, it's fine to submit the CL as it is. https://codereview.chromium.org/369323003/diff/40001/components/autofill/cont... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/369323003/diff/40001/components/autofill/cont... components/autofill/content/renderer/password_form_conversion_utils.cc:59: // If we have seen an element with either autocomplete attribute, take that as optional nit: either autocomplete attribute -> either of the autocomplete attributes above (To make it clear that a password element with other autocomplete elements, notably autocomplete=off, do not count.)
https://codereview.chromium.org/369323003/diff/20001/components/autofill/cont... File components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc (right): https://codereview.chromium.org/369323003/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc:364: ASSERT_NE(static_cast<PasswordForm*>(NULL), password_form.get()); On 2014/07/07 10:01:46, vabr (Chromium) wrote: > On 2014/07/07 08:00:39, engedy wrote: > > On 2014/07/05 09:21:39, vabr (Chromium) wrote: > > > nit: > > > ASSERT_TRUE(password_form) > > > This is more readable than the current line, because it only contains the > > > important stuff (assert, and the object). > > > Usage of (scoped) pointers as Booleans is widespread enough in Chromium for > > this > > > shortcut to make sense. > > > Also, there is no added value in having ASSERT_NE output the value f the > > pointer > > > in the failing case, because it will be "NULL vs. NULL". > > > > I could not agree more. However, for consistency with the rest of the tests, > let > > us keep this as is, just for a day or two. > > > > I am planning to file a follow-up CL to fix this and some other smells in this > > test file. > > OK, as soon as fixing this is scheduled soon and tracked, it's fine to keep this > CL as is. I have prepared https://codereview.chromium.org/372683003/. https://codereview.chromium.org/369323003/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc:376: ASSERT_EQ(1u, password_form->other_possible_usernames.size()); On 2014/07/07 10:01:46, vabr (Chromium) wrote: > On 2014/07/07 08:00:39, engedy wrote: > > On 2014/07/05 09:21:40, vabr (Chromium) wrote: > > > Why is this an assertion and all others are expectation? > > > I could not find a canonical guide to choose between those two, but my > > > understanding is that assertions are to make sure that the environment is > > sane, > > > whereas expectations make sure that the tested code (not its dependencies) > > works > > > as expected. > > > So in this case I would suggest using EXPECT_EQ. It's OK to guard the next > > > expectation with an if-condition, to make sure you don't go beyond the > vector > > > size, because if that condition fails, the expectation on this current line > > will > > > already be failing. > > > > I think it is common practice [1] in Chromium to use ASSERTs to short-circuit > > the test case and prevent a crash regardless of what is being asserted. Unless > > you feel strongly about this, I would prefer to keep it as is. > > > > [1] > > > https://code.google.com/p/chromium/codesearch#search/&q=ASSERT_EQ%5C(.*%5C.si... > > So here is one more for not using the ASSERT: when somebody modifies the test, > and adds more checks at the end, which will not be affected by the property you > assert on, those checks will not be done if the assert fails. It is generally > useful to do as many checks as possible even in failing tests, to make > diagnostics easier. > Also, I'm not sure I buy your link [1] completely -- there are plenty of places > where such asserts are used correctly in the sense I described in my comment. > Often, though common, the code practises found in our tests are not the best to > follow as well. > > This is just a suggestion, though. I'm not forcing this change on you, it's fine > to submit the CL as it is. I stand convinced. I have also changed this in https://codereview.chromium.org/372683003/. https://codereview.chromium.org/369323003/diff/40001/components/autofill/cont... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/369323003/diff/40001/components/autofill/cont... components/autofill/content/renderer/password_form_conversion_utils.cc:59: // If we have seen an element with either autocomplete attribute, take that as On 2014/07/07 10:01:47, vabr (Chromium) wrote: > optional nit: either autocomplete attribute -> either of the autocomplete > attributes above > (To make it clear that a password element with other autocomplete elements, > notably autocomplete=off, do not count.) Done. This is exactly the kind of ambiguity I am really annoyed by as a reader, so thanks a lot for pointing this out!
Thanks, still LGTM. Please make sure that the EXPECT_THAT solution, employed by you in https://codereview.chromium.org/372683003/, to the ASSERT vs. EXPECT debate we had here also applies to the code in this CL. If you land this CL, then rebase https://codereview.chromium.org/372683003/ and change the ASSERT+EXPECT added here to EXPECT_THAT, that's fine. Cheers, Vaclav
On 2014/07/07 12:45:32, vabr (Chromium) wrote: > Thanks, still LGTM. > Please make sure that the EXPECT_THAT solution, employed by you in > https://codereview.chromium.org/372683003/, to the ASSERT vs. EXPECT debate we > had here also applies to the code in this CL. If you land this CL, then rebase > https://codereview.chromium.org/372683003/ and change the ASSERT+EXPECT added > here to EXPECT_THAT, that's fine. > > Cheers, > Vaclav Yes, it is already based on that.
LGTM Disabling heuristics in the presence of these attributes seem reasonable to me. It should prevent bad habits from sites expecting behavior that isn't specified. https://codereview.chromium.org/369323003/diff/60001/components/autofill/cont... File components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc (right): https://codereview.chromium.org/369323003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc:278: {{"current-password", NULL, NULL}, "password1", "alpha", "", ""}, Nit: Given how much information there is here, I think formatting the same way throughout would be helpful for reading. Line breaking after the "autocomplete" field seems reasonable. https://codereview.chromium.org/369323003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc:369: // TODO(engedy): Double-check whether this is the intended behavior. I guess there are a few options we could use. Before first password field or before first annotated password field seem like the most obvious choices. I don't see any particular reason to prefer one over another, so this seems fine. Hopefully site owners will also set the username autocomplete attribute as well.
https://codereview.chromium.org/369323003/diff/60001/components/autofill/cont... File components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc (right): https://codereview.chromium.org/369323003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc:278: {{"current-password", NULL, NULL}, "password1", "alpha", "", ""}, On 2014/07/07 22:32:36, Garrett Casto wrote: > Nit: Given how much information there is here, I think formatting the same way > throughout would be helpful for reading. Line breaking after the "autocomplete" > field seems reasonable. Done. https://codereview.chromium.org/369323003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc:369: // TODO(engedy): Double-check whether this is the intended behavior. On 2014/07/07 22:32:36, Garrett Casto wrote: > I guess there are a few options we could use. Before first password field or > before first annotated password field seem like the most obvious choices. I > don't see any particular reason to prefer one over another, so this seems fine. > Hopefully site owners will also set the username autocomplete attribute as well. I have kept the current implementation, then.
The CQ bit was checked by engedy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/engedy@chromium.org/369323003/100001
Message was sent while issue was closed.
Change committed as 282356 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
