Make PersonalDataManager use GetCreditCards() (rather than using credit_cards_ directly) so Autofill

Make PersonalDataManager use GetCreditCards() (rather than using credit_cards_ directly) so AutofillDialogControllerImpl is testable (i.e. GetCreditCardByGUID() acutally works).

R=isherman@chromium.org,thestig@chromium.org
BUG=none
TEST=compiles

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=197799

[Ilya Sherman, 2013-04-30 08:20:28]

LGTM with nits

https://codereview.chromium.org/14564003/diff/1/components/autofill/browser/a...
File components/autofill/browser/autofill_manager.cc (left):

https://codereview.chromium.org/14564003/diff/1/components/autofill/browser/a...
components/autofill/browser/autofill_manager.cc:343: const
std::vector<CreditCard*>& credit_cards = personal_data_->credit_cards();
nit: Please preserve this variable name.

https://codereview.chromium.org/14564003/diff/1/components/autofill/browser/p...
File components/autofill/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/14564003/diff/1/components/autofill/browser/p...
components/autofill/browser/personal_data_manager.cc:647: for
(std::vector<CreditCard*>::const_iterator iter = credit_cards_.begin();
Why not use GetCreditCards() for this method as well?

[Dan Beam, 2013-05-01 03:46:44]

https://codereview.chromium.org/14564003/diff/1/components/autofill/browser/a...
File components/autofill/browser/autofill_manager.cc (left):

https://codereview.chromium.org/14564003/diff/1/components/autofill/browser/a...
components/autofill/browser/autofill_manager.cc:343: const
std::vector<CreditCard*>& credit_cards = personal_data_->credit_cards();
On 2013/04/30 08:20:29, Ilya Sherman wrote:
> nit: Please preserve this variable name.

Done.

https://codereview.chromium.org/14564003/diff/1/components/autofill/browser/p...
File components/autofill/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/14564003/diff/1/components/autofill/browser/p...
components/autofill/browser/personal_data_manager.cc:647: for
(std::vector<CreditCard*>::const_iterator iter = credit_cards_.begin();
On 2013/04/30 08:20:29, Ilya Sherman wrote:
> Why not use GetCreditCards() for this method as well?

Done.

[commit-bot: I haz the power, 2013-05-01 03:52:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/14564003/5011

[Dan Beam, 2013-05-01 04:01:51]

+thestig@ for chrome/OWNERS rename stamp

[Lei Zhang, 2013-05-01 04:08:52]

lgtm

https://codereview.chromium.org/14564003/diff/5011/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_browsertest.cc (right):

https://codereview.chromium.org/14564003/diff/5011/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_browsertest.cc:1141: ASSERT_EQ(cards[i],
*personal_data_manager()->GetCreditCards()[i]);
nits you may want to fix later - maybe just call GetCreditCard() once rather
than over and over again in the loop?

https://codereview.chromium.org/14564003/diff/5011/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_browsertest.cc:1141: ASSERT_EQ(cards[i],
*personal_data_manager()->GetCreditCards()[i]);
you may also want to just EXPECT_EQ here. Ditto with some of the tests below
once you know you are not going to go out of bounds.

https://codereview.chromium.org/14564003/diff/5011/components/autofill/browse...
File components/autofill/browser/test_personal_data_manager.cc (right):

https://codereview.chromium.org/14564003/diff/5011/components/autofill/browse...
components/autofill/browser/test_personal_data_manager.cc:32: const
std::vector<CreditCard*>& TestPersonalDataManager::
nit: I prefer this style, but if you've been doing this consistently, then ok.

const std::vector<CreditCard*>&
TestPersonalDataManager::GetCreditCards() const {

[Dan Beam, 2013-05-01 04:19:36]

isherman@ what do you think about thestig@'s nits?  should I change in a
followup CL?

https://codereview.chromium.org/14564003/diff/5011/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_browsertest.cc (right):

https://codereview.chromium.org/14564003/diff/5011/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_browsertest.cc:1141: ASSERT_EQ(cards[i],
*personal_data_manager()->GetCreditCards()[i]);
On 2013/05/01 04:08:52, Lei Zhang wrote:
> you may also want to just EXPECT_EQ here. Ditto with some of the tests below
> once you know you are not going to go out of bounds.

while I agree it should be EXPECT_EQ() here I really don't want to change this
in this CL

https://codereview.chromium.org/14564003/diff/5011/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_browsertest.cc:1141: ASSERT_EQ(cards[i],
*personal_data_manager()->GetCreditCards()[i]);
On 2013/05/01 04:08:52, Lei Zhang wrote:
> nits you may want to fix later - maybe just call GetCreditCard() once rather
> than over and over again in the loop?

yeah, I'm not sure of the point of that... though it's probably quite cheap.  I
think just the name being GetCreditCards() makes it scarier than credit_cards()
because of chrome's dumb_accessor() vs MayDoALot() naming...

https://codereview.chromium.org/14564003/diff/5011/components/autofill/browse...
File components/autofill/browser/test_personal_data_manager.cc (right):

https://codereview.chromium.org/14564003/diff/5011/components/autofill/browse...
components/autofill/browser/test_personal_data_manager.cc:32: const
std::vector<CreditCard*>& TestPersonalDataManager::
On 2013/05/01 04:08:52, Lei Zhang wrote:
> nit: I prefer this style, but if you've been doing this consistently, then ok.
> 
> const std::vector<CreditCard*>&
> TestPersonalDataManager::GetCreditCards() const {

yeah, estade@ has been doing this consistently (that I've seen), yes

[commit-bot: I haz the power, 2013-05-01 04:20:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/14564003/13002

[commit-bot: I haz the power, 2013-05-01 04:42:27]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Ilya Sherman, 2013-05-01 07:42:09]

Fine either way for those nits

[commit-bot: I haz the power, 2013-05-01 10:31:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/14564003/13002

[commit-bot: I haz the power, 2013-05-01 10:55:30]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-05-01 18:16:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/14564003/13002

[commit-bot: I haz the power, 2013-05-01 18:54:51]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Dan Beam, 2013-05-01 19:32:27]

why the hell do we even have a CQ again?

[commit-bot: I haz the power, 2013-05-01 19:32:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/14564003/13002

[commit-bot: I haz the power, 2013-05-01 19:51:23]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-01 23:19:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/14564003/34001

[commit-bot: I haz the power, 2013-05-02 01:04:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/14564003/45002

[commit-bot: I haz the power, 2013-05-02 03:20:40]

Change committed as 197799