Give access to a network time kept in the variation service.

Give access to a network time kept in the variation service.

This is temporary and will eventually be moved to a dedicated network time service.

OWNER TBR for simple gyp file changes.
TBR=thestig@chromium.org

BUG=146090
TEST=NetworkTimeTrackerTest.ProperTimeTracking

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

[MAD, 2013-01-31 21:18:15]

As discussed, would this be OK with you?

Thanks!

BYE
MAD...

[Ilya Sherman, 2013-01-31 22:15:01]

Not sure if it's useful, but there was a thread related to this on chromium-dev
a few months ago:
https://groups.google.com/a/chromium.org/forum/?fromgroups=#!topic/chromium-d...

[MAD, 2013-01-31 22:44:19]

Yep, thanks, akalin@ started something and I inspired this code from:
http://src.chromium.org/viewvc/chrome/trunk/src/net/tools/get_server_time/get...

I'll then sync with him when he's back so that we get a more generic
service. But I need this for M26 so I asked Alexei if I could borrow the
variations service in the mean time.

On Thu, Jan 31, 2013 at 5:15 PM, <isherman@chromium.org> wrote:

> Not sure if it's useful, but there was a thread related to this on
> chromium-dev
> a few months ago:
> https://groups.google.com/a/**chromium.org/forum/?**
>
fromgroups=#!topic/chromium-**dev/vlRAW10f4Mc<https://groups.google.com/a/chromium.org/forum/?fromgroups=#!topic/chromium-dev/vlRAW10f4Mc>
>
>
https://codereview.chromium.**org/12096096/<https://codereview.chromium.org/1...
>

[MAD, 2013-02-01 00:04:15]

Wait, don't look at the code just yet, I noticed I left some test code in
there, and I just realized we might as well keep updating the network time
on each request... A new version is coming up shortly...

Sorry about that...

On Thu, Jan 31, 2013 at 5:43 PM, Marc-Andre Decoste <mad@chromium.org>wrote:

> Yep, thanks, akalin@ started something and I inspired this code from:
>
>
http://src.chromium.org/viewvc/chrome/trunk/src/net/tools/get_server_time/get...
>
> I'll then sync with him when he's back so that we get a more generic
> service. But I need this for M26 so I asked Alexei if I could borrow the
> variations service in the mean time.
>
> On Thu, Jan 31, 2013 at 5:15 PM, <isherman@chromium.org> wrote:
>
>> Not sure if it's useful, but there was a thread related to this on
>> chromium-dev
>> a few months ago:
>> https://groups.google.com/a/**chromium.org/forum/?**
>>
fromgroups=#!topic/chromium-**dev/vlRAW10f4Mc<https://groups.google.com/a/chromium.org/forum/?fromgroups=#!topic/chromium-dev/vlRAW10f4Mc>
>>
>>
https://codereview.chromium.**org/12096096/<https://codereview.chromium.org/1...
>>
>
>

[MAD, 2013-02-01 02:25:41]

OK, should be good now, let me know what you think...

Thanks!

BYE
MAD

[Alexei Svitkine (slow), 2013-02-01 19:38:55]

How about making a separate helper class that keeps track of the new variables
you're adding and encapsulates all the new logic?

Then, when it's time to move this out of VariationsService, it will just be a
matter of moving that class somewhere else.

https://codereview.chromium.org/12096096/diff/6002/chrome/browser/metrics/var...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/12096096/diff/6002/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.cc:281: int response_code =
request->GetResponseCode();
Nit: const

https://codereview.chromium.org/12096096/diff/6002/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.cc:290: if (response_code
== 200 || response_code == 304) {
Can you change the code to use the constants from net/http/http_status_code.h?

[Alexei Svitkine (slow), 2013-02-01 19:40:04]

On 2013/02/01 19:38:55, Alexei Svitkine wrote:
> How about making a separate helper class that keeps track of the new variables
> you're adding and encapsulates all the new logic?

Just to be clear, an instance of that helper class will reside inside
VariationsService in-place of the new variables your current patch adds.

[MAD, 2013-02-01 19:44:06]

Yes, got it, good idea, will do...

Thanks!

On Fri, Feb 1, 2013 at 2:40 PM, <asvitkine@chromium.org> wrote:

> On 2013/02/01 19:38:55, Alexei Svitkine wrote:
>
>> How about making a separate helper class that keeps track of the new
>> variables
>> you're adding and encapsulates all the new logic?
>>
>
> Just to be clear, an instance of that helper class will reside inside
> VariationsService in-place of the new variables your current patch adds.
>
>
>
>
https://codereview.chromium.**org/12096096/<https://codereview.chromium.org/1...
>

[MAD, 2013-02-01 22:01:41]

Done... How about this now?

Thanks!

BYE
MAD

https://codereview.chromium.org/12096096/diff/6002/chrome/browser/metrics/var...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/12096096/diff/6002/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.cc:281: int response_code =
request->GetResponseCode();
On 2013/02/01 19:38:55, Alexei Svitkine wrote:
> Nit: const

Done.

https://codereview.chromium.org/12096096/diff/6002/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.cc:290: if (response_code
== 200 || response_code == 304) {
On 2013/02/01 19:38:55, Alexei Svitkine wrote:
> Can you change the code to use the constants from net/http/http_status_code.h?

Done.

https://codereview.chromium.org/12096096/diff/6002/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.cc:290: if (response_code
== 200 || response_code == 304) {
On 2013/02/01 19:38:55, Alexei Svitkine wrote:
> Can you change the code to use the constants from net/http/http_status_code.h?

Done.

[Alexei Svitkine (slow), 2013-02-01 22:18:08]

Also, please add a simple unit test for the new class.

https://codereview.chromium.org/12096096/diff/4005/chrome/browser/metrics/var...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/12096096/diff/4005/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.cc:589: base::Time*
network_time, base::TimeDelta* uncertainty) const {
Each param on a separate line.

https://codereview.chromium.org/12096096/diff/4005/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.cc:594: *network_time =
network_time_ + (base::TimeTicks::Now() -
Nit: No need for ()s.

https://codereview.chromium.org/12096096/diff/4005/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.cc:604: if
(!network_time_.is_null()) {
Can you make a separate function for this check code?

https://codereview.chromium.org/12096096/diff/4005/chrome/browser/metrics/var...
File chrome/browser/metrics/variations/variations_service.h (right):

https://codereview.chromium.org/12096096/diff/4005/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.h:191: class
NetworkTimeService {
Put it in a separate file. "Service" is too ambitious a name for this, for now.

How about NetworkTimeTracker?

You can put the separate file in the same directory as VariationsService with a
TODO comment explaining the eventual plan.

https://codereview.chromium.org/12096096/diff/4005/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.h:193: NetworkTimeService()
{
When this is a file, add a dtor and don't inline their impls.

https://codereview.chromium.org/12096096/diff/4005/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.h:203: friend class
VariationsService;
Remove this and change the SetNetworkTime() call to UpdateNetworkTime(). You can
make it public, since only VariationsService has access to the instance.

[MAD, 2013-02-04 21:07:43]

Thanks for the feedback... PTAL...

BYE
MAD

https://codereview.chromium.org/12096096/diff/4005/chrome/browser/metrics/var...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/12096096/diff/4005/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.cc:589: base::Time*
network_time, base::TimeDelta* uncertainty) const {
On 2013/02/01 22:18:08, Alexei Svitkine wrote:
> Each param on a separate line.

Done.

https://codereview.chromium.org/12096096/diff/4005/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.cc:594: *network_time =
network_time_ + (base::TimeTicks::Now() -
On 2013/02/01 22:18:08, Alexei Svitkine wrote:
> Nit: No need for ()s.

Actually, we get a compile error without them:
 error C2679: binary '+' : no operator found which takes a right-hand operand of
type 'base::TimeTicks' (or there is no acceptable conversion)

https://codereview.chromium.org/12096096/diff/4005/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.cc:604: if
(!network_time_.is_null()) {
On 2013/02/01 22:18:08, Alexei Svitkine wrote:
> Can you make a separate function for this check code?

Done.

https://codereview.chromium.org/12096096/diff/4005/chrome/browser/metrics/var...
File chrome/browser/metrics/variations/variations_service.h (right):

https://codereview.chromium.org/12096096/diff/4005/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.h:191: class
NetworkTimeService {
On 2013/02/01 22:18:08, Alexei Svitkine wrote:
> Put it in a separate file. "Service" is too ambitious a name for this, for
now.
> 
> How about NetworkTimeTracker?
> 
> You can put the separate file in the same directory as VariationsService with
a
> TODO comment explaining the eventual plan.

Done.

https://codereview.chromium.org/12096096/diff/4005/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.h:193: NetworkTimeService()
{
On 2013/02/01 22:18:08, Alexei Svitkine wrote:
> When this is a file, add a dtor and don't inline their impls.

Done.

https://codereview.chromium.org/12096096/diff/4005/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.h:203: friend class
VariationsService;
On 2013/02/01 22:18:08, Alexei Svitkine wrote:
> Remove this and change the SetNetworkTime() call to UpdateNetworkTime(). You
can
> make it public, since only VariationsService has access to the instance.

Done.

[Alexei Svitkine (slow), 2013-02-04 21:23:18]

Looking good, just a few more nits and suggestions.

https://codereview.chromium.org/12096096/diff/20001/chrome/browser/metrics/va...
File chrome/browser/metrics/variations/network_time_tracker.cc (right):

https://codereview.chromium.org/12096096/diff/20001/chrome/browser/metrics/va...
chrome/browser/metrics/variations/network_time_tracker.cc:16: bool
ValidateTimeTracking(const NetworkTimeTracker& network_time_tracker,
If you're passing in the NetworkTimeTracker object already, might as well make
it a private function instead of a function in the anon namespace.

https://codereview.chromium.org/12096096/diff/20001/chrome/browser/metrics/va...
chrome/browser/metrics/variations/network_time_tracker.cc:64: // Update network
time on every request to limit dependency on ticks lag.
Nit: add a blank line before this to make it more readable.

https://codereview.chromium.org/12096096/diff/20001/chrome/browser/metrics/va...
chrome/browser/metrics/variations/network_time_tracker.cc:72: // We can't assume
a better time than the resolution of the given time
Nit: add a blank line before this to make it more readable.

https://codereview.chromium.org/12096096/diff/20001/chrome/browser/metrics/va...
File chrome/browser/metrics/variations/network_time_tracker.h (right):

https://codereview.chromium.org/12096096/diff/20001/chrome/browser/metrics/va...
chrome/browser/metrics/variations/network_time_tracker.h:10: class
NetworkTimeTracker {
Nit: Add a short comment.

https://codereview.chromium.org/12096096/diff/20001/chrome/browser/metrics/va...
chrome/browser/metrics/variations/network_time_tracker.h:31: // The estimated
ticks count when |network_time_| was set.
Mention that this is relative to system time.

https://codereview.chromium.org/12096096/diff/20001/chrome/browser/metrics/va...
File chrome/browser/metrics/variations/network_time_tracker_unittest.cc (right):

https://codereview.chromium.org/12096096/diff/20001/chrome/browser/metrics/va...
chrome/browser/metrics/variations/network_time_tracker_unittest.cc:32: }
Nit: Add "// namespace".

https://codereview.chromium.org/12096096/diff/20001/chrome/browser/metrics/va...
chrome/browser/metrics/variations/network_time_tracker_unittest.cc:51:
base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(kSleepTime1));
Instead of making the test sleep (which makes the test slow and bad for cycle
times, etc), how about mocking out base::TimeTicks::Now().

If there's no facility to do that already globally, just add a GetCurrentTime()
virtual method to the class and override it in the test.

Alternatively, you can add it as a parameter to the function APIs. Then, you can
make VariationsService pass base::Time::Now() to them, so that users getting the
time via VariationsService don't have to deal with it.

[MAD, 2013-02-05 01:05:55]

How about this now?

Thanks again!

BYE
MAD...

https://codereview.chromium.org/12096096/diff/20001/chrome/browser/metrics/va...
File chrome/browser/metrics/variations/network_time_tracker.cc (right):

https://codereview.chromium.org/12096096/diff/20001/chrome/browser/metrics/va...
chrome/browser/metrics/variations/network_time_tracker.cc:16: bool
ValidateTimeTracking(const NetworkTimeTracker& network_time_tracker,
On 2013/02/04 21:23:18, Alexei Svitkine wrote:
> If you're passing in the NetworkTimeTracker object already, might as well make
> it a private function instead of a function in the anon namespace.

D'Ho! This evolved from something else... Makes sense... :-)

https://codereview.chromium.org/12096096/diff/20001/chrome/browser/metrics/va...
chrome/browser/metrics/variations/network_time_tracker.cc:64: // Update network
time on every request to limit dependency on ticks lag.
On 2013/02/04 21:23:18, Alexei Svitkine wrote:
> Nit: add a blank line before this to make it more readable.

Done.

https://codereview.chromium.org/12096096/diff/20001/chrome/browser/metrics/va...
chrome/browser/metrics/variations/network_time_tracker.cc:72: // We can't assume
a better time than the resolution of the given time
On 2013/02/04 21:23:18, Alexei Svitkine wrote:
> Nit: add a blank line before this to make it more readable.

Done.

https://codereview.chromium.org/12096096/diff/20001/chrome/browser/metrics/va...
File chrome/browser/metrics/variations/network_time_tracker.h (right):

https://codereview.chromium.org/12096096/diff/20001/chrome/browser/metrics/va...
chrome/browser/metrics/variations/network_time_tracker.h:10: class
NetworkTimeTracker {
On 2013/02/04 21:23:18, Alexei Svitkine wrote:
> Nit: Add a short comment.

Done.

https://codereview.chromium.org/12096096/diff/20001/chrome/browser/metrics/va...
chrome/browser/metrics/variations/network_time_tracker.h:31: // The estimated
ticks count when |network_time_| was set.
On 2013/02/04 21:23:18, Alexei Svitkine wrote:
> Mention that this is relative to system time.

Done.

https://codereview.chromium.org/12096096/diff/20001/chrome/browser/metrics/va...
File chrome/browser/metrics/variations/network_time_tracker_unittest.cc (right):

https://codereview.chromium.org/12096096/diff/20001/chrome/browser/metrics/va...
chrome/browser/metrics/variations/network_time_tracker_unittest.cc:32: }
On 2013/02/04 21:23:18, Alexei Svitkine wrote:
> Nit: Add "// namespace".

Done.

https://codereview.chromium.org/12096096/diff/20001/chrome/browser/metrics/va...
chrome/browser/metrics/variations/network_time_tracker_unittest.cc:51:
base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(kSleepTime1));
On 2013/02/04 21:23:18, Alexei Svitkine wrote:
> Instead of making the test sleep (which makes the test slow and bad for cycle
> times, etc), how about mocking out base::TimeTicks::Now().
> 
> If there's no facility to do that already globally, just add a
GetCurrentTime()
> virtual method to the class and override it in the test.
> 
> Alternatively, you can add it as a parameter to the function APIs. Then, you
can
> make VariationsService pass base::Time::Now() to them, so that users getting
the
> time via VariationsService don't have to deal with it.

Done.

[Alexei Svitkine (slow), 2013-02-05 02:29:07]

LGTM with some final nits.

Since it's not a trivial change, can you make a corresponding crbug?

https://codereview.chromium.org/12096096/diff/40001/chrome/browser/metrics/va...
File chrome/browser/metrics/variations/network_time_tracker.h (right):

https://codereview.chromium.org/12096096/diff/40001/chrome/browser/metrics/va...
chrome/browser/metrics/variations/network_time_tracker.h:38: // Remember the
network time based on last call to UpdateNetworkTime().
Nit: blank line before this

https://codereview.chromium.org/12096096/diff/40001/chrome/browser/metrics/va...
File chrome/browser/metrics/variations/network_time_tracker_unittest.cc (right):

https://codereview.chromium.org/12096096/diff/40001/chrome/browser/metrics/va...
chrome/browser/metrics/variations/network_time_tracker_unittest.cc:23: class
NetworkTimeTrackerTest : public NetworkTimeTracker {
Nit: NetworkTimeTrackerTest -> TestNetworkTimeTracker

The NetworkTimeTrackerTest implies that it's an actual test case class (e.g.
extends an existing test class that has things like SetUp() and TearDown()
methods), whereas this is just a test version of the NetworkTimeTracker class.

https://codereview.chromium.org/12096096/diff/40001/chrome/browser/metrics/va...
chrome/browser/metrics/variations/network_time_tracker_unittest.cc:46: virtual
base::TimeTicks GetTicksNow() const { return ticks_now_; }
Nits:

- OVERRIDE
- put method body on a new line
- add a blank line before the variable decl

[MAD, 2013-02-05 15:56:00]

Thanks again!

https://codereview.chromium.org/12096096/diff/40001/chrome/browser/metrics/va...
File chrome/browser/metrics/variations/network_time_tracker.h (right):

https://codereview.chromium.org/12096096/diff/40001/chrome/browser/metrics/va...
chrome/browser/metrics/variations/network_time_tracker.h:38: // Remember the
network time based on last call to UpdateNetworkTime().
On 2013/02/05 02:29:07, Alexei Svitkine wrote:
> Nit: blank line before this

Done.

https://codereview.chromium.org/12096096/diff/40001/chrome/browser/metrics/va...
File chrome/browser/metrics/variations/network_time_tracker_unittest.cc (right):

https://codereview.chromium.org/12096096/diff/40001/chrome/browser/metrics/va...
chrome/browser/metrics/variations/network_time_tracker_unittest.cc:23: class
NetworkTimeTrackerTest : public NetworkTimeTracker {
On 2013/02/05 02:29:07, Alexei Svitkine wrote:
> Nit: NetworkTimeTrackerTest -> TestNetworkTimeTracker
> 
> The NetworkTimeTrackerTest implies that it's an actual test case class (e.g.
> extends an existing test class that has things like SetUp() and TearDown()
> methods), whereas this is just a test version of the NetworkTimeTracker class.

Done.

https://codereview.chromium.org/12096096/diff/40001/chrome/browser/metrics/va...
chrome/browser/metrics/variations/network_time_tracker_unittest.cc:46: virtual
base::TimeTicks GetTicksNow() const { return ticks_now_; }
On 2013/02/05 02:29:07, Alexei Svitkine wrote:
> Nits:
> 
> - OVERRIDE
> - put method body on a new line
> - add a blank line before the variable decl

Done.

[commit-bot: I haz the power, 2013-02-05 16:02:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mad@chromium.org/12096096/50001

[Lei Zhang, 2013-02-05 19:42:48]

.gyp* changes lgtm

[commit-bot: I haz the power, 2013-02-05 19:53:15]

Change committed as 180766