Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2411333004:
Make HasAutocompleteAttributeValue handle multi-valued strings (Closed)
|
Created:
4 years, 2 months ago by jdoerrie Modified:
4 years, 2 months ago Reviewers:
vabr (Chromium) CC:
chromium-reviews, mlamouri+watch-content_chromium.org, vabr+watchlistpasswordmanager_chromium.org, rouslan+autofill_chromium.org, jam, browser-components-watch_chromium.org, jdonnelly+autofillwatch_chromium.org, darin-cc_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, gcasto+watchlist_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake HasAutocompleteAttributeValue handle multi-valued strings
This change addresses a bug where HasAutocompleteAttributeValue assumes that
the attribute always only has one value. In reality the attribute can consist
of multiple values separated by whitespace. A similar bug was already fixed in
FormStructure::ParseFieldTypesFromAutocompleteAttributes.
This change fixes the bug by extracting the corresponding logic into
a LowercaseAndTokenizeAttributeString method, residing in
components/autofill/core/common/autofill_util. Both the implementation in
password_form_conversion_utils and autofill_util now reference this method.
Furthermore a corresponding unit test was added to autofill_util_unittest.cc
BUG=655157
Committed: https://crrev.com/810f2c7d12a60a8ce27bc4e5ed8561d1d1c749bb
Cr-Commit-Position: refs/heads/master@{#425025}
Patch Set 1 #
Total comments: 20
Patch Set 2 : Make HasAutocompleteAttributeValue handle multi-valued strings #
Messages
Total messages: 19 (12 generated)
jdoerrie@chromium.org changed reviewers: + vabr@chromium.org
The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM with a few comments. Also, please do not forget to edit the issue description to include: * what is the pre-CL state * why is it bad * how exactly does this CL address it Thanks for the patch! Vaclav https://codereview.chromium.org/2411333004/diff/1/components/autofill/content... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/2411333004/diff/1/components/autofill/content... components/autofill/content/renderer/password_form_conversion_utils.cc:707: base::string16 autocomplete_attribute(element.getAttribute("autocomplete")); Please #include "base/strings/string16.h" https://codereview.chromium.org/2411333004/diff/1/components/autofill/content... components/autofill/content/renderer/password_form_conversion_utils.cc:708: // Lowercase and tokenize attribute string. Convert to UTF8 beforehand. Your comments here and on line 712 are correct and well written, but perhaps not necessary. The style guide [1] suggests writing comments for "tricky, non-obvious, interesting, or important parts of your code". Here, the code is short and the method names are descriptive, so perhaps even without comments, their purpose is clear. [1] https://google.github.io/styleguide/cppguide.html#Implementation_Comments https://codereview.chromium.org/2411333004/diff/1/components/autofill/content... components/autofill/content/renderer/password_form_conversion_utils.cc:712: // Return if any of the obtained attribute tokens equals the value. optional nit: if -> whether https://codereview.chromium.org/2411333004/diff/1/components/autofill/core/br... File components/autofill/core/browser/form_structure.cc (right): https://codereview.chromium.org/2411333004/diff/1/components/autofill/core/br... components/autofill/core/browser/form_structure.cc:900: // Lowercase and tokenize the attribute value. Per the spec, the tokens are nit: The comment about the reverse order is related to the code on line 922 and below. Please consider moving the comment there. https://codereview.chromium.org/2411333004/diff/1/components/autofill/core/br... components/autofill/core/browser/form_structure.cc:900: // Lowercase and tokenize the attribute value. Per the spec, the tokens are nit: The first sentence of the comment can probably again be left out, given the well-chosen name of the new function. https://codereview.chromium.org/2411333004/diff/1/components/autofill/core/co... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/2411333004/diff/1/components/autofill/core/co... components/autofill/core/common/autofill_util.cc:138: base::kWhitespaceASCII, base::KEEP_WHITESPACE, I'm curious about the KEEP_WHITESPACE: Because whitespaces are token separators, they cannot (by definition) be part of tokens. So KEEP_WHITESPACE leads to the same result as TRIM_WHITESPACE here. However, TRIM_WHITESPACE would lead to more work inside SplitString. If you use KEEP_WHITESPACE on purpose, to save work in SplitString (or for any other reason), please mention the reason in a comment. It may happen that in the future that reason might end up being valid, and will make it easier to change this code to reflect that. If you did not use KEEP_WHITESPACE on purpose, then I recommend changing it to TRIM_WHITESPACE, which describes better the result (it will contain no whitespace). Saving work in SplitString is a nice thought, but likely not noticeable in its effect here, while the benefit of the code creating the right expectations is rather important. https://codereview.chromium.org/2411333004/diff/1/components/autofill/core/co... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/2411333004/diff/1/components/autofill/core/co... components/autofill/core/common/autofill_util.h:54: // Lowercases and tokenizes a given attribute string. nit: Here a comment is desired, but perhaps it could say things not covered by the function name, in particular what is considered a separator and that empty tokens are ignored. https://codereview.chromium.org/2411333004/diff/1/components/autofill/core/co... components/autofill/core/common/autofill_util.h:55: std::vector<std::string> LowercaseAndTokenizeAttributeString( Please #include <string> and #include <vector>. Once you do that, you can remove #include <vector> from the .cc file. https://codereview.chromium.org/2411333004/diff/1/components/autofill/core/co... File components/autofill/core/common/autofill_util_unittest.cc (right): https://codereview.chromium.org/2411333004/diff/1/components/autofill/core/co... components/autofill/core/common/autofill_util_unittest.cc:104: const std::vector<std::string> empty_vector; nit: Constants have a different naming scheme. Here it would be: kEmptyVector. Even better, just inlining the constructor call should be easy to read and less verbose: std::vector<std::string>() https://codereview.chromium.org/2411333004/diff/1/components/autofill/core/co... components/autofill/core/common/autofill_util_unittest.cc:129: EXPECT_EQ(LowercaseAndTokenizeAttributeString(kTestCases[i].attribute), nit: Please reverse the order of the arguments of EXPECT_EQ. While all equivalence relations are symmetric, the effects of the EXPECT_EQ are not: On failure, the macro will output the first value as "expected" and the second one as "actual". Therefore we try to match that to avoid confusing error logs.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2411333004/diff/1/components/autofill/content... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/2411333004/diff/1/components/autofill/content... components/autofill/content/renderer/password_form_conversion_utils.cc:707: base::string16 autocomplete_attribute(element.getAttribute("autocomplete")); On 2016/10/13 12:49:31, vabr (Chromium) wrote: > Please > #include "base/strings/string16.h" Done. https://codereview.chromium.org/2411333004/diff/1/components/autofill/content... components/autofill/content/renderer/password_form_conversion_utils.cc:708: // Lowercase and tokenize attribute string. Convert to UTF8 beforehand. On 2016/10/13 12:49:31, vabr (Chromium) wrote: > Your comments here and on line 712 are correct and well written, but perhaps not > necessary. The style guide [1] suggests writing comments for "tricky, > non-obvious, interesting, or important parts of your code". Here, the code is > short and the method names are descriptive, so perhaps even without comments, > their purpose is clear. > > [1] https://google.github.io/styleguide/cppguide.html#Implementation_Comments Done. https://codereview.chromium.org/2411333004/diff/1/components/autofill/content... components/autofill/content/renderer/password_form_conversion_utils.cc:712: // Return if any of the obtained attribute tokens equals the value. On 2016/10/13 12:49:31, vabr (Chromium) wrote: > optional nit: if -> whether Acknowledged. https://codereview.chromium.org/2411333004/diff/1/components/autofill/core/br... File components/autofill/core/browser/form_structure.cc (right): https://codereview.chromium.org/2411333004/diff/1/components/autofill/core/br... components/autofill/core/browser/form_structure.cc:900: // Lowercase and tokenize the attribute value. Per the spec, the tokens are On 2016/10/13 12:49:31, vabr (Chromium) wrote: > nit: The comment about the reverse order is related to the code on line 922 and > below. Please consider moving the comment there. Done. https://codereview.chromium.org/2411333004/diff/1/components/autofill/core/br... components/autofill/core/browser/form_structure.cc:900: // Lowercase and tokenize the attribute value. Per the spec, the tokens are On 2016/10/13 12:49:31, vabr (Chromium) wrote: > nit: The comment about the reverse order is related to the code on line 922 and > below. Please consider moving the comment there. Done. https://codereview.chromium.org/2411333004/diff/1/components/autofill/core/co... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/2411333004/diff/1/components/autofill/core/co... components/autofill/core/common/autofill_util.cc:138: base::kWhitespaceASCII, base::KEEP_WHITESPACE, On 2016/10/13 12:49:31, vabr (Chromium) wrote: > I'm curious about the KEEP_WHITESPACE: > Because whitespaces are token separators, they cannot (by definition) be part of > tokens. So KEEP_WHITESPACE leads to the same result as TRIM_WHITESPACE here. > However, TRIM_WHITESPACE would lead to more work inside SplitString. > > If you use KEEP_WHITESPACE on purpose, to save work in SplitString (or for any > other reason), please mention the reason in a comment. It may happen that in the > future that reason might end up being valid, and will make it easier to change > this code to reflect that. > > If you did not use KEEP_WHITESPACE on purpose, then I recommend changing it to > TRIM_WHITESPACE, which describes better the result (it will contain no > whitespace). Saving work in SplitString is a nice thought, but likely not > noticeable in its effect here, while the benefit of the code creating the right > expectations is rather important. I agree, changed it to TRIM_WHITESPACE. https://codereview.chromium.org/2411333004/diff/1/components/autofill/core/co... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/2411333004/diff/1/components/autofill/core/co... components/autofill/core/common/autofill_util.h:54: // Lowercases and tokenizes a given attribute string. On 2016/10/13 12:49:31, vabr (Chromium) wrote: > nit: Here a comment is desired, but perhaps it could say things not covered by > the function name, in particular what is considered a separator and that empty > tokens are ignored. Done. https://codereview.chromium.org/2411333004/diff/1/components/autofill/core/co... components/autofill/core/common/autofill_util.h:55: std::vector<std::string> LowercaseAndTokenizeAttributeString( On 2016/10/13 12:49:31, vabr (Chromium) wrote: > Please > #include <string> > and > #include <vector>. > > Once you do that, you can remove #include <vector> from the .cc file. Done. https://codereview.chromium.org/2411333004/diff/1/components/autofill/core/co... File components/autofill/core/common/autofill_util_unittest.cc (right): https://codereview.chromium.org/2411333004/diff/1/components/autofill/core/co... components/autofill/core/common/autofill_util_unittest.cc:104: const std::vector<std::string> empty_vector; On 2016/10/13 12:49:31, vabr (Chromium) wrote: > nit: Constants have a different naming scheme. Here it would be: kEmptyVector. > > Even better, just inlining the constructor call should be easy to read and less > verbose: > std::vector<std::string>() Done. https://codereview.chromium.org/2411333004/diff/1/components/autofill/core/co... components/autofill/core/common/autofill_util_unittest.cc:129: EXPECT_EQ(LowercaseAndTokenizeAttributeString(kTestCases[i].attribute), On 2016/10/13 12:49:31, vabr (Chromium) wrote: > nit: Please reverse the order of the arguments of EXPECT_EQ. While all > equivalence relations are symmetric, the effects of the EXPECT_EQ are not: On > failure, the macro will output the first value as "expected" and the second one > as "actual". Therefore we try to match that to avoid confusing error logs. Done.
The CQ bit was checked by jdoerrie@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org Link to the patchset: https://codereview.chromium.org/2411333004/#ps20001 (title: "Make HasAutocompleteAttributeValue handle multi-valued strings")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by jdoerrie@chromium.org
Description was changed from ========== Make HasAutocompleteAttributeValue handle multi-valued strings BUG=655157 ========== to ========== Make HasAutocompleteAttributeValue handle multi-valued strings This change addresses a bug where HasAutocompleteAttributeValue assumes that the attribute always only has one value. In reality the attribute can consist of multiple values separated by whitespace. A similar bug was already fixed in FormStructure::ParseFieldTypesFromAutocompleteAttributes. This change fixes the bug by extracting the corresponding logic into a LowercaseAndTokenizeAttributeString method, residing in components/autofill/core/common/autofill_util. Both the implementation in password_form_conversion_utils and autofill_util now reference this method. Furthermore a corresponding unit test was added to autofill_util_unittest.cc BUG=655157 ==========
The CQ bit was checked by jdoerrie@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Make HasAutocompleteAttributeValue handle multi-valued strings This change addresses a bug where HasAutocompleteAttributeValue assumes that the attribute always only has one value. In reality the attribute can consist of multiple values separated by whitespace. A similar bug was already fixed in FormStructure::ParseFieldTypesFromAutocompleteAttributes. This change fixes the bug by extracting the corresponding logic into a LowercaseAndTokenizeAttributeString method, residing in components/autofill/core/common/autofill_util. Both the implementation in password_form_conversion_utils and autofill_util now reference this method. Furthermore a corresponding unit test was added to autofill_util_unittest.cc BUG=655157 ========== to ========== Make HasAutocompleteAttributeValue handle multi-valued strings This change addresses a bug where HasAutocompleteAttributeValue assumes that the attribute always only has one value. In reality the attribute can consist of multiple values separated by whitespace. A similar bug was already fixed in FormStructure::ParseFieldTypesFromAutocompleteAttributes. This change fixes the bug by extracting the corresponding logic into a LowercaseAndTokenizeAttributeString method, residing in components/autofill/core/common/autofill_util. Both the implementation in password_form_conversion_utils and autofill_util now reference this method. Furthermore a corresponding unit test was added to autofill_util_unittest.cc BUG=655157 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Make HasAutocompleteAttributeValue handle multi-valued strings This change addresses a bug where HasAutocompleteAttributeValue assumes that the attribute always only has one value. In reality the attribute can consist of multiple values separated by whitespace. A similar bug was already fixed in FormStructure::ParseFieldTypesFromAutocompleteAttributes. This change fixes the bug by extracting the corresponding logic into a LowercaseAndTokenizeAttributeString method, residing in components/autofill/core/common/autofill_util. Both the implementation in password_form_conversion_utils and autofill_util now reference this method. Furthermore a corresponding unit test was added to autofill_util_unittest.cc BUG=655157 ========== to ========== Make HasAutocompleteAttributeValue handle multi-valued strings This change addresses a bug where HasAutocompleteAttributeValue assumes that the attribute always only has one value. In reality the attribute can consist of multiple values separated by whitespace. A similar bug was already fixed in FormStructure::ParseFieldTypesFromAutocompleteAttributes. This change fixes the bug by extracting the corresponding logic into a LowercaseAndTokenizeAttributeString method, residing in components/autofill/core/common/autofill_util. Both the implementation in password_form_conversion_utils and autofill_util now reference this method. Furthermore a corresponding unit test was added to autofill_util_unittest.cc BUG=655157 Committed: https://crrev.com/810f2c7d12a60a8ce27bc4e5ed8561d1d1c749bb Cr-Commit-Position: refs/heads/master@{#425025} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/810f2c7d12a60a8ce27bc4e5ed8561d1d1c749bb Cr-Commit-Position: refs/heads/master@{#425025} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
