Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(14)

Issue 7013016: Autofill should reject aggregated profiles where email address is found in non-email fields (Closed)

Created:
9 years, 7 months ago by dhollowa
Modified:
9 years, 7 months ago
Reviewers:
Ilya Sherman
CC:
chromium-reviews, GeorgeY
Visibility:
Public.

Description

Autofill should reject aggregated profiles where email address is found in non-email fields Adds code to reject import of profiles and credit cards that have email address data in non-email fields. BUG=77113 TEST=AutofillMergeTest.DataDrivenMergeProfiles with email.in and email.out. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=85489

Patch Set 1 #

Patch Set 2 : Adds test files. #

Total comments: 2

Patch Set 3 : Ilya's comments. #

Patch Set 4 : Revert last tweak. It breaks migration code. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+204 lines, -27 lines) Patch
M chrome/browser/autofill/personal_data_manager.h View 1 chunk +0 lines, -4 lines 0 comments Download
M chrome/browser/autofill/personal_data_manager.cc View 3 3 chunks +37 lines, -24 lines 0 comments Download
A chrome/test/data/autofill/merge/input/email.in View 1 1 chunk +168 lines, -0 lines 0 comments Download
A chrome/test/data/autofill/merge/output/email.out View 1 0 chunks +-1 lines, --1 lines 0 comments Download

Messages

Total messages: 7 (0 generated)
dhollowa
9 years, 7 months ago (2011-05-12 01:27:12 UTC) #1
Ilya Sherman
http://codereview.chromium.org/7013016/diff/2001/chrome/browser/autofill/personal_data_manager.cc File chrome/browser/autofill/personal_data_manager.cc (right): http://codereview.chromium.org/7013016/diff/2001/chrome/browser/autofill/personal_data_manager.cc#newcode123 chrome/browser/autofill/personal_data_manager.cc:123: return false; It's a little confusing to me that ...
9 years, 7 months ago (2011-05-12 21:40:03 UTC) #2
dhollowa
http://codereview.chromium.org/7013016/diff/2001/chrome/browser/autofill/personal_data_manager.cc File chrome/browser/autofill/personal_data_manager.cc (right): http://codereview.chromium.org/7013016/diff/2001/chrome/browser/autofill/personal_data_manager.cc#newcode123 chrome/browser/autofill/personal_data_manager.cc:123: return false; On 2011/05/12 21:40:03, Ilya Sherman wrote: > ...
9 years, 7 months ago (2011-05-12 23:24:11 UTC) #3
Ilya Sherman
LGTM, thanks :)
9 years, 7 months ago (2011-05-12 23:53:38 UTC) #4
dhollowa
On 2011/05/12 23:53:38, Ilya Sherman wrote: > LGTM, thanks :) I had to restore the ...
9 years, 7 months ago (2011-05-13 02:12:34 UTC) #5
Ilya Sherman
On 2011/05/13 02:12:34, dhollowa wrote: > On 2011/05/12 23:53:38, Ilya Sherman wrote: > > LGTM, ...
9 years, 7 months ago (2011-05-13 06:33:01 UTC) #6
dhollowa
9 years, 7 months ago (2011-05-13 17:26:15 UTC) #7
On 2011/05/13 06:33:01, Ilya Sherman wrote:
> On 2011/05/13 02:12:34, dhollowa wrote:
> > On 2011/05/12 23:53:38, Ilya Sherman wrote:
> > > LGTM, thanks :)
> > 
> > I had to restore the original.  Forgot that the migration code needs the
email
> > check in |IsValidLearnableProfile|.
> 
> That suggests that the other check should also go in |IsValidLearnableProfile|
> -- no?

I'm reluctant to do that because then |IsValidLearnableProfile| needs to
replicate the traversal of the various fields.  This seems like overkill an
overly complicated.  Especially since the migration code has "set sail".  I
prefer it as it stands.

Powered by Google App Engine
This is Rietveld 408576698