Add support to process favicons from Web Manifests

components/favicon/core/favicon_handler_unittest.cc

Index: components/favicon/core/favicon_handler_unittest.cc
diff --git a/components/favicon/core/favicon_handler_unittest.cc b/components/favicon/core/favicon_handler_unittest.cc
index 4a997f382474f3ff6783867958eccd253e6ccf70..69824ad521953864f560bd996043569358dcaafb 100644
--- a/components/favicon/core/favicon_handler_unittest.cc
+++ b/components/favicon/core/favicon_handler_unittest.cc
@@ -104,14 +104,16 @@ class FakeImageDownloader {
     SizeVector original_bitmap_sizes;
   };
 
-  FakeImageDownloader() : next_download_id_(1) {}
+  // |downloads| must not be nullptr and must outlive this object.
+  FakeImageDownloader(URLVector* downloads)
+      : downloads_(downloads), next_download_id_(1) {}
 
   // Implementation of FaviconHalder::Delegate's DownloadImage(). If a given
   // URL is not known (i.e. not previously added via Add()), it produces 404s.
   int DownloadImage(const GURL& url,
                     int max_image_size,
                     FaviconHandler::Delegate::ImageDownloadCallback callback) {
-    downloads_.push_back(url);
+    downloads_->push_back(url);
 
     const Response& response = responses_[url];
     int download_id = next_download_id_++;
@@ -169,17 +171,10 @@ class FakeImageDownloader {
     return true;
   }
 
-  // Returns pending and completed download URLs.
-  const URLVector& downloads() const { return downloads_; }
-
-  void ClearDownloads() { downloads_.clear(); }
-
  private:
+  URLVector* downloads_;
   int next_download_id_;
 
-  // Pending and completed download URLs.
-  URLVector downloads_;
-
   // URL to disable automatic callbacks for.
   GURL manual_callback_url_;
 
@@ -192,19 +187,105 @@ class FakeImageDownloader {
   DISALLOW_COPY_AND_ASSIGN(FakeImageDownloader);
 };
 
+// Fake that implements the calls to FaviconHandler::Delegate's
+// DownloadManifest(), delegated to this class through MockDelegate.
+class FakeManifestDownloader {
+ public:
+  struct Response {
+    int http_status_code = 404;
+    std::vector<favicon::FaviconURL> favicon_urls;
+  };
+
+  // |downloads| must not be nullptr and must outlive this object.
+  FakeManifestDownloader(URLVector* downloads) : downloads_(downloads) {}
+
+  // Implementation of FaviconHalder::Delegate's DownloadManifest(). If a given
+  // URL is not known (i.e. not previously added via Add()), it produces 404s.
+  void DownloadManifest(
+      const GURL& url,
+      FaviconHandler::Delegate::ManifestDownloadCallback callback) {
+    downloads_->push_back(url);
+
+    const Response& response = responses_[url];
+    base::Closure bound_callback =
+        base::Bind(callback, response.http_status_code, response.favicon_urls);
+    if (url == manual_callback_url_)
+      manual_callback_ = bound_callback;
+    else
+      base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, bound_callback);
+  }
+
+  void Add(const GURL& manifest_url,
+           const std::vector<favicon::FaviconURL>& favicon_urls) {
+    Response response;
+    response.http_status_code = 200;
+    response.favicon_urls = favicon_urls;
+    responses_[manifest_url] = response;
+  }
+
+  void AddError(const GURL& manifest_url, int http_status_code) {
+    Response response;
+    response.http_status_code = http_status_code;
+    responses_[manifest_url] = response;
+  }
+
+  // Disables automatic callback for |url|. This is useful for emulating a
+  // download taking a long time. The callback for DownloadManifest() will be
+  // stored in |manual_callback_|.
+  void SetRunCallbackManuallyForUrl(const GURL& url) {
+    manual_callback_url_ = url;
+  }
+
+  // Returns whether an ongoing download exists for a url previously selected
+  // via SetRunCallbackManuallyForUrl().
+  bool HasPendingManualCallback() { return !manual_callback_.is_null(); }
+
+  // Triggers the response for a download previously selected for manual
+  // triggering via SetRunCallbackManuallyForUrl().
+  bool RunCallbackManually() {
+    if (!HasPendingManualCallback())
+      return false;
+    manual_callback_.Run();
+    manual_callback_.Reset();
+    return true;
+  }
+
+ private:
+  URLVector* downloads_;
+
+  // URL to disable automatic callbacks for.
+  GURL manual_callback_url_;
+
+  // Callback for DownloadManifest() request for |manual_callback_url_|.
+  base::Closure manual_callback_;
+
+  // Registered responses.
+  std::map<GURL, Response> responses_;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeManifestDownloader);
+};
+
 class MockDelegate : public FaviconHandler::Delegate {
  public:
-  MockDelegate() {
+  MockDelegate()
+      : fake_image_downloader_(&downloads_),
+        fake_manifest_downloader_(&downloads_) {
     // Delegate image downloading to FakeImageDownloader.
     ON_CALL(*this, DownloadImage(_, _, _))
-        .WillByDefault(
-            Invoke(&fake_downloader_, &FakeImageDownloader::DownloadImage));
+        .WillByDefault(Invoke(&fake_image_downloader_,
+                              &FakeImageDownloader::DownloadImage));
+    // Delegate manifest downloading to FakeManifestDownloader.
+    ON_CALL(*this, DownloadManifest(_, _))
+        .WillByDefault(Invoke(&fake_manifest_downloader_,
+                              &FakeManifestDownloader::DownloadManifest));
   }
 
   MOCK_METHOD3(DownloadImage,
                int(const GURL& url,
                    int max_image_size,
                    ImageDownloadCallback callback));
+  MOCK_METHOD2(DownloadManifest,
+               void(const GURL& url, ManifestDownloadCallback callback));
   MOCK_METHOD0(IsOffTheRecord, bool());
   MOCK_METHOD1(IsBookmarked, bool(const GURL& url));
   MOCK_METHOD5(OnFaviconUpdated,
@@ -214,14 +295,24 @@ class MockDelegate : public FaviconHandler::Delegate {
                     bool icon_url_changed,
                     const gfx::Image& image));
 
-  FakeImageDownloader& fake_downloader() { return fake_downloader_; }
+  FakeImageDownloader& fake_image_downloader() {
+    return fake_image_downloader_;
+  }
+
+  FakeManifestDownloader& fake_manifest_downloader() {
+    return fake_manifest_downloader_;
+  }
 
-  // Convenience getter for test readability. Returns pending and completed
-  // download URLs.
-  const URLVector& downloads() const { return fake_downloader_.downloads(); }
+  // Returns pending and completed download URLs.
+  const URLVector& downloads() const { return downloads_; }
+
+  void ClearDownloads() { downloads_.clear(); }
 
  private:
-  FakeImageDownloader fake_downloader_;
+  // Pending and completed download URLs.
+  URLVector downloads_;
+  FakeImageDownloader fake_image_downloader_;
+  FakeManifestDownloader fake_manifest_downloader_;
 };
 
 // FakeFaviconService mimics a FaviconService backend that allows setting up
@@ -327,10 +418,10 @@ class FaviconHandlerTest : public testing::Test {
 
   FaviconHandlerTest() {
     // Register various known icon URLs.
-    delegate_.fake_downloader().Add(kIconURL10x10, IntVector{10});
-    delegate_.fake_downloader().Add(kIconURL12x12, IntVector{12});
-    delegate_.fake_downloader().Add(kIconURL16x16, IntVector{16});
-    delegate_.fake_downloader().Add(kIconURL64x64, IntVector{64});
+    delegate_.fake_image_downloader().Add(kIconURL10x10, IntVector{10});
+    delegate_.fake_image_downloader().Add(kIconURL12x12, IntVector{12});
+    delegate_.fake_image_downloader().Add(kIconURL16x16, IntVector{16});
+    delegate_.fake_image_downloader().Add(kIconURL64x64, IntVector{64});
 
     // The score computed by SelectFaviconFrames() is dependent on the supported
     // scale factors of the platform. It is used for determining the goodness of
@@ -344,7 +435,7 @@ class FaviconHandlerTest : public testing::Test {
   bool VerifyAndClearExpectations() {
     base::RunLoop().RunUntilIdle();
     favicon_service_.fake()->ClearDbRequests();
-    delegate_.fake_downloader().ClearDownloads();
+    delegate_.ClearDownloads();
     return testing::Mock::VerifyAndClearExpectations(&favicon_service_) &&
            testing::Mock::VerifyAndClearExpectations(&delegate_);
   }
@@ -353,14 +444,15 @@ class FaviconHandlerTest : public testing::Test {
   // Returns the handler in case tests want to exercise further steps.
   std::unique_ptr<FaviconHandler> RunHandlerWithCandidates(
       FaviconDriverObserver::NotificationIconType handler_type,
-      const std::vector<favicon::FaviconURL>& candidates) {
+      const std::vector<favicon::FaviconURL>& candidates,
+      const base::Optional<GURL>& manifest_url = base::nullopt) {
     auto handler = base::MakeUnique<FaviconHandler>(&favicon_service_,
                                                     &delegate_, handler_type);
     handler->FetchFavicon(kPageURL);
     // The first RunUntilIdle() causes the FaviconService lookups be faster than
-    // OnUpdateFaviconURL(), which is the most likely scenario.
+    // OnUpdateCandidates(), which is the most likely scenario.
     base::RunLoop().RunUntilIdle();
-    handler->OnUpdateFaviconURL(kPageURL, candidates);
+    handler->OnUpdateCandidates(kPageURL, candidates, manifest_url);
     base::RunLoop().RunUntilIdle();
     return handler;
   }
@@ -368,13 +460,14 @@ class FaviconHandlerTest : public testing::Test {
   // Same as above, but for the simplest case where all types are FAVICON and
   // no sizes are provided, using a FaviconHandler of type NON_TOUCH_16_DIP.
   std::unique_ptr<FaviconHandler> RunHandlerWithSimpleFaviconCandidates(
-      const std::vector<GURL>& urls) {
+      const std::vector<GURL>& urls,
+      const base::Optional<GURL>& manifest_url = base::nullopt) {
     std::vector<favicon::FaviconURL> candidates;
     for (const GURL& url : urls) {
       candidates.emplace_back(url, FAVICON, kEmptySizes);
     }
     return RunHandlerWithCandidates(FaviconDriverObserver::NON_TOUCH_16_DIP,
-                                    candidates);
+                                    candidates, manifest_url);
   }
 
   base::MessageLoopForUI message_loop_;
@@ -419,7 +512,7 @@ TEST_F(FaviconHandlerTest, UpdateFaviconMappingsAndFetch) {
 // - There is data in the database for neither the page URL nor the icon URL.
 // AND
 // - FaviconService::GetFaviconForPageURL() callback returns before
-//   FaviconHandler::OnUpdateFaviconURL() is called.
+//   FaviconHandler::OnUpdateCandidates() is called.
 TEST_F(FaviconHandlerTest, DownloadUnknownFaviconIfCandidatesSlower) {
   EXPECT_CALL(favicon_service_, SetFavicons(kPageURL, kIconURL16x16, FAVICON,
                                             ImageSizeIs(16, 16)));
@@ -430,10 +523,11 @@ TEST_F(FaviconHandlerTest, DownloadUnknownFaviconIfCandidatesSlower) {
   FaviconHandler handler(&favicon_service_, &delegate_,
                          FaviconDriverObserver::NON_TOUCH_16_DIP);
   handler.FetchFavicon(kPageURL);
-  // Causes FaviconService lookups be faster than OnUpdateFaviconURL().
+  // Causes FaviconService lookups be faster than OnUpdateCandidates().
   base::RunLoop().RunUntilIdle();
-  handler.OnUpdateFaviconURL(kPageURL,
-                             {FaviconURL(kIconURL16x16, FAVICON, kEmptySizes)});
+  handler.OnUpdateCandidates(kPageURL,
+                             {FaviconURL(kIconURL16x16, FAVICON, kEmptySizes)},
+                             base::nullopt);
   base::RunLoop().RunUntilIdle();
 
   EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL16x16));
@@ -445,7 +539,7 @@ TEST_F(FaviconHandlerTest, DownloadUnknownFaviconIfCandidatesSlower) {
 // - There is data in the database for neither the page URL nor the icon URL.
 // AND
 // - FaviconService::GetFaviconForPageURL() callback returns after
-//   FaviconHandler::OnUpdateFaviconURL() is called.
+//   FaviconHandler::OnUpdateCandidates() is called.
 TEST_F(FaviconHandlerTest, DownloadUnknownFaviconIfCandidatesFaster) {
   EXPECT_CALL(favicon_service_, SetFavicons(kPageURL, kIconURL16x16, FAVICON,
                                             ImageSizeIs(16, 16)));
@@ -457,8 +551,9 @@ TEST_F(FaviconHandlerTest, DownloadUnknownFaviconIfCandidatesFaster) {
   ASSERT_THAT(favicon_service_.fake()->db_requests(), ElementsAre(kPageURL));
 
   // Feed in favicons without processing posted tasks (RunUntilIdle()).
-  handler.OnUpdateFaviconURL(kPageURL,
-                             {FaviconURL(kIconURL16x16, FAVICON, kEmptySizes)});
+  handler.OnUpdateCandidates(kPageURL,
+                             {FaviconURL(kIconURL16x16, FAVICON, kEmptySizes)},
+                             base::nullopt);
   base::RunLoop().RunUntilIdle();
 
   EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL16x16));
@@ -589,7 +684,7 @@ TEST_F(FaviconHandlerTest, UpdateFavicon) {
 TEST_F(FaviconHandlerTest, Download2ndFaviconURLCandidate) {
   const GURL kIconURLReturning500("http://www.google.com/500.png");
 
-  delegate_.fake_downloader().AddError(kIconURLReturning500, 500);
+  delegate_.fake_image_downloader().AddError(kIconURLReturning500, 500);
 
   favicon_service_.fake()->Store(
       kPageURL, kIconURL64x64,
@@ -616,7 +711,7 @@ TEST_F(FaviconHandlerTest, Download2ndFaviconURLCandidate) {
 
 // Test that download data for icon URLs other than the current favicon
 // candidate URLs is ignored. This test tests the scenario where a download is
-// in flight when FaviconHandler::OnUpdateFaviconURL() is called.
+// in flight when FaviconHandler::OnUpdateCandidates() is called.
 // TODO(mastiz): Make this test deal with FaviconURLs of type
 // favicon_base::FAVICON and add new ones like OnlyDownloadMatchingIconType and
 // CallSetFaviconsWithCorrectIconType.
@@ -627,27 +722,27 @@ TEST_F(FaviconHandlerTest, UpdateDuringDownloading) {
 
   // Defer the download completion such that RunUntilIdle() doesn't complete
   // the download.
-  delegate_.fake_downloader().SetRunCallbackManuallyForUrl(kIconURL1);
+  delegate_.fake_image_downloader().SetRunCallbackManuallyForUrl(kIconURL1);
 
-  delegate_.fake_downloader().Add(kIconURL1, IntVector{16});
-  delegate_.fake_downloader().Add(kIconURL3, IntVector{64});
+  delegate_.fake_image_downloader().Add(kIconURL1, IntVector{16});
+  delegate_.fake_image_downloader().Add(kIconURL3, IntVector{64});
 
   std::unique_ptr<FaviconHandler> handler =
       RunHandlerWithSimpleFaviconCandidates({kIconURL1, kIconURL2});
 
   ASSERT_TRUE(VerifyAndClearExpectations());
-  ASSERT_TRUE(delegate_.fake_downloader().HasPendingManualCallback());
+  ASSERT_TRUE(delegate_.fake_image_downloader().HasPendingManualCallback());
 
   // Favicon update should invalidate the ongoing download.
   EXPECT_CALL(favicon_service_, SetFavicons(_, kIconURL3, _, _));
   EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL3, _, _));
 
-  handler->OnUpdateFaviconURL(kPageURL,
-                              {FaviconURL(kIconURL3, FAVICON, kEmptySizes)});
+  handler->OnUpdateCandidates(
+      kPageURL, {FaviconURL(kIconURL3, FAVICON, kEmptySizes)}, base::nullopt);
 
   // Finalizes download, which should be thrown away as the favicon URLs were
   // updated.
-  EXPECT_TRUE(delegate_.fake_downloader().RunCallbackManually());
+  EXPECT_TRUE(delegate_.fake_image_downloader().RunCallbackManually());
   base::RunLoop().RunUntilIdle();
 
   EXPECT_THAT(favicon_service_.fake()->db_requests(), ElementsAre(kIconURL3));
@@ -666,8 +761,9 @@ TEST_F(FaviconHandlerTest, UpdateSameIconURLsWhileProcessingShouldBeNoop) {
 
   // Defer the download completion such that RunUntilIdle() doesn't complete
   // the download.
-  delegate_.fake_downloader().SetRunCallbackManuallyForUrl(kSlowLoadingIconURL);
-  delegate_.fake_downloader().Add(kSlowLoadingIconURL, IntVector{16});
+  delegate_.fake_image_downloader().SetRunCallbackManuallyForUrl(
+      kSlowLoadingIconURL);
+  delegate_.fake_image_downloader().Add(kSlowLoadingIconURL, IntVector{16});
 
   std::unique_ptr<FaviconHandler> handler = RunHandlerWithCandidates(
       FaviconDriverObserver::NON_TOUCH_16_DIP, favicon_urls);
@@ -675,17 +771,17 @@ TEST_F(FaviconHandlerTest, UpdateSameIconURLsWhileProcessingShouldBeNoop) {
   ASSERT_THAT(favicon_service_.fake()->db_requests(),
               ElementsAre(kPageURL, kIconURL64x64, kSlowLoadingIconURL));
   ASSERT_TRUE(VerifyAndClearExpectations());
-  ASSERT_TRUE(delegate_.fake_downloader().HasPendingManualCallback());
+  ASSERT_TRUE(delegate_.fake_image_downloader().HasPendingManualCallback());
 
-  // Calling OnUpdateFaviconURL() with the same icon URLs should have no effect,
+  // Calling OnUpdateCandidates() with the same icon URLs should have no effect,
   // despite the ongoing download.
-  handler->OnUpdateFaviconURL(kPageURL, favicon_urls);
+  handler->OnUpdateCandidates(kPageURL, favicon_urls, base::nullopt);
   base::RunLoop().RunUntilIdle();
 
   // Complete the download.
   EXPECT_CALL(favicon_service_, SetFavicons(_, _, _, _));
   EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, _, _, _));
-  EXPECT_TRUE(delegate_.fake_downloader().RunCallbackManually());
+  EXPECT_TRUE(delegate_.fake_image_downloader().RunCallbackManually());
   base::RunLoop().RunUntilIdle();
   EXPECT_THAT(delegate_.downloads(), IsEmpty());
 }
@@ -704,11 +800,11 @@ TEST_F(FaviconHandlerTest, UpdateSameIconURLsAfterFinishedShouldBeNoop) {
 
   ASSERT_TRUE(VerifyAndClearExpectations());
 
-  // Calling OnUpdateFaviconURL() with identical data should be a no-op.
+  // Calling OnUpdateCandidates() with identical data should be a no-op.
   EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, _, _, _)).Times(0);
   EXPECT_CALL(favicon_service_, SetFavicons(_, _, _, _)).Times(0);
 
-  handler->OnUpdateFaviconURL(kPageURL, favicon_urls);
+  handler->OnUpdateCandidates(kPageURL, favicon_urls, base::nullopt);
   base::RunLoop().RunUntilIdle();
   EXPECT_THAT(favicon_service_.fake()->db_requests(), IsEmpty());
   EXPECT_THAT(delegate_.downloads(), IsEmpty());
@@ -729,8 +825,8 @@ TEST_F(FaviconHandlerTest,
       "http://wwww.page_which_animates_favicon.com/frame2.png");
 
   // |kIconURL1| is the better match.
-  delegate_.fake_downloader().Add(kIconURL1, IntVector{15});
-  delegate_.fake_downloader().Add(kIconURL2, IntVector{10});
+  delegate_.fake_image_downloader().Add(kIconURL1, IntVector{15});
+  delegate_.fake_image_downloader().Add(kIconURL2, IntVector{10});
 
   // Two FaviconDriver::OnFaviconUpdated() notifications should be sent for
   // |kIconURL1|, one before and one after the download.
@@ -750,8 +846,8 @@ TEST_F(FaviconHandlerTest,
   // Simulate the page changing it's icon URL to just |kIconURL2| via
   // Javascript.
   EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL2, _, _));
-  handler->OnUpdateFaviconURL(kPageURL,
-                              {FaviconURL(kIconURL2, FAVICON, kEmptySizes)});
+  handler->OnUpdateCandidates(
+      kPageURL, {FaviconURL(kIconURL2, FAVICON, kEmptySizes)}, base::nullopt);
   base::RunLoop().RunUntilIdle();
 }
 
@@ -788,7 +884,7 @@ class FaviconHandlerMultipleFaviconsTest : public FaviconHandlerTest {
       const GURL icon_url(base::StringPrintf(
           "https://www.google.com/generated/%dx%d", icon_size, icon_size));
       // Set up 200 responses for all images, and the corresponding size.
-      delegate_.fake_downloader().Add(icon_url, IntVector{icon_size});
+      delegate_.fake_image_downloader().Add(icon_url, IntVector{icon_size});
       // Create test candidates of type FAVICON and a fake URL.
       candidate_icons.emplace_back(icon_url, FAVICON, kEmptySizes);
 
@@ -844,7 +940,7 @@ TEST_F(FaviconHandlerTest, Report404) {
 TEST_F(FaviconHandlerTest, NotReport503) {
   const GURL k503IconURL("http://www.google.com/503.png");
 
-  delegate_.fake_downloader().AddError(k503IconURL, 503);
+  delegate_.fake_image_downloader().AddError(k503IconURL, 503);
 
   EXPECT_CALL(favicon_service_, UnableToDownloadFavicon(_)).Times(0);
 
@@ -977,8 +1073,8 @@ TEST_F(FaviconHandlerTest, TestSelectLargestFavicon) {
   const GURL kIconURL1("http://www.google.com/b");
   const GURL kIconURL2("http://www.google.com/c");
 
-  delegate_.fake_downloader().Add(kIconURL1, IntVector{15});
-  delegate_.fake_downloader().Add(kIconURL2, IntVector{14, 16});
+  delegate_.fake_image_downloader().Add(kIconURL1, IntVector{15});
+  delegate_.fake_image_downloader().Add(kIconURL2, IntVector{14, 16});
 
   // Verify NotifyFaviconAvailable().
   EXPECT_CALL(delegate_,
@@ -1003,9 +1099,9 @@ TEST_F(FaviconHandlerTest, TestFaviconWasScaledAfterDownload) {
   const int kOriginalSize1 = kMaximalSize + 1;
   const int kOriginalSize2 = kMaximalSize + 2;
 
-  delegate_.fake_downloader().AddWithOriginalSizes(
+  delegate_.fake_image_downloader().AddWithOriginalSizes(
       kIconURL1, IntVector{kMaximalSize}, IntVector{kOriginalSize1});
-  delegate_.fake_downloader().AddWithOriginalSizes(
+  delegate_.fake_image_downloader().AddWithOriginalSizes(
       kIconURL2, IntVector{kMaximalSize}, IntVector{kOriginalSize2});
 
   // Verify the best bitmap was selected (although smaller than |kIconURL2|)
@@ -1165,7 +1261,7 @@ TEST_F(FaviconHandlerTest, TestRecordFailingDownloadAttempt) {
   base::HistogramTester histogram_tester;
   const GURL k404IconURL("http://www.google.com/404.png");
 
-  delegate_.fake_downloader().AddError(k404IconURL, 404);
+  delegate_.fake_image_downloader().AddError(k404IconURL, 404);
 
   EXPECT_CALL(favicon_service_, UnableToDownloadFavicon(k404IconURL));
 
@@ -1192,5 +1288,349 @@ TEST_F(FaviconHandlerTest, TestRecordSkippedDownloadForKnownFailingUrl) {
                                /*expected_count=*/1)));
 }
 

[pkotwicz, 2017/05/01 04:56:53]

Each test should be preceeded with a comment which documents what the test is
testing

[mastiz, 2017/05/04 10:57:53]

Done. No intent for sarcasm here but, based on my earlier experience, you seem
quite opinionated about comment phrasing so it might be best that you propose
you version of these summaries, thanks!

+TEST_F(FaviconHandlerTest, GetFaviconFromManifestInHistory) {
+  const GURL kManifestURL("http://www.google.com/manifest.json");
+
+  favicon_service_.fake()->Store(kPageURL, kManifestURL,
+                                 CreateRawBitmapResult(kManifestURL));
+
+  EXPECT_CALL(delegate_, DownloadManifest(_, _)).Times(0);
+  EXPECT_CALL(favicon_service_, UnableToDownloadFavicon(_)).Times(0);
+
+  EXPECT_CALL(favicon_service_, UpdateFaviconMappingsAndFetch(
+                                    kPageURL, URLVector{kManifestURL}, FAVICON,
+                                    /*desired_size_in_dip=*/16, _, _));
+  EXPECT_CALL(delegate_, OnFaviconUpdated(
+                             kPageURL, FaviconDriverObserver::NON_TOUCH_16_DIP,
+                             kManifestURL, _, _))
+      .Times(2);
+
+  RunHandlerWithSimpleFaviconCandidates({kIconURL12x12}, kManifestURL);
+  EXPECT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kManifestURL));
+  EXPECT_THAT(delegate_.downloads(), IsEmpty());
+}
+
+TEST_F(FaviconHandlerTest, GetFaviconFromManifestInHistoryIfCandidatesFaster) {
+  const GURL kManifestURL("http://www.google.com/manifest.json");
+
+  favicon_service_.fake()->Store(kPageURL, kManifestURL,
+                                 CreateRawBitmapResult(kManifestURL));
+
+  EXPECT_CALL(delegate_, DownloadManifest(_, _)).Times(0);
+  EXPECT_CALL(favicon_service_, UnableToDownloadFavicon(_)).Times(0);
+
+  EXPECT_CALL(favicon_service_, UpdateFaviconMappingsAndFetch(
+                                    kPageURL, URLVector{kManifestURL}, FAVICON,
+                                    /*desired_size_in_dip=*/16, _, _));
+  EXPECT_CALL(delegate_, OnFaviconUpdated(
+                             kPageURL, FaviconDriverObserver::NON_TOUCH_16_DIP,
+                             kManifestURL, _, _))
+      .Times(2);
+
+  FaviconHandler handler(&favicon_service_, &delegate_,
+                         FaviconDriverObserver::NON_TOUCH_16_DIP);
+  handler.FetchFavicon(kPageURL);
+  // Feed in candidates without processing posted tasks (RunUntilIdle()).
+  handler.OnUpdateCandidates(kPageURL,
+                             {FaviconURL(kIconURL12x12, FAVICON, kEmptySizes)},
+                             kManifestURL);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kManifestURL));
+  EXPECT_THAT(delegate_.downloads(), IsEmpty());
+}
+
+TEST_F(FaviconHandlerTest, GetFaviconFromUnknownManifest) {
+  const GURL kManifestURL("http://www.google.com/manifest.json");
+  const std::vector<favicon::FaviconURL> kManifestIcons = {
+      FaviconURL(kIconURL16x16, FAVICON, kEmptySizes),
+  };
+
+  delegate_.fake_manifest_downloader().Add(kManifestURL, kManifestIcons);
+
+  EXPECT_CALL(favicon_service_, UnableToDownloadFavicon(_)).Times(0);
+
+  EXPECT_CALL(delegate_, DownloadManifest(kManifestURL, _));
+  EXPECT_CALL(favicon_service_,
+              SetFavicons(kPageURL, kManifestURL, FAVICON, _));

[pkotwicz, 2017/05/01 04:56:53]

kManifestURL is the only parameter we care about

[mastiz, 2017/05/04 10:57:53]

Removed kPageURL. I think FAVICON is worth verifying, since it's a new codepath.

[pkotwicz, 2017/05/04 17:28:25]

Fair enough

+  EXPECT_CALL(delegate_, OnFaviconUpdated(kPageURL, _, kManifestURL, _, _));

[pkotwicz, 2017/05/01 04:56:53]

We don't care about the page URL

[mastiz, 2017/05/04 10:57:53]

Done.

+
+  RunHandlerWithSimpleFaviconCandidates({kIconURL12x12}, kManifestURL);
+  EXPECT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kManifestURL));
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kManifestURL, kIconURL16x16));
+}
+
+TEST_F(FaviconHandlerTest, GetFaviconFromExpiredManifest) {
+  const GURL kManifestURL("http://www.google.com/manifest.json");
+  const std::vector<favicon::FaviconURL> kManifestIcons = {
+      FaviconURL(kIconURL64x64, FAVICON, kEmptySizes),
+  };
+
+  favicon_service_.fake()->Store(kPageURL, kManifestURL,
+                                 CreateRawBitmapResult(kManifestURL, FAVICON,
+                                                       /*expired=*/true));
+  delegate_.fake_manifest_downloader().Add(kManifestURL, kManifestIcons);
+
+  // The third notification is unnecessary, but allows simplifying the code.
+  EXPECT_CALL(delegate_, OnFaviconUpdated(kPageURL, _, kManifestURL, _, _))
+      .Times(3);
+  EXPECT_CALL(favicon_service_, SetFavicons(kPageURL, kManifestURL, _, _));

[pkotwicz, 2017/05/01 04:56:53]

We don't care about the page URL for either OnFaviconUpdated() or SetFavicons()

[mastiz, 2017/05/04 10:57:53]

Done.

+
+  RunHandlerWithSimpleFaviconCandidates({kIconURL12x12}, kManifestURL);
+  EXPECT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kManifestURL));
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kManifestURL, kIconURL64x64));
+}
+
+TEST_F(FaviconHandlerTest, GetFaviconFromExpiredManifestLinkedFromOtherPage) {
+  const GURL kSomePreviousPageURL("https://www.google.com/previous");
+  const GURL kManifestURL("http://www.google.com/manifest.json");
+  const std::vector<favicon::FaviconURL> kManifestIcons = {
+      FaviconURL(kIconURL64x64, FAVICON, kEmptySizes),
+  };
+
+  favicon_service_.fake()->Store(kSomePreviousPageURL, kManifestURL,
+                                 CreateRawBitmapResult(kManifestURL, FAVICON,
+                                                       /*expired=*/true));
+  delegate_.fake_manifest_downloader().Add(kManifestURL, kManifestIcons);
+
+  EXPECT_CALL(delegate_, OnFaviconUpdated(kPageURL, _, kManifestURL, _, _))
+      .Times(2);
+  EXPECT_CALL(favicon_service_, SetFavicons(kPageURL, kManifestURL, _, _));
+
+  RunHandlerWithSimpleFaviconCandidates({kIconURL12x12}, kManifestURL);
+  EXPECT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kManifestURL));
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kManifestURL, kIconURL64x64));
+}
+
+TEST_F(FaviconHandlerTest, GetFaviconFromUnknownManifestButKnownIcon) {
+  const GURL kSomePreviousPageURL("https://www.google.com/previous");
+  const GURL kManifestURL("http://www.google.com/manifest.json");
+  const std::vector<favicon::FaviconURL> kManifestIcons = {
+      FaviconURL(kIconURL16x16, FAVICON, kEmptySizes),
+  };
+
+  favicon_service_.fake()->Store(kSomePreviousPageURL, kIconURL16x16,
+                                 CreateRawBitmapResult(kIconURL16x16));
+  delegate_.fake_manifest_downloader().Add(kManifestURL, kManifestIcons);
+
+  EXPECT_CALL(favicon_service_,
+              SetFavicons(kPageURL, kManifestURL, FAVICON, _));
+  EXPECT_CALL(delegate_, OnFaviconUpdated(kPageURL, _, kManifestURL, _, _));
+
+  RunHandlerWithSimpleFaviconCandidates(URLVector(), kManifestURL);
+  EXPECT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kManifestURL));
+  // In the current implementation, the icon is downloaded although it's in the
+  // database, simply because it's not associated to the manifest's URL.

[pkotwicz, 2017/05/01 04:56:53]

This comment is confusing.

Maybe remove the "simply because ..." part

[mastiz, 2017/05/04 10:57:53]

Rephrased, since I believe the "because" part is precisely the interesting part
(the rest is redundant with the code).

+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kManifestURL, kIconURL16x16));
+}
+
+TEST_F(FaviconHandlerTest, UnknownManifestReturning404) {
+  const GURL kManifestURL("http://www.google.com/manifest.json");
+
+  EXPECT_CALL(favicon_service_, UnableToDownloadFavicon(kManifestURL));
+  EXPECT_CALL(favicon_service_,
+              SetFavicons(kPageURL, kIconURL12x12, FAVICON, _));
+
+  RunHandlerWithSimpleFaviconCandidates({kIconURL12x12}, kManifestURL);
+  EXPECT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kManifestURL, kIconURL12x12));
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kManifestURL, kIconURL12x12));
+}
+
+TEST_F(FaviconHandlerTest, UnknownManifestReturning503) {
+  const GURL kManifestURL("http://www.google.com/manifest.json");
+
+  delegate_.fake_manifest_downloader().AddError(kManifestURL, 503);
+
+  EXPECT_CALL(favicon_service_, UnableToDownloadFavicon(_)).Times(0);
+
+  RunHandlerWithSimpleFaviconCandidates({kIconURL12x12}, kManifestURL);
+}
+
+TEST_F(FaviconHandlerTest, IgnoreManifestWithPrior404) {
+  const GURL kManifestURL("http://www.google.com/manifest.json");
+
+  ON_CALL(favicon_service_, WasUnableToDownloadFavicon(kManifestURL))
+      .WillByDefault(Return(true));
+
+  EXPECT_CALL(delegate_, DownloadManifest(_, _)).Times(0);

[pkotwicz, 2017/05/01 04:56:53]

Isn't this covered by the expectation on line 1469?

[mastiz, 2017/05/04 10:57:53]

Unless DownloadManifest were used with kIconURL12x12. I can drop it if you
prefer.

[pkotwicz, 2017/05/04 17:28:25]

I would rather that you drop it

[mastiz, 2017/05/10 10:03:52]

Done.

+  EXPECT_CALL(favicon_service_,
+              SetFavicons(kPageURL, kIconURL12x12, FAVICON, _));
+
+  RunHandlerWithSimpleFaviconCandidates({kIconURL12x12}, kManifestURL);
+  EXPECT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kIconURL12x12));
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL12x12));
+}
+
+// Test that the regular favicon is selected when:
+// - The page links to a Web Manifest.
+// - The Web Manifest does not contain any icon URLs (it is not a 404).
+// - The page has an icon URL provided via a <link rel="icon"> tag.
+// - The database does not know about the page URL, manifest URL or icon URL.
+TEST_F(FaviconHandlerTest, UnknownManifestWithoutIcons) {
+  const GURL kManifestURL("http://www.google.com/manifest.json");
+
+  delegate_.fake_manifest_downloader().Add(kManifestURL,
+                                           std::vector<favicon::FaviconURL>());
+
+  // UnableToDownloadFavicon() is expected to prevent repeated downloads of the
+  // same manifest (which is not otherwise cached, since it doesn't contain
+  // icons).
+  EXPECT_CALL(favicon_service_, UnableToDownloadFavicon(kManifestURL));
+  EXPECT_CALL(favicon_service_,
+              SetFavicons(kPageURL, kIconURL12x12, FAVICON, _));
+
+  RunHandlerWithSimpleFaviconCandidates({kIconURL12x12}, kManifestURL);
+  EXPECT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kManifestURL, kIconURL12x12));
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kManifestURL, kIconURL12x12));
+}
+
+// Test that the regular favicon is selected when:
+// - The page links to a Web Manifest.
+// - The Web Manifest does not contain any icon URLs (it is not a 404).
+// - The page has an icon URL provided via a <link rel="icon"> tag.
+// - The database does not know about the page URL.
+// - The database does not know about the manifest URL.
+// - The database knows about the icon URL.
+TEST_F(FaviconHandlerTest, UnknownManifestWithoutIconsAndKnownRegularIcons) {
+  const GURL kSomePreviousPageURL("https://www.google.com/previous");
+  const GURL kManifestURL("http://www.google.com/manifest.json");
+
+  delegate_.fake_manifest_downloader().Add(kManifestURL,
+                                           std::vector<favicon::FaviconURL>());
+  favicon_service_.fake()->Store(kSomePreviousPageURL, kIconURL12x12,
+                                 CreateRawBitmapResult(kIconURL12x12));
+
+  EXPECT_CALL(favicon_service_, SetFavicons(_, _, _, _)).Times(0);
+
+  // UnableToDownloadFavicon() is expected to prevent repeated downloads of the
+  // same manifest (which is not otherwise cached, since it doesn't contain
+  // icons).
+  EXPECT_CALL(favicon_service_, UnableToDownloadFavicon(kManifestURL));
+  EXPECT_CALL(favicon_service_,
+              UpdateFaviconMappingsAndFetch(kPageURL, URLVector{kManifestURL},
+                                            _, _, _, _));
+  EXPECT_CALL(favicon_service_,
+              UpdateFaviconMappingsAndFetch(kPageURL, URLVector{kIconURL12x12},
+                                            _, _, _, _));
+  EXPECT_CALL(delegate_, OnFaviconUpdated(kPageURL, _, kIconURL12x12, _, _));
+
+  RunHandlerWithSimpleFaviconCandidates({kIconURL12x12}, kManifestURL);
+  EXPECT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kManifestURL, kIconURL12x12));
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kManifestURL));
+}
+
+TEST_F(FaviconHandlerTest, ManifestUpdateViaJavascript) {
+  const GURL kManifestURL1("http://www.google.com/manifest1.json");
+  const GURL kManifestURL2("http://www.google.com/manifest2.json");
+  const std::vector<favicon::FaviconURL> kManifestIcons1 = {
+      FaviconURL(kIconURL64x64, FAVICON, kEmptySizes),
+  };
+  const std::vector<favicon::FaviconURL> kManifestIcons2 = {
+      FaviconURL(kIconURL10x10, FAVICON, kEmptySizes),
+  };
+
+  delegate_.fake_manifest_downloader().Add(kManifestURL1, kManifestIcons1);
+  delegate_.fake_manifest_downloader().Add(kManifestURL2, kManifestIcons2);
+
+  EXPECT_CALL(delegate_, DownloadManifest(kManifestURL1, _));

[pkotwicz, 2017/05/01 04:56:53]

This is redundant with line 1551?

[mastiz, 2017/05/04 10:57:53]

Ditto. I can either ignore the problem and drop this line, leave it as is, or
split image downloads from manifest downloads. WDYT?

[pkotwicz, 2017/05/04 17:28:25]

I would vote for dropping the line and ignoring the problem.

[mastiz, 2017/05/10 10:03:52]

Done.

+
+  std::unique_ptr<FaviconHandler> handler =
+      RunHandlerWithSimpleFaviconCandidates({kIconURL12x12}, kManifestURL1);
+  ASSERT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kManifestURL1));
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kManifestURL1, kIconURL64x64));
+  ASSERT_TRUE(VerifyAndClearExpectations());
+
+  // Simulate the page changing it's manifest URL via Javascript.
+  EXPECT_CALL(delegate_, DownloadManifest(kManifestURL2, _));

[pkotwicz, 2017/05/01 04:56:53]

Isn't this redundant with line 1563?

[mastiz, 2017/05/04 10:57:53]

Ditto.

+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kManifestURL2, _, _));
+  handler->OnUpdateCandidates(kPageURL,
+                              {FaviconURL(kIconURL12x12, FAVICON, kEmptySizes)},
+                              kManifestURL2);
+  base::RunLoop().RunUntilIdle();
+  ASSERT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kManifestURL2));
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kManifestURL2, kIconURL10x10));
+}
+
+TEST_F(FaviconHandlerTest, RemoveManifestViaJavascriptWhileHistoryQuery) {
+  const GURL kManifestURL("http://www.google.com/manifest.json");
+  const std::vector<favicon::FaviconURL> kManifestIcons = {
+      FaviconURL(kIconURL64x64, FAVICON, kEmptySizes),
+  };
+
+  delegate_.fake_manifest_downloader().Add(kManifestURL, kManifestIcons);
+
+  EXPECT_CALL(delegate_, DownloadManifest(_, _)).Times(0);
+
+  auto handler = base::MakeUnique<FaviconHandler>(
+      &favicon_service_, &delegate_, FaviconDriverObserver::NON_TOUCH_16_DIP);
+  handler->FetchFavicon(kPageURL);
+  ASSERT_THAT(favicon_service_.fake()->db_requests(), ElementsAre(kPageURL));
+  base::RunLoop().RunUntilIdle();
+  ASSERT_FALSE(handler->HasPendingTasksForTest());
+  handler->OnUpdateCandidates(kPageURL,
+                              {FaviconURL(kIconURL12x12, FAVICON, kEmptySizes)},
+                              kManifestURL);
+  ASSERT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kManifestURL));
+  // Database lookup for |kManifestURL1| is ongoing.
+  ASSERT_TRUE(handler->HasPendingTasksForTest());
+
+  // Simulate the page changing it's manifest URL to empty via Javascript.
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL12x12, _, _));
+  handler->OnUpdateCandidates(kPageURL,
+                              {FaviconURL(kIconURL12x12, FAVICON, kEmptySizes)},
+                              base::nullopt);
+  base::RunLoop().RunUntilIdle();
+  ASSERT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kManifestURL, kIconURL12x12));
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL12x12));
+}
+
+TEST_F(FaviconHandlerTest, AddManifestViaJavascriptWhileHistoryQuery) {
+  const GURL kManifestURL("http://www.google.com/manifest.json");
+  const std::vector<favicon::FaviconURL> kManifestIcons = {
+      FaviconURL(kIconURL64x64, FAVICON, kEmptySizes),
+  };
+
+  delegate_.fake_manifest_downloader().Add(kManifestURL, kManifestIcons);
+
+  auto handler = base::MakeUnique<FaviconHandler>(
+      &favicon_service_, &delegate_, FaviconDriverObserver::NON_TOUCH_16_DIP);
+  handler->FetchFavicon(kPageURL);
+  ASSERT_THAT(favicon_service_.fake()->db_requests(), ElementsAre(kPageURL));
+  base::RunLoop().RunUntilIdle();
+  ASSERT_FALSE(handler->HasPendingTasksForTest());
+  handler->OnUpdateCandidates(kPageURL,
+                              {FaviconURL(kIconURL12x12, FAVICON, kEmptySizes)},
+                              base::nullopt);
+  ASSERT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kIconURL12x12));
+  // Database lookup for |kIconURL12x12| is ongoing.
+  ASSERT_TRUE(handler->HasPendingTasksForTest());
+
+  // Simulate the page changing it's manifest URL to |kManifestURL| via
+  // Javascript.
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kManifestURL, _, _));
+  handler->OnUpdateCandidates(kPageURL,
+                              {FaviconURL(kIconURL12x12, FAVICON, kEmptySizes)},
+                              kManifestURL);
+  base::RunLoop().RunUntilIdle();
+  ASSERT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kIconURL12x12, kManifestURL));
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kManifestURL, kIconURL64x64));
+}
+
 }  // namespace
 }  // namespace favicon