Autofill saves duplicate profiles and credit card info in preferences

Autofill saves duplicate profiles and credit card info in preferences

Adds logic to filter out duplicate profiles and credit cards when the Autofill preferences dialog commits changes.

BUG=67395
TEST=Manual according to bug.

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

[Ilya Sherman, 2010-12-20 20:57:04]

Two behavioral oddities I noticed on Mac:

1) If I add profile A, then add profile B, then try to add profile A again,
profile B ends up selected.  I would expect the original copy of profile A to be
selected.

2) If I add the same profile twice, the preferences window flickers as a
temporary item is first added, then removed.  I would prefer if we could avoid
adding the profile to the view until we're sure it's not a duplicate.

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

http://codereview.chromium.org/6082001/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager.cc:114: };
Wow, that's a lot of new functors.  I think we just need one -- something like
this:

template<typename T>
class FormGroupMatchesByCompareFunctor {
 public:
  explicit FormGroupMatchesByCompareFunctor(const T& form_group)
      : form_group_(form_group) {
  }

  bool operator()(const T* form_group) {
    return form_group->Compare(form_group_) == 0;
  }

 private:
  const T& form_group_;
};

http://codereview.chromium.org/6082001/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager.cc:150: }
Likewise, here we just need one function -- something like this:

template<typename T, typename C>
bool FindByContents(const C& container, const T& form_group) {
  return std::find_if(
      container.begin(),
      container.end(),
      FormGroupMatchesByCompareFunctor<T>(form_group)) != container.end();
}

http://codereview.chromium.org/6082001/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager.cc:396:
!FindByContents<AutoFillProfile*>(web_profiles_, *iter))
nit: With the above changes, you should be able to call
FindByContents(web_profiles_, *iter) without specifying the template type (and
below too)

[dhollowa, 2010-12-20 23:16:44]

>1) If I add profile A, then add profile B, then try to add profile A again,
profile B ends up selected.  I would expect the original copy of profile A to be
selected.

See below.

> 2) If I add the same profile twice, the preferences window flickers as a
temporary item is first added, then removed.  I would prefer if we could avoid
adding the profile to the view until we're sure it's not a duplicate.

Yes, I've factored the duplicate detection stuff out into the new autofill-inl.h
file and am keeping duplicates out at the view level as well now.

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

http://codereview.chromium.org/6082001/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager.cc:114: };
Yes, better.  But to do this I needed to add a second "operator()" for the const
ref case.

On 2010/12/20 20:57:04, Ilya Sherman wrote:
> Wow, that's a lot of new functors.  I think we just need one -- something like
> this:
> 
> template<typename T>
> class FormGroupMatchesByCompareFunctor {
>  public:
>   explicit FormGroupMatchesByCompareFunctor(const T& form_group)
>       : form_group_(form_group) {
>   }
> 
>   bool operator()(const T* form_group) {
>     return form_group->Compare(form_group_) == 0;
>   }
> 
>  private:
>   const T& form_group_;
> };

http://codereview.chromium.org/6082001/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager.cc:150: }
On 2010/12/20 20:57:04, Ilya Sherman wrote:
> Likewise, here we just need one function -- something like this:
> 
> template<typename T, typename C>
> bool FindByContents(const C& container, const T& form_group) {
>   return std::find_if(
>       container.begin(),
>       container.end(),
>       FormGroupMatchesByCompareFunctor<T>(form_group)) != container.end();
> }

Done.

http://codereview.chromium.org/6082001/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager.cc:396:
!FindByContents<AutoFillProfile*>(web_profiles_, *iter))
On 2010/12/20 20:57:04, Ilya Sherman wrote:
> nit: With the above changes, you should be able to call
> FindByContents(web_profiles_, *iter) without specifying the template type (and
> below too)

Done.

[Ilya Sherman, 2010-12-20 23:41:31]

Looks good for profiles, hooray!

I think you forgot about the credit cards view though ;)

http://codereview.chromium.org/6082001/diff/5001/chrome/browser/autofill/pers...
File chrome/browser/autofill/personal_data_manager.cc (right):

http://codereview.chromium.org/6082001/diff/5001/chrome/browser/autofill/pers...
chrome/browser/autofill/personal_data_manager.cc:59: };
nit: Can we similarly merge these two |FormGroupMatchesByGUIDFunctor| classes
into one with an overloaded parens-operator?

[dhollowa, 2010-12-20 23:55:33]

Bwah.  Yes, credit cards too now.  Thx.

http://codereview.chromium.org/6082001/diff/5001/chrome/browser/autofill/pers...
File chrome/browser/autofill/personal_data_manager.cc (right):

http://codereview.chromium.org/6082001/diff/5001/chrome/browser/autofill/pers...
chrome/browser/autofill/personal_data_manager.cc:59: };
On 2010/12/20 23:41:32, Ilya Sherman wrote:
> nit: Can we similarly merge these two |FormGroupMatchesByGUIDFunctor| classes
> into one with an overloaded parens-operator?

Done.

[Ilya Sherman, 2010-12-21 00:04:48]

LGTM, thanks