Add use_count and use_date to unmasked server cards as well.

Add use_count and use_date to unmasked server cards as well.

TODO: I think my plan for masked cards is to rank them just below the worst ranked unmasked card, but not above any local card that's been used at least 5 times. That way they'll start near the top, but eventually filter down to the bottom (unless you unlock them), while staying above cards you've used zero times.

For wallet addresses, perhaps stick them just below the worst ranked "complete" local address. A "complete" address is defined by addressinput::HasAllRequiredFields.

BUG=342426
Committed: https://crrev.com/87362031d4731faeed342d554b231916958774f5
Cr-Commit-Position: refs/heads/master@{#318165}

[Evan Stade, 2015-02-21 02:19:56]

estade@chromium.org changed reviewers:
+ brettw@chromium.org

[Evan Stade, 2015-02-21 02:20:08]

estade@chromium.org changed reviewers:
+ pkasting@google.com

[Peter Kasting, 2015-02-21 02:29:11]

Reminder: Say what you want multiple reviewers to review (I assume I'm reviewing
the not-autofill stuff)

[Peter Kasting, 2015-02-21 02:30:59]

pkasting@chromium.org changed reviewers:
+ pkasting@chromium.org

[Peter Kasting, 2015-02-21 02:30:59]

LGTM

https://codereview.chromium.org/947693002/diff/1/components/webdata/common/we...
File components/webdata/common/web_database_migration_unittest.cc (right):

https://codereview.chromium.org/947693002/diff/1/components/webdata/common/we...
components/webdata/common/web_database_migration_unittest.cc:2848:
EXPECT_TRUE(connection.DoesColumnExist("autofill_profiles", "use_date"));
Is it worth checking the actual counts or dates in these tests?

[Evan Stade, 2015-02-21 02:31:09]

On 2015/02/21 02:29:11, Peter Kasting wrote:
> Reminder: Say what you want multiple reviewers to review (I assume I'm
reviewing
> the not-autofill stuff)

your assumption is accurate

[Evan Stade, 2015-02-21 02:32:56]

https://codereview.chromium.org/947693002/diff/1/components/webdata/common/we...
File components/webdata/common/web_database_migration_unittest.cc (right):

https://codereview.chromium.org/947693002/diff/1/components/webdata/common/we...
components/webdata/common/web_database_migration_unittest.cc:2848:
EXPECT_TRUE(connection.DoesColumnExist("autofill_profiles", "use_date"));
On 2015/02/21 02:30:59, Peter Kasting wrote:
> Is it worth checking the actual counts or dates in these tests?

I'm not a huge fan of testing a constant by copying that constant into a test
file. But I don't feel strongly.

[Peter Kasting, 2015-02-21 02:44:53]

https://codereview.chromium.org/947693002/diff/1/components/webdata/common/we...
File components/webdata/common/web_database_migration_unittest.cc (right):

https://codereview.chromium.org/947693002/diff/1/components/webdata/common/we...
components/webdata/common/web_database_migration_unittest.cc:2848:
EXPECT_TRUE(connection.DoesColumnExist("autofill_profiles", "use_date"));
On 2015/02/21 02:32:56, Evan Stade wrote:
> On 2015/02/21 02:30:59, Peter Kasting wrote:
> > Is it worth checking the actual counts or dates in these tests?
> 
> I'm not a huge fan of testing a constant by copying that constant into a test
> file.

Sounds like the answer is no, then.  :)

[brettw, 2015-02-23 19:04:34]

lgtm

https://codereview.chromium.org/947693002/diff/1/components/autofill/core/bro...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/947693002/diff/1/components/autofill/core/bro...
components/autofill/core/browser/webdata/autofill_table.cc:1276:
card->set_use_date(base::Time::FromTimeT(use_date));
I think it's better to use Time::FromInternalValue and ::ToInternalValue than to
convert everything to time_t. This is really what the internal value functions
are for.

https://codereview.chromium.org/947693002/diff/1/components/autofill/core/bro...
File components/autofill/core/browser/webdata/autofill_table.h (right):

https://codereview.chromium.org/947693002/diff/1/components/autofill/core/bro...
components/autofill/core/browser/webdata/autofill_table.h:166: //   use_date    
      The date this card was last used to fill a form.
Can we call this (and the other one above) "last_used_date"? Also, the comments
should mention what units this is in (see comment about units below)

https://codereview.chromium.org/947693002/diff/1/components/webdata/common/we...
File components/webdata/common/web_database_migration_unittest.cc (right):

https://codereview.chromium.org/947693002/diff/1/components/webdata/common/we...
components/webdata/common/web_database_migration_unittest.cc:2848:
EXPECT_TRUE(connection.DoesColumnExist("autofill_profiles", "use_date"));
FWIW I would have done it like Evan did. I haven't found SQL code to be amenable
to doing it the way you prefer.

[Peter Kasting, 2015-02-23 21:14:42]

https://codereview.chromium.org/947693002/diff/1/components/webdata/common/we...
File components/webdata/common/web_database_migration_unittest.cc (right):

https://codereview.chromium.org/947693002/diff/1/components/webdata/common/we...
components/webdata/common/web_database_migration_unittest.cc:2848:
EXPECT_TRUE(connection.DoesColumnExist("autofill_profiles", "use_date"));
On 2015/02/23 19:04:34, brettw wrote:
> FWIW I would have done it like Evan did. I haven't found SQL code to be
amenable
> to doing it the way you prefer.

I'm not sure which way you're thinking I prefer?  I think Evan's code is fine. 
I wasn't sure whether there were any other checks that it would make sense to do
and it sounds like there aren't.

[Evan Stade, 2015-02-23 22:57:22]

https://codereview.chromium.org/947693002/diff/1/components/autofill/core/bro...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/947693002/diff/1/components/autofill/core/bro...
components/autofill/core/browser/webdata/autofill_table.cc:1276:
card->set_use_date(base::Time::FromTimeT(use_date));
On 2015/02/23 19:04:34, brettw wrote:
> I think it's better to use Time::FromInternalValue and ::ToInternalValue than
to
> convert everything to time_t. This is really what the internal value functions
> are for.

I think you're right but I wasn't aware of FromInternalValue until now. Since
the other tables already use time t, I think it's best to stay consistent. What
to do you think?

https://codereview.chromium.org/947693002/diff/1/components/autofill/core/bro...
File components/autofill/core/browser/webdata/autofill_table.h (right):

https://codereview.chromium.org/947693002/diff/1/components/autofill/core/bro...
components/autofill/core/browser/webdata/autofill_table.h:166: //   use_date    
      The date this card was last used to fill a form.
On 2015/02/23 19:04:34, brettw wrote:
> Can we call this (and the other one above) "last_used_date"? Also, the
comments
> should mention what units this is in (see comment about units below)

I'd like to be consistent with the names used in the other tables (the comments
in this file were wrong). I've updated the docs regarding the units and when the
fields were added.

[Evan Stade, 2015-02-25 00:07:22]

On 2015/02/23 22:57:22, Evan Stade wrote:
>
https://codereview.chromium.org/947693002/diff/1/components/autofill/core/bro...
> File components/autofill/core/browser/webdata/autofill_table.cc (right):
> 
>
https://codereview.chromium.org/947693002/diff/1/components/autofill/core/bro...
> components/autofill/core/browser/webdata/autofill_table.cc:1276:
> card->set_use_date(base::Time::FromTimeT(use_date));
> On 2015/02/23 19:04:34, brettw wrote:
> > I think it's better to use Time::FromInternalValue and ::ToInternalValue
than
> to
> > convert everything to time_t. This is really what the internal value
functions
> > are for.
> 
> I think you're right but I wasn't aware of FromInternalValue until now. Since
> the other tables already use time t, I think it's best to stay consistent.
What
> to do you think?
> 
>
https://codereview.chromium.org/947693002/diff/1/components/autofill/core/bro...
> File components/autofill/core/browser/webdata/autofill_table.h (right):
> 
>
https://codereview.chromium.org/947693002/diff/1/components/autofill/core/bro...
> components/autofill/core/browser/webdata/autofill_table.h:166: //   use_date  
 
>       The date this card was last used to fill a form.
> On 2015/02/23 19:04:34, brettw wrote:
> > Can we call this (and the other one above) "last_used_date"? Also, the
> comments
> > should mention what units this is in (see comment about units below)
> 
> I'd like to be consistent with the names used in the other tables (the
comments
> in this file were wrong). I've updated the docs regarding the units and when
the
> fields were added.

ping brettw for post-lgtm questions

[brettw, 2015-02-25 00:19:25]

My preference would be to use the internal value for the times. I think we
should be trying to fix the other ones eventually. This makes a non-theoretical
difference when writing tests because writing to the database is lossy and all
of the times come out differently. The result is you have to write a bunch of
extra test code to truncate the expected values to time_t.

I'm OK with the column names matching the existing ones.

[Evan Stade, 2015-02-25 22:54:51]

On 2015/02/25 00:19:25, brettw wrote:
> My preference would be to use the internal value for the times.

done

[Evan Stade, 2015-02-25 22:54:53]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-02-25 22:54:54]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/947693002/#ps40001 (title:
"internal time instead of time_t")

[commit-bot: I haz the power, 2015-02-25 22:55:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/947693002/40001

[commit-bot: I haz the power, 2015-02-26 01:05:38]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-26 01:06:35]

Patchset 3 (id:??) landed as
https://crrev.com/87362031d4731faeed342d554b231916958774f5
Cr-Commit-Position: refs/heads/master@{#318165}