Fix broken clockstate.network2 histogram and add unit test

components/network_time/network_time_tracker.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/network_time/network_time_tracker.h"
 
 #include <stdint.h>
 #include <string>
 #include <utility>
 
@@ skipping 287 matching lines @@
   run_loop_for_testing_ = &run_loop;
   run_loop.Run();
   run_loop_for_testing_ = nullptr;
 }
 
 base::TimeDelta NetworkTimeTracker::GetTimerDelayForTesting() const {
   DCHECK(timer_.IsRunning());
   return timer_.GetCurrentDelay();
 }
 
+void NetworkTimeTracker::EnableTimeQueriesForTesting() {
+  enable_time_queries_for_testing_ = true;

[meacer, 2016/10/17 23:45:48]

If you want to keep the bool as is, I feel like
force_enable_time_queries_for_testing_ is slightly more descriptive. The current
name implies that time queries is always disabled in testing.

+}
+
 NetworkTimeTracker::NetworkTimeResult NetworkTimeTracker::GetNetworkTime(
     base::Time* network_time,
     base::TimeDelta* uncertainty) const {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(network_time);
   if (network_time_at_last_measurement_.is_null()) {
     if (time_query_completed_) {
       // Time query attempts have been made in the past and failed.
       if (time_fetcher_) {
         // A fetch (not the first attempt) is in progress.
@@ skipping 175 matching lines @@
   if (run_loop_for_testing_ != nullptr)
     run_loop_for_testing_->QuitWhenIdle();
 }
 
 void NetworkTimeTracker::QueueCheckTime(base::TimeDelta delay) {
   timer_.Start(FROM_HERE, delay, this, &NetworkTimeTracker::CheckTime);
 }
 
 bool NetworkTimeTracker::ShouldIssueTimeQuery() {
   // Do not query the time service if not enabled via Variations Service.
-  if (!base::FeatureList::IsEnabled(kNetworkTimeServiceQuerying)) {
+  if (!base::FeatureList::IsEnabled(kNetworkTimeServiceQuerying) &&
+      !enable_time_queries_for_testing_) {

[meacer, 2016/10/17 21:35:20]

Instead of adding a bool, can the test force enable the feature using
base::FieldTrialList::CreateFieldTrial? Or is that only for field trials and not
features?

[estark, 2016/10/17 21:45:37]

It's possible, but it's takes an annoying large amount of code that I didn't
want to copy-paste:
https://cs.chromium.org/chromium/src/components/network_time/network_time_tra...

WDYT? Better to use that code?

[meacer, 2016/10/17 23:45:48]

Unless you want to refactor that code, I think this is fine.

Otherwise, it would probably make sense to put that code into
network_time_tracker_test_utils.h/cc and then call it from both sides. Hmm, now
that I think about it, maybe we should do this so that we can avoid adding
another bool here? (larger CL, but makes it marginally clearer to read this
class)

[estark, 2016/10/18 03:58:37]

I like that idea, done.

     return false;
   }
 
   // If GetNetworkTime() does not return NETWORK_TIME_AVAILABLE,
   // synchronization has been lost and a query is needed.
   base::Time network_time;
   if (GetNetworkTime(&network_time, nullptr) != NETWORK_TIME_AVAILABLE) {
     return true;
   }
 
   // Otherwise, make the decision at random.
   return base::RandDouble() < RandomQueryProbability();
 }
 
 }  // namespace network_time