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>
 #include <set>
 #include <vector>
 
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/run_loop.h"
 #include "base/strings/stringprintf.h"
 #include "base/test/histogram_tester.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/test/scoped_task_environment.h"
 #include "base/test/test_simple_task_runner.h"
 #include "components/favicon/core/favicon_driver.h"
 #include "components/favicon/core/test/mock_favicon_service.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "ui/base/layout.h"
 #include "ui/gfx/codec/png_codec.h"
 #include "ui/gfx/favicon_size.h"
 #include "ui/gfx/image/image.h"
 
 namespace favicon {
 namespace {
 
 using favicon_base::FAVICON;
 using favicon_base::FaviconRawBitmapResult;
 using favicon_base::TOUCH_ICON;
 using favicon_base::TOUCH_PRECOMPOSED_ICON;
+using testing::AnyNumber;
 using testing::Assign;
+using testing::Contains;
 using testing::ElementsAre;
 using testing::InSequence;
 using testing::Invoke;
 using testing::IsEmpty;
+using testing::Not;
 using testing::Return;
 using testing::_;
 
 using DownloadOutcome = FaviconHandler::DownloadOutcome;
 using IntVector = std::vector<int>;
 using URLVector = std::vector<GURL>;
 using BitmapVector = std::vector<SkBitmap>;
 using SizeVector = std::vector<gfx::Size>;
 
 MATCHER_P2(ImageSizeIs, width, height, "") {
@@ skipping 56 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;
+      manual_callbacks_.push_back(bound_callback);
     else
       base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, bound_callback);
     return download_id;
   }
 
   void Add(const GURL& icon_url,
            const IntVector& sizes,
            const IntVector& original_sizes,
            SkColor color) {
     DCHECK_EQ(sizes.size(), original_sizes.size());
@@ skipping 12 matching lines @@
   }
 
   void AddError(const GURL& icon_url, int http_status_code) {
     Response response;
     response.http_status_code = http_status_code;
     responses_[icon_url] = response;
   }
 
   // Disables automatic callback for |url|. This is useful for emulating a
   // download taking a long time. The callback for DownloadImage() will be
-  // stored in |manual_callback_|.
+  // stored in |manual_callbacks_|.
   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(); }
+  bool HasPendingManualCallback() { return !manual_callbacks_.empty(); }
 
-  // Triggers the response for a download previously selected for manual
-  // triggering via SetRunCallbackManuallyForUrl().
+  // Triggers responses for downloads previously selected for manual triggering
+  // via SetRunCallbackManuallyForUrl().
   bool RunCallbackManually() {
     if (!HasPendingManualCallback())
       return false;
-    manual_callback_.Run();
-    manual_callback_.Reset();
+    for (base::Closure& callback : std::move(manual_callbacks_))
+      callback.Run();
     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_;
+  std::vector<base::Closure> manual_callbacks_;
 
   // 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 {
+    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.favicon_urls);
+    if (url == manual_callback_url_)
+      manual_callbacks_.push_back(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.favicon_urls = favicon_urls;
+    responses_[manifest_url] = response;
+  }
+
+  void AddError(const GURL& manifest_url) {
+    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_callbacks_.empty(); }
+
+  // Triggers responses for downloads previously selected for manual triggering
+  // via SetRunCallbackManuallyForUrl().
+  bool RunCallbackManually() {
+    if (!HasPendingManualCallback())
+      return false;
+    for (base::Closure& callback : std::move(manual_callbacks_))
+      callback.Run();
+    return true;
+  }
+
+ private:
+  URLVector* downloads_;
+
+  // URL to disable automatic callbacks for.
+  GURL manual_callback_url_;
+
+  // Callback for DownloadManifest() request for |manual_callback_url_|.
+  std::vector<base::Closure> manual_callbacks_;
+
+  // 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_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_image_downloader() {
     return fake_image_downloader_;
   }
 
-  // Convenience getter for test readability. Returns pending and completed
-  // download URLs.
-  const URLVector& downloads() const {
-    return fake_image_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:
+  // 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()
       : manual_callback_task_runner_(new base::TestSimpleTaskRunner()) {}
 
@@ skipping 143 matching lines @@
     // 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_image_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 GURL& manifest_url = GURL()) {
     auto handler = base::MakeUnique<FaviconHandler>(&favicon_service_,
                                                     &delegate_, handler_type);
     handler->FetchFavicon(kPageURL);
     // The first RunUntilIdle() causes the FaviconService lookups be faster than
     // OnUpdateCandidates(), which is the most likely scenario.
     base::RunLoop().RunUntilIdle();
-    handler->OnUpdateCandidates(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 GURL& manifest_url = GURL()) {
     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::test::ScopedTaskEnvironment scoped_task_environment_;
   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 48 matching lines @@
   // Causes FaviconService lookups be faster than OnUpdateCandidates().
   ASSERT_TRUE(favicon_service_.fake()->RunCallbackManually());
   ASSERT_TRUE(VerifyAndClearExpectations());
 
   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, _));
   // Feed in favicons now that the database lookup is completed.
-  handler.OnUpdateCandidates(kPageURL,
-                             {FaviconURL(kIconURL16x16, FAVICON, kEmptySizes)});
+  handler.OnUpdateCandidates(
+      kPageURL, {FaviconURL(kIconURL16x16, FAVICON, kEmptySizes)}, GURL());
   base::RunLoop().RunUntilIdle();
 
   EXPECT_THAT(favicon_service_.fake()->db_requests(),
               ElementsAre(kIconURL16x16));
   EXPECT_THAT(delegate_.downloads(), ElementsAre(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::OnUpdateCandidates() is called.
 TEST_F(FaviconHandlerTest, DownloadUnknownFaviconIfCandidatesFaster) {
   // Defer the database lookup completion to control the exact timing.
   favicon_service_.fake()->SetRunCallbackManuallyForUrl(kPageURL);
 
   EXPECT_CALL(favicon_service_, SetFavicons(_, _, _, _)).Times(0);
   EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, _, _, _)).Times(0);
 
   FaviconHandler handler(&favicon_service_, &delegate_,
                          FaviconDriverObserver::NON_TOUCH_16_DIP);
   handler.FetchFavicon(kPageURL);
   base::RunLoop().RunUntilIdle();
   // Feed in favicons before completing the database lookup.
-  handler.OnUpdateCandidates(kPageURL,
-                             {FaviconURL(kIconURL16x16, FAVICON, kEmptySizes)});
+  handler.OnUpdateCandidates(
+      kPageURL, {FaviconURL(kIconURL16x16, FAVICON, kEmptySizes)}, GURL());
 
   ASSERT_TRUE(VerifyAndClearExpectations());
   // Database lookup for |kPageURL| is ongoing.
   ASSERT_TRUE(favicon_service_.fake()->HasPendingManualCallback());
 
   EXPECT_CALL(favicon_service_, SetFavicons(kPageURL, kIconURL16x16, FAVICON,
                                             ImageSizeIs(16, 16)));
   EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL16x16, _, _));
 
   // Complete the lookup for |kPageURL|.
@@ skipping 193 matching lines @@
   std::unique_ptr<FaviconHandler> handler =
       RunHandlerWithSimpleFaviconCandidates({kIconURL1, kIconURL2});
 
   ASSERT_TRUE(VerifyAndClearExpectations());
   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->OnUpdateCandidates(kPageURL,
-                              {FaviconURL(kIconURL3, FAVICON, kEmptySizes)});
+  handler->OnUpdateCandidates(
+      kPageURL, {FaviconURL(kIconURL3, FAVICON, kEmptySizes)}, GURL());
 
   // Finalizes download, which should be thrown away as the favicon URLs were
   // updated.
   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));
 }
 
@@ skipping 15 matching lines @@
       RunHandlerWithSimpleFaviconCandidates(URLVector{kIconURL1});
 
   ASSERT_TRUE(VerifyAndClearExpectations());
   ASSERT_TRUE(favicon_service_.fake()->HasPendingManualCallback());
 
   // SetFavicons() and OnFaviconUpdated() should be called for the new icon URL
   // and not |kIconURL1|.
   EXPECT_CALL(favicon_service_, SetFavicons(_, kIconURL2, _, _));
   EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL2, _, _));
 
-  handler->OnUpdateCandidates(kPageURL,
-                              {FaviconURL(kIconURL2, FAVICON, kEmptySizes)});
+  handler->OnUpdateCandidates(
+      kPageURL, {FaviconURL(kIconURL2, FAVICON, kEmptySizes)}, GURL());
 
   // Finalizes the DB lookup, which should be thrown away as the favicon URLs
   // were updated.
   EXPECT_TRUE(favicon_service_.fake()->RunCallbackManually());
   base::RunLoop().RunUntilIdle();
 
   EXPECT_THAT(favicon_service_.fake()->db_requests(), ElementsAre(kIconURL2));
   EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL2));
 }
 
@@ skipping 16 matching lines @@
   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_image_downloader().HasPendingManualCallback());
 
   // Calling OnUpdateCandidates() with the same icon URLs should have no effect,
   // despite the ongoing download.
-  handler->OnUpdateCandidates(kPageURL, favicon_urls);
+  handler->OnUpdateCandidates(kPageURL, favicon_urls, GURL());
   base::RunLoop().RunUntilIdle();
   EXPECT_THAT(favicon_service_.fake()->db_requests(), IsEmpty());
   EXPECT_THAT(delegate_.downloads(), IsEmpty());
 
   // Complete the download.
   EXPECT_CALL(favicon_service_, SetFavicons(_, _, _, _));
   EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, _, _, _));
   EXPECT_TRUE(delegate_.fake_image_downloader().RunCallbackManually());
   base::RunLoop().RunUntilIdle();
   EXPECT_THAT(delegate_.downloads(), IsEmpty());
@@ skipping 13 matching lines @@
 
   // Ongoing database lookup.
   ASSERT_THAT(favicon_service_.fake()->db_requests(),
               ElementsAre(kPageURL, kIconURL64x64));
   ASSERT_THAT(delegate_.downloads(), IsEmpty());
   ASSERT_TRUE(VerifyAndClearExpectations());
   ASSERT_TRUE(favicon_service_.fake()->HasPendingManualCallback());
 
   // Calling OnUpdateCandidates() with the same icon URLs should have no effect,
   // despite the ongoing DB lookup.
-  handler->OnUpdateCandidates(kPageURL, favicon_urls);
+  handler->OnUpdateCandidates(kPageURL, favicon_urls, GURL());
   base::RunLoop().RunUntilIdle();
   EXPECT_THAT(favicon_service_.fake()->db_requests(), IsEmpty());
   EXPECT_THAT(delegate_.downloads(), IsEmpty());
 
   // Complete the lookup.
   EXPECT_CALL(favicon_service_, SetFavicons(_, _, _, _));
   EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, _, _, _));
   EXPECT_TRUE(favicon_service_.fake()->RunCallbackManually());
   base::RunLoop().RunUntilIdle();
   EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL64x64));
@@ skipping 10 matching lines @@
 
   std::unique_ptr<FaviconHandler> handler = RunHandlerWithCandidates(
       FaviconDriverObserver::NON_TOUCH_16_DIP, favicon_urls);
 
   ASSERT_TRUE(VerifyAndClearExpectations());
 
   // Calling OnUpdateCandidates() with identical data should be a no-op.
   EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, _, _, _)).Times(0);
   EXPECT_CALL(favicon_service_, SetFavicons(_, _, _, _)).Times(0);
 
-  handler->OnUpdateCandidates(kPageURL, favicon_urls);
+  handler->OnUpdateCandidates(kPageURL, favicon_urls, GURL());
   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
@@ skipping 21 matching lines @@
   // 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->OnUpdateCandidates(kPageURL,
-                              {FaviconURL(kIconURL2, FAVICON, kEmptySizes)});
+  handler->OnUpdateCandidates(
+      kPageURL, {FaviconURL(kIconURL2, FAVICON, kEmptySizes)}, GURL());
   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 422 matching lines @@
       .WillByDefault(Return(true));
 
   RunHandlerWithSimpleFaviconCandidates({k404IconURL});
 
   EXPECT_THAT(
       histogram_tester.GetAllSamples("Favicons.DownloadOutcome"),
       ElementsAre(base::Bucket(static_cast<int>(DownloadOutcome::SKIPPED),
                                /*expected_count=*/1)));
 }
 
+// Test that the support for Web Manifest is disabled by default, unless the
+// feature is enabled.
+TEST_F(FaviconHandlerTest, IgnoreWebManifestByDefault) {
+  const GURL kManifestURL("http://www.google.com/manifest.json");
+
+  RunHandlerWithSimpleFaviconCandidates({kIconURL16x16}, kManifestURL);
+  EXPECT_THAT(favicon_service_.fake()->db_requests(),
+              Not(Contains(kManifestURL)));
+  EXPECT_THAT(delegate_.downloads(), Not(Contains(kManifestURL)));
+}
+
+class FaviconHandlerManifestsEnabledTest : public FaviconHandlerTest {
+ protected:
+  const GURL kManifestURL = GURL("http://www.google.com/manifest.json");
+
+  FaviconHandlerManifestsEnabledTest() {
+    override_features_.InitAndEnableFeature(kFaviconsFromWebManifest);
+  }
+
+ private:
+  base::test::ScopedFeatureList override_features_;
+
+  DISALLOW_COPY_AND_ASSIGN(FaviconHandlerManifestsEnabledTest);
+};
+
+// Test that a favicon corresponding to a web manifest is reported when:
+// - There is data in the favicon database for the manifest URL.
+// AND
+// - FaviconService::OnFaviconDataForManifestFromFaviconService() runs before
+//   FaviconHandler::OnUpdateCandidates() is called.
+TEST_F(FaviconHandlerManifestsEnabledTest,
+       GetFaviconFromManifestInHistoryIfCandidatesSlower) {
+  favicon_service_.fake()->Store(kPageURL, kManifestURL,
+                                 CreateRawBitmapResult(kManifestURL));
+
+  EXPECT_CALL(favicon_service_, UnableToDownloadFavicon(_)).Times(0);
+
+  EXPECT_CALL(favicon_service_,
+              UpdateFaviconMappingsAndFetch(_, kManifestURL, FAVICON,
+                                            /*desired_size_in_dip=*/16, _, _));
+  EXPECT_CALL(delegate_,
+              OnFaviconUpdated(_, FaviconDriverObserver::NON_TOUCH_16_DIP,
+                               kManifestURL, _, _))
+      .Times(2);

[pkotwicz, 2017/05/16 06:36:37]

In a follow up CL, it would be nice to decrease this to '1'

[mastiz, 2017/05/16 11:05:23]

Done, in this CL.

+
+  RunHandlerWithSimpleFaviconCandidates({kIconURL12x12}, kManifestURL);
+  EXPECT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kManifestURL));
+  EXPECT_THAT(delegate_.downloads(), IsEmpty());
+}
+
+// Test that a favicon corresponding to a web manifest is reported when:
+// - There is data in the favicon database for the manifest URL.
+// AND
+// - FaviconHandler::OnUpdateCandidates() is called before
+//   FaviconService::OnFaviconDataForManifestFromFaviconService() runs.
+TEST_F(FaviconHandlerManifestsEnabledTest,
+       GetFaviconFromManifestInHistoryIfCandidatesFaster) {
+  favicon_service_.fake()->Store(kPageURL, kManifestURL,
+                                 CreateRawBitmapResult(kManifestURL));
+  // Defer the database lookup completion to control the exact timing.
+  favicon_service_.fake()->SetRunCallbackManuallyForUrl(kManifestURL);
+
+  EXPECT_CALL(favicon_service_, UnableToDownloadFavicon(_)).Times(0);
+
+  EXPECT_CALL(favicon_service_,
+              UpdateFaviconMappingsAndFetch(_, kManifestURL, FAVICON,
+                                            /*desired_size_in_dip=*/16, _, _));
+  EXPECT_CALL(delegate_,
+              OnFaviconUpdated(_, FaviconDriverObserver::NON_TOUCH_16_DIP,
+                               kManifestURL, _, _));

[pkotwicz, 2017/05/16 06:36:36]

This should be Times(2) with https://codereview.chromium.org/2876263003/ applied

[mastiz, 2017/05/16 11:05:23]

I had already rebased on top of that CL.

+
+  RunHandlerWithSimpleFaviconCandidates({kIconURL12x12}, kManifestURL);

[pkotwicz, 2017/05/16 06:36:36]

You need to store the returned FaviconHandler. Otherwise the manual callback is
cancelled.

[mastiz, 2017/05/16 11:05:23]

Done.

+  ASSERT_TRUE(favicon_service_.fake()->HasPendingManualCallback());
+
+  EXPECT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kManifestURL));
+  EXPECT_THAT(delegate_.downloads(), IsEmpty());

[pkotwicz, 2017/05/16 06:36:36]

Nit: It is probably worth moving this two expectations after line 1542

[mastiz, 2017/05/16 11:05:23]

Done.

+
+  // Complete the lookup.
+  EXPECT_TRUE(favicon_service_.fake()->RunCallbackManually());
+  base::RunLoop().RunUntilIdle();
+}
+
+// Test that a favicon corresponding to a web manifest is reported when there is
+// data in the database for neither the page URL nor the manifest URL.
+TEST_F(FaviconHandlerManifestsEnabledTest, GetFaviconFromUnknownManifest) {
+  const std::vector<favicon::FaviconURL> kManifestIcons = {
+      FaviconURL(kIconURL16x16, FAVICON, kEmptySizes),
+  };
+

[pkotwicz, 2017/05/16 06:36:37]

Nit: Get rid of the new line

[mastiz, 2017/05/16 11:05:22]

All (or most) tests prior to this CL use a new line to separate local constants
from the rest.

+  delegate_.fake_manifest_downloader().Add(kManifestURL, kManifestIcons);
+
+  EXPECT_CALL(favicon_service_, UnableToDownloadFavicon(_)).Times(0);
+
+  EXPECT_CALL(favicon_service_, SetFavicons(_, kManifestURL, FAVICON, _));
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kManifestURL, _, _));
+
+  RunHandlerWithSimpleFaviconCandidates({kIconURL12x12}, kManifestURL);
+  EXPECT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kManifestURL));
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kManifestURL, kIconURL16x16));
+}
+
+// Test that the manifest and icon are redownloaded if the icon cached for the
+// page URL expired.
+TEST_F(FaviconHandlerManifestsEnabledTest, GetFaviconFromExpiredManifest) {
+  const std::vector<favicon::FaviconURL> kManifestIcons = {
+      FaviconURL(kIconURL64x64, FAVICON, kEmptySizes),
+  };
+

[pkotwicz, 2017/05/16 06:36:37]

Nit: Nuke the new line

[mastiz, 2017/05/16 11:05:23]

Ditto.

+  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(_, _, kManifestURL, _, _)).Times(3);
+  EXPECT_CALL(favicon_service_, SetFavicons(_, kManifestURL, _, _));
+
+  RunHandlerWithSimpleFaviconCandidates({kIconURL12x12}, kManifestURL);
+  EXPECT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kManifestURL));
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kManifestURL, kIconURL64x64));
+}
+
+// Test that the manifest and icon are redownloaded if the icon cached for the
+// manifest URL expired, which was observed during a visit to a different page
+// URL.
+TEST_F(FaviconHandlerManifestsEnabledTest,
+       GetFaviconFromExpiredManifestLinkedFromOtherPage) {
+  const GURL kSomePreviousPageURL("https://www.google.com/previous");
+  const std::vector<favicon::FaviconURL> kManifestIcons = {
+      FaviconURL(kIconURL64x64, FAVICON, kEmptySizes),
+  };
+

[pkotwicz, 2017/05/16 06:36:36]

Nit: nuke the new line

[mastiz, 2017/05/16 11:05:23]

Ditto.

+  favicon_service_.fake()->Store(kSomePreviousPageURL, kManifestURL,
+                                 CreateRawBitmapResult(kManifestURL, FAVICON,
+                                                       /*expired=*/true));
+  delegate_.fake_manifest_downloader().Add(kManifestURL, kManifestIcons);
+
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kManifestURL, _, _)).Times(2);
+  EXPECT_CALL(favicon_service_, SetFavicons(_, kManifestURL, _, _));
+
+  RunHandlerWithSimpleFaviconCandidates({kIconURL12x12}, kManifestURL);
+  EXPECT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kManifestURL));
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kManifestURL, kIconURL64x64));
+}
+
+// Test that a favicon corresponding to a web manifest is reported when:
+// - There is data in the database for neither the page URL nor the manifest
+//   URL.
+// - There is data in the database for the icon URL listed in the manifest.
+TEST_F(FaviconHandlerManifestsEnabledTest,
+       GetFaviconFromUnknownManifestButKnownIcon) {
+  const GURL kSomePreviousPageURL("https://www.google.com/previous");
+  const std::vector<favicon::FaviconURL> kManifestIcons = {
+      FaviconURL(kIconURL16x16, FAVICON, kEmptySizes),
+  };
+

[pkotwicz, 2017/05/16 06:36:36]

Nit: Nuke the new line

[mastiz, 2017/05/16 11:05:23]

Ditto.

+  favicon_service_.fake()->Store(kSomePreviousPageURL, kIconURL16x16,
+                                 CreateRawBitmapResult(kIconURL16x16));
+  delegate_.fake_manifest_downloader().Add(kManifestURL, kManifestIcons);
+
+  EXPECT_CALL(favicon_service_, SetFavicons(_, kManifestURL, _, _));
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kManifestURL, _, _));
+
+  RunHandlerWithSimpleFaviconCandidates(URLVector(), kManifestURL);
+  EXPECT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kManifestURL));
+  // This is because, in the current implementation, FaviconHandler only checks
+  // whether there is an icon cached with the manifest URL as the "icon URL"
+  // when a page has a non empty Web Manifest.

[pkotwicz, 2017/05/16 06:36:36]

Nit: "non empty" -> "non-empty"

[mastiz, 2017/05/16 11:05:23]

Done and change other preceding occurrences too.

+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kManifestURL, kIconURL16x16));
+}
+
+// Test that attempting to download a manifest that returns a 404 gets

[pkotwicz, 2017/05/16 06:36:36]

Nit: "attempting to download a manifest that returns a 404" -> "a manifest URL
that returns a 404"

In my version it is clearer what gets blacklisted.

[mastiz, 2017/05/16 11:05:23]

Done.

+// blacklisted via UnableToDownloadFavicon() AND that the regular favicon is
+// selected as fallback.
+TEST_F(FaviconHandlerManifestsEnabledTest, UnknownManifestReturning404) {
+  EXPECT_CALL(favicon_service_, UnableToDownloadFavicon(kManifestURL));
+  EXPECT_CALL(favicon_service_, SetFavicons(_, kIconURL12x12, _, _));
+
+  RunHandlerWithSimpleFaviconCandidates({kIconURL12x12}, kManifestURL);
+  EXPECT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kManifestURL, kIconURL12x12));
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kManifestURL, kIconURL12x12));
+}
+
+// Test that a manifest that was previously blacklisted via
+// UnableToDownloadFavicon() is ignored and that the regular favicon is selected
+// as fallback.
+TEST_F(FaviconHandlerManifestsEnabledTest, IgnoreManifestWithPrior404) {
+  ON_CALL(favicon_service_, WasUnableToDownloadFavicon(kManifestURL))
+      .WillByDefault(Return(true));
+
+  EXPECT_CALL(favicon_service_, SetFavicons(_, kIconURL12x12, _, _));
+
+  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(FaviconHandlerManifestsEnabledTest, UnknownManifestWithoutIcons) {
+  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(_, kIconURL12x12, _, _));
+
+  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(FaviconHandlerManifestsEnabledTest,
+       UnknownManifestWithoutIconsAndKnownRegularIcons) {
+  const GURL kSomePreviousPageURL("https://www.google.com/previous");
+

[pkotwicz, 2017/05/16 06:36:36]

Nit: Nuke the new line

[mastiz, 2017/05/16 11:05:23]

Same as earlier.

+  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(_, kManifestURL, _, _, _, _));
+  EXPECT_CALL(favicon_service_,
+              UpdateFaviconMappingsAndFetch(_, kIconURL12x12, _, _, _, _));
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL12x12, _, _));
+
+  RunHandlerWithSimpleFaviconCandidates({kIconURL12x12}, kManifestURL);
+  EXPECT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kManifestURL, kIconURL12x12));
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kManifestURL));
+}
+
+// Test that the database remains unmodified 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 has a mapping between the page URL to the favicon URL.
+TEST_F(FaviconHandlerManifestsEnabledTest,
+       UnknownManifestWithoutIconsAndRegularIconInHistory) {
+  delegate_.fake_manifest_downloader().Add(kManifestURL,
+                                           std::vector<favicon::FaviconURL>());
+  favicon_service_.fake()->Store(kPageURL, kIconURL16x16,
+                                 CreateRawBitmapResult(kIconURL16x16));
+
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL16x16, _, _))
+      .Times(AnyNumber());
+
+  InSequence seq;
+  EXPECT_CALL(favicon_service_,
+              UpdateFaviconMappingsAndFetch(_, kManifestURL, _, _, _, _));
+  // TODO(mastiz): This second write might never happen if for example another
+  // page is loaded.
+  EXPECT_CALL(favicon_service_,
+              UpdateFaviconMappingsAndFetch(_, kIconURL16x16, _, _, _, _));
+
+  RunHandlerWithSimpleFaviconCandidates({kIconURL16x16}, kManifestURL);
+
+  ASSERT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kManifestURL, kIconURL16x16));
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kManifestURL));
+}
+
+// Test that Delegate::OnFaviconUpdated() is called if a page uses Javascript to
+// modify the page's <link rel="manifest"> tag to point to a different manifest.
+TEST_F(FaviconHandlerManifestsEnabledTest, 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);
+

[pkotwicz, 2017/05/16 06:36:36]

For the sake of consistency with the assertions on line 1774 - 1781 you should
probably check that OnFaviconUdpated() is called in this block too

[mastiz, 2017/05/16 11:05:22]

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_, 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 that Delegate::OnFaviconUpdated() is called if a page uses Javascript to
+// remove the page's <link rel="manifest"> tag (i.e. no web manifest) WHILE a
+// lookup to the history database is ongoing for the manifest URL.
+TEST_F(FaviconHandlerManifestsEnabledTest,
+       RemoveManifestViaJavascriptWhileDatabaseLookup) {
+  const std::vector<favicon::FaviconURL> kManifestIcons = {
+      FaviconURL(kIconURL64x64, FAVICON, kEmptySizes),
+  };
+

[pkotwicz, 2017/05/16 06:36:37]

Nit: nuke new line

[mastiz, 2017/05/16 11:05:23]

Same as earlier.

+  delegate_.fake_manifest_downloader().Add(kManifestURL, kManifestIcons);
+  // Defer the database lookup completion to control the exact timing.
+  favicon_service_.fake()->SetRunCallbackManuallyForUrl(kManifestURL);
+
+  std::unique_ptr<FaviconHandler> handler =
+      RunHandlerWithSimpleFaviconCandidates({kIconURL12x12}, kManifestURL);
+  ASSERT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kManifestURL));
+  // Database lookup for |kManifestURL1| is ongoing.

[pkotwicz, 2017/05/16 06:36:35]

Nit: kManifestURL1 -> kManifestURL

[mastiz, 2017/05/16 11:05:23]

Done.

+  ASSERT_TRUE(favicon_service_.fake()->HasPendingManualCallback());
+
+  // Simulate the page changing it's manifest URL to empty via Javascript.
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL12x12, _, _));
+  handler->OnUpdateCandidates(
+      kPageURL, {FaviconURL(kIconURL12x12, FAVICON, kEmptySizes)}, GURL());
+  // Complete the lookup.
+  EXPECT_TRUE(favicon_service_.fake()->RunCallbackManually());
+  base::RunLoop().RunUntilIdle();
+  ASSERT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kManifestURL, kIconURL12x12));
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL12x12));
+}
+
+// Test that Delegate::OnFaviconUpdated() is called a page without manifest uses
+// Javascript to add a <link rel="manifest"> tag (i.e. a new web manifest) WHILE
+// a lookup to the history database is ongoing for the icon URL.
+TEST_F(FaviconHandlerManifestsEnabledTest,
+       AddManifestViaJavascriptWhileDatabaseLookup) {
+  const std::vector<favicon::FaviconURL> kManifestIcons = {
+      FaviconURL(kIconURL64x64, FAVICON, kEmptySizes),
+  };
+
+  delegate_.fake_manifest_downloader().Add(kManifestURL, kManifestIcons);
+  // Defer the database lookup completion to control the exact timing.
+  favicon_service_.fake()->SetRunCallbackManuallyForUrl(kIconURL12x12);
+
+  std::unique_ptr<FaviconHandler> handler =
+      RunHandlerWithSimpleFaviconCandidates({kIconURL12x12});
+  ASSERT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kIconURL12x12));
+  // Database lookup for |kIconURL12x12| is ongoing.
+  ASSERT_TRUE(favicon_service_.fake()->HasPendingManualCallback());
+
+  // 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);
+  // Complete the lookup.
+  EXPECT_TRUE(favicon_service_.fake()->RunCallbackManually());
+  base::RunLoop().RunUntilIdle();
+  ASSERT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kIconURL12x12, kManifestURL));
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kManifestURL, kIconURL64x64));
+}
+
+// Test that SetFavicons() is not called when:
+// - The page doesn't initially link to a Web Manifest.
+// - The page has an icon URL provided via a <link rel="icon"> tag.
+// - The database does not know about the page URL or icon URL.
+// - While the icon is being downloaded, the page uses Javascript to add a
+//   <link rel="manifest"> tag.
+// - The database has bitmap data for the manifest URL.
+TEST_F(FaviconHandlerManifestsEnabledTest,
+       AddKnownManifestViaJavascriptWhileImageDownload) {
+  const GURL kSomePreviousPageURL("https://www.google.com/previous");
+
+  favicon_service_.fake()->Store(kSomePreviousPageURL, kManifestURL,
+                                 CreateRawBitmapResult(kManifestURL));
+
+  // Defer the image download completion to control the exact timing.
+  delegate_.fake_image_downloader().SetRunCallbackManuallyForUrl(kIconURL16x16);
+
+  std::unique_ptr<FaviconHandler> handler =
+      RunHandlerWithSimpleFaviconCandidates({kIconURL16x16});
+
+  ASSERT_TRUE(VerifyAndClearExpectations());
+  ASSERT_TRUE(delegate_.fake_image_downloader().HasPendingManualCallback());
+
+  // Simulate the page changing it's manifest URL to |kManifestURL| via
+  // Javascript. Should invalidate the ongoing image download.
+  EXPECT_CALL(favicon_service_, SetFavicons(_, _, _, _)).Times(0);
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kManifestURL, _, _));
+
+  handler->OnUpdateCandidates(kPageURL,
+                              {FaviconURL(kIconURL16x16, FAVICON, kEmptySizes)},
+                              kManifestURL);
+
+  // Finalizes download, which should be thrown away as the manifest URLs was

[pkotwicz, 2017/05/16 06:36:36]

Nit: URLs -> URL

[mastiz, 2017/05/16 11:05:23]

Done.

+  // provided.
+  EXPECT_TRUE(delegate_.fake_image_downloader().RunCallbackManually());
+  base::RunLoop().RunUntilIdle();
+}
+
+// Test that SetFavicons() is not called when:
+// - The page doesn't initially link to a Web Manifest.
+// - The page has an icon URL provided via a <link rel="icon"> tag.
+// - The database does not know about the page URL or icon URL.
+// - During the database lookup, the page uses Javascript to add a
+//   <link rel="manifest"> tag.
+// - The database does not know about the manifest URL.
+// - The manifest contains no icons.
+TEST_F(FaviconHandlerManifestsEnabledTest,
+       AddManifestWithoutIconsViaJavascriptWhileDatabaseLookup) {
+  delegate_.fake_manifest_downloader().Add(kManifestURL,
+                                           std::vector<favicon::FaviconURL>());
+
+  // Defer the database lookup completion to control the exact timing.
+  favicon_service_.fake()->SetRunCallbackManuallyForUrl(kIconURL16x16);
+
+  std::unique_ptr<FaviconHandler> handler =
+      RunHandlerWithSimpleFaviconCandidates({kIconURL16x16});
+
+  ASSERT_TRUE(VerifyAndClearExpectations());
+  ASSERT_TRUE(favicon_service_.fake()->HasPendingManualCallback());
+
+  EXPECT_CALL(favicon_service_, SetFavicons(_, _, _, _)).Times(0);
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, _, _, _)).Times(0);

[pkotwicz, 2017/05/16 06:36:36]

This test seems wrong. FaviconHandler should download |kIconURL16x16| and call
SetFavicons() with |kIconURL16x16|

[mastiz, 2017/05/16 11:05:23]

Fixed, PTAL.

+
+  handler->OnUpdateCandidates(kPageURL,
+                              {FaviconURL(kIconURL16x16, FAVICON, kEmptySizes)},
+                              kManifestURL);
+
+  // Finalizes lookup, which should be thrown away as the manifest URLs was
+  // provided.
+  EXPECT_TRUE(favicon_service_.fake()->RunCallbackManually());
+  base::RunLoop().RunUntilIdle();
+}
+
+// Test that SetFavicons() is called when:
+// - The page link to one Web Manifest, which contains one icon.

[pkotwicz, 2017/05/16 06:36:36]

Nit: link -> links

[mastiz, 2017/05/16 11:05:24]

Done.

+// - The database does not know about the page URL, icon URL or manifest URL.
+// - During image download, the page updates the manifest URL to point to
+//   another manifest.
+// - The second manifest contains the same icons as the first.
+TEST_F(FaviconHandlerManifestsEnabledTest,
+       UpdateManifestWithSameIconURLsWhileDownloading) {
+  const GURL kManifestURL1("http://www.google.com/manifest1.json");
+  const GURL kManifestURL2("http://www.google.com/manifest2.json");
+  const std::vector<favicon::FaviconURL> kManifestIcons = {
+      FaviconURL(kIconURL64x64, FAVICON, kEmptySizes),
+  };
+
+  delegate_.fake_manifest_downloader().Add(kManifestURL1, kManifestIcons);
+  delegate_.fake_manifest_downloader().Add(kManifestURL2, kManifestIcons);
+
+  // Defer the download completion to control the exact timing.
+  delegate_.fake_image_downloader().SetRunCallbackManuallyForUrl(kIconURL64x64);
+
+  std::unique_ptr<FaviconHandler> handler =
+      RunHandlerWithSimpleFaviconCandidates(URLVector(), kManifestURL1);
+
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kManifestURL1, kIconURL64x64));
+  ASSERT_TRUE(VerifyAndClearExpectations());
+  ASSERT_TRUE(delegate_.fake_image_downloader().HasPendingManualCallback());
+
+  // Calling OnUpdateCandidates() with a different manifest URL should trigger
+  // its download.
+  handler->OnUpdateCandidates(kPageURL, std::vector<favicon::FaviconURL>(),
+                              kManifestURL2);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kManifestURL2, kIconURL64x64));
+
+  // Complete the download.
+  EXPECT_CALL(favicon_service_, SetFavicons(_, kManifestURL2, _, _));
+  EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kManifestURL2, _, _));
+  EXPECT_TRUE(delegate_.fake_image_downloader().RunCallbackManually());
+  base::RunLoop().RunUntilIdle();
+}
+
 }  // namespace
 }  // namespace favicon