Record NTP.LogoShownTime for timely refreshs only

components/doodle/doodle_service_unittest.cc

 // Copyright 2017 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/doodle/doodle_service.h"
 
 #include <memory>
 #include <utility>
 #include <vector>
 
@@ skipping 472 matching lines @@
   ASSERT_THAT(fetcher()->num_pending_callbacks(), Eq(0u));
 
   // The outcome should still have been recorded, but not the time - we only
   // record that for successful downloads.
   histograms.ExpectUniqueSample("Doodle.ConfigDownloadOutcome",
                                 7,  // OUTCOME_REFRESH_INTERVAL_NOT_PASSED
                                 1);
   histograms.ExpectTotalCount("Doodle.ConfigDownloadTime", 0);
 }
 
+TEST_F(DoodleServiceTest, RefreshReturnsFalseOnlyForSkippedRefreshAttempts) {
+  // Create a service with some refresh interval.
+  RecreateService(/*min_refresh_interval=*/base::TimeDelta::FromMinutes(10));
+
+  // Trigger a refresh within the min refresh interval that is not skipped.

[tschumann, 2017/04/20 18:53:11]

if find the last part of the sentence extremely confusing. I guess you're trying
to say that this actually results in a fetch? (also see next comment below).

[fhorschig, 2017/04/24 13:52:35]

Yes, changed/merged.

+  EXPECT_TRUE(service()->Refresh());
+  fetcher()->ServeAllCallbacks(DoodleState::NO_DOODLE, base::TimeDelta(),

[tschumann, 2017/04/20 18:53:11]

can you explain the parameters with /**/ comments? 
Alternatively extending the comment above to something like:
// Trigger a refresh that gets the information about no doodle being available.

[fhorschig, 2017/04/24 13:52:35]

Done.

+                               base::nullopt);
+
+  // Request a refresh before the min refresh interval has passed.
+  task_runner()->FastForwardBy(base::TimeDelta::FromMinutes(5));

[tschumann, 2017/04/20 18:53:11]

why forward the time at all?

[fhorschig, 2017/04/24 13:52:35]

Gone.

+  EXPECT_FALSE(service()->Refresh());
+
+  // This should not have resulted in a request.

[tschumann, 2017/04/20 18:53:11]

nit: combine this comment with the one above and write the 2 expectations in one
block?

[fhorschig, 2017/04/24 13:52:34]

Done.

+  EXPECT_THAT(fetcher()->num_pending_callbacks(), Eq(0u));
+}
+
 TEST_F(DoodleServiceTest, DoesNotRecordMetricsAtStartup) {
   // Creating the service should not emit any histogram samples.
   base::HistogramTester histograms;
   RecreateServiceWithZeroRefreshInterval();
   ASSERT_THAT(service()->config(), Eq(base::nullopt));
   histograms.ExpectTotalCount("Doodle.ConfigDownloadOutcome", 0);
   histograms.ExpectTotalCount("Doodle.ConfigDownloadTime", 0);
 }
 
 TEST_F(DoodleServiceTest, DoesNotRecordMetricsAtStartupWithConfig) {
@@ skipping 26 matching lines @@
   // Fast-forward time so that the config expires.
   task_runner()->FastForwardBy(base::TimeDelta::FromHours(1));
   ASSERT_THAT(service()->config(), Eq(base::nullopt));
 
   // This should not have resulted in any metrics being emitted.
   histograms.ExpectTotalCount("Doodle.ConfigDownloadOutcome", 0);
   histograms.ExpectTotalCount("Doodle.ConfigDownloadTime", 0);
 }
 
 }  // namespace doodle