Serialize date_created in PasswordStoreX properly

Serialize date_created in PasswordStoreX properly

BUG=374132
Committed: https://crrev.com/174f0fb391c2504354548571d83d79c93ce1c380
Cr-Commit-Position: refs/heads/master@{#314581}

[dvadym, 2015-02-02 16:13:13]

dvadym@chromium.org changed reviewers:
+ vabr@chromium.org

[dvadym, 2015-02-02 16:13:13]

Hi Vaclav,

 Could you please review this CL? This is changes only for libsecret, after
reviewing this I'll add changes for keyring and KWallet.

Best regards,
Vadym

[vabr (Chromium), 2015-02-02 17:01:18]

Hi Vadym,

Do I understand it right, that INT_MAX interpreted as internal value is a date
sufficiently far in the past, so that it is unlikely, that a date stored as an
internal value would be under INT_MAX?

If yes, then LGTM.

Cheers,
Vaclav

https://codereview.chromium.org/895653002/diff/1/chrome/browser/password_mana...
File chrome/browser/password_manager/native_backend_libsecret.cc (right):

https://codereview.chromium.org/895653002/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/native_backend_libsecret.cc:24: const int
kMaxPossibleTimeTValue = INT_MAX;
What about std::numeric_limits<int>::max() ?

[dvadym, 2015-02-03 11:53:50]

Thanks, Vaclav. 

>Do I understand it right, that INT_MAX interpreted as >internal value is a date
>sufficiently far in the past, so that it is unlikely, that >a date stored as an
>internal value would be under INT_MAX?

Yeah, correct. Internal value is number of microseconds from Jan 1 1601, so
INT_MAX as internal values is something like 1am Jan 1601, that definitely can't
happen :)

Best regards,
Vadym

https://codereview.chromium.org/895653002/diff/1/chrome/browser/password_mana...
File chrome/browser/password_manager/native_backend_libsecret.cc (right):

https://codereview.chromium.org/895653002/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/native_backend_libsecret.cc:24: const int
kMaxPossibleTimeTValue = INT_MAX;
On 2015/02/02 17:01:18, vabr (Chromium) wrote:
> What about std::numeric_limits<int>::max() ?

Done.

[dvadym, 2015-02-03 14:12:47]

Hi Vaclav,

 Could you please have a look at keyring and kwallet changes?

Best regards,
Vadym

[vabr (Chromium), 2015-02-03 16:06:42]

Still LGTM, with some comments.

Cheers,
Vaclav

https://codereview.chromium.org/895653002/diff/60001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/895653002/diff/60001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:139: // them by taking
into consideration that INT_MAX correspond to
nit: INT_MAX -> kMaxPossibleTimeTValue
(Also for libsecret.)

https://codereview.chromium.org/895653002/diff/60001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:140: // 2038 year in
time_t and internal values of base::Time are much bigger.
Perhaps you could explain more clearly:
In the past date_created was stored as time_t, currently is stored as
base::Time's internal value. We need to distinguish, which format the number in
|date_created| was stored in. We use tha fact that kMaxPossibleTimeTValue
interpreted as the internal value corresponds to an unlikely date back in 17th
century, and anything above kMaxPossibleTimeTValue clearly must be in the
internal value format.

https://codereview.chromium.org/895653002/diff/60001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:141:
form->date_created = date_created < kMaxPossibleTimeTValue
I suggest pulling this check and kMaxPossibleTimeTValue out of the two files,
and putting it as a function in
chrome/browser/password_manager/password_manager_util.{h|cc}, to avoid code
duplication.

https://codereview.chromium.org/895653002/diff/60001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc
(right):

https://codereview.chromium.org/895653002/diff/60001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc:205: int
version) {
I suggest adding a flag saying whether the date is in internal format or not,
rather than the pickle version. On places like line 615 the version argument
will stop begin the newest version ultimately, and it will be unclear whether it
needs to be bumped or not.

https://codereview.chromium.org/895653002/diff/60001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc:949: bool
date_created_internal,
Please add a comment explaining the arguments, in particular
date_created_internal

https://codereview.chromium.org/895653002/diff/60001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc:1161:
CheckVersion3Pickle();
3->5

[dvadym, 2015-02-04 10:40:25]

Thanks, Vaclav. Fixed.

https://codereview.chromium.org/895653002/diff/60001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/895653002/diff/60001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:139: // them by taking
into consideration that INT_MAX correspond to
On 2015/02/03 16:06:42, vabr (Chromium) wrote:
> nit: INT_MAX -> kMaxPossibleTimeTValue
> (Also for libsecret.)

Done.

https://codereview.chromium.org/895653002/diff/60001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:140: // 2038 year in
time_t and internal values of base::Time are much bigger.
On 2015/02/03 16:06:42, vabr (Chromium) wrote:
> Perhaps you could explain more clearly:
> In the past date_created was stored as time_t, currently is stored as
> base::Time's internal value. We need to distinguish, which format the number
in
> |date_created| was stored in. We use tha fact that kMaxPossibleTimeTValue
> interpreted as the internal value corresponds to an unlikely date back in 17th
> century, and anything above kMaxPossibleTimeTValue clearly must be in the
> internal value format.

Done.

https://codereview.chromium.org/895653002/diff/60001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:141:
form->date_created = date_created < kMaxPossibleTimeTValue
On 2015/02/03 16:06:42, vabr (Chromium) wrote:
> I suggest pulling this check and kMaxPossibleTimeTValue out of the two files,
> and putting it as a function in
> chrome/browser/password_manager/password_manager_util.{h|cc}, to avoid code
> duplication.

Keyring backend will finally disappear, so after offline disccusion it was
decided to leave as is.

https://codereview.chromium.org/895653002/diff/60001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc
(right):

https://codereview.chromium.org/895653002/diff/60001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc:205: int
version) {
On 2015/02/03 16:06:42, vabr (Chromium) wrote:
> I suggest adding a flag saying whether the date is in internal format or not,
> rather than the pickle version. On places like line 615 the version argument
> will stop begin the newest version ultimately, and it will be unclear whether
it
> needs to be bumped or not.

Done.

https://codereview.chromium.org/895653002/diff/60001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc:949: bool
date_created_internal,
On 2015/02/03 16:06:42, vabr (Chromium) wrote:
> Please add a comment explaining the arguments, in particular
> date_created_internal

Done.

https://codereview.chromium.org/895653002/diff/60001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc:1161:
CheckVersion3Pickle();
On 2015/02/03 16:06:42, vabr (Chromium) wrote:
> 3->5

Done.

[vabr (Chromium), 2015-02-04 12:23:51]

LGTM with nits (mostly caused by mistakes I made in the previous comments :)).

Cheers,
Vaclav

https://codereview.chromium.org/895653002/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/895653002/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:138: // In the past
|date_created| was stored as time_t, currently is stored as
To improve my grammar, some punctuation and missing subject supplied:

In the past, |date_created| was stored as time_t. Currently it is stored as...

https://codereview.chromium.org/895653002/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:140: // number in
|date_created| was stored in. We use tha fact that
Seems like I planted a typo in my suggestion. :)
tha -> the

https://codereview.chromium.org/895653002/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc
(right):

https://codereview.chromium.org/895653002/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc:947: //
|date_created_internal| store date_created as base::Time's internal value.
Please make this comment into full sentences, and clarify vague terms.
Suggestion:

Creates a Pickle from |form|. If |size_32| is true, stores the number of forms
in the pickle as a 32bit uint, otherwise as size_t. The latter should be the
case for versions > 0. If |date_created_internal| is true, stores date_created
as base::Time's internal value, otherwise as time_t.

[dvadym, 2015-02-04 14:23:51]

New patchsets have been uploaded after l-g-t-m from vabr@chromium.org

[dvadym, 2015-02-04 14:24:00]

Thanks, Done.

https://codereview.chromium.org/895653002/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/895653002/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:138: // In the past
|date_created| was stored as time_t, currently is stored as
On 2015/02/04 12:23:51, vabr (Chromium) wrote:
> To improve my grammar, some punctuation and missing subject supplied:
> 
> In the past, |date_created| was stored as time_t. Currently it is stored as...

Done.

https://codereview.chromium.org/895653002/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:140: // number in
|date_created| was stored in. We use tha fact that
On 2015/02/04 12:23:51, vabr (Chromium) wrote:
> Seems like I planted a typo in my suggestion. :)
> tha -> the

Done.

https://codereview.chromium.org/895653002/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc
(right):

https://codereview.chromium.org/895653002/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc:947: //
|date_created_internal| store date_created as base::Time's internal value.
On 2015/02/04 12:23:51, vabr (Chromium) wrote:
> Please make this comment into full sentences, and clarify vague terms.
> Suggestion:
> 
> Creates a Pickle from |form|. If |size_32| is true, stores the number of forms
> in the pickle as a 32bit uint, otherwise as size_t. The latter should be the
> case for versions > 0. If |date_created_internal| is true, stores date_created
> as base::Time's internal value, otherwise as time_t.

Done.

[dvadym, 2015-02-04 14:41:50]

The CQ bit was checked by dvadym@chromium.org

[commit-bot: I haz the power, 2015-02-04 14:42:27]

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

[commit-bot: I haz the power, 2015-02-04 14:50:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-04 14:50:56]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dvadym, 2015-02-04 16:39:14]

The CQ bit was checked by dvadym@chromium.org

[commit-bot: I haz the power, 2015-02-04 16:39:58]

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

[commit-bot: I haz the power, 2015-02-04 16:54:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-04 16:54:42]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dvadym, 2015-02-04 17:26:35]

The CQ bit was checked by dvadym@chromium.org

[commit-bot: I haz the power, 2015-02-04 17:27:22]

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

[commit-bot: I haz the power, 2015-02-04 18:02:22]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-02-04 18:04:15]

Patchset 8 (id:??) landed as
https://crrev.com/174f0fb391c2504354548571d83d79c93ce1c380
Cr-Commit-Position: refs/heads/master@{#314581}