Add the ability to save and remove AutoFill profiles from the AutoFillDialog....

Add the ability to save and remove AutoFill profiles from the AutoFillDialog.

BUG=18201
TEST=PersonalDataManagerTest.SetProfiles

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

[sky, 2010-01-22 20:59:35]

http://codereview.chromium.org/545175/diff/1/8
File chrome/browser/autofill/autofill_dialog.h (right):

http://codereview.chromium.org/545175/diff/1/8#newcode21
chrome/browser/autofill/autofill_dialog.h:21: std::vector<AutoFillProfile>*
delete_profiles,
delete_profiles -> deleted_profiles

And mention that things can be null, unless that's temporary.

http://codereview.chromium.org/545175/diff/1/8#newcode34
chrome/browser/autofill/autofill_dialog.h:34: void
ShowAutoFillDialog(AutoFillDialogObserver* observer,
You should mention who owns the contents of the vectors here and their lifetime.

http://codereview.chromium.org/545175/diff/1/9
File chrome/browser/autofill/autofill_dialog_gtk.cc (right):

http://codereview.chromium.org/545175/diff/1/9#newcode89
chrome/browser/autofill/autofill_dialog_gtk.cc:89:
g_object_set_data(G_OBJECT(widget), kLabelEntry, entry);
Should kLabelEntry be kButtonData to match your function names?

http://codereview.chromium.org/545175/diff/1/9#newcode501
chrome/browser/autofill/autofill_dialog_gtk.cc:501:
std::vector<AddressWidgets>::iterator iter;
I much prefer when you have the iterator defined in the for loop like on 490.
That way it's clear where the iterator is used.

http://codereview.chromium.org/545175/diff/1/9#newcode512
chrome/browser/autofill/autofill_dialog_gtk.cc:512: GtkWidget* widget =
GTK_WIDGET(button);
Can you use gtk_widget_get_ancestor here?

http://codereview.chromium.org/545175/diff/1/10
File chrome/browser/autofill/credit_card.h (right):

http://codereview.chromium.org/545175/diff/1/10#newcode19
chrome/browser/autofill/credit_card.h:19: void operator=(const CreditCard&
card);
Move the implementation to match this position.

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

http://codereview.chromium.org/545175/diff/1/5#newcode32
chrome/browser/autofill/personal_data_manager.cc:32: 
result can be null if there is an error initializing the db and other error
situations.

http://codereview.chromium.org/545175/diff/1/5#newcode40
chrome/browser/autofill/personal_data_manager.cc:40: }
Doesn't this need to update the max id some how?

http://codereview.chromium.org/545175/diff/1/5#newcode129
chrome/browser/autofill/personal_data_manager.cc:129: WebDataService* wds =
profile_->GetWebDataService(Profile::EXPLICIT_ACCESS);
Make this this is non-null.

http://codereview.chromium.org/545175/diff/1/5#newcode135
chrome/browser/autofill/personal_data_manager.cc:135: if (iter->unique_id() ==
0) {
Don't you also need to update profiles_?

http://codereview.chromium.org/545175/diff/1/5#newcode136
chrome/browser/autofill/personal_data_manager.cc:136:
iter->set_unique_id(CreateNextUniqueID());
If you haven't initialized by now, your id is bogus. Are you enforcing that
you've loaded by the time the dialog is shown?

http://codereview.chromium.org/545175/diff/1/5#newcode145
chrome/browser/autofill/personal_data_manager.cc:145:
std::vector<AutoFillProfile>* profiles) {
Make this take a const& as it doesn't need to mutate the profiles.

http://codereview.chromium.org/545175/diff/1/5#newcode149
chrome/browser/autofill/personal_data_manager.cc:149: WebDataService* wds =
profile_->GetWebDataService(Profile::EXPLICIT_ACCESS);
null check.

http://codereview.chromium.org/545175/diff/1/5#newcode153
chrome/browser/autofill/personal_data_manager.cc:153: for (iter =
profiles->begin(); iter != profiles->end(); ++iter)
move into for loop.

http://codereview.chromium.org/545175/diff/1/5#newcode155
chrome/browser/autofill/personal_data_manager.cc:155: }
Doesn't this need to update profile_?

http://codereview.chromium.org/545175/diff/1/5#newcode179
chrome/browser/autofill/personal_data_manager.cc:179:
ScopedVector<FormGroup>::iterator iter;
move into for loop.

http://codereview.chromium.org/545175/diff/1/5#newcode216
chrome/browser/autofill/personal_data_manager.cc:216: int id = 1;
Just use an ever increasing integer here.

http://codereview.chromium.org/545175/diff/1/5#newcode257
chrome/browser/autofill/personal_data_manager.cc:257: CancelPendingQuery();
If you've already got a load going, why don't you just return?

http://codereview.chromium.org/545175/diff/1/11
File chrome/browser/autofill/personal_data_manager.h (right):

http://codereview.chromium.org/545175/diff/1/11#newcode56
chrome/browser/autofill/personal_data_manager.h:56:
std::vector<AutoFillProfile*>& profiles() { return profiles_.get(); }
You need to mention who owns these and what their lifetime is. And can it be
const&?

http://codereview.chromium.org/545175/diff/1/14
File chrome/browser/webdata/web_data_service.cc (right):

http://codereview.chromium.org/545175/diff/1/14#newcode8
chrome/browser/webdata/web_data_service.cc:8: #include "base/stl_util-inl.h"
Is this needed?

http://codereview.chromium.org/545175/diff/1/14#newcode778
chrome/browser/webdata/web_data_service.cc:778:
db_->GetAutoFillProfiles(&profiles);
If the request gets canceled later on, you leak the profiles.

http://codereview.chromium.org/545175/diff/1/15
File chrome/browser/webdata/web_data_service.h (right):

http://codereview.chromium.org/545175/diff/1/15#newcode429
chrome/browser/webdata/web_data_service.h:429: Handle
GetAutoFillProfiles(WebDataServiceConsumer* consumer);
Mention who owns the profiles in the callback.

http://codereview.chromium.org/545175/diff/1/12
File chrome/browser/webdata/web_database.cc (right):

http://codereview.chromium.org/545175/diff/1/12#newcode1223
chrome/browser/webdata/web_database.cc:1223: printf("Saving profile: %s\n",
UTF16ToASCII(profile.Label()).c_str());
nuke this.

http://codereview.chromium.org/545175/diff/1/12#newcode1296
chrome/browser/webdata/web_database.cc:1296: profiles->clear();
If you're going to add this, it should be before the return on 1293.

http://codereview.chromium.org/545175/diff/1/12#newcode1299
chrome/browser/webdata/web_database.cc:1299: profiles->push_back(profile);
nit: combine this into a single line:
profiles->push_back(AutoFillProfileFromStatement(s));

http://codereview.chromium.org/545175/diff/1/13
File chrome/browser/webdata/web_database.h (right):

http://codereview.chromium.org/545175/diff/1/13#newcode214
chrome/browser/webdata/web_database.h:214: // Retrieves all profiles in the
database.
document that caller owns returned pointers.

[James Hawkins, 2010-01-22 21:51:13]

http://codereview.chromium.org/545175/diff/1/8
File chrome/browser/autofill/autofill_dialog.h (right):

http://codereview.chromium.org/545175/diff/1/8#newcode21
chrome/browser/autofill/autofill_dialog.h:21: std::vector<AutoFillProfile>*
delete_profiles,
On 2010/01/22 20:59:35, sky wrote:
> delete_profiles -> deleted_profiles
> 
> And mention that things can be null, unless that's temporary.

Done.

http://codereview.chromium.org/545175/diff/1/8#newcode34
chrome/browser/autofill/autofill_dialog.h:34: void
ShowAutoFillDialog(AutoFillDialogObserver* observer,
On 2010/01/22 20:59:35, sky wrote:
> You should mention who owns the contents of the vectors here and their
lifetime.

Done.

http://codereview.chromium.org/545175/diff/1/9
File chrome/browser/autofill/autofill_dialog_gtk.cc (right):

http://codereview.chromium.org/545175/diff/1/9#newcode89
chrome/browser/autofill/autofill_dialog_gtk.cc:89:
g_object_set_data(G_OBJECT(widget), kLabelEntry, entry);
On 2010/01/22 20:59:35, sky wrote:
> Should kLabelEntry be kButtonData to match your function names?

I renamed it kButtonDataKey, which signifies that it's a key to the button data.

http://codereview.chromium.org/545175/diff/1/9#newcode501
chrome/browser/autofill/autofill_dialog_gtk.cc:501:
std::vector<AddressWidgets>::iterator iter;
On 2010/01/22 20:59:35, sky wrote:
> I much prefer when you have the iterator defined in the for loop like on 490.
> That way it's clear where the iterator is used.

Ok, I've modified it, but I'm not sure of the new spacing (which is why I left
it out of the for loop originally).

http://codereview.chromium.org/545175/diff/1/9#newcode512
chrome/browser/autofill/autofill_dialog_gtk.cc:512: GtkWidget* widget =
GTK_WIDGET(button);
On 2010/01/22 20:59:35, sky wrote:
> Can you use gtk_widget_get_ancestor here?

Yes, thanks!  You learn something new every day.

http://codereview.chromium.org/545175/diff/1/10
File chrome/browser/autofill/credit_card.h (right):

http://codereview.chromium.org/545175/diff/1/10#newcode19
chrome/browser/autofill/credit_card.h:19: void operator=(const CreditCard&
card);
On 2010/01/22 20:59:35, sky wrote:
> Move the implementation to match this position.

Actually I don't use this yet, so I've removed the change.

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

http://codereview.chromium.org/545175/diff/1/5#newcode32
chrome/browser/autofill/personal_data_manager.cc:32: 
On 2010/01/22 20:59:35, sky wrote:
> result can be null if there is an error initializing the db and other error
> situations.

Ok, I've modified this so that we return if result is NULL.

http://codereview.chromium.org/545175/diff/1/5#newcode40
chrome/browser/autofill/personal_data_manager.cc:40: }
On 2010/01/22 20:59:35, sky wrote:
> Doesn't this need to update the max id some how?

Which max ID are you referring to?

http://codereview.chromium.org/545175/diff/1/5#newcode129
chrome/browser/autofill/personal_data_manager.cc:129: WebDataService* wds =
profile_->GetWebDataService(Profile::EXPLICIT_ACCESS);
On 2010/01/22 20:59:35, sky wrote:
> Make this this is non-null.

I've added a DCHECK for wds.  Is it possible that GetWebDataService can
legitimately return a NULL WebDataService, in which case I'd just return.

http://codereview.chromium.org/545175/diff/1/5#newcode135
chrome/browser/autofill/personal_data_manager.cc:135: if (iter->unique_id() ==
0) {
On 2010/01/22 20:59:35, sky wrote:
> Don't you also need to update profiles_?

I've added a call to LoadProfiles.  So now we're writing into the webdb, and
then reading it back again. Does that seem like too much work?

http://codereview.chromium.org/545175/diff/1/5#newcode136
chrome/browser/autofill/personal_data_manager.cc:136:
iter->set_unique_id(CreateNextUniqueID());
On 2010/01/22 20:59:35, sky wrote:
> If you haven't initialized by now, your id is bogus. Are you enforcing that
> you've loaded by the time the dialog is shown?

Not yet.  In the patch set after the next, I'll add a way for
PersonalDataManager to notify AutoFillManager that the profile data has been
loaded, and then we can show the autofill dialog.

http://codereview.chromium.org/545175/diff/1/5#newcode145
chrome/browser/autofill/personal_data_manager.cc:145:
std::vector<AutoFillProfile>* profiles) {
On 2010/01/22 20:59:35, sky wrote:
> Make this take a const& as it doesn't need to mutate the profiles.

Done.

http://codereview.chromium.org/545175/diff/1/5#newcode149
chrome/browser/autofill/personal_data_manager.cc:149: WebDataService* wds =
profile_->GetWebDataService(Profile::EXPLICIT_ACCESS);
On 2010/01/22 20:59:35, sky wrote:
> null check.

Done.

http://codereview.chromium.org/545175/diff/1/5#newcode153
chrome/browser/autofill/personal_data_manager.cc:153: for (iter =
profiles->begin(); iter != profiles->end(); ++iter)
On 2010/01/22 20:59:35, sky wrote:
> move into for loop.

Done.

http://codereview.chromium.org/545175/diff/1/5#newcode155
chrome/browser/autofill/personal_data_manager.cc:155: }
On 2010/01/22 20:59:35, sky wrote:
> Doesn't this need to update profile_?

I've added a call to LoadProfiles.  See above comment.

http://codereview.chromium.org/545175/diff/1/5#newcode179
chrome/browser/autofill/personal_data_manager.cc:179:
ScopedVector<FormGroup>::iterator iter;
On 2010/01/22 20:59:35, sky wrote:
> move into for loop.

Done.

http://codereview.chromium.org/545175/diff/1/5#newcode216
chrome/browser/autofill/personal_data_manager.cc:216: int id = 1;
On 2010/01/22 20:59:35, sky wrote:
> Just use an ever increasing integer here.

I now have unique_ids_ being loaded properly when we load profiles.  If we just
use an ever-increasing ID, we'll eventually wrap around (profiles can be
removed, leaving holes.)  This method makes sure we have unique IDs for profiles
while still being able to have maxvalue(int) profiles.

http://codereview.chromium.org/545175/diff/1/5#newcode257
chrome/browser/autofill/personal_data_manager.cc:257: CancelPendingQuery();
On 2010/01/22 20:59:35, sky wrote:
> If you've already got a load going, why don't you just return?

Isn't it possible that we have this flow:

Load()--------------|
  Save()---|
             Load()---------|

So we'd be loading stale data if we don't cancel the old request?

http://codereview.chromium.org/545175/diff/1/11
File chrome/browser/autofill/personal_data_manager.h (right):

http://codereview.chromium.org/545175/diff/1/11#newcode56
chrome/browser/autofill/personal_data_manager.h:56:
std::vector<AutoFillProfile*>& profiles() { return profiles_.get(); }
On 2010/01/22 20:59:35, sky wrote:
> You need to mention who owns these and what their lifetime is. And can it be
> const&?

Done.

http://codereview.chromium.org/545175/diff/1/14
File chrome/browser/webdata/web_data_service.cc (right):

http://codereview.chromium.org/545175/diff/1/14#newcode8
chrome/browser/webdata/web_data_service.cc:8: #include "base/stl_util-inl.h"
On 2010/01/22 20:59:35, sky wrote:
> Is this needed?

Not anymore, thanks.

http://codereview.chromium.org/545175/diff/1/14#newcode778
chrome/browser/webdata/web_data_service.cc:778:
db_->GetAutoFillProfiles(&profiles);
On 2010/01/22 20:59:35, sky wrote:
> If the request gets canceled later on, you leak the profiles.

I had a call to STLDeleteContainerPointers here before, which is wrong because
that frees the profiles that consumer owns.  I don't see how we can not leak
returned webdata in every circumstance, as the consumer owns the data.  I looked
at the other examples of this type of call, and they do the same.  Any ideas?

http://codereview.chromium.org/545175/diff/1/15
File chrome/browser/webdata/web_data_service.h (right):

http://codereview.chromium.org/545175/diff/1/15#newcode429
chrome/browser/webdata/web_data_service.h:429: Handle
GetAutoFillProfiles(WebDataServiceConsumer* consumer);
On 2010/01/22 20:59:35, sky wrote:
> Mention who owns the profiles in the callback.

Done.

http://codereview.chromium.org/545175/diff/1/12
File chrome/browser/webdata/web_database.cc (right):

http://codereview.chromium.org/545175/diff/1/12#newcode1223
chrome/browser/webdata/web_database.cc:1223: printf("Saving profile: %s\n",
UTF16ToASCII(profile.Label()).c_str());
On 2010/01/22 20:59:35, sky wrote:
> nuke this.

oops.  Done.

http://codereview.chromium.org/545175/diff/1/12#newcode1296
chrome/browser/webdata/web_database.cc:1296: profiles->clear();
On 2010/01/22 20:59:35, sky wrote:
> If you're going to add this, it should be before the return on 1293.

Done.

http://codereview.chromium.org/545175/diff/1/12#newcode1299
chrome/browser/webdata/web_database.cc:1299: profiles->push_back(profile);
On 2010/01/22 20:59:35, sky wrote:
> nit: combine this into a single line:
> profiles->push_back(AutoFillProfileFromStatement(s));

Done.

http://codereview.chromium.org/545175/diff/1/13
File chrome/browser/webdata/web_database.h (right):

http://codereview.chromium.org/545175/diff/1/13#newcode214
chrome/browser/webdata/web_database.h:214: // Retrieves all profiles in the
database.
On 2010/01/22 20:59:35, sky wrote:
> document that caller owns returned pointers.

Done.

[sky, 2010-01-22 22:37:49]

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

http://codereview.chromium.org/545175/diff/1/5#newcode40
chrome/browser/autofill/personal_data_manager.cc:40: }
On 2010/01/22 21:51:13, James Hawkins wrote:
> On 2010/01/22 20:59:35, sky wrote:
> > Doesn't this need to update the max id some how?
> 
> Which max ID are you referring to?

I was referring to unique_ids_. But you've fixed it now.

http://codereview.chromium.org/545175/diff/1/5#newcode135
chrome/browser/autofill/personal_data_manager.cc:135: if (iter->unique_id() ==
0) {
On 2010/01/22 21:51:13, James Hawkins wrote:
> On 2010/01/22 20:59:35, sky wrote:
> > Don't you also need to update profiles_?
> 
> I've added a call to LoadProfiles.  So now we're writing into the webdb, and
> then reading it back again. Does that seem like too much work?

That means you're out of sync for a window of time. That feels a bit lame. I
think you should update your profiles_ directly and not refetch.

One other thing to check on. What is the order returned from the SELECT *
statement to get all the profiles? If ones with lower ids are always lower it
means the order shown on screen might not match the order displayed next time
through.

http://codereview.chromium.org/545175/diff/1/5#newcode257
chrome/browser/autofill/personal_data_manager.cc:257: CancelPendingQuery();
On 2010/01/22 21:51:13, James Hawkins wrote:
> On 2010/01/22 20:59:35, sky wrote:
> > If you've already got a load going, why don't you just return?
> 
> Isn't it possible that we have this flow:
> 
> Load()--------------|
>   Save()---|
>              Load()---------|
> 
> So we'd be loading stale data if we don't cancel the old request?

Ah yes. I assumed you would only do the loading once.

http://codereview.chromium.org/545175/diff/3001/3005#newcode24
chrome/browser/autofill/personal_data_manager.cc:24: }
Invoke CancelPendingQuery from the destructor.

http://codereview.chromium.org/545175/diff/3001/3010
File chrome/browser/autofill/personal_data_manager.h (right):

http://codereview.chromium.org/545175/diff/3001/3010#newcode122
chrome/browser/autofill/personal_data_manager.h:122: };
DISALLOW_COPY_AND_ASSIGN

[James Hawkins, 2010-01-22 22:49:00]

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

http://codereview.chromium.org/545175/diff/1/5#newcode135
chrome/browser/autofill/personal_data_manager.cc:135: if (iter->unique_id() ==
0) {
On 2010/01/22 22:37:49, sky wrote:
> On 2010/01/22 21:51:13, James Hawkins wrote:
> > On 2010/01/22 20:59:35, sky wrote:
> > > Don't you also need to update profiles_?
> > 
> > I've added a call to LoadProfiles.  So now we're writing into the webdb, and
> > then reading it back again. Does that seem like too much work?
> 
> That means you're out of sync for a window of time. That feels a bit lame. I
> think you should update your profiles_ directly and not refetch.
> 
> One other thing to check on. What is the order returned from the SELECT *
> statement to get all the profiles? If ones with lower ids are always lower it
> means the order shown on screen might not match the order displayed next time
> through.

I've made it so that SaveProfiles clears and re-adds new copies of the profiles.
 RemoveProfiles doesn't need the same treatment, because the removed profiles
are not in the new profile list, and RemoveProfiles is always called in
conjunction with SaveProfiles.

The SELECT * statement has no ordering specified, so the records will be
returned in the order they were added to the table, which is the same order they
see them in the dialog.

http://codereview.chromium.org/545175/diff/1/5#newcode136
chrome/browser/autofill/personal_data_manager.cc:136:
iter->set_unique_id(CreateNextUniqueID());
On 2010/01/22 21:51:13, James Hawkins wrote:
> On 2010/01/22 20:59:35, sky wrote:
> > If you haven't initialized by now, your id is bogus. Are you enforcing that
> > you've loaded by the time the dialog is shown?
> 
> Not yet.  In the patch set after the next, I'll add a way for
> PersonalDataManager to notify AutoFillManager that the profile data has been
> loaded, and then we can show the autofill dialog.

I'm not sure the best way to handle this.  If we show the dialog before the
personal data is loaded, the dialog will have no profiles.  I think waiting
until the personal data is loaded (using a callback) is the best method, because
the delay in most cases should be nil or unnoticeable.  What do you think?

http://codereview.chromium.org/545175/diff/3001/3005#newcode24
chrome/browser/autofill/personal_data_manager.cc:24: }
On 2010/01/22 22:37:50, sky wrote:
> Invoke CancelPendingQuery from the destructor.

Done.

http://codereview.chromium.org/545175/diff/3001/3010
File chrome/browser/autofill/personal_data_manager.h (right):

http://codereview.chromium.org/545175/diff/3001/3010#newcode122
chrome/browser/autofill/personal_data_manager.h:122: };
On 2010/01/22 22:37:50, sky wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

[sky, 2010-01-22 23:21:03]

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

http://codereview.chromium.org/545175/diff/1/5#newcode135
chrome/browser/autofill/personal_data_manager.cc:135: if (iter->unique_id() ==
0) {
On 2010/01/22 22:49:01, James Hawkins wrote:
> On 2010/01/22 22:37:49, sky wrote:
> > On 2010/01/22 21:51:13, James Hawkins wrote:
> > > On 2010/01/22 20:59:35, sky wrote:
> > > > Don't you also need to update profiles_?
> > > 
> > > I've added a call to LoadProfiles.  So now we're writing into the webdb,
and
> > > then reading it back again. Does that seem like too much work?
> > 
> > That means you're out of sync for a window of time. That feels a bit lame. I
> > think you should update your profiles_ directly and not refetch.
> > 
> > One other thing to check on. What is the order returned from the SELECT *
> > statement to get all the profiles? If ones with lower ids are always lower
it
> > means the order shown on screen might not match the order displayed next
time
> > through.
> 
> I've made it so that SaveProfiles clears and re-adds new copies of the
profiles.
>  RemoveProfiles doesn't need the same treatment, because the removed profiles
> are not in the new profile list, and RemoveProfiles is always called in
> conjunction with SaveProfiles.

I'm fine with that, but this subtlety is easy to screw up for others using your
API. You should make this clear by having a single SetProfiles methods. It's a
but more cumbersome for PersonalDataManager to figure out what changed, OTOH
things won't get completely wonky if someone invokes Remove a couple of times.

> The SELECT * statement has no ordering specified, so the records will be
> returned in the order they were added to the table, which is the same order
they
> see them in the dialog.

Excellent!

http://codereview.chromium.org/545175/diff/1/5#newcode136
chrome/browser/autofill/personal_data_manager.cc:136:
iter->set_unique_id(CreateNextUniqueID());
On 2010/01/22 22:49:01, James Hawkins wrote:
> On 2010/01/22 21:51:13, James Hawkins wrote:
> > On 2010/01/22 20:59:35, sky wrote:
> > > If you haven't initialized by now, your id is bogus. Are you enforcing
that
> > > you've loaded by the time the dialog is shown?
> > 
> > Not yet.  In the patch set after the next, I'll add a way for
> > PersonalDataManager to notify AutoFillManager that the profile data has been
> > loaded, and then we can show the autofill dialog.
> 
> I'm not sure the best way to handle this.  If we show the dialog before the
> personal data is loaded, the dialog will have no profiles.  I think waiting
> until the personal data is loaded (using a callback) is the best method,
because
> the delay in most cases should be nil or unnoticeable.  What do you think?

Waiting until loaded sounds fine to me.

[James Hawkins, 2010-01-23 00:32:40]

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

http://codereview.chromium.org/545175/diff/1/5#newcode135
chrome/browser/autofill/personal_data_manager.cc:135: if (iter->unique_id() ==
0) {
On 2010/01/22 23:21:03, sky wrote:
> On 2010/01/22 22:49:01, James Hawkins wrote:
> > On 2010/01/22 22:37:49, sky wrote:
> > > On 2010/01/22 21:51:13, James Hawkins wrote:
> > > > On 2010/01/22 20:59:35, sky wrote:
> > > > > Don't you also need to update profiles_?
> > > > 
> > > > I've added a call to LoadProfiles.  So now we're writing into the webdb,
> and
> > > > then reading it back again. Does that seem like too much work?
> > > 
> > > That means you're out of sync for a window of time. That feels a bit lame.
I
> > > think you should update your profiles_ directly and not refetch.
> > > 
> > > One other thing to check on. What is the order returned from the SELECT *
> > > statement to get all the profiles? If ones with lower ids are always lower
> it
> > > means the order shown on screen might not match the order displayed next
> time
> > > through.
> > 
> > I've made it so that SaveProfiles clears and re-adds new copies of the
> profiles.
> >  RemoveProfiles doesn't need the same treatment, because the removed
profiles
> > are not in the new profile list, and RemoveProfiles is always called in
> > conjunction with SaveProfiles.
> 
> I'm fine with that, but this subtlety is easy to screw up for others using
your
> API. You should make this clear by having a single SetProfiles methods. It's a
> but more cumbersome for PersonalDataManager to figure out what changed, OTOH
> things won't get completely wonky if someone invokes Remove a couple of times.
> 
> > The SELECT * statement has no ordering specified, so the records will be
> > returned in the order they were added to the table, which is the same order
> they
> > see them in the dialog.
> 
> Excellent!

Done.

http://codereview.chromium.org/545175/diff/1/5#newcode136
chrome/browser/autofill/personal_data_manager.cc:136:
iter->set_unique_id(CreateNextUniqueID());
On 2010/01/22 23:21:03, sky wrote:
> On 2010/01/22 22:49:01, James Hawkins wrote:
> > On 2010/01/22 21:51:13, James Hawkins wrote:
> > > On 2010/01/22 20:59:35, sky wrote:
> > > > If you haven't initialized by now, your id is bogus. Are you enforcing
> that
> > > > you've loaded by the time the dialog is shown?
> > > 
> > > Not yet.  In the patch set after the next, I'll add a way for
> > > PersonalDataManager to notify AutoFillManager that the profile data has
been
> > > loaded, and then we can show the autofill dialog.
> > 
> > I'm not sure the best way to handle this.  If we show the dialog before the
> > personal data is loaded, the dialog will have no profiles.  I think waiting
> > until the personal data is loaded (using a callback) is the best method,
> because
> > the delay in most cases should be nil or unnoticeable.  What do you think?
> 
> Waiting until loaded sounds fine to me.

Done.

[James Hawkins, 2010-01-23 05:41:23]

I added PersonalDataManagerTest.SetProfiles.

[sky, 2010-01-23 17:11:01]

http://codereview.chromium.org/545175/diff/2007/8024
File chrome/browser/autofill/personal_data_manager.cc (right):

http://codereview.chromium.org/545175/diff/2007/8024#newcode8
chrome/browser/autofill/personal_data_manager.cc:8: #include <set>
nit: sort these, also the header has the set include, so you likely don't need
it here too.

http://codereview.chromium.org/545175/diff/2007/8024#newcode60
chrome/browser/autofill/personal_data_manager.cc:60: }
Since you only support one observer, you should add a DCHECK that observer_ ==
NULL before setting the new one.

http://codereview.chromium.org/545175/diff/2007/8024#newcode177
chrome/browser/autofill/personal_data_manager.cc:177:
iter->set_unique_id(CreateNextUniqueID());
This seems problematic because you just cleared out the unique_ids_ set on 169.
If consumers always add new profiles to the end, you're likely fine, but that
seems a bit easy to break.

http://codereview.chromium.org/545175/diff/2007/8024#newcode194
chrome/browser/autofill/personal_data_manager.cc:194:
wds->RemoveAutoFillProfile(*profile);
I believe this won't work because you may have assigned ids around 177 that
overlap with this.

I think you should do the following in this method:

remove_ids = unique_ids_;
iterate through new profiles removing the id from remove_ids if the profile is
not new.
iterate through removed_ids_ telling the WDS to remove the profile, and remove
the entry from unique_ids_.
Iterate through new profiles again, adding/updating.

I would also change WDS::ReomveAutoFillProfile to only take the id.

http://codereview.chromium.org/545175/diff/2007/8029
File chrome/browser/autofill/personal_data_manager.h (right):

http://codereview.chromium.org/545175/diff/2007/8029#newcode53
chrome/browser/autofill/personal_data_manager.h:53: void
NotifyObserverWhenLoaded(PersonalDataManager::Observer* observer);
Is this method really needed? Can't the consumer check if loaded and if not
loaded set itself as the observer?

http://codereview.chromium.org/545175/diff/2007/8040
File chrome/browser/autofill/personal_data_manager_unittest.cc (right):

http://codereview.chromium.org/545175/diff/2007/8040#newcode120
chrome/browser/autofill/personal_data_manager_unittest.cc:120:
profile0.set_unique_id(1);
Can you set the id from that in update to be more correct, eg:

profile0.set_unique_id(update[0].unique_id());

http://codereview.chromium.org/545175/diff/2007/8040#newcode133
chrome/browser/autofill/personal_data_manager_unittest.cc:133:
update.push_back(profile0);
You should add a new profile at the same time to exercise more code paths.

http://codereview.chromium.org/545175/diff/2007/8040#newcode136
chrome/browser/autofill/personal_data_manager_unittest.cc:136: const
std::vector<AutoFillProfile*>& results2 = personal_data_->profiles();
It would be good if you also shut down the PDM, reloaded it and verified the
profiles again. This way you're making sure everything makes it to the db and
back again.

[James Hawkins, 2010-01-23 21:48:53]

http://codereview.chromium.org/545175/diff/2007/8024
File chrome/browser/autofill/personal_data_manager.cc (right):

http://codereview.chromium.org/545175/diff/2007/8024#newcode8
chrome/browser/autofill/personal_data_manager.cc:8: #include <set>
On 2010/01/23 17:11:01, sky wrote:
> nit: sort these, also the header has the set include, so you likely don't need
> it here too.

Done.

http://codereview.chromium.org/545175/diff/2007/8024#newcode60
chrome/browser/autofill/personal_data_manager.cc:60: }
On 2010/01/23 17:11:01, sky wrote:
> Since you only support one observer, you should add a DCHECK that observer_ ==
> NULL before setting the new one.

Done.

http://codereview.chromium.org/545175/diff/2007/8024#newcode177
chrome/browser/autofill/personal_data_manager.cc:177:
iter->set_unique_id(CreateNextUniqueID());
On 2010/01/23 17:11:01, sky wrote:
> This seems problematic because you just cleared out the unique_ids_ set on
169.
> If consumers always add new profiles to the end, you're likely fine, but that
> seems a bit easy to break.

Fixed.

http://codereview.chromium.org/545175/diff/2007/8024#newcode194
chrome/browser/autofill/personal_data_manager.cc:194:
wds->RemoveAutoFillProfile(*profile);
On 2010/01/23 17:11:01, sky wrote:
> I believe this won't work because you may have assigned ids around 177 that
> overlap with this.
> 
> I think you should do the following in this method:
> 
> remove_ids = unique_ids_;
> iterate through new profiles removing the id from remove_ids if the profile is
> not new.
> iterate through removed_ids_ telling the WDS to remove the profile, and remove
> the entry from unique_ids_.
> Iterate through new profiles again, adding/updating.
> 
> I would also change WDS::ReomveAutoFillProfile to only take the id.

I've followed the above algorithm and made RemoveAutoFillProfile take the
profile ID.

http://codereview.chromium.org/545175/diff/2007/8029
File chrome/browser/autofill/personal_data_manager.h (right):

http://codereview.chromium.org/545175/diff/2007/8029#newcode53
chrome/browser/autofill/personal_data_manager.h:53: void
NotifyObserverWhenLoaded(PersonalDataManager::Observer* observer);
On 2010/01/23 17:11:01, sky wrote:
> Is this method really needed? Can't the consumer check if loaded and if not
> loaded set itself as the observer?

I had a concern that there was a race condition, but AutoFillManager and
PersonalDataManager are on the same thread, so there's no worry.  I've removed
this method.

http://codereview.chromium.org/545175/diff/2007/8040
File chrome/browser/autofill/personal_data_manager_unittest.cc (right):

http://codereview.chromium.org/545175/diff/2007/8040#newcode120
chrome/browser/autofill/personal_data_manager_unittest.cc:120:
profile0.set_unique_id(1);
On 2010/01/23 17:11:01, sky wrote:
> Can you set the id from that in update to be more correct, eg:
> 
> profile0.set_unique_id(update[0].unique_id());

Done.

http://codereview.chromium.org/545175/diff/2007/8040#newcode133
chrome/browser/autofill/personal_data_manager_unittest.cc:133:
update.push_back(profile0);
On 2010/01/23 17:11:01, sky wrote:
> You should add a new profile at the same time to exercise more code paths.

Done.

http://codereview.chromium.org/545175/diff/2007/8040#newcode136
chrome/browser/autofill/personal_data_manager_unittest.cc:136: const
std::vector<AutoFillProfile*>& results2 = personal_data_->profiles();
On 2010/01/23 17:11:01, sky wrote:
> It would be good if you also shut down the PDM, reloaded it and verified the
> profiles again. This way you're making sure everything makes it to the db and
> back again.

Done.

[James Hawkins, 2010-01-23 22:39:13]

One more problem left that I'm not sure how to fix.  The mac and win bots show
the flakiness of the test.  On lines 127 and 179, we set up an EXPECT_CALL
because we're waiting for the personal data manager to load the profiles.  The
personal data manager is waiting for the web database service to load the
profiles from the database. We Run() the current message loop, to give the db
thread the chance to load the profiles, but sometimes the profiles aren't loaded
in time and OnPersonalDataLoaded is not called.  How can we block the tests
until the data is loaded in the PDM, besides a simple loop?

[sky, 2010-01-24 07:16:56]

I think you're seeing flakiness because you have two EXPECT_CALLS with quits but
only one Run. You want to block the message loop until your observer is
notified, you don't really care about waiting for the WebDB to load (well, you
do, but waiting for the PDM observer to be notified tests for the WebDB at the
same time). I think if you nuke lines 114 and 120, the flakiness will go away.

I'm giving you the LGTM, assuming you straighten out the flakiness.

http://codereview.chromium.org/545175/diff/1032/37
File chrome/browser/autofill/personal_data_manager.cc (right):

http://codereview.chromium.org/545175/diff/1032/37#newcode79
chrome/browser/autofill/personal_data_manager.cc:79: CreateNextUniqueID()));
Does this need to block until you've finished loading?

http://codereview.chromium.org/545175/diff/1032/37#newcode160
chrome/browser/autofill/personal_data_manager.cc:160: std::set<int> remove_ids =
unique_ids_;
nit: the way you've written it (and because you clear on 174) you don't need
remove_ids, you can just update unique_ids_ as you're going.

[James Hawkins, 2010-01-24 07:52:24]

Thanks for the tip about the flakiness.  I can't repro the flakiness anymore
locally, when I could before.  I'm going to wait until the trybots are green
before committing.  Thanks again for reviewing on the weekend.

http://codereview.chromium.org/545175/diff/1032/37
File chrome/browser/autofill/personal_data_manager.cc (right):

http://codereview.chromium.org/545175/diff/1032/37#newcode79
chrome/browser/autofill/personal_data_manager.cc:79: CreateNextUniqueID()));
On 2010/01/24 07:16:56, sky wrote:
> Does this need to block until you've finished loading?

In all current instances that ImportFormData is called, we've already waited for
the personal data to load.  We have the same expectation for callers of
SetProfiles too, so it should be fine?

It's on my TODO list to add a callback instead of using the observer interface.

http://codereview.chromium.org/545175/diff/1032/37#newcode160
chrome/browser/autofill/personal_data_manager.cc:160: std::set<int> remove_ids =
unique_ids_;
On 2010/01/24 07:16:56, sky wrote:
> nit: the way you've written it (and because you clear on 174) you don't need
> remove_ids, you can just update unique_ids_ as you're going.

Done.