[Autofill] Suggest expired credit cards last.

They are currently sorted by frecency with the rest of the cards.

BUG=601165
TEST=PersonalDataManagerTest

Committed: https://crrev.com/037e1d8c996380be7dc34fce29e6f70d6dc318c3
Cr-Commit-Position: refs/heads/master@{#386098}

[sebsg, 2016-04-06 20:08:54]

Description was changed from

==========
[Autofill] Suggest expired credit cards last.

They are currently sorted by frecency with the rest of the cards.

BUG=601165
TEST=CreditCardTest
==========

to

==========
They are currently sorted by frecency with the rest of the cards.

BUG=601165
TEST=CreditCardTest
==========

[sebsg, 2016-04-06 20:12:35]

Patchset #1 (id:1) has been deleted

[sebsg, 2016-04-06 22:06:48]

Patchset #2 (id:40001) has been deleted

[sebsg, 2016-04-07 14:58:52]

Patchset #2 (id:60001) has been deleted

[sebsg, 2016-04-07 15:42:28]

sebsg@chromium.org changed reviewers:
+ mathp@chromium.org

[sebsg, 2016-04-07 15:42:29]

Hi Math, could you please review this change? Thanks!

[Mathieu, 2016-04-07 16:57:54]

Thanks

https://codereview.chromium.org/1867523003/diff/80001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/1867523003/diff/80001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:814: if (a_is_expired
!= b->IsExpired(comparison_time_exploded))
let's use CreditCard::IsValid

https://codereview.chromium.org/1867523003/diff/80001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager_unittest.cc (right):

https://codereview.chromium.org/1867523003/diff/80001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager_unittest.cc:3383: // Test
that expired cards are ordered by frecency.
Can we add a test where there is 

- a non expired card that is not used at all
- an expired card that has been used 5 times.
- an expired card that has been used 3 times.

I would expect this order to come out.

[sebsg, 2016-04-07 18:01:44]

Description was changed from

==========
They are currently sorted by frecency with the rest of the cards.

BUG=601165
TEST=CreditCardTest
==========

to

==========
They are currently sorted by frecency with the rest of the cards.

BUG=601165
TEST=PersonalDataManagerTest
==========

[sebsg, 2016-04-07 18:04:32]

Patchset #3 (id:100001) has been deleted

[sebsg, 2016-04-07 18:08:20]

Patchset #3 (id:120001) has been deleted

[sebsg, 2016-04-07 18:26:06]

Hi math, I addressed you comments. Could you please take another look? I'm also
rebasing now. Thanks!

https://codereview.chromium.org/1867523003/diff/80001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/1867523003/diff/80001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:814: if (a_is_expired
!= b->IsExpired(comparison_time_exploded))
On 2016/04/07 16:57:54, Mathieu Perreault wrote:
> let's use CreditCard::IsValid

IsValid does not work for MASKED_SERVER_CARDs because the number is not
complete.

However, I can use IsValidCreditCardExpirationDate which is half what IsValid
does (The other part being validating the number).

https://codereview.chromium.org/1867523003/diff/80001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager_unittest.cc (right):

https://codereview.chromium.org/1867523003/diff/80001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager_unittest.cc:3383: // Test
that expired cards are ordered by frecency.
On 2016/04/07 16:57:54, Mathieu Perreault wrote:
> Can we add a test where there is 
> 
> - a non expired card that is not used at all
> - an expired card that has been used 5 times.
> - an expired card that has been used 3 times.
> 
> I would expect this order to come out.

Done.

[Mathieu, 2016-04-07 18:32:14]

lgtm, sweet!

https://codereview.chromium.org/1867523003/diff/80001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/1867523003/diff/80001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:814: if (a_is_expired
!= b->IsExpired(comparison_time_exploded))
On 2016/04/07 18:26:05, sebsg wrote:
> On 2016/04/07 16:57:54, Mathieu Perreault wrote:
> > let's use CreditCard::IsValid
> 
> IsValid does not work for MASKED_SERVER_CARDs because the number is not
> complete.
> 
> However, I can use IsValidCreditCardExpirationDate which is half what IsValid
> does (The other part being validating the number).

Sounds good!

[sebsg, 2016-04-08 15:27:00]

The CQ bit was checked by sebsg@chromium.org

[sebsg, 2016-04-08 15:27:01]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org
Link to the patchset: https://codereview.chromium.org/1867523003/#ps160001
(title: "Rebase")

[commit-bot: I haz the power, 2016-04-08 15:27:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1867523003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1867523003/160001

[commit-bot: I haz the power, 2016-04-08 16:39:54]

Description was changed from

==========
They are currently sorted by frecency with the rest of the cards.

BUG=601165
TEST=PersonalDataManagerTest
==========

to

==========
They are currently sorted by frecency with the rest of the cards.

BUG=601165
TEST=PersonalDataManagerTest
==========

[commit-bot: I haz the power, 2016-04-08 16:39:55]

Committed patchset #4 (id:160001)

[commit-bot: I haz the power, 2016-04-08 16:41:23]

Description was changed from

==========
They are currently sorted by frecency with the rest of the cards.

BUG=601165
TEST=PersonalDataManagerTest
==========

to

==========
They are currently sorted by frecency with the rest of the cards.

BUG=601165
TEST=PersonalDataManagerTest

Committed: https://crrev.com/037e1d8c996380be7dc34fce29e6f70d6dc318c3
Cr-Commit-Position: refs/heads/master@{#386098}
==========

[commit-bot: I haz the power, 2016-04-08 16:41:24]

Patchset 4 (id:??) landed as
https://crrev.com/037e1d8c996380be7dc34fce29e6f70d6dc318c3
Cr-Commit-Position: refs/heads/master@{#386098}