Autofill should filter malformed emails addresses when form is submitted

Autofill should filter malformed emails addresses when form is submitted

BUG=71738
TEST=PersonalDataManagerTest.ImportFormDataBadEmail

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

[Ilya Sherman, 2011-02-03 04:24:20]

LGTM with nits addressed

http://codereview.chromium.org/6368067/diff/1/chrome/browser/autofill/persona...
File chrome/browser/autofill/personal_data_manager.cc (right):

http://codereview.chromium.org/6368067/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager.cc:22: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebRegularExpression.h"
nit: 80-col?  I'm not exactly sure what our style guide says for this...

http://codereview.chromium.org/6368067/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager.cc:76: // subject, but it does
reject obvious non-email addresses.
nit: I find raw regexes hard to read; it would be nice to mention in the comment
what this regex is looking for.

http://codereview.chromium.org/6368067/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager.cc:78:
ASCIIToUTF16("^[^@]+@.+\\.[a-z]{2,4}$");
nit: Looks like ".museum" and ".travel" are valid TLDs:
http://en.wikipedia.org/wiki/List_of_Internet_top-level_domains

http://codereview.chromium.org/6368067/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager.cc:82:
WebKit::WebString(StringToLowerASCII(value))) != -1;
nit: I would skip the local variable and just "return re.match(...", but I don't
feel at all strongly about it.

http://codereview.chromium.org/6368067/diff/1/chrome/browser/autofill/persona...
File chrome/browser/autofill/personal_data_manager_unittest.cc (right):

http://codereview.chromium.org/6368067/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager_unittest.cc:546:
TEST_F(PersonalDataManagerTest, ImportFormDataBadEmail) {
nit: This test needs to updated for your other CL, right?  (It needs to include
address data in order for the profile to be saved.)

[dhollowa, 2011-02-03 16:23:02]

http://codereview.chromium.org/6368067/diff/1/chrome/browser/autofill/persona...
File chrome/browser/autofill/personal_data_manager.cc (right):

http://codereview.chromium.org/6368067/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager.cc:22: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebRegularExpression.h"
On 2011/02/03 04:24:20, Ilya Sherman wrote:
> nit: 80-col?  I'm not exactly sure what our style guide says for this...

Nothing to be done.  Can't be split.

http://codereview.chromium.org/6368067/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager.cc:76: // subject, but it does
reject obvious non-email addresses.
On 2011/02/03 04:24:20, Ilya Sherman wrote:
> nit: I find raw regexes hard to read; it would be nice to mention in the
comment
> what this regex is looking for.

I can't think of a comment that does any better than either saying it's an email
address, or look at the regex.  Leaving as is.

http://codereview.chromium.org/6368067/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager.cc:78:
ASCIIToUTF16("^[^@]+@.+\\.[a-z]{2,4}$");
On 2011/02/03 04:24:20, Ilya Sherman wrote:
> nit: Looks like ".museum" and ".travel" are valid TLDs:
> http://en.wikipedia.org/wiki/List_of_Internet_top-level_domains

Done.

http://codereview.chromium.org/6368067/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager.cc:82:
WebKit::WebString(StringToLowerASCII(value))) != -1;
On 2011/02/03 04:24:20, Ilya Sherman wrote:
> nit: I would skip the local variable and just "return re.match(...", but I
don't
> feel at all strongly about it.

Done.

http://codereview.chromium.org/6368067/diff/1/chrome/browser/autofill/persona...
File chrome/browser/autofill/personal_data_manager_unittest.cc (right):

http://codereview.chromium.org/6368067/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager_unittest.cc:546:
TEST_F(PersonalDataManagerTest, ImportFormDataBadEmail) {
On 2011/02/03 04:24:20, Ilya Sherman wrote:
> nit: This test needs to updated for your other CL, right?  (It needs to
include
> address data in order for the profile to be saved.)

Yes, merged.