New issue
Advanced search Search tips

Issue 655157 link

Starred by 0 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

HasAutocompleteAttributeValue should handle multi-valued strings

Project Member Reported by vabr@chromium.org, Oct 12 2016

Issue description

bool 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.
 
Owner: jdoerrie@chromium.org
Status: Started (was: Available)
I started working on this, but encountered a couple of questions.
Looking at the ParseFieldTypesFromAutocompleteAttributes code most relevant pieces are canonicalizing the attribute string (collapsed whitespace and lowercase) and splitting it into tokens. This can be achieved with ~5 lines of code, so I am not sure whether we should pull out this short logic into an external function. Another complication is that ParseFieldTypesFromAutocompleteAttributes deals with std::strings, while HasAutocompleteAttributeValue deals with base::string16s (blink::WebString to be exact). So the called methods are different as of now.

In addition I am not quite sure how to set the trim_sequences_with_line_breaks argument of base::CollapseWhitespace. Do we expect line breaks in an autocomplete argument? I assume yes, correct?

Comment 2 by vabr@chromium.org, 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.*'
Project Member

Comment 3 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)

Sign in to add a comment