Add support to process favicons from Web Manifests

components/favicon/core/favicon_handler_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/favicon/core/favicon_handler.h"
 
 #include <stddef.h>
 
 #include <map>
 #include <memory>
@@ skipping 86 matching lines @@
 // Fake that implements the calls to FaviconHandler::Delegate's DownloadImage(),
 // delegated to this class through MockDelegate.
 class FakeImageDownloader {
  public:
   struct Response {
     int http_status_code = 404;
     BitmapVector bitmaps;
     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_++;
     base::Closure bound_callback =
         base::Bind(callback, download_id, response.http_status_code, url,
                    response.bitmaps, response.original_bitmap_sizes);
     if (url == manual_callback_url_)
       manual_callback_ = bound_callback;
     else
       base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, bound_callback);
@@ skipping 37 matching lines @@
   // 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;
   }
 
-  // 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_;
 
   // Callback for DownloadImage() request for |manual_callback_url_|.
   base::Closure manual_callback_;
 
   // Registered responses.
   std::map<GURL, Response> responses_;
 
   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,
                void(const GURL& page_url,
                     FaviconDriverObserver::NotificationIconType type,
                     const GURL& icon_url,
                     bool icon_url_changed,
                     const gfx::Image& image));
 
-  FakeImageDownloader& fake_downloader() { return fake_downloader_; }
+  FakeImageDownloader& fake_image_downloader() {
+    return fake_image_downloader_;
+  }
 
-  // Convenience getter for test readability. Returns pending and completed
-  // download URLs.
-  const URLVector& downloads() const { return fake_downloader_.downloads(); }
+  FakeManifestDownloader& fake_manifest_downloader() {
+    return fake_manifest_downloader_;
+  }
+
+  // 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
 // test data stored via Store(). If Store() has not been called for a
 // particular URL, the callback is called with empty database results.
 class FakeFaviconService {
  public:
   FakeFaviconService() = default;
 
   // Stores favicon with bitmap data in |results| at |page_url| and |icon_url|.
@@ skipping 85 matching lines @@
 
   // Some known icons for which download will succeed.
   const GURL kPageURL = GURL("http://www.google.com");
   const GURL kIconURL10x10 = GURL("http://www.google.com/favicon10x10");
   const GURL kIconURL12x12 = GURL("http://www.google.com/favicon12x12");
   const GURL kIconURL16x16 = GURL("http://www.google.com/favicon16x16");
   const GURL kIconURL64x64 = GURL("http://www.google.com/favicon64x64");
 
   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
     // a downloaded bitmap in FaviconHandler::OnDidDownloadFavicon().
     // Force the values of the scale factors so that the tests produce the same
     // results on all platforms.
     scoped_set_supported_scale_factors_.reset(
         new ui::test::ScopedSetSupportedScaleFactors({ui::SCALE_FACTOR_100P}));
   }
 
   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_);
   }
 
   // Creates a new handler and feeds in the page URL and the candidates.
   // 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;
   }
 
   // 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_;
   std::unique_ptr<ui::test::ScopedSetSupportedScaleFactors>
       scoped_set_supported_scale_factors_;
   testing::NiceMock<MockFaviconServiceWithFake> favicon_service_;
   testing::NiceMock<MockDelegate> delegate_;
 };
 
 TEST_F(FaviconHandlerTest, GetFaviconFromHistory) {
@@ skipping 24 matching lines @@
                                     kPageURL, URLVector{kIconURL16x16}, FAVICON,
                                     /*desired_size_in_dip=*/16, _, _));
 
   RunHandlerWithSimpleFaviconCandidates({kIconURL16x16});
 }
 
 // Test that the FaviconHandler process finishes when:
 // - 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)));
   EXPECT_CALL(delegate_, OnFaviconUpdated(
                              kPageURL, FaviconDriverObserver::NON_TOUCH_16_DIP,
                              kIconURL16x16, /*icon_url_changed=*/true, _));
 
   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));
   EXPECT_THAT(favicon_service_.fake()->db_requests(),
               ElementsAre(kPageURL, kIconURL16x16));
 }
 
 // Test that the FaviconHandler process finishes when:
 // - 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)));
   EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL16x16, _, _));
 
   FaviconHandler handler(&favicon_service_, &delegate_,
                          FaviconDriverObserver::NON_TOUCH_16_DIP);
   handler.FetchFavicon(kPageURL);
   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));
 }
 
 // Test that the FaviconHandler process does not save anything to the database
 // for incognito tabs.
 TEST_F(FaviconHandlerTest, DownloadUnknownFaviconInIncognito) {
   ON_CALL(delegate_, IsOffTheRecord()).WillByDefault(Return(true));
 
@@ skipping 110 matching lines @@
 
   RunHandlerWithSimpleFaviconCandidates({kNewIconURL});
   EXPECT_THAT(favicon_service_.fake()->db_requests(),
               ElementsAre(kPageURL, kNewIconURL));
   EXPECT_THAT(delegate_.downloads(), IsEmpty());
 }
 
 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,
       CreateRawBitmapResult(kIconURL64x64, TOUCH_ICON,
                             /*expired=*/true));
 
   EXPECT_CALL(delegate_,
               OnFaviconUpdated(kPageURL, FaviconDriverObserver::TOUCH_LARGEST,
                                kIconURL64x64, /*icon_url_changed=*/true, _));
   EXPECT_CALL(delegate_,
               OnFaviconUpdated(kPageURL, FaviconDriverObserver::TOUCH_LARGEST,
                                kIconURL64x64, /*icon_url_changed=*/false, _));
 
   RunHandlerWithCandidates(
       FaviconDriverObserver::TOUCH_LARGEST,
       {
           FaviconURL(kIconURLReturning500, TOUCH_PRECOMPOSED_ICON, kEmptySizes),
           FaviconURL(kIconURL64x64, TOUCH_ICON, kEmptySizes),
       });
   // First download fails, second succeeds.
   EXPECT_THAT(delegate_.downloads(),
               ElementsAre(kIconURLReturning500, kIconURL64x64));
 }
 
 // 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.
 TEST_F(FaviconHandlerTest, UpdateDuringDownloading) {
   const GURL kIconURL1("http://www.google.com/favicon");
   const GURL kIconURL2 = kIconURL16x16;
   const GURL kIconURL3 = kIconURL64x64;
 
   // 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));
   EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL3));
 }
 
 // Test that sending an icon URL update identical to the previous icon URL
 // update is a no-op.
 TEST_F(FaviconHandlerTest, UpdateSameIconURLsWhileProcessingShouldBeNoop) {
   const GURL kSlowLoadingIconURL("http://www.google.com/slow_favicon");
 
   const std::vector<FaviconURL> favicon_urls = {
       FaviconURL(kIconURL64x64, FAVICON, kEmptySizes),
       FaviconURL(kSlowLoadingIconURL, FAVICON, kEmptySizes),
   };
 
   // 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);
 
   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());
 }
 
 // Test that calling OnUpdateFaviconUrl() with the same icon URLs as before is a
 // no-op. This is important because OnUpdateFaviconUrl() is called when the page
 // finishes loading. This can occur several times for pages with iframes.
 TEST_F(FaviconHandlerTest, UpdateSameIconURLsAfterFinishedShouldBeNoop) {
   const std::vector<FaviconURL> favicon_urls = {
       FaviconURL(kIconURL10x10, FAVICON, kEmptySizes),
       FaviconURL(kIconURL16x16, FAVICON, kEmptySizes),
   };
 
   std::unique_ptr<FaviconHandler> handler = RunHandlerWithCandidates(
       FaviconDriverObserver::NON_TOUCH_16_DIP, favicon_urls);
 
   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());
 }
 
 // Fixes crbug.com/544560
 // Tests that Delegate::OnFaviconUpdated() is called if:
 // - The best icon on the initial page is not the last icon.
 // - All of the initial page's icons are downloaded.
 // AND
 // - JavaScript modifies the page's <link rel="icon"> tags to contain only the
 //   last icon.
 TEST_F(FaviconHandlerTest,
        OnFaviconAvailableNotificationSentAfterIconURLChange) {
   const GURL kIconURL1(
       "http://wwww.page_which_animates_favicon.com/frame1.png");
   const GURL kIconURL2(
       "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.
   EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL1, _, _));
 
   std::unique_ptr<FaviconHandler> handler =
       RunHandlerWithSimpleFaviconCandidates({kIconURL1, kIconURL2});
 
   // Both |kIconURL1| and |kIconURL2| should have been requested from the
   // database and downloaded. |kIconURL2| should have been fetched from the
   // database and downloaded last.
   ASSERT_THAT(delegate_.downloads(), ElementsAre(kIconURL1, kIconURL2));
   ASSERT_THAT(favicon_service_.fake()->db_requests(),
               ElementsAre(kPageURL, kIconURL1, kIconURL2));
   ASSERT_TRUE(VerifyAndClearExpectations());
 
   // 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();
 }
 
 // Test the favicon which is selected when the web page provides several
 // favicons and none of the favicons are cached in history.
 // The goal of this test is to be more of an integration test than
 // SelectFaviconFramesTest.*.
 class FaviconHandlerMultipleFaviconsTest : public FaviconHandlerTest {
  protected:
   FaviconHandlerMultipleFaviconsTest() {
@@ skipping 16 matching lines @@
   // Returns the chosen size among |candidate_icon_sizes| or -1 if none was
   // chosen.
   int DownloadTillDoneIgnoringHistory(const IntVector& candidate_icon_sizes) {
     std::vector<FaviconURL> candidate_icons;
     int chosen_icon_size = -1;
 
     for (int icon_size : candidate_icon_sizes) {
       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);
 
       ON_CALL(delegate_, OnFaviconUpdated(_, _, icon_url, _, _))
           .WillByDefault(Assign(&chosen_icon_size, icon_size));
     }
 
     RunHandlerWithCandidates(FaviconDriverObserver::NON_TOUCH_16_DIP,
                              candidate_icons);
     return chosen_icon_size;
@@ skipping 35 matching lines @@
 
   RunHandlerWithSimpleFaviconCandidates({k404IconURL});
   EXPECT_THAT(delegate_.downloads(), ElementsAre(k404IconURL));
 }
 
 // Test that WasUnableToDownloadFavicon() is not called if a download returns
 // HTTP status 503.
 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);
 
   RunHandlerWithSimpleFaviconCandidates({k503IconURL});
   EXPECT_THAT(delegate_.downloads(), ElementsAre(k503IconURL));
 }
 
 // Test that the best favicon is selected when:
 // - The page provides several favicons.
 // - Downloading one of the page's icon URLs previously returned a 404.
@@ skipping 112 matching lines @@
   // last.
   EXPECT_THAT(delegate_.downloads(),
               ElementsAre(kIconURL1024_512, kIconURL16_512, kIconURL15_14,
                           kIconURLWithoutSize1, kIconURLWithoutSize2));
 }
 
 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_,
               OnFaviconUpdated(_, FaviconDriverObserver::NON_TOUCH_LARGEST,
                                kIconURL2, _, _));
 
   RunHandlerWithCandidates(
       FaviconDriverObserver::NON_TOUCH_LARGEST,
       {FaviconURL(kIconURL1, FAVICON, {gfx::Size(15, 15)}),
        FaviconURL(kIconURL2, FAVICON, {gfx::Size(14, 14), gfx::Size(16, 16)})});
 
   EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL2));
 }
 
 TEST_F(FaviconHandlerTest, TestFaviconWasScaledAfterDownload) {
   const int kMaximalSize = FaviconHandler::GetMaximalIconSize(
       FaviconDriverObserver::NON_TOUCH_LARGEST);
 
   const GURL kIconURL1("http://www.google.com/b");
   const GURL kIconURL2("http://www.google.com/c");
 
   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|)
   // and that it was scaled down to |kMaximalSize|.
   EXPECT_CALL(delegate_,
               OnFaviconUpdated(_, _, kIconURL1, _,
                                ImageSizeIs(kMaximalSize, kMaximalSize)));
 
   RunHandlerWithCandidates(
       FaviconDriverObserver::NON_TOUCH_LARGEST,
@@ skipping 139 matching lines @@
 
   EXPECT_THAT(
       histogram_tester.GetAllSamples("Favicons.DownloadAttempts.Favicons"),
       ElementsAre(base::Bucket(/*sample=*/1, /*expected_count=*/1)));
 }
 
 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));
 
   RunHandlerWithSimpleFaviconCandidates({k404IconURL});
 
   EXPECT_THAT(
       histogram_tester.GetAllSamples("Favicons.DownloadOutcome"),
       ElementsAre(base::Bucket(static_cast<int>(DownloadOutcome::FAILED),
                                /*expected_count=*/1)));
 }
 
 TEST_F(FaviconHandlerTest, TestRecordSkippedDownloadForKnownFailingUrl) {
   base::HistogramTester histogram_tester;
   const GURL k404IconURL("http://www.google.com/404.png");
 
   ON_CALL(favicon_service_, WasUnableToDownloadFavicon(k404IconURL))
       .WillByDefault(Return(true));
 
   RunHandlerWithSimpleFaviconCandidates({k404IconURL});
 
   EXPECT_THAT(
       histogram_tester.GetAllSamples("Favicons.DownloadOutcome"),
       ElementsAre(base::Bucket(static_cast<int>(DownloadOutcome::SKIPPED),
                                /*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