Add OfflinePages.DownloadUI.PrefetchedItemHasThumbnail UMA

This will help us understand how many articles show
up with a thumbnail in downloads home.

Bug: 794828
Change-Id: I4576620f877147e0d897e29af11186150ddcf816
Reviewed-on: https://chromium-review.googlesource.com/1024639Reviewed-by: Carlos Knippschild <carlosk@chromium.org>
Reviewed-by: Brian White <bcwhite@chromium.org>
Commit-Queue: Dan H <harringtond@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553684}

components/offline_pages/core/downloads/download_ui_adapter.cc

@@ -8,6 +8,7 @@
 #include "base/bind_helpers.h"
 #include "base/guid.h"
 #include "base/logging.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/trace_event/trace_event.h"
 #include "components/offline_pages/core/background/request_coordinator.h"
@@ -262,39 +263,53 @@ void DownloadUIAdapter::GetVisualsForItem(
     return;
   }
   const ItemInfo* item = it->second.get();
+
+  VisualResultCallback callback = base::BindOnce(visuals_callback, id);
+  if (item->client_id.name_space == kSuggestedArticlesNamespace) {
+    // Report PrefetchedItemHasThumbnail along with result callback.
+    auto report_and_callback =
+        [](VisualResultCallback result_callback,
+           std::unique_ptr<offline_items_collection::OfflineItemVisuals>
+               visuals) {
+          UMA_HISTOGRAM_BOOLEAN(
+              "OfflinePages.DownloadUI.PrefetchedItemHasThumbnail",
+              visuals.get() != nullptr);
+          std::move(result_callback).Run(std::move(visuals));
+        };
+    callback = base::BindOnce(report_and_callback, std::move(callback));
+  }
+
   model_->GetThumbnailByOfflineId(
       item->offline_id,
       base::BindOnce(&DownloadUIAdapter::OnThumbnailLoaded,
-                     weak_ptr_factory_.GetWeakPtr(), id, visuals_callback));
+                     weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
 }
 
 void DownloadUIAdapter::OnThumbnailLoaded(
-    const ContentId& content_id,
-    const VisualsCallback& visuals_callback,
+    VisualResultCallback callback,
     std::unique_ptr<OfflinePageThumbnail> thumbnail) {
   DCHECK(thumbnail_decoder_);
   if (!thumbnail || thumbnail->thumbnail.empty()) {
     // PostTask not required, GetThumbnailByOfflineId does it for us.
-    visuals_callback.Run(content_id, nullptr);
+    std::move(callback).Run(nullptr);
     return;
   }
 
-  auto forward_visuals_lambda = [](const ContentId& content_id,
-                                   const VisualsCallback& visuals_callback,
+  auto forward_visuals_lambda = [](VisualResultCallback callback,
                                    const gfx::Image& image) {
     if (image.IsEmpty()) {
-      visuals_callback.Run(content_id, nullptr);
+      std::move(callback).Run(nullptr);
       return;
     }
     auto visuals =
         std::make_unique<offline_items_collection::OfflineItemVisuals>();
     visuals->icon = image;
-    visuals_callback.Run(content_id, std::move(visuals));
+    std::move(callback).Run(std::move(visuals));
   };
 
   thumbnail_decoder_->DecodeAndCropThumbnail(
       thumbnail->thumbnail,
-      base::BindOnce(forward_visuals_lambda, content_id, visuals_callback));
+      base::BindOnce(forward_visuals_lambda, std::move(callback)));
 }
 
 void DownloadUIAdapter::ThumbnailAdded(OfflinePageModel* model,

components/offline_pages/core/downloads/download_ui_adapter.h

@@ -82,7 +82,7 @@ class DownloadUIAdapter : public OfflineContentProvider,
 
   int64_t GetOfflineIdByGuid(const std::string& guid) const;
 
-  // OfflineContentProvider implmentation.
+  // OfflineContentProvider implementation.
   void OpenItem(const ContentId& id) override;
   void RemoveItem(const ContentId& id) override;
   void CancelDownload(const ContentId& id) override;
@@ -159,6 +159,8 @@ class DownloadUIAdapter : public OfflineContentProvider,
   };
 
   typedef std::map<std::string, std::unique_ptr<ItemInfo>> OfflineItems;
+  using VisualResultCallback = base::OnceCallback<void(
+      std::unique_ptr<offline_items_collection::OfflineItemVisuals>)>;
 
   void LoadCache();
   void ClearCache();
@@ -174,9 +176,8 @@ class DownloadUIAdapter : public OfflineContentProvider,
       const std::string& guid,
       std::vector<std::unique_ptr<SavePageRequest>> requests);
   void OnOfflinePagesLoaded(const MultipleOfflinePageItemResult& pages);
-  void OnThumbnailLoaded(const ContentId& content_id,
-                         const VisualsCallback& visuals_callback,
-                         std::unique_ptr<OfflinePageThumbnail>);
+  void OnThumbnailLoaded(VisualResultCallback callback,
+                         std::unique_ptr<OfflinePageThumbnail> thumbnail);
   void OnRequestsLoaded(std::vector<std::unique_ptr<SavePageRequest>> requests);
 
   void OnDeletePagesDone(DeletePageResult result);

components/offline_pages/core/downloads/download_ui_adapter_unittest.cc

@@ -19,6 +19,7 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/bind_test_util.h"
+#include "base/test/histogram_tester.h"
 #include "base/test/mock_callback.h"
 #include "base/test/test_mock_time_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
@@ -54,6 +55,8 @@ static const ClientId kTestClientIdOtherNamespace(kLastNNamespace, kTestGuid1);
 static const ClientId kTestClientIdOtherGuid(kLastNNamespace, kTestBadGuid);
 static const ClientId kTestClientId1(kAsyncNamespace, kTestGuid1);
 static const ClientId kTestClientId2(kAsyncNamespace, kTestGuid2);
+static const ClientId kTestClientIdPrefetch(kSuggestedArticlesNamespace,
+                                            kTestGuid1);
 static const ContentId kTestContentId1(kOfflinePageNamespace, kTestGuid1);
 static const base::FilePath kTestFilePath =
     base::FilePath(FILE_PATH_LITERAL("foo/bar.mhtml"));
@@ -115,8 +118,8 @@ class MockOfflinePageModel : public StubOfflinePageModel {
 
   ~MockOfflinePageModel() override {}
 
-  void AddInitialPage() {
-    OfflinePageItem page(GURL(kTestUrl), kTestOfflineId1, kTestClientId1,
+  void AddInitialPage(ClientId client_id) {
+    OfflinePageItem page(GURL(kTestUrl), kTestOfflineId1, client_id,
                          kTestFilePath, kFileSize, kTestCreationTime);
     page.title = kTestTitle;
     pages[kTestOfflineId1] = page;
@@ -230,7 +233,7 @@ class DownloadUIAdapterTest : public testing::Test,
     return request_coordinator_taco_->request_coordinator();
   }
   bool items_loaded() { return adapter->IsCacheLoadedForTest(); }
-  void AddInitialPage();
+  void AddInitialPage(const ClientId client_id);
   int64_t AddInitialRequest(const GURL& url, const ClientId& client_id);
 
   std::vector<std::string> added_guids, updated_guids, deleted_guids;
@@ -307,8 +310,9 @@ int64_t DownloadUIAdapterTest::AddRequest(const GURL& url,
       params, base::Bind(&SavePageLaterCallback));
 }
 
-void DownloadUIAdapterTest::AddInitialPage() {
-  model->AddInitialPage();
+void DownloadUIAdapterTest::AddInitialPage(
+    const ClientId client_id = kTestClientId1) {
+  model->AddInitialPage(client_id);
   // Trigger cache load in the adapter.
   adapter->GetAllItems(base::DoNothing());
   PumpLoop();
@@ -626,7 +630,7 @@ TEST_F(DownloadUIAdapterTest, UpdateProgress) {
 }
 
 TEST_F(DownloadUIAdapterTest, GetVisualsForItem) {
-  AddInitialPage();
+  AddInitialPage(kTestClientIdPrefetch);
   model->thumbnail_by_offline_id_result =
       std::make_unique<OfflinePageThumbnail>(kThumbnail);
   const int kImageWidth = 24;
@@ -641,19 +645,25 @@ TEST_F(DownloadUIAdapterTest, GetVisualsForItem) {
       [&](const offline_items_collection::ContentId& id,
           std::unique_ptr<offline_items_collection::OfflineItemVisuals>
               visuals) {
+        EXPECT_TRUE(visuals);
         EXPECT_EQ(kImageWidth, visuals->icon.Width());
         called = true;
       });
   adapter->GetAllItems(base::DoNothing());
+  base::HistogramTester histogram_tester;
+
   adapter->GetVisualsForItem(kTestContentId1, callback);
   PumpLoop();
+
+  histogram_tester.ExpectUniqueSample(
+      "OfflinePages.DownloadUI.PrefetchedItemHasThumbnail", true, 1);
   EXPECT_TRUE(called);
 }
 
 TEST_F(DownloadUIAdapterTest, GetVisualsForItemInvalidItem) {
   EXPECT_CALL(*thumbnail_decoder, DecodeAndCropThumbnail_(kThumbnailData, _))
       .Times(0);
-  AddInitialPage();
+  AddInitialPage(kTestClientIdPrefetch);
   const ContentId kContentID("not", "valid");
   bool called = false;
   auto callback = base::BindLambdaForTesting(
@@ -665,13 +675,18 @@ TEST_F(DownloadUIAdapterTest, GetVisualsForItemInvalidItem) {
         called = true;
       });
   adapter->GetAllItems(base::DoNothing());
+  base::HistogramTester histogram_tester;
+
   adapter->GetVisualsForItem(kContentID, callback);
   PumpLoop();
+
+  histogram_tester.ExpectTotalCount(
+      "OfflinePages.DownloadUI.PrefetchedItemHasThumbnail", 0);
   EXPECT_TRUE(called);
 }
 
 TEST_F(DownloadUIAdapterTest, GetVisualsForItemNoThumbnail) {
-  AddInitialPage();
+  AddInitialPage(kTestClientIdPrefetch);
   model->thumbnail_by_offline_id_result = nullptr;
   EXPECT_CALL(*thumbnail_decoder, DecodeAndCropThumbnail_(_, _)).Times(0);
   bool called = false;
@@ -684,13 +699,18 @@ TEST_F(DownloadUIAdapterTest, GetVisualsForItemNoThumbnail) {
         called = true;
       });
   adapter->GetAllItems(base::DoNothing());
+  base::HistogramTester histogram_tester;
+
   adapter->GetVisualsForItem(kTestContentId1, callback);
   PumpLoop();
+
+  histogram_tester.ExpectUniqueSample(
+      "OfflinePages.DownloadUI.PrefetchedItemHasThumbnail", false, 1);
   EXPECT_TRUE(called);
 }
 
 TEST_F(DownloadUIAdapterTest, GetVisualsForItemBadDecode) {
-  AddInitialPage();
+  AddInitialPage(kTestClientIdPrefetch);
   model->thumbnail_by_offline_id_result =
       std::make_unique<OfflinePageThumbnail>(kThumbnail);
   EXPECT_CALL(*thumbnail_decoder, DecodeAndCropThumbnail_(kThumbnailData, _))
@@ -708,8 +728,13 @@ TEST_F(DownloadUIAdapterTest, GetVisualsForItemBadDecode) {
         called = true;
       });
   adapter->GetAllItems(base::DoNothing());
+  base::HistogramTester histogram_tester;
+
   adapter->GetVisualsForItem(kTestContentId1, callback);
   PumpLoop();
+
+  histogram_tester.ExpectUniqueSample(
+      "OfflinePages.DownloadUI.PrefetchedItemHasThumbnail", false, 1);
   EXPECT_TRUE(called);
 }
 

tools/metrics/histograms/histograms.xml

@@ -58804,6 +58804,15 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   </summary>
 </histogram>
 
+<histogram name="OfflinePages.DownloadUI.PrefetchedItemHasThumbnail"
+    units="BooleanAvailable">
+  <owner>harringtond@chromium.org</owner>
+  <summary>
+    Whether or not a thumbnail was provided for a prefetched offline article.
+    Recorded when the item is shown in Downloads Home.
+  </summary>
+</histogram>
+
 <histogram name="OfflinePages.Edit.BookmarkUrlChangedForOfflinePage"
     enum="BooleanMatched">
   <obsolete>