HasAutocompleteAttributeValue should handle multi-valued strings |
||
Issue descriptionbool HasAutocompleteAttributeValue(element, value) from components/autofill/content/renderer/password_form_conversion_utils.* should return whether an HTML |element| has |value| listed in its autocomplete attribute. Currently, this function assumes that the attribute always only has one value, such as autocomplete="username". However, it is valid for that attribute to have multiple values, separated by spaces, such as autocomplete="username tel". HasAutocompleteAttributeValue should understand that and return true if |value| is one of the tokens of the attribute's string. The non-password autofill code does this well already: FormStructure::ParseFieldTypesFromAutocompleteAttributes. Ideally, we would share that code in both places.
,
Oct 13 2016
Summarising an off-line conversation we had: The motivation for de-duplication here is to ensure that autofill and password manager parse the autocomplete attribute in the same way. Performance is not relevant, because this is unlikely a hot spot. The helper function should work with std::string, HasAutocompleteAttributeValue needs to convert its date to UTF8 before using the helper. Which whitespace to accept between the tokens in the attribute value depends on the HTML specification. Having said that, if we understand a super-set of what is the valid syntax, that's hardly a problem. So allowing linebreaks sounds safe. The two call-sites of the future helper function are one in browser code, and one in render code. Moreover, one is in core of the component, while the other in the content layer. So the helper needs to be in components/autofill/core/common. components/autofill/core/common/autofill_util.h seems like the best home for it. Once we get rid of the content layer completely, we might reconsider moving it, but that's an unclear future at this moment. To execute the autofill_util_unittest.cc, compile the components_unittests target: ninja -C out/something components_unittests and then run it with the proper gtest_filter: out/something/components_unittests --gtest_filter='AutofillUtilTest.MyTestCase' or even out/something/components_unittests --gtest_filter='AutofillUtilTest.*'
,
Oct 13 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/810f2c7d12a60a8ce27bc4e5ed8561d1d1c749bb commit 810f2c7d12a60a8ce27bc4e5ed8561d1d1c749bb Author: jdoerrie <jdoerrie@chromium.org> Date: Thu Oct 13 14:33:55 2016 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 Review-Url: https://codereview.chromium.org/2411333004 Cr-Commit-Position: refs/heads/master@{#425025} [modify] https://crrev.com/810f2c7d12a60a8ce27bc4e5ed8561d1d1c749bb/components/autofill/content/renderer/password_form_conversion_utils.cc [modify] https://crrev.com/810f2c7d12a60a8ce27bc4e5ed8561d1d1c749bb/components/autofill/core/browser/form_structure.cc [modify] https://crrev.com/810f2c7d12a60a8ce27bc4e5ed8561d1d1c749bb/components/autofill/core/common/autofill_util.cc [modify] https://crrev.com/810f2c7d12a60a8ce27bc4e5ed8561d1d1c749bb/components/autofill/core/common/autofill_util.h [modify] https://crrev.com/810f2c7d12a60a8ce27bc4e5ed8561d1d1c749bb/components/autofill/core/common/autofill_util_unittest.cc
,
Oct 13 2016
|
||
►
Sign in to add a comment |
||
Comment 1 by jdoerrie@chromium.org
, Oct 13 2016Status: Started (was: Available)