WebApps: Replace install_source with Histogram enum in WebAppDataRetriever::GetIcons()

This is a simple refactor CL that replaces the WebappInstallSource
parameter in WebAppDataRetriever with WebAppIconDownloader::Histogram.
This removes the strong dependency on install_source in preparation for update
install tasks that don't have an install_source:
https://chromium-review.googlesource.com/c/chromium/src/+/1811402

Bug: 926083
Change-Id: I239d5094f545a5d86d2d282ed53185de935caa22
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1837573
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Alexey Baskakov <loyso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#705851}

chrome/browser/web_applications/components/web_app_data_retriever.cc

@@ -15,8 +15,6 @@
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/installable/installable_data.h"
 #include "chrome/browser/installable/installable_manager.h"
-#include "chrome/browser/installable/installable_metrics.h"
-#include "chrome/browser/web_applications/components/web_app_icon_downloader.h"
 #include "chrome/browser/web_applications/components/web_app_icon_generator.h"
 #include "chrome/common/chrome_render_frame.mojom.h"
 #include "chrome/common/web_application_info.h"
@@ -101,7 +99,7 @@ void WebAppDataRetriever::CheckInstallabilityAndRetrieveManifest(
 void WebAppDataRetriever::GetIcons(content::WebContents* web_contents,
                                    const std::vector<GURL>& icon_urls,
                                    bool skip_page_favicons,
-                                   WebappInstallSource install_source,
+                                   WebAppIconDownloader::Histogram histogram,
                                    GetIconsCallback callback) {
   Observe(web_contents);
 
@@ -109,14 +107,9 @@ void WebAppDataRetriever::GetIcons(content::WebContents* web_contents,
   CHECK(!get_icons_callback_);
   get_icons_callback_ = std::move(callback);
 
-  const char* https_status_code_class_histogram_name =
-      install_source == WebappInstallSource::SYNC
-          ? "WebApp.Icon.HttpStatusCodeClassOnSync"
-          : "WebApp.Icon.HttpStatusCodeClassOnCreate";
-
   // TODO(loyso): Refactor WebAppIconDownloader: crbug.com/907296.
   icon_downloader_ = std::make_unique<WebAppIconDownloader>(
-      web_contents, icon_urls, https_status_code_class_histogram_name,
+      web_contents, icon_urls, histogram,
       base::BindOnce(&WebAppDataRetriever::OnIconsDownloaded,
                      weak_ptr_factory_.GetWeakPtr()));
 

chrome/browser/web_applications/components/web_app_data_retriever.h

@@ -12,13 +12,13 @@
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "chrome/browser/web_applications/components/web_app_icon_downloader.h"
 #include "chrome/browser/web_applications/components/web_app_install_utils.h"
 #include "chrome/common/chrome_render_frame.mojom.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "mojo/public/cpp/bindings/associated_remote.h"
 
 class GURL;
-enum class WebappInstallSource;
 struct InstallableData;
 struct WebApplicationInfo;
 
@@ -32,8 +32,6 @@ class WebContents;
 
 namespace web_app {
 
-class WebAppIconDownloader;
-
 // Class used by WebAppInstallTask to retrieve the necessary information to
 // install an app. Should only be called from the UI thread.
 class WebAppDataRetriever : content::WebContentsObserver {
@@ -68,7 +66,7 @@ class WebAppDataRetriever : content::WebContentsObserver {
   virtual void GetIcons(content::WebContents* web_contents,
                         const std::vector<GURL>& icon_urls,
                         bool skip_page_favicons,
-                        WebappInstallSource install_source,
+                        WebAppIconDownloader::Histogram histogram,
                         GetIconsCallback callback);
 
   // WebContentsObserver:

chrome/browser/web_applications/components/web_app_data_retriever_unittest.cc

@@ -323,12 +323,11 @@ TEST_F(WebAppDataRetrieverTest, GetIcons_WebContentsDestroyed) {
 
   const std::vector<GURL> icon_urls;
   bool skip_page_favicons = true;
-  auto install_source = WebappInstallSource::MENU_BROWSER_TAB;
 
   base::RunLoop run_loop;
   WebAppDataRetriever retriever;
   retriever.GetIcons(web_contents(), icon_urls, skip_page_favicons,
-                     install_source,
+                     WebAppIconDownloader::Histogram::kForCreate,
                      base::BindLambdaForTesting([&](IconsMap icons_map) {
                        EXPECT_TRUE(icons_map.empty());
                        run_loop.Quit();

chrome/browser/web_applications/components/web_app_icon_downloader.cc

@@ -20,14 +20,13 @@ namespace web_app {
 WebAppIconDownloader::WebAppIconDownloader(
     content::WebContents* web_contents,
     const std::vector<GURL>& extra_favicon_urls,
-    const char* https_status_code_class_histogram_name,
+    Histogram histogram,
     WebAppIconDownloaderCallback callback)
     : content::WebContentsObserver(web_contents),
       need_favicon_urls_(true),
       extra_favicon_urls_(extra_favicon_urls),
       callback_(std::move(callback)),
-      https_status_code_class_histogram_name_(
-          https_status_code_class_histogram_name) {}
+      histogram_(histogram) {}
 
 WebAppIconDownloader::~WebAppIconDownloader() {}
 
@@ -113,12 +112,19 @@ void WebAppIconDownloader::DidDownloadFavicon(
   if (in_progress_requests_.erase(id) == 0)
     return;
 
-  if (!https_status_code_class_histogram_name_.empty() &&
-      http_status_code != 0) {
+  if (http_status_code != 0) {
+    const char* histogram_name = nullptr;
+    switch (histogram_) {
+      case Histogram::kForCreate:
+        histogram_name = "WebApp.Icon.HttpStatusCodeClassOnCreate";
+        break;
+      case Histogram::kForSync:
+        histogram_name = "WebApp.Icon.HttpStatusCodeClassOnSync";
+        break;
+    }
     DCHECK_LE(100, http_status_code);
     DCHECK_GT(600, http_status_code);
-    base::UmaHistogramExactLinear(https_status_code_class_histogram_name_,
-                                  http_status_code / 100, 5);
+    base::UmaHistogramExactLinear(histogram_name, http_status_code / 100, 5);
   }
 
   icons_map_[image_url] = bitmaps;

chrome/browser/web_applications/components/web_app_icon_downloader.h

@@ -32,6 +32,11 @@ namespace web_app {
 // icons) for a tab.
 class WebAppIconDownloader : public content::WebContentsObserver {
  public:
+  enum class Histogram {
+    kForCreate,
+    kForSync,
+  };
+
   using WebAppIconDownloaderCallback =
       base::OnceCallback<void(bool success, IconsMap icons_map)>;
 
@@ -41,7 +46,7 @@ class WebAppIconDownloader : public content::WebContentsObserver {
   // to use for logging http status code class results from fetch attempts.
   WebAppIconDownloader(content::WebContents* web_contents,
                        const std::vector<GURL>& extra_favicon_urls,
-                       const char* https_status_code_class_histogram_name,
+                       Histogram histogram,
                        WebAppIconDownloaderCallback callback);
   ~WebAppIconDownloader() override;
 
@@ -102,8 +107,8 @@ class WebAppIconDownloader : public content::WebContentsObserver {
   // Callback to run on favicon download completion.
   WebAppIconDownloaderCallback callback_;
 
-  // The histogram name to log individual fetch results under.
-  const std::string https_status_code_class_histogram_name_;
+  // Which histogram to log individual fetch results under.
+  Histogram histogram_;
 
   base::WeakPtrFactory<WebAppIconDownloader> weak_ptr_factory_{this};
 

chrome/browser/web_applications/components/web_app_icon_downloader_unittest.cc

@@ -48,7 +48,7 @@ class WebAppIconDownloaderTest : public ChromeRenderViewHostTestHarness {
   DISALLOW_COPY_AND_ASSIGN(WebAppIconDownloaderTest);
 };
 
-const char* kTestHistogramName = "WebAppIconDownloader.TestHistogram";
+const char* kHistogramForCreateName = "WebApp.Icon.HttpStatusCodeClassOnCreate";
 
 }  // namespace
 
@@ -59,7 +59,7 @@ class TestWebAppIconDownloader : public WebAppIconDownloader {
       : WebAppIconDownloader(
             web_contents,
             extra_favicon_urls,
-            kTestHistogramName,
+            Histogram::kForCreate,
             base::BindOnce(&TestWebAppIconDownloader::DownloadsComplete,
                            base::Unretained(this))),
         id_counter_(0) {}
@@ -130,7 +130,7 @@ TEST_F(WebAppIconDownloaderTest, SimpleDownload) {
   EXPECT_EQ(1u, downloader.favicon_map().size());
   EXPECT_EQ(1u, downloader.favicon_map()[favicon_url].size());
   EXPECT_TRUE(downloader.downloads_succeeded());
-  histogram_tester_.ExpectUniqueSample(kTestHistogramName, 2, 1);
+  histogram_tester_.ExpectUniqueSample(kHistogramForCreateName, 2, 1);
 }
 
 TEST_F(WebAppIconDownloaderTest, NoHTTPStatusCode) {
@@ -156,7 +156,7 @@ TEST_F(WebAppIconDownloaderTest, NoHTTPStatusCode) {
   EXPECT_EQ(1u, downloader.favicon_map()[favicon_url].size());
   EXPECT_TRUE(downloader.downloads_succeeded())
       << "Should not consider data: URL or HTTP status code of 0 a failure";
-  histogram_tester_.ExpectTotalCount(kTestHistogramName, 0);
+  histogram_tester_.ExpectTotalCount(kHistogramForCreateName, 0);
 }
 
 TEST_F(WebAppIconDownloaderTest, DownloadWithUrlsFromWebContentsNotification) {
@@ -183,7 +183,7 @@ TEST_F(WebAppIconDownloaderTest, DownloadWithUrlsFromWebContentsNotification) {
   EXPECT_EQ(1u, downloader.favicon_map().size());
   EXPECT_EQ(1u, downloader.favicon_map()[favicon_url].size());
   EXPECT_TRUE(downloader.downloads_succeeded());
-  histogram_tester_.ExpectUniqueSample(kTestHistogramName, 2, 1);
+  histogram_tester_.ExpectUniqueSample(kHistogramForCreateName, 2, 1);
 }
 
 TEST_F(WebAppIconDownloaderTest, DownloadMultipleUrls) {
@@ -233,7 +233,7 @@ TEST_F(WebAppIconDownloaderTest, DownloadMultipleUrls) {
   EXPECT_EQ(1u, downloader.favicon_map()[favicon_url_1].size());
   EXPECT_EQ(2u, downloader.favicon_map()[favicon_url_2].size());
   EXPECT_TRUE(downloader.downloads_succeeded());
-  histogram_tester_.ExpectUniqueSample(kTestHistogramName, 2, 3);
+  histogram_tester_.ExpectUniqueSample(kHistogramForCreateName, 2, 3);
 }
 
 TEST_F(WebAppIconDownloaderTest, SkipPageFavicons) {
@@ -269,7 +269,7 @@ TEST_F(WebAppIconDownloaderTest, SkipPageFavicons) {
   EXPECT_EQ(1u, downloader.favicon_map()[favicon_url_1].size());
   EXPECT_EQ(0u, downloader.favicon_map()[favicon_url_2].size());
   EXPECT_TRUE(downloader.downloads_succeeded());
-  histogram_tester_.ExpectUniqueSample(kTestHistogramName, 2, 1);
+  histogram_tester_.ExpectUniqueSample(kHistogramForCreateName, 2, 1);
 }
 
 TEST_F(WebAppIconDownloaderTest, PageNavigates) {
@@ -320,7 +320,7 @@ TEST_F(WebAppIconDownloaderTest, PageNavigatesSameDocument) {
   EXPECT_EQ(1u, downloader.favicon_map().size());
   EXPECT_EQ(1u, downloader.favicon_map()[favicon_url].size());
   EXPECT_TRUE(downloader.downloads_succeeded());
-  histogram_tester_.ExpectUniqueSample(kTestHistogramName, 2, 1);
+  histogram_tester_.ExpectUniqueSample(kHistogramForCreateName, 2, 1);
 }
 
 }  // namespace web_app

chrome/browser/web_applications/test/test_data_retriever.cc

@@ -45,7 +45,7 @@ void TestDataRetriever::CheckInstallabilityAndRetrieveManifest(
 void TestDataRetriever::GetIcons(content::WebContents* web_contents,
                                  const std::vector<GURL>& icon_urls,
                                  bool skip_page_favicons,
-                                 WebappInstallSource install_source,
+                                 WebAppIconDownloader::Histogram histogram,
                                  GetIconsCallback callback) {
   if (get_icons_delegate_) {
     icons_map_ =

chrome/browser/web_applications/test/test_data_retriever.h

@@ -35,7 +35,7 @@ class TestDataRetriever : public WebAppDataRetriever {
   void GetIcons(content::WebContents* web_contents,
                 const std::vector<GURL>& icon_urls,
                 bool skip_page_favicons,
-                WebappInstallSource install_source,
+                WebAppIconDownloader::Histogram histogram,
                 GetIconsCallback callback) override;
 
   // Set info to respond on |GetWebApplicationInfo|.

chrome/browser/web_applications/web_app_install_task.cc

@@ -171,7 +171,10 @@ void WebAppInstallTask::InstallWebAppFromInfoRetrieveIcons(
 
   // Skip downloading the page favicons as everything in is the URL list.
   data_retriever_->GetIcons(
-      web_contents, icon_urls, /*skip_page_fav_icons*/ true, install_source_,
+      web_contents, icon_urls, /*skip_page_fav_icons*/ true,
+      install_source_ == WebappInstallSource::SYNC
+          ? WebAppIconDownloader::Histogram::kForSync
+          : WebAppIconDownloader::Histogram::kForCreate,
       base::BindOnce(&WebAppInstallTask::OnIconsRetrieved,
                      base::Unretained(this), std::move(web_application_info),
                      is_locally_installed));
@@ -372,7 +375,10 @@ void WebAppInstallTask::OnDidCheckForIntentToPlayStore(
 #endif  // defined(OS_CHROMEOS)
 
   data_retriever_->GetIcons(
-      web_contents(), icon_urls, skip_page_favicons, install_source_,
+      web_contents(), icon_urls, skip_page_favicons,
+      install_source_ == WebappInstallSource::SYNC
+          ? WebAppIconDownloader::Histogram::kForSync
+          : WebAppIconDownloader::Histogram::kForCreate,
       base::BindOnce(&WebAppInstallTask::OnIconsRetrievedShowDialog,
                      base::Unretained(this), std::move(web_app_info),
                      for_installable_site));