[Sync] Add timestamp to TrafficRecorder records

[Sync] Add timestamp to TrafficRecorder records

All client server traffic records have a timestamp field to
indicate creation time of each record. Timestamps are
dumped with the traffic serializations.

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

[rlarocque, 2013-01-31 19:13:09]

There's a few style issues, but otherwise this looks OK.

https://codereview.chromium.org/12088080/diff/1/sync/engine/traffic_recorder.h
File sync/engine/traffic_recorder.h (right):

https://codereview.chromium.org/12088080/diff/1/sync/engine/traffic_recorder....
sync/engine/traffic_recorder.h:50: // Time of record creation
nit: Comments should end with periods.

https://codereview.chromium.org/12088080/diff/1/sync/engine/traffic_recorder....
sync/engine/traffic_recorder.h:71: // Method to get record creation time
nit: This comment should end with a period, too.

https://codereview.chromium.org/12088080/diff/1/sync/engine/traffic_recorder....
sync/engine/traffic_recorder.h:83: 
Please remove the unnecessary whitespace diff.

https://codereview.chromium.org/12088080/diff/1/sync/engine/traffic_recorder_...
File sync/engine/traffic_recorder_unittest.cc (right):

https://codereview.chromium.org/12088080/diff/1/sync/engine/traffic_recorder_...
sync/engine/traffic_recorder_unittest.cc:46: // Ensure that timestamp is
recorded correctly in traffic record
nit: Please put periods after comments throughout this file.

https://codereview.chromium.org/12088080/diff/1/sync/engine/traffic_recorder_...
sync/engine/traffic_recorder_unittest.cc:91: EXPECT_TRUE(value->GetDictionary(0,
&record_value));
Make this ASSERT_TRUE?  ASSERTs will abort the test, which is probably the right
thing to do here.  If GetDictionary were to fail, then the next line would
likely crash.

https://codereview.chromium.org/12088080/diff/1/sync/engine/traffic_recorder_...
sync/engine/traffic_recorder_unittest.cc:95: 
nit: Please remove extra whitespace.

https://codereview.chromium.org/12088080/diff/1/sync/test/engine/fake_traffic...
File sync/test/engine/fake_traffic_recorder.h (right):

https://codereview.chromium.org/12088080/diff/1/sync/test/engine/fake_traffic...
sync/test/engine/fake_traffic_recorder.h:14: class FakeTrafficRecorder : public
TrafficRecorder {
I'm not a big fan of this sort of inheritance.  Normally we would prefer
dependency-injection, ie. have a 'TimeGetter' class that we pass in to the
TrafficRecorder at construction time. 

However, in this case, the class being tested is so trivial and so unlikely to
become more complex that I can't see any harm coming from this.  Feel free to
leave it as is.

[Nicolas Zea, 2013-01-31 19:30:45]

One high level comment: given that the fake isn't really going to be used
anywhere else, I think it might make more sense to declare it inside of the
unittest and just call it a TestTrafficRecorder (which is something we do pretty
often)

[mgist1, 2013-02-01 01:24:55]

I did a bit of reorganizing, and addressed the style issues that were raised :)
PTAL.

Thanks!

[rlarocque, 2013-02-01 01:43:41]

LGTM.

[commit-bot: I haz the power, 2013-02-01 01:52:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgist@chromium.org/12088080/6004

[commit-bot: I haz the power, 2013-02-01 04:53:03]

Change committed as 180073

[Lei Zhang, 2013-02-01 05:19:11]

https://chromiumcodereview.appspot.com/12088080/diff/6004/sync/engine/traffic...
File sync/engine/traffic_recorder_unittest.cc (right):

https://chromiumcodereview.appspot.com/12088080/diff/6004/sync/engine/traffic...
sync/engine/traffic_recorder_unittest.cc:86: base::Time sample_time_1 =
ProtoTimeToTime(1359484676659324);
These big numbers need GG_INT64_C.