Makes sync code persist the date the node was added. I'm hoping this covers

Makes sync code persist the date node was added. I'm hoping this covers
most of the cases folks are encountering. We may need to persist
date_folder_modified to really cover everything.

BUG=84880
TEST=covered by tests.
R=akalin@chromium.org


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

[sky, 2012-10-11 21:23:42]

Fred, if you're not the right person for sync related bookmark changes could you
suggest who would be?

Thanks,

  -Scott

[akalin, 2012-10-11 22:03:49]

Nicolas and I will look at this.

[Nicolas Zea, 2012-10-11 22:39:03]

I'm not a big fan of the running_count approach to generating/checking creation
times. Seems kind of brittle, but at the same time, aside from hardcoding a
bunch of creation times into the test data, I'm not sure of a better way.

LGTM

http://codereview.chromium.org/11090083/diff/8/sync/protocol/bookmark_specifi...
File sync/protocol/bookmark_specifics.proto (right):

http://codereview.chromium.org/11090083/diff/8/sync/protocol/bookmark_specifi...
sync/protocol/bookmark_specifics.proto:22: optional int64 creation_time = 4;
Comment about what this is used for?

[akalin, 2012-10-11 22:39:29]

http://codereview.chromium.org/11090083/diff/8/sync/protocol/bookmark_specifi...
File sync/protocol/bookmark_specifics.proto (right):

http://codereview.chromium.org/11090083/diff/8/sync/protocol/bookmark_specifi...
sync/protocol/bookmark_specifics.proto:22: optional int64 creation_time = 4;
There's a unfortunate situation with timestamps.  Sync uses ms since the Unix
epoch (for legacy reasons) but base::Time uses us since the Windows epoch.  Do
we need us resolution for this timestamp?  If not, I'd prefer converting to ms
since the Unix epoch (and adding a comment / _ms suffix).  There are already
functions for this:  sync::TimeToProtoTime/ProtoTimeToTime in
sync/api/util/time.h.

Nicolas, wdyt?

[Nicolas Zea, 2012-10-11 22:45:43]

On 2012/10/11 22:39:29, akalin wrote:
>
http://codereview.chromium.org/11090083/diff/8/sync/protocol/bookmark_specifi...
> File sync/protocol/bookmark_specifics.proto (right):
> 
>
http://codereview.chromium.org/11090083/diff/8/sync/protocol/bookmark_specifi...
> sync/protocol/bookmark_specifics.proto:22: optional int64 creation_time = 4;
> There's a unfortunate situation with timestamps.  Sync uses ms since the Unix
> epoch (for legacy reasons) but base::Time uses us since the Windows epoch.  Do
> we need us resolution for this timestamp?  If not, I'd prefer converting to ms
> since the Unix epoch (and adding a comment / _ms suffix).  There are already
> functions for this:  sync::TimeToProtoTime/ProtoTimeToTime in
> sync/api/util/time.h.
> 
> Nicolas, wdyt?

_us/_ms suffix is a valid point. I'm kind of ambivalent about the whether we use
the ToInternalValue or ms since Unix epoch, as long as it's commented
appropriately, so I'll defer to Fred here.

[akalin, 2012-10-11 22:52:17]

On 2012/10/11 22:45:43, nzea wrote:
> On 2012/10/11 22:39:29, akalin wrote:
> >
>
http://codereview.chromium.org/11090083/diff/8/sync/protocol/bookmark_specifi...
> > File sync/protocol/bookmark_specifics.proto (right):
> > 
> >
>
http://codereview.chromium.org/11090083/diff/8/sync/protocol/bookmark_specifi...
> > sync/protocol/bookmark_specifics.proto:22: optional int64 creation_time = 4;
> > There's a unfortunate situation with timestamps.  Sync uses ms since the
Unix
> > epoch (for legacy reasons) but base::Time uses us since the Windows epoch. 
Do
> > we need us resolution for this timestamp?  If not, I'd prefer converting to
ms
> > since the Unix epoch (and adding a comment / _ms suffix).  There are already
> > functions for this:  sync::TimeToProtoTime/ProtoTimeToTime in
> > sync/api/util/time.h.
> > 
> > Nicolas, wdyt?
> 
> _us/_ms suffix is a valid point. I'm kind of ambivalent about the whether we
use
> the ToInternalValue or ms since Unix epoch, as long as it's commented
> appropriately, so I'll defer to Fred here.

How about _us suffix + comment saying it's the value of
base::TIme::ToInternalValue()?

[sky, 2012-10-11 23:16:16]

Updated to _us.

[akalin, 2012-10-12 01:23:55]

lgtm

not a huge fan of running_count either, but it'll do for now.

[commit-bot: I haz the power, 2012-10-12 14:11:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/11090083/7002

[commit-bot: I haz the power, 2012-10-12 15:08:37]

Retried try job too often for step(s) sync_unit_tests

[akalin, 2012-10-12 16:58:36]

On 2012/10/12 15:08:37, I haz the power (commit-bot) wrote:
> Retried try job too often for step(s) sync_unit_tests

Also, can you change the CL description to say "persist the date the node was
added" instead of "persist date node was added"?  I had to read the latter a few
times to parse it

[sky, 2012-10-12 17:59:41]

On 2012/10/12 16:58:36, akalin wrote:
> On 2012/10/12 15:08:37, I haz the power (commit-bot) wrote:
> > Retried try job too often for step(s) sync_unit_tests
> 
> Also, can you change the CL description to say "persist the date the node was
> added" instead of "persist date node was added"?  I had to read the latter a
few
> times to parse it

Done and test updated.

[commit-bot: I haz the power, 2012-10-12 18:00:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/11090083/8005

[commit-bot: I haz the power, 2012-10-12 21:28:46]

Change committed as 161655