Add support to process favicons from Web Manifests

components/favicon/core/favicon_handler_unittest.cc

Index: components/favicon/core/favicon_handler_unittest.cc
diff --git a/components/favicon/core/favicon_handler_unittest.cc b/components/favicon/core/favicon_handler_unittest.cc
index 5e651a646febcaca5d9b28ba8b03e129f4fb5da8..5775b4fbfacb536658f027914720a187214004a6 100644
--- a/components/favicon/core/favicon_handler_unittest.cc
+++ b/components/favicon/core/favicon_handler_unittest.cc
@@ -51,6 +51,11 @@ MATCHER_P2(ImageSizeIs, width, height, "") {
   return arg.Size() == gfx::Size(width, height);
 }
 
+ACTION_P2(PostDownloadReply, status_code, candidates) {
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::Bind(arg1, status_code, candidates));
+}
+
 // Fill the given bmp with some test data.
 SkBitmap CreateBitmapWithEdgeSize(int size) {
   SkBitmap bmp;
@@ -202,6 +207,8 @@ class MockDelegate : public FaviconHandler::Delegate {
                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,
@@ -350,14 +357,15 @@ class FaviconHandlerTest : public testing::Test {
   // Returns the handler in case tests want to exercise further steps.
   std::unique_ptr<FaviconHandler> RunHandlerWithCandidates(
       FaviconDriverObserver::NotificationIconType handler_type,
-      const std::vector<favicon::FaviconURL>& candidates) {
+      const std::vector<favicon::FaviconURL>& candidates,
+      const base::Optional<GURL>& manifest_url = base::nullopt) {
     auto handler = base::MakeUnique<FaviconHandler>(&favicon_service_,
                                                     &delegate_, handler_type);
     handler->FetchFavicon(kPageURL);
     // The first RunUntilIdle() causes the FaviconService lookups be faster than
-    // OnUpdateFaviconURL(), which is the most likely scenario.
+    // OnUpdateCandidates(), which is the most likely scenario.
     base::RunLoop().RunUntilIdle();
-    handler->OnUpdateFaviconURL(kPageURL, candidates);
+    handler->OnUpdateCandidates(kPageURL, candidates, manifest_url);
     base::RunLoop().RunUntilIdle();
     return handler;
   }
@@ -365,13 +373,14 @@ class FaviconHandlerTest : public testing::Test {
   // Same as above, but for the simplest case where all types are FAVICON and
   // no sizes are provided, using a FaviconHandler of type NON_TOUCH_16_DIP.
   std::unique_ptr<FaviconHandler> RunHandlerWithSimpleFaviconCandidates(
-      const std::vector<GURL>& urls) {
+      const std::vector<GURL>& urls,
+      const base::Optional<GURL>& manifest_url = base::nullopt) {
     std::vector<favicon::FaviconURL> candidates;
     for (const GURL& url : urls) {
       candidates.emplace_back(url, FAVICON, kEmptySizes);
     }
     return RunHandlerWithCandidates(FaviconDriverObserver::NON_TOUCH_16_DIP,
-                                    candidates);
+                                    candidates, manifest_url);
   }
 
   base::MessageLoopForUI message_loop_;
@@ -409,7 +418,7 @@ TEST_F(FaviconHandlerTest, UpdateFaviconMappingsAndFetch) {
 // - There is data in the database for neither the page URL nor the icon URL.
 // AND
 // - FaviconService::GetFaviconForPageURL() callback returns before
-//   FaviconHandler::OnUpdateFaviconURL() is called.
+//   FaviconHandler::OnUpdateCandidates() is called.
 TEST_F(FaviconHandlerTest, DownloadUnknownFaviconIfCandidatesSlower) {
   EXPECT_CALL(favicon_service_, SetFavicons(kPageURL, kIconURL16x16, FAVICON,
                                             ImageSizeIs(16, 16)));
@@ -420,10 +429,11 @@ TEST_F(FaviconHandlerTest, DownloadUnknownFaviconIfCandidatesSlower) {
   FaviconHandler handler(&favicon_service_, &delegate_,
                          FaviconDriverObserver::NON_TOUCH_16_DIP);
   handler.FetchFavicon(kPageURL);
-  // Causes FaviconService lookups be faster than OnUpdateFaviconURL().
+  // Causes FaviconService lookups be faster than OnUpdateCandidates().
   base::RunLoop().RunUntilIdle();
-  handler.OnUpdateFaviconURL(kPageURL,
-                             {FaviconURL(kIconURL16x16, FAVICON, kEmptySizes)});
+  handler.OnUpdateCandidates(kPageURL,
+                             {FaviconURL(kIconURL16x16, FAVICON, kEmptySizes)},
+                             base::nullopt);
   base::RunLoop().RunUntilIdle();
 
   EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL16x16));
@@ -435,7 +445,7 @@ TEST_F(FaviconHandlerTest, DownloadUnknownFaviconIfCandidatesSlower) {
 // - There is data in the database for neither the page URL nor the icon URL.
 // AND
 // - FaviconService::GetFaviconForPageURL() callback returns after
-//   FaviconHandler::OnUpdateFaviconURL() is called.
+//   FaviconHandler::OnUpdateCandidates() is called.
 TEST_F(FaviconHandlerTest, DownloadUnknownFaviconIfCandidatesFaster) {
   EXPECT_CALL(favicon_service_, SetFavicons(kPageURL, kIconURL16x16, FAVICON,
                                             ImageSizeIs(16, 16)));
@@ -447,8 +457,9 @@ TEST_F(FaviconHandlerTest, DownloadUnknownFaviconIfCandidatesFaster) {
   ASSERT_THAT(favicon_service_.fake()->db_requests(), ElementsAre(kPageURL));
 
   // Feed in favicons without processing posted tasks (RunUntilIdle()).
-  handler.OnUpdateFaviconURL(kPageURL,
-                             {FaviconURL(kIconURL16x16, FAVICON, kEmptySizes)});
+  handler.OnUpdateCandidates(kPageURL,
+                             {FaviconURL(kIconURL16x16, FAVICON, kEmptySizes)},
+                             base::nullopt);
   base::RunLoop().RunUntilIdle();
 
   EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL16x16));
@@ -609,7 +620,7 @@ TEST_F(FaviconHandlerTest, Download2ndFaviconURLCandidate) {
 
 // Test that download data for icon URLs other than the current favicon
 // candidate URLs is ignored. This test tests the scenario where a download is
-// in flight when FaviconHandler::OnUpdateFaviconURL() is called.
+// in flight when FaviconHandler::OnUpdateCandidates() is called.
 // TODO(mastiz): Make this test deal with FaviconURLs of type
 // favicon_base::FAVICON and add new ones like OnlyDownloadMatchingIconType and
 // CallSetFaviconsWithCorrectIconType.
@@ -635,8 +646,8 @@ TEST_F(FaviconHandlerTest, UpdateDuringDownloading) {
   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.
@@ -670,9 +681,9 @@ TEST_F(FaviconHandlerTest, UpdateSameIconURLsWhileProcessingShouldBeNoop) {
   ASSERT_TRUE(VerifyAndClearExpectations());
   ASSERT_TRUE(delegate_.fake_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.
@@ -697,11 +708,11 @@ TEST_F(FaviconHandlerTest, UpdateSameIconURLsAfterFinishedShouldBeNoop) {
 
   ASSERT_TRUE(VerifyAndClearExpectations());
 
-  // Calling OnUpdateFaviconURL() with identical data should be a no-op.
+  // Calling OnUpdateCandidates() with identical data should be a no-op.
   EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, _, _, _)).Times(0);
   EXPECT_CALL(favicon_service_, SetFavicons(_, _, _, _)).Times(0);
 
-  handler->OnUpdateFaviconURL(kPageURL, favicon_urls);
+  handler->OnUpdateCandidates(kPageURL, favicon_urls, base::nullopt);
   base::RunLoop().RunUntilIdle();
   EXPECT_THAT(favicon_service_.fake()->db_requests(), IsEmpty());
   EXPECT_THAT(delegate_.downloads(), IsEmpty());
@@ -743,8 +754,8 @@ TEST_F(FaviconHandlerTest,
   // Simulate the page changing it's icon URL to just |kIconURL2| via
   // Javascript.
   EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, kIconURL2, _, _));
-  handler->OnUpdateFaviconURL(kPageURL,
-                              {FaviconURL(kIconURL2, FAVICON, kEmptySizes)});
+  handler->OnUpdateCandidates(
+      kPageURL, {FaviconURL(kIconURL2, FAVICON, kEmptySizes)}, base::nullopt);
   base::RunLoop().RunUntilIdle();
 }
 
@@ -1031,5 +1042,200 @@ TEST_F(FaviconHandlerTest, TestKeepDownloadedLargestFavicon) {
        FaviconURL(kIconURL16x16, FAVICON, kEmptySizes)});
 }
 
+TEST_F(FaviconHandlerTest, GetFaviconFromManifestInHistory) {
+  const GURL kManifestURL("http://www.google.com/manifest.json");
+
+  favicon_service_.fake()->Store(kManifestURL, kIconURL16x16,
+                                 CreateRawBitmapResult(kIconURL16x16));

[pkotwicz, 2017/04/12 22:10:07]

This test looks like it is left over from when the "manifest URL" was used as
the "page URL"

[mastiz, 2017/04/20 18:06:33]

Done.

+
+  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, /*icon_url_changed=*/true, _));
+
+  RunHandlerWithSimpleFaviconCandidates({kIconURL12x12}, kManifestURL);
+  EXPECT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kManifestURL));
+  EXPECT_THAT(delegate_.downloads(), IsEmpty());
+}
+
+TEST_F(FaviconHandlerTest, GetFaviconFromManifestInHistoryIfCandidatesFaster) {

[pkotwicz, 2017/04/12 22:10:07]

This test looks like it is left over from when the "manifest URL" was used as
the "page URL"

[mastiz, 2017/04/20 18:06:33]

Done.

+  const GURL kManifestURL("http://www.google.com/manifest.json");
+
+  favicon_service_.fake()->Store(kManifestURL, kIconURL16x16,
+                                 CreateRawBitmapResult(kIconURL16x16));
+
+  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, /*icon_url_changed=*/true, _));
+
+  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),
+  };
+
+  EXPECT_CALL(favicon_service_, UnableToDownloadFavicon(_)).Times(0);
+
+  EXPECT_CALL(delegate_, DownloadManifest(kManifestURL, _))
+      .WillOnce(PostDownloadReply(200, kManifestIcons));
+  EXPECT_CALL(favicon_service_,
+              SetFavicons(kPageURL, kManifestURL, FAVICON, _));
+  EXPECT_CALL(delegate_, OnFaviconUpdated(kPageURL, _, kManifestURL, _, _));
+
+  RunHandlerWithSimpleFaviconCandidates({kIconURL12x12}, kManifestURL);
+  EXPECT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kManifestURL));
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL16x16));
+}
+
+TEST_F(FaviconHandlerTest, GetFaviconFromExpiredManifestIcon) {
+  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));
+
+  EXPECT_CALL(delegate_, OnFaviconUpdated(kPageURL, _, kManifestURL, _, _))
+      .Times(2);
+  EXPECT_CALL(delegate_, DownloadManifest(kManifestURL, _))
+      .WillOnce(PostDownloadReply(200, kManifestIcons));
+  EXPECT_CALL(favicon_service_, SetFavicons(kPageURL, kManifestURL, _, _));
+
+  RunHandlerWithSimpleFaviconCandidates({kIconURL12x12}, kManifestURL);
+  EXPECT_THAT(favicon_service_.fake()->db_requests(), ElementsAre(kPageURL));
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL64x64));
+}
+
+TEST_F(FaviconHandlerTest, UnknownManifestReturning404) {
+  const GURL kManifestURL("http://www.google.com/manifest.json");
+
+  ON_CALL(delegate_, DownloadManifest(kManifestURL, _))
+      .WillByDefault(
+          PostDownloadReply(404, std::vector<favicon::FaviconURL>()));
+
+  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(kIconURL12x12));
+}
+
+TEST_F(FaviconHandlerTest, UnknownManifestReturning503) {
+  const GURL kManifestURL("http://www.google.com/manifest.json");
+
+  ON_CALL(delegate_, DownloadManifest(kManifestURL, _))
+      .WillByDefault(
+          PostDownloadReply(503, std::vector<favicon::FaviconURL>()));
+
+  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);
+  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_F(FaviconHandlerTest, UnknownManifestWithoutIcons) {
+  const GURL kManifestURL("http://www.google.com/manifest.json");
+
+  ON_CALL(delegate_, DownloadManifest(kManifestURL, _))
+      .WillByDefault(
+          PostDownloadReply(200, 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(kIconURL12x12));
+}
+
+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),
+  };
+
+  ON_CALL(delegate_, DownloadManifest(kManifestURL1, _))
+      .WillByDefault(PostDownloadReply(200, kManifestIcons1));
+  ON_CALL(delegate_, DownloadManifest(kManifestURL2, _))
+      .WillByDefault(PostDownloadReply(200, kManifestIcons2));
+
+  EXPECT_CALL(delegate_, DownloadManifest(kManifestURL1, _));
+  std::unique_ptr<FaviconHandler> handler =
+      RunHandlerWithSimpleFaviconCandidates({kIconURL12x12}, kManifestURL1);
+  ASSERT_THAT(favicon_service_.fake()->db_requests(),
+              ElementsAre(kPageURL, kManifestURL1));
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL64x64));
+  ASSERT_TRUE(VerifyAndClearExpectations());
+
+  // Simulate the page changing it's manifest URL via Javascript.
+  EXPECT_CALL(delegate_, DownloadManifest(kManifestURL2, _));
+  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(kIconURL10x10));
+}
+
+// TODO(mastiz) / DONOTSUBMIT: Add missing tests.

[pkotwicz, 2017/04/12 22:10:07]

Can you please add a test for when:
- There is 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 knows about the icon URL

[mastiz, 2017/04/20 18:06:33]

Done, UnknownManifestWithoutIconsAndKnownRegularIcons.

+
 }  // namespace
 }  // namespace favicon