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

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

BUG=159291
TEST=(see bug)


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=166164

[Dan Beam, 2012-11-03 03:56:20]

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

https://chromiumcodereview.appspot.com/11364066/diff/3001/chrome/renderer/aut...
chrome/renderer/autofill/autofill_agent.cc:611: if
(element.getAttribute("autocomplete") == ASCIIToUTF16("off") ||
does this need to be lowercased?

[Ilya Sherman, 2012-11-06 02:14:43]

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

https://chromiumcodereview.appspot.com/11364066/diff/3001/chrome/renderer/aut...
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.

[Dan Beam, 2012-11-06 02:22:17]

lgtm w/nits & comment

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

https://chromiumcodereview.appspot.com/11364066/diff/2003/chrome/renderer/aut...
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/aut...
File chrome/renderer/autofill/autofill_renderer_browsertest.cc (right):

https://chromiumcodereview.appspot.com/11364066/diff/2003/chrome/renderer/aut...
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?

[Dan Beam, 2012-11-06 02:24:11]

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

https://chromiumcodereview.appspot.com/11364066/diff/2003/chrome/renderer/aut...
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.).

[Ilya Sherman, 2012-11-06 03:45:08]

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

https://chromiumcodereview.appspot.com/11364066/diff/2003/chrome/renderer/aut...
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/aut...
File chrome/renderer/autofill/autofill_renderer_browsertest.cc (right):

https://chromiumcodereview.appspot.com/11364066/diff/2003/chrome/renderer/aut...
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/aut...
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.

[commit-bot: I haz the power, 2012-11-06 03:46:27]

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

[Evan Stade, 2012-11-06 04:16:27]

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

https://chromiumcodereview.appspot.com/11364066/diff/2003/chrome/renderer/aut...
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?

[Ilya Sherman, 2012-11-06 04:28:42]

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

https://chromiumcodereview.appspot.com/11364066/diff/2003/chrome/renderer/aut...
chrome/renderer/autofill/autofill_agent.cc:612:
StringToLowerASCII(string16(element.getAttribute("autocomplete")));
On 2012/11/06 04:16:27, Evan Stade wrote:
> 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?

Ah, good call.  Done.

[commit-bot: I haz the power, 2012-11-06 04:28:52]

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

[commit-bot: I haz the power, 2012-11-06 07:03:46]

Change committed as 166164