Fix bookmark icon download status logging to account for data: URLs

This CL makes Extensions.BookmarkApp.Icon.HttpStatusCodeClassOnSync and
Extensions.BookmarkApp.Icon.HttpStatusCodeClassOnCreate ignore data: URLs
which have no HTTP status code.

Bug: 901552
Change-Id: I5a7ec6f9300c0b91668e9994b769073a638aff5b
Reviewed-on: https://chromium-review.googlesource.com/c/1317213
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610777}

chrome/browser/web_applications/components/web_app_icon_downloader.cc

@@ -109,7 +109,8 @@ void WebAppIconDownloader::DidDownloadFavicon(
   if (in_progress_requests_.erase(id) == 0)
     return;
 
-  if (!https_status_code_class_histogram_name_.empty()) {
+  if (!https_status_code_class_histogram_name_.empty() &&
+      http_status_code != 0) {
     DCHECK_LE(100, http_status_code);
     DCHECK_GT(600, http_status_code);
     base::UmaHistogramExactLinear(https_status_code_class_histogram_name_,

chrome/browser/web_applications/components/web_app_icon_downloader_unittest.cc

@@ -82,11 +82,12 @@ class TestWebAppIconDownloader : public WebAppIconDownloader {
 
   void CompleteImageDownload(
       int id,
+      int http_status_code,
       const GURL& image_url,
       const std::vector<gfx::Size>& original_bitmap_sizes) {
     WebAppIconDownloader::DidDownloadFavicon(
-        id, 200, image_url, CreateTestBitmaps(original_bitmap_sizes),
-        original_bitmap_sizes);
+        id, http_status_code, image_url,
+        CreateTestBitmaps(original_bitmap_sizes), original_bitmap_sizes);
   }
 
   void UpdateFaviconURLs(const std::vector<content::FaviconURL>& candidates) {
@@ -123,7 +124,7 @@ TEST_F(WebAppIconDownloaderTest, SimpleDownload) {
   EXPECT_EQ(1u, downloader.pending_requests());
 
   std::vector<gfx::Size> sizes(1, gfx::Size(32, 32));
-  downloader.CompleteImageDownload(0, favicon_urls[0].icon_url, sizes);
+  downloader.CompleteImageDownload(0, 200, favicon_urls[0].icon_url, sizes);
   EXPECT_EQ(0u, downloader.pending_requests());
 
   EXPECT_EQ(1u, downloader.favicon_map().size());
@@ -132,6 +133,32 @@ TEST_F(WebAppIconDownloaderTest, SimpleDownload) {
   histogram_tester_.ExpectUniqueSample(kTestHistogramName, 2, 1);
 }
 
+TEST_F(WebAppIconDownloaderTest, NoHTTPStatusCode) {
+  const GURL favicon_url("data:image/png,");
+  TestWebAppIconDownloader downloader(web_contents(), std::vector<GURL>());
+
+  std::vector<content::FaviconURL> favicon_urls;
+  favicon_urls.push_back(
+      content::FaviconURL(favicon_url, content::FaviconURL::IconType::kFavicon,
+                          std::vector<gfx::Size>()));
+  downloader.set_initial_favicon_urls(favicon_urls);
+  EXPECT_EQ(0u, downloader.pending_requests());
+
+  downloader.Start();
+  EXPECT_EQ(1u, downloader.pending_requests());
+
+  std::vector<gfx::Size> sizes = {gfx::Size(0, 0)};
+  // data: URLs have a 0 HTTP status code.
+  downloader.CompleteImageDownload(0, 0, favicon_urls[0].icon_url, sizes);
+  EXPECT_EQ(0u, downloader.pending_requests());
+
+  EXPECT_EQ(1u, downloader.favicon_map().size());
+  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);
+}
+
 TEST_F(WebAppIconDownloaderTest, DownloadWithUrlsFromWebContentsNotification) {
   const GURL favicon_url("http://www.google.com/favicon.ico");
   TestWebAppIconDownloader downloader(web_contents(), std::vector<GURL>());
@@ -150,7 +177,7 @@ TEST_F(WebAppIconDownloaderTest, DownloadWithUrlsFromWebContentsNotification) {
   EXPECT_EQ(1u, downloader.pending_requests());
 
   std::vector<gfx::Size> sizes(1, gfx::Size(32, 32));
-  downloader.CompleteImageDownload(0, favicon_urls[0].icon_url, sizes);
+  downloader.CompleteImageDownload(0, 200, favicon_urls[0].icon_url, sizes);
   EXPECT_EQ(0u, downloader.pending_requests());
 
   EXPECT_EQ(1u, downloader.favicon_map().size());
@@ -188,16 +215,17 @@ TEST_F(WebAppIconDownloaderTest, DownloadMultipleUrls) {
   EXPECT_EQ(3u, downloader.pending_requests());
 
   std::vector<gfx::Size> sizes_1(1, gfx::Size(16, 16));
-  downloader.CompleteImageDownload(0, favicon_url_1, sizes_1);
+  downloader.CompleteImageDownload(0, 200, favicon_url_1, sizes_1);
 
   std::vector<gfx::Size> sizes_2;
   sizes_2.push_back(gfx::Size(32, 32));
   sizes_2.push_back(gfx::Size(64, 64));
-  downloader.CompleteImageDownload(1, favicon_url_2, sizes_2);
+  downloader.CompleteImageDownload(1, 200, favicon_url_2, sizes_2);
 
   // Only 1 download should have been initiated for |empty_favicon| even though
   // the URL was in both the web app info and the favicon urls.
-  downloader.CompleteImageDownload(2, empty_favicon, std::vector<gfx::Size>());
+  downloader.CompleteImageDownload(2, 200, empty_favicon,
+                                   std::vector<gfx::Size>());
   EXPECT_EQ(0u, downloader.pending_requests());
 
   EXPECT_EQ(3u, downloader.favicon_map().size());
@@ -228,13 +256,13 @@ TEST_F(WebAppIconDownloaderTest, SkipPageFavicons) {
   EXPECT_EQ(1u, downloader.pending_requests());
 
   std::vector<gfx::Size> sizes_1(1, gfx::Size(16, 16));
-  downloader.CompleteImageDownload(0, favicon_url_1, sizes_1);
+  downloader.CompleteImageDownload(0, 200, favicon_url_1, sizes_1);
 
   // This download should not be finished and inserted into the map.
   std::vector<gfx::Size> sizes_2;
   sizes_2.push_back(gfx::Size(32, 32));
   sizes_2.push_back(gfx::Size(64, 64));
-  downloader.CompleteImageDownload(1, favicon_url_2, sizes_2);
+  downloader.CompleteImageDownload(1, 200, favicon_url_2, sizes_2);
   EXPECT_EQ(0u, downloader.pending_requests());
 
   EXPECT_EQ(1u, downloader.favicon_map().size());
@@ -286,7 +314,7 @@ TEST_F(WebAppIconDownloaderTest, PageNavigatesSameDocument) {
   EXPECT_EQ(1u, downloader.pending_requests());
 
   std::vector<gfx::Size> sizes(1, gfx::Size(32, 32));
-  downloader.CompleteImageDownload(0, favicon_url, sizes);
+  downloader.CompleteImageDownload(0, 200, favicon_url, sizes);
   EXPECT_EQ(0u, downloader.pending_requests());
 
   EXPECT_EQ(1u, downloader.favicon_map().size());

content/common/image_downloader/image_downloader.mojom

@@ -11,6 +11,7 @@ import "url/mojom/url.mojom";
 interface ImageDownloader {
   // Fetch and decode an image from a given URL.
   // Returns the decoded images, or http_status_code to indicate error.
+  // "data:" URLs have a http_status_code of 0.
   // Each call is independent, overlapping calls are possible.
   DownloadImage(url.mojom.Url url,
                 bool is_favicon,

content/public/browser/web_contents.h

@@ -789,15 +789,15 @@ class WebContents : public PageNavigator,
   virtual device::mojom::WakeLockContext* GetWakeLockContext() = 0;
 
   using ImageDownloadCallback = base::OnceCallback<void(
-      int,                          /* id */
-      int,                          /* HTTP status code */
-      const GURL&,                  /* image_url */
-      const std::vector<SkBitmap>&, /* bitmaps */
+      int id,
+      int http_status_code,  // Can be 0 e.g. for data: URLs.
+      const GURL& image_url,
+      const std::vector<SkBitmap>& bitmaps,
       /* The sizes in pixel of the bitmaps before they were resized due to the
          max bitmap size passed to DownloadImage(). Each entry in the bitmaps
          vector corresponds to an entry in the sizes vector. If a bitmap was
          resized, there should be a single returned bitmap. */
-      const std::vector<gfx::Size>&)>;
+      const std::vector<gfx::Size>& sizes)>;
 
   // Sends a request to download the given image |url| and returns the unique
   // id of the download request. When the download is finished, |callback| will