[Autofill] Show a warning if the form disables Autofill. (issue 11364066)

[ishe...@chromium.org]

Reviewers: Dan Beam,

Description:
[Autofill] Show a warning if the form disables Autofill.

BUG=159291
TEST=(see bug)


Please review this at https://chromiumcodereview.appspot.com/11364066/

SVN Base: svn://svn.chromium.org/chrome/trunk/src

Affected files:
M chrome/renderer/autofill/autofill_agent.cc


Index: chrome/renderer/autofill/autofill_agent.cc
diff --git a/chrome/renderer/autofill/autofill_agent.cc
b/chrome/renderer/autofill/autofill_agent.cc
index
8af890ab029926c37efce9cfb7d83c3699cd94ca..6df42395213d670ed1d457c22cca8acb905d570e
100644
--- a/chrome/renderer/autofill/autofill_agent.cc
+++ b/chrome/renderer/autofill/autofill_agent.cc
@@ -605,8 +605,7 @@ void AutofillAgent::ShowSuggestions(const
WebInputElement& element,
// specifically for this field, we never want to show a warning.
Otherwise,
// we might interfere with custom popups (e.g. search suggestions) used
by
// the website. Also, if the field has no name, then we won't have
values.
- const WebFormElement form = element.form();
- if ((!element.autoComplete() && (form.isNull() || form.autoComplete())) |
|
+ if (element.getAttribute("autocomplete") == ASCIIToUTF16("off") ||
element.nameForAutofill().isEmpty()) {
CombineDataListEntriesAndShow(element, std::vector<string16>(),
std::vector<string16>(),
@@ -624,10 +623,19 @@ void AutofillAgent::QueryAutofillSuggestions(const
WebInputElement& element,
autofill_query_id_ = query_counter++;
display_warning_if_disabled_ = display_warning_if_disabled;

+ // If autocomplete is disabled at the form level, we want to see if there
+ // would have been any suggestions were it enabled, so that we can show a
+ // warning. Otherwise, we want to ignore fields that disable
autocomplete, so
+ // that the suggestions list does not include suggestions for these form
+ // fields -- see comment 1 on http://crbug.com/69914
+ RequirementsMask requirements = REQUIRE_AUTOCOMPLETE;
+ const WebFormElement form_element = element.form();
+ if (!form_element.isNull() && !form_element.autoComplete())
+ requirements = REQUIRE_NONE;
+
FormData form;
FormFieldData field;
- if (!FindFormAndFieldForInputElement(element, &form, &field,
- REQUIRE_AUTOCOMPLETE)) {
+ if (!FindFormAndFieldForInputElement(element, &form, &field,
requirements)) {
// If we didn't find the cached form, at least let autocomplete have a
shot
// at providing suggestions.
WebFormControlElementToFormField(element, EXTRACT_VALUE, &field);

[db...@chromium.org]

https://chromiumcodereview.appspot.com/11364066/diff/3001/chrome/renderer/autofill/autofill_agent.cc
File chrome/renderer/autofill/autofill_agent.cc (right):

https://chromiumcodereview.appspot.com/11364066/diff/3001/chrome/renderer/autofill/autofill_agent.cc#newcode611
chrome/renderer/autofill/autofill_agent.cc:611: if

(element.getAttribute("autocomplete") == ASCIIToUTF16("off") ||

does this need to be lowercased?

https://chromiumcodereview.appspot.com/11364066/

[ishe...@chromium.org]

https://chromiumcodereview.appspot.com/11364066/diff/3001/chrome/renderer/autofill/autofill_agent.cc
File chrome/renderer/autofill/autofill_agent.cc (right):

https://chromiumcodereview.appspot.com/11364066/diff/3001/chrome/renderer/autofill/autofill_agent.cc#newcode611
chrome/renderer/autofill/autofill_agent.cc:611: if
(element.getAttribute("autocomplete") == ASCIIToUTF16("off") ||

On 2012/11/03 03:56:21, Dan Beam wrote:
> does this need to be lowercased?

Good call -- done.

https://chromiumcodereview.appspot.com/11364066/

[db...@chromium.org]

lgtm w/nits & comment


https://chromiumcodereview.appspot.com/11364066/diff/2003/chrome/renderer/autofill/autofill_agent.cc
File chrome/renderer/autofill/autofill_agent.cc (right):

https://chromiumcodereview.appspot.com/11364066/diff/2003/chrome/renderer/autofill/autofill_agent.cc#newcode612
chrome/renderer/autofill/autofill_agent.cc:612:
StringToLowerASCII(string16(element.getAttribute("autocomplete")));
I'm confused by these conversions - are you doing the comparison in
UTF16 or ASCII? Is this the most efficient way? (it obviously doesn't
matter a whole bunch but it's kind of confusing to me). If "off" is the
only magical attribute value that triggers this, can't you just use
std::string / const char[] ?

https://chromiumcodereview.appspot.com/11364066/diff/2003/chrome/renderer/autofill/autofill_renderer_browsertest.cc
File chrome/renderer/autofill/autofill_renderer_browsertest.cc (right):

https://chromiumcodereview.appspot.com/11364066/diff/2003/chrome/renderer/autofill/autofill_renderer_browsertest.cc#newcode177
chrome/renderer/autofill/autofill_renderer_browsertest.cc:177: // Don't
want any delay for form state sync changes. This will still post a
nit: shouldn't this be a double space?

https://chromiumcodereview.appspot.com/11364066/

[db...@chromium.org]

https://chromiumcodereview.appspot.com/11364066/diff/2003/chrome/renderer/autofill/autofill_renderer_browsertest.cc
File chrome/renderer/autofill/autofill_renderer_browsertest.cc (right):

https://chromiumcodereview.appspot.com/11364066/diff/2003/chrome/renderer/autofill/autofill_renderer_browsertest.cc#newcode176
chrome/renderer/autofill/autofill_renderer_browsertest.cc:176:
TEST_F(ChromeRenderViewTest, ShowAutofillWarning) {
btw, I don't have a ton of experience with these kind of tests - I can
look at them to make sure you're testing the right thing (which you seem
to be, though you may want a test that has a form without
[autocomplete='off'] and verify a warning *isn't* sent from that form as
well). however, I'm not a very good reviewer to look at *how* you're
testing these things (i.e. if your test will be flakey, etc.).

https://chromiumcodereview.appspot.com/11364066/

[ishe...@chromium.org]

https://chromiumcodereview.appspot.com/11364066/diff/2003/chrome/renderer/autofill/autofill_agent.cc
File chrome/renderer/autofill/autofill_agent.cc (right):

https://chromiumcodereview.appspot.com/11364066/diff/2003/chrome/renderer/autofill/autofill_agent.cc#newcode612
chrome/renderer/autofill/autofill_agent.cc:612:
StringToLowerASCII(string16(element.getAttribute("autocomplete")));

On 2012/11/06 02:22:17, Dan Beam wrote:
> I'm confused by these conversions - are you doing the comparison in
UTF16 or
> ASCII? Is this the most efficient way? (it obviously doesn't matter
a whole
> bunch but it's kind of confusing to me). If "off" is the only magical
attribute
> value that triggers this, can't you just use std::string / const
char[] ?

The WebKit data is utf-16 encoded, but uses a WebKit wrapper type rather
than Chrome's string16. Converting to lowercase is generally only
supported for ASCII characters, as it's pretty friggin' complicated to
get right more generally; and in the end we're comparing to what is an
ASCII string. I could add an extra wrapper of UTF16ToUTF8 for the first
string and drop the ASCIIToUTF16 from the literal string; but otherwise,
I'm not sure how to reduce the number of string conversions needed here.

https://chromiumcodereview.appspot.com/11364066/diff/2003/chrome/renderer/autofill/autofill_renderer_browsertest.cc
File chrome/renderer/autofill/autofill_renderer_browsertest.cc (right):

https://chromiumcodereview.appspot.com/11364066/diff/2003/chrome/renderer/autofill/autofill_renderer_browsertest.cc#newcode176
chrome/renderer/autofill/autofill_renderer_browsertest.cc:176:
TEST_F(ChromeRenderViewTest, ShowAutofillWarning) {

On 2012/11/06 02:24:11, Dan Beam wrote:
> btw, I don't have a ton of experience with these kind of tests - I can
look at
> them to make sure you're testing the right thing (which you seem to
be, though
> you may want a test that has a form without [autocomplete='off'] and
verify a
> warning *isn't* sent from that form as well). however, I'm not a very
good
> reviewer to look at *how* you're testing these things (i.e. if your
test will be
> flakey, etc.).

Yeah, I haven't figured out how to write great tests for this sort of
thing. I'm hoping that this test will offer at least *some* coverage,
which is better than none.

https://chromiumcodereview.appspot.com/11364066/diff/2003/chrome/renderer/autofill/autofill_renderer_browsertest.cc#newcode177
chrome/renderer/autofill/autofill_renderer_browsertest.cc:177: // Don't
want any delay for form state sync changes. This will still post a

On 2012/11/06 02:22:17, Dan Beam wrote:
> nit: shouldn't this be a double space?

Done.

https://chromiumcodereview.appspot.com/11364066/

[commi...@chromium.org]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ishe...@chromium.org/11364066/9001


https://chromiumcodereview.appspot.com/11364066/

[est...@chromium.org]

https://chromiumcodereview.appspot.com/11364066/diff/2003/chrome/renderer/autofill/autofill_agent.cc
File chrome/renderer/autofill/autofill_agent.cc (right):

https://chromiumcodereview.appspot.com/11364066/diff/2003/chrome/renderer/autofill/autofill_agent.cc#newcode612
chrome/renderer/autofill/autofill_agent.cc:612:
StringToLowerASCII(string16(element.getAttribute("autocomplete")));

On 2012/11/06 03:45:08, Ilya Sherman wrote:
> On 2012/11/06 02:22:17, Dan Beam wrote:
> > I'm confused by these conversions - are you doing the comparison in
UTF16 or
> > ASCII? Is this the most efficient way? (it obviously doesn't
matter a whole
> > bunch but it's kind of confusing to me). If "off" is the only
magical
> attribute
> > value that triggers this, can't you just use std::string / const
char[] ?

> The WebKit data is utf-16 encoded, but uses a WebKit wrapper type
rather than
> Chrome's string16. Converting to lowercase is generally only
supported for
> ASCII characters, as it's pretty friggin' complicated to get right
more
> generally; and in the end we're comparing to what is an ASCII string.
I could
> add an extra wrapper of UTF16ToUTF8 for the first string and drop the
> ASCIIToUTF16 from the literal string; but otherwise, I'm not sure how
to reduce
> the number of string conversions needed here.

LowerCaseEqualsASCII?

https://chromiumcodereview.appspot.com/11364066/

[ishe...@chromium.org]

https://chromiumcodereview.appspot.com/11364066/diff/2003/chrome/renderer/autofill/autofill_agent.cc
File chrome/renderer/autofill/autofill_agent.cc (right):

https://chromiumcodereview.appspot.com/11364066/diff/2003/chrome/renderer/autofill/autofill_agent.cc#newcode612
chrome/renderer/autofill/autofill_agent.cc:612:
StringToLowerASCII(string16(element.getAttribute("autocomplete")));

Ah, good call. Done.

https://chromiumcodereview.appspot.com/11364066/

[commi...@chromium.org]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/ishe...@chromium.org/11364066/5003


https://chromiumcodereview.appspot.com/11364066/

[commi...@chromium.org]

Change committed as 166164

https://chromiumcodereview.appspot.com/11364066/