provide static popular sites for first run

components/ntp_tiles/popular_sites_impl_unittest.cc

Index: components/ntp_tiles/popular_sites_impl_unittest.cc
diff --git a/components/ntp_tiles/popular_sites_impl_unittest.cc b/components/ntp_tiles/popular_sites_impl_unittest.cc
index 86fbfde2fd26ec406224532cb39d0dd28a0c74e0..a9c88a253dc558cb669309f82351e788d24ae5d3 100644
--- a/components/ntp_tiles/popular_sites_impl_unittest.cc
+++ b/components/ntp_tiles/popular_sites_impl_unittest.cc
@@ -117,15 +117,12 @@ class PopularSitesTest : public ::testing::Test {
     scoped_refptr<net::TestURLRequestContextGetter> url_request_context(
         new net::TestURLRequestContextGetter(
             base::ThreadTaskRunnerHandle::Get()));
-    PopularSitesImpl popular_sites(worker_pool_owner_.pool().get(), &prefs_,
-                                   /*template_url_service=*/nullptr,
-                                   /*variations_service=*/nullptr,
-                                   url_request_context.get(), cache_dir_,
-                                   base::Bind(JsonUnsafeParser::Parse));
+    std::unique_ptr<PopularSites> popular_sites =
+        CreatePopularSites(url_request_context.get());
 
     base::RunLoop loop;
     base::Optional<bool> save_success;
-    if (popular_sites.MaybeStartFetch(
+    if (popular_sites->MaybeStartFetch(
             force_download, base::Bind(
                                 [](base::Optional<bool>* save_success,
                                    base::RunLoop* loop, bool success) {
@@ -135,10 +132,19 @@ class PopularSitesTest : public ::testing::Test {
                                 &save_success, &loop))) {
       loop.Run();
     }
-    *sites = popular_sites.sites();
+    *sites = popular_sites->sites();
     return save_success;
   }
 
+  std::unique_ptr<PopularSites> CreatePopularSites(
+      net::URLRequestContextGetter* context) {
+    return base::MakeUnique<PopularSitesImpl>(
+        worker_pool_owner_.pool().get(), &prefs_,
+        /*template_url_service=*/nullptr,
+        /*variations_service=*/nullptr, context, cache_dir_,
+        base::Bind(JsonUnsafeParser::Parse));
+  }
+
   const TestPopularSite kWikipedia;
   const TestPopularSite kYouTube;
   const TestPopularSite kChromium;
@@ -169,6 +175,15 @@ TEST_F(PopularSitesTest, Basic) {
   EXPECT_THAT(sites[0].favicon_url, URLEq(""));
 }
 
+TEST_F(PopularSitesTest, ContainsDefaultTilesRightAfterConstruction) {
+  scoped_refptr<net::TestURLRequestContextGetter> url_request_context(
+      new net::TestURLRequestContextGetter(
+          base::ThreadTaskRunnerHandle::Get()));
+
+  EXPECT_THAT(CreatePopularSites(url_request_context.get())->sites().size(),
+              Eq(8ul));
+}
+
 TEST_F(PopularSitesTest, Fallback) {
   SetCountryAndVersion("ZZ", "9");
   RespondWith404(
@@ -194,7 +209,7 @@ TEST_F(PopularSitesTest, Fallback) {
               URLEq("https://www.chromium.org/favicon.ico"));
 }
 
-TEST_F(PopularSitesTest, Failure) {
+TEST_F(PopularSitesTest, PopulatesWithDefaultResoucesOnFailure) {
   SetCountryAndVersion("ZZ", "9");
   RespondWith404(
       "https://www.gstatic.com/chrome/ntp/suggested_sites_ZZ_9.json");
@@ -204,7 +219,43 @@ TEST_F(PopularSitesTest, Failure) {
   PopularSites::SitesVector sites;
   EXPECT_THAT(FetchPopularSites(/*force_download=*/false, &sites),
               Eq(base::Optional<bool>(false)));
-  ASSERT_THAT(sites, IsEmpty());
+  EXPECT_THAT(sites.size(), Eq(8ul));
+}
+
+TEST_F(PopularSitesTest, ProvidesDefaultSitesUntilCallbackReturns) {
+  SetCountryAndVersion("ZZ", "9");
+  RespondWithJSON(
+      "https://www.gstatic.com/chrome/ntp/suggested_sites_ZZ_9.json",
+      {kWikipedia});
+  scoped_refptr<net::TestURLRequestContextGetter> url_request_context(
+      new net::TestURLRequestContextGetter(
+          base::ThreadTaskRunnerHandle::Get()));
+  std::unique_ptr<PopularSites> popular_sites =
+      CreatePopularSites(url_request_context.get());
+
+  base::RunLoop loop;
+  base::Optional<bool> save_success;
+
+  bool callback_was_scheduled = popular_sites->MaybeStartFetch(
+      /*force_download=*/true, base::Bind(
+                                   [](base::Optional<bool>* save_success,
+                                      base::RunLoop* loop, bool success) {
+                                     save_success->emplace(success);
+                                     loop->Quit();
+                                   },
+                                   &save_success, &loop));
+
+  // Assert that callback was scheduled so we can wait for its completion.
+  ASSERT_TRUE(callback_was_scheduled);
+  // There should be 8 default sites as nothing was fetched yet.
+  EXPECT_THAT(popular_sites->sites().size(), Eq(8ul));
+
+  loop.Run();  // Wait for the fetch to finish and the callback to return.
+
+  ASSERT_TRUE(save_success.has_value());

[sfiera, 2017/02/10 10:40:03]

I would just start with "bool save_success = false". Then, EXPECT_TRUE is
enough, and you don't need an ASSERT that would potentially block the erst of
the test body.

[fhorschig, 2017/02/13 10:34:13]

Done.

+  EXPECT_TRUE(save_success.value());
+  // The 1 fetched site should replace the default sites.
+  EXPECT_THAT(popular_sites->sites().size(), Eq(1ul));
 }
 
 TEST_F(PopularSitesTest, ClearsCacheFileFromOldVersions) {