Fix broken clockstate.network2 histogram and add unit test

Fix broken clockstate.network2 histogram and add unit test

The 'interstitial.clockstate.network2' histogram has been recording the
wrong enum, meaning that the reported values were mislabeled, and also
the enum that was actually being histogrammed has changed around
recently. Thus, this CL deprecates the bogus network2 histogram and
replaces it with the network3 histogram that records the correct enum. It
also adds a unit test to check that values for this histogram are properly
recorded.

BUG=589700
Committed: https://crrev.com/27f12b4b0b0c0ad0dd04667bb0dca00487438b62
Cr-Commit-Position: refs/heads/master@{#426035}

[estark, 2016-10-16 17:45:44]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-16 17:45:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estark, 2016-10-16 18:28:18]

Description was changed from

==========
Fix broken clockstate.network2 histogram and add unit test

The 'interstitial.clockstate.network2' histogram has been recording the
wrong enum, meaning that the reported values were mislabeled, and also
the enum that was actually being histogrammed has changed around over
time. Thus, this CL deprecates the network2 histogram and replaces it
with the network3 histogram that records the correct enum. It also adds
a unit test to check that values for this histogram are properly
recorded.

BUG=589700
==========

to

==========
Fix broken clockstate.network2 histogram and add unit test

The 'interstitial.clockstate.network2' histogram has been recording the
wrong enum, meaning that the reported values were mislabeled, and also
the enum that was actually being histogrammed has changed around
recently. Thus, this CL deprecates the network2 histogram and replaces it
with the network3 histogram that records the correct enum. It also adds
a unit test to check that values for this histogram are properly
recorded.

BUG=589700
==========

[estark, 2016-10-16 18:28:40]

Description was changed from

==========
Fix broken clockstate.network2 histogram and add unit test

The 'interstitial.clockstate.network2' histogram has been recording the
wrong enum, meaning that the reported values were mislabeled, and also
the enum that was actually being histogrammed has changed around
recently. Thus, this CL deprecates the network2 histogram and replaces it
with the network3 histogram that records the correct enum. It also adds
a unit test to check that values for this histogram are properly
recorded.

BUG=589700
==========

to

==========
Fix broken clockstate.network2 histogram and add unit test

The 'interstitial.clockstate.network2' histogram has been recording the
wrong enum, meaning that the reported values were mislabeled, and also
the enum that was actually being histogrammed has changed around
recently. Thus, this CL deprecates the bogus network2 histogram and
replaces it with the network3 histogram that records the correct enum. It
also adds a unit test to check that values for this histogram are properly
recorded.

BUG=589700
==========

[estark, 2016-10-16 18:32:14]

estark@chromium.org changed reviewers:
+ jwd@chromium.org, meacer@chromium.org

[estark, 2016-10-16 18:32:15]

jwd: please review histograms.xml
meacer: please review the rest
Thanks!

[commit-bot: I haz the power, 2016-10-16 18:32:30]

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

[commit-bot: I haz the power, 2016-10-16 18:32:31]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2421143002/diff/1/components/network_time/net...
File components/network_time/network_time_tracker.cc (right):

https://codereview.chromium.org/2421143002/diff/1/components/network_time/net...
components/network_time/network_time_tracker.cc:508:
!enable_time_queries_for_testing_) {
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?

https://codereview.chromium.org/2421143002/diff/1/components/ssl_errors/error...
File components/ssl_errors/error_classification_unittest.cc (right):

https://codereview.chromium.org/2421143002/diff/1/components/ssl_errors/error...
components/ssl_errors/error_classification_unittest.cc:336:
base::SimpleTestTickClock* tick_clock = new base::SimpleTestTickClock;
nit: Make this and clock unique_ptr here and std::move below.

Also fyi, new A isn't always equivalent to new A(). It's fine here since the
class has a user defined constructor.

https://codereview.chromium.org/2421143002/diff/1/components/ssl_errors/error...
components/ssl_errors/error_classification_unittest.cc:342: base::MessageLoop
loop;
Is this necessary? The test runs fine without it.

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

https://codereview.chromium.org/2421143002/diff/1/components/network_time/net...
File components/network_time/network_time_tracker.cc (right):

https://codereview.chromium.org/2421143002/diff/1/components/network_time/net...
components/network_time/network_time_tracker.cc:508:
!enable_time_queries_for_testing_) {
On 2016/10/17 21:35:20, Mustafa Emre Acer wrote:
> 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?

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?

https://codereview.chromium.org/2421143002/diff/1/components/ssl_errors/error...
File components/ssl_errors/error_classification_unittest.cc (right):

https://codereview.chromium.org/2421143002/diff/1/components/ssl_errors/error...
components/ssl_errors/error_classification_unittest.cc:336:
base::SimpleTestTickClock* tick_clock = new base::SimpleTestTickClock;
On 2016/10/17 21:35:20, Mustafa Emre Acer wrote:
> nit: Make this and clock unique_ptr here and std::move below.

I need a raw pointer below to advance the clocks and such. Are you suggesting
make them unique_ptrs and save raw pointers before std::move'ing them?

> 
> Also fyi, new A isn't always equivalent to new A(). It's fine here since the
> class has a user defined constructor.

Oh, thanks, I didn't know that.

https://codereview.chromium.org/2421143002/diff/1/components/ssl_errors/error...
components/ssl_errors/error_classification_unittest.cc:342: base::MessageLoop
loop;
On 2016/10/17 21:35:20, Mustafa Emre Acer wrote:
> Is this necessary? The test runs fine without it.

Alas, it hits a DCHECK without it:

[ RUN      ] SSLErrorClassificationTest.NetworkClockStateHistogram
[28318:28318:1017/144457:6940148020845:FATAL:thread_task_runner_handle.cc(26)]
Check failed: current. 
#0 0x7febd04ca79e base::debug::StackTrace::StackTrace()
#1 0x7febd04eca1b logging::LogMessage::~LogMessage()
#2 0x7febd0561666 base::ThreadTaskRunnerHandle::Get()
#3 0x7febd056562c base::Timer::PostNewScheduledTask()
#4 0x7febd056514b base::Timer::Reset()
#5 0x0000027ed1a1 network_time::NetworkTimeTracker::NetworkTimeTracker()
#6 0x0000014df44d
SSLErrorClassificationTest_NetworkClockStateHistogram_Test::TestBody()
#7 0x0000021e0547 testing::Test::Run()
#8 0x0000021e1143 testing::TestInfo::Run()
#9 0x0000021e15d7 testing::TestCase::Run()
#10 0x0000021e8797 testing::internal::UnitTestImpl::RunAllTests()
#11 0x0000021e83ea testing::UnitTest::Run()
#12 0x0000021b8ca1 base::TestSuite::Run()
#13 0x0000021bb289 base::(anonymous namespace)::LaunchUnitTestsInternal()
#14 0x0000021bb154 base::LaunchUnitTests()
#15 0x000000706a83 main
#16 0x7febc6b54f45 __libc_start_main
#17 0x0000007068e5 <unknown>

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

Lgtm % enable_time_queries_for_testing_ question

https://codereview.chromium.org/2421143002/diff/1/components/network_time/net...
File components/network_time/network_time_tracker.cc (right):

https://codereview.chromium.org/2421143002/diff/1/components/network_time/net...
components/network_time/network_time_tracker.cc:309:
enable_time_queries_for_testing_ = true;
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.

https://codereview.chromium.org/2421143002/diff/1/components/network_time/net...
components/network_time/network_time_tracker.cc:508:
!enable_time_queries_for_testing_) {
On 2016/10/17 21:45:37, estark wrote:
> On 2016/10/17 21:35:20, Mustafa Emre Acer wrote:
> > 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?
> 
> 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?

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)

https://codereview.chromium.org/2421143002/diff/1/components/ssl_errors/error...
File components/ssl_errors/error_classification_unittest.cc (right):

https://codereview.chromium.org/2421143002/diff/1/components/ssl_errors/error...
components/ssl_errors/error_classification_unittest.cc:336:
base::SimpleTestTickClock* tick_clock = new base::SimpleTestTickClock;
On 2016/10/17 21:45:37, estark wrote:
> On 2016/10/17 21:35:20, Mustafa Emre Acer wrote:
> > nit: Make this and clock unique_ptr here and std::move below.
> 
> I need a raw pointer below to advance the clocks and such. Are you suggesting
> make them unique_ptrs and save raw pointers before std::move'ing them?

Ah, I only saw the usages until NetworkTimeTracker constructor and missed the
rest. Please ignore.

> 
> > 
> > Also fyi, new A isn't always equivalent to new A(). It's fine here since the
> > class has a user defined constructor.
> 
> Oh, thanks, I didn't know that.

https://codereview.chromium.org/2421143002/diff/1/components/ssl_errors/error...
components/ssl_errors/error_classification_unittest.cc:342: base::MessageLoop
loop;
On 2016/10/17 21:45:37, estark wrote:
> On 2016/10/17 21:35:20, Mustafa Emre Acer wrote:
> > Is this necessary? The test runs fine without it.
> 
> Alas, it hits a DCHECK without it:
> 
> [ RUN      ] SSLErrorClassificationTest.NetworkClockStateHistogram
> [28318:28318:1017/144457:6940148020845:FATAL:thread_task_runner_handle.cc(26)]
> Check failed: current. 
> #0 0x7febd04ca79e base::debug::StackTrace::StackTrace()
> #1 0x7febd04eca1b logging::LogMessage::~LogMessage()
> #2 0x7febd0561666 base::ThreadTaskRunnerHandle::Get()
> #3 0x7febd056562c base::Timer::PostNewScheduledTask()
> #4 0x7febd056514b base::Timer::Reset()
> #5 0x0000027ed1a1 network_time::NetworkTimeTracker::NetworkTimeTracker()
> #6 0x0000014df44d
> SSLErrorClassificationTest_NetworkClockStateHistogram_Test::TestBody()
> #7 0x0000021e0547 testing::Test::Run()
> #8 0x0000021e1143 testing::TestInfo::Run()
> #9 0x0000021e15d7 testing::TestCase::Run()
> #10 0x0000021e8797 testing::internal::UnitTestImpl::RunAllTests()
> #11 0x0000021e83ea testing::UnitTest::Run()
> #12 0x0000021b8ca1 base::TestSuite::Run()
> #13 0x0000021bb289 base::(anonymous namespace)::LaunchUnitTestsInternal()
> #14 0x0000021bb154 base::LaunchUnitTests()
> #15 0x000000706a83 main
> #16 0x7febc6b54f45 __libc_start_main
> #17 0x0000007068e5 <unknown>

I patched the CL and ran the test in debug and it worked fine the first time,
but now it hits the check. Well. /facepalm

[estark, 2016-10-18 03:57:58]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 03:58:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2421143002/diff/1/components/network_time/net...
File components/network_time/network_time_tracker.cc (right):

https://codereview.chromium.org/2421143002/diff/1/components/network_time/net...
components/network_time/network_time_tracker.cc:508:
!enable_time_queries_for_testing_) {
On 2016/10/17 23:45:48, Mustafa Emre Acer wrote:
> On 2016/10/17 21:45:37, estark wrote:
> > On 2016/10/17 21:35:20, Mustafa Emre Acer wrote:
> > > 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?
> > 
> > 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?
> 
> 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)

I like that idea, done.

[commit-bot: I haz the power, 2016-10-18 04:48:04]

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

[commit-bot: I haz the power, 2016-10-18 04:48:05]

Dry run: This issue passed the CQ dry run.

[estark, 2016-10-18 18:47:15]

Friendly ping to jwd

[meacer, 2016-10-18 19:48:51]

LGTM again, thanks for making the util change.

[jwd, 2016-10-18 20:16:08]

lgtm

[estark, 2016-10-18 20:20:03]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2016-10-18 20:20:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-18 20:30:47]

Description was changed from

==========
Fix broken clockstate.network2 histogram and add unit test

The 'interstitial.clockstate.network2' histogram has been recording the
wrong enum, meaning that the reported values were mislabeled, and also
the enum that was actually being histogrammed has changed around
recently. Thus, this CL deprecates the bogus network2 histogram and
replaces it with the network3 histogram that records the correct enum. It
also adds a unit test to check that values for this histogram are properly
recorded.

BUG=589700
==========

to

==========
Fix broken clockstate.network2 histogram and add unit test

The 'interstitial.clockstate.network2' histogram has been recording the
wrong enum, meaning that the reported values were mislabeled, and also
the enum that was actually being histogrammed has changed around
recently. Thus, this CL deprecates the bogus network2 histogram and
replaces it with the network3 histogram that records the correct enum. It
also adds a unit test to check that values for this histogram are properly
recorded.

BUG=589700
==========

[commit-bot: I haz the power, 2016-10-18 20:30:48]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-21 13:02:44]

Description was changed from

==========
Fix broken clockstate.network2 histogram and add unit test

The 'interstitial.clockstate.network2' histogram has been recording the
wrong enum, meaning that the reported values were mislabeled, and also
the enum that was actually being histogrammed has changed around
recently. Thus, this CL deprecates the bogus network2 histogram and
replaces it with the network3 histogram that records the correct enum. It
also adds a unit test to check that values for this histogram are properly
recorded.

BUG=589700
==========

to

==========
Fix broken clockstate.network2 histogram and add unit test

The 'interstitial.clockstate.network2' histogram has been recording the
wrong enum, meaning that the reported values were mislabeled, and also
the enum that was actually being histogrammed has changed around
recently. Thus, this CL deprecates the bogus network2 histogram and
replaces it with the network3 histogram that records the correct enum. It
also adds a unit test to check that values for this histogram are properly
recorded.

BUG=589700

Committed: https://crrev.com/27f12b4b0b0c0ad0dd04667bb0dca00487438b62
Cr-Commit-Position: refs/heads/master@{#426035}
==========

[commit-bot: I haz the power, 2016-10-21 13:02:45]

Patchset 2 (id:??) landed as
https://crrev.com/27f12b4b0b0c0ad0dd04667bb0dca00487438b62
Cr-Commit-Position: refs/heads/master@{#426035}