Make FaviconService::GetRawFaviconForPageURL() select the best candidate among all the icon types

components/history/core/browser/thumbnail_database_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 <stddef.h>
 
 #include <algorithm>
 #include <vector>
 
 #include "base/files/file_enumerator.h"
@@ skipping 23 matching lines @@
     "goiwuegrqrcomizqyzkjalitbahxfjytrqvpqeroicxmnlkhlzunacxaneviawrtxcywhgef";
 
 // Page and icon urls shared by tests.  Present in golden database
 // files (see VersionN tests).
 const GURL kPageUrl1 = GURL("http://google.com/");
 const GURL kPageUrl2 = GURL("http://yahoo.com/");
 const GURL kPageUrl3 = GURL("http://www.google.com/");
 const GURL kPageUrl4 = GURL("http://www.google.com/blank.html");
 const GURL kPageUrl5 = GURL("http://www.bing.com/");
 
-const GURL kIconUrl1 = GURL("http://www.google.com/favicon.ico");
-const GURL kIconUrl2 = GURL("http://www.yahoo.com/favicon.ico");
-const GURL kIconUrl3 = GURL("http://www.google.com/touch.ico");
-const GURL kIconUrl5 = GURL("http://www.bing.com/favicon.ico");
+const GURL kIconUrl1 = GURL("http://www.bing.com/favicon.ico");
+const GURL kIconUrl2 = GURL("http://www.duckduckgo.com/favicon.ico");
+const GURL kIconUrl3 = GURL("http://www.google.com/favicon.ico");
+const GURL kIconUrl4 = GURL("http://www.google.com/touch.ico");
+const GURL kIconUrl5 = GURL("http://www.yahoo.com/favicon.ico");
 
 const gfx::Size kSmallSize = gfx::Size(16, 16);
 const gfx::Size kLargeSize = gfx::Size(32, 32);
 
 // Verify that the up-to-date database has the expected tables and
 // columns.  Functional tests only check whether the things which
 // should be there are, but do not check if extraneous items are
 // present.  Any extraneous items have the potential to interact
 // negatively with future schema changes.
 void VerifyTablesAndColumns(sql::Connection* db) {
@@ skipping 11 matching lines @@
   EXPECT_EQ(3u, sql::test::CountTableColumns(db, "favicons"));
 
   // [id], [icon_id], [last_updated], [image_data], [width], [height] and
   // [last_requested].
   EXPECT_EQ(7u, sql::test::CountTableColumns(db, "favicon_bitmaps"));
 
   // [id], [page_url], and [icon_id].
   EXPECT_EQ(3u, sql::test::CountTableColumns(db, "icon_mapping"));
 }
 
+// Adds a favicon at |icon_url| with |icon_type| with default bitmap data and
+// maps |page_url| to |icon_url|.
+void AddFaviconAndMapSimple(ThumbnailDatabase* db,

[mastiz, 2017/04/20 08:55:59]

Nit: s/Map/Mapping/?

'Map' can otherwise interpreted as verb, i.e. AddAndMap.

[pkotwicz, 2017/04/30 03:49:56]

Done.

+                            const GURL& page_url,
+                            const GURL& icon_url,
+                            favicon_base::IconType icon_type) {
+  scoped_refptr<base::RefCountedStaticMemory> data(
+      new base::RefCountedStaticMemory(kBlob1, sizeof(kBlob1)));
+  favicon_base::FaviconID favicon_id =
+      db->AddFavicon(icon_url, icon_type, data, base::Time::Now(), gfx::Size());
+  db->AddIconMapping(page_url, favicon_id);
+}
+
 void VerifyDatabaseEmpty(sql::Connection* db) {
   size_t rows = 0;
   EXPECT_TRUE(sql::test::CountTableRows(db, "favicons", &rows));
   EXPECT_EQ(0u, rows);
   EXPECT_TRUE(sql::test::CountTableRows(db, "favicon_bitmaps", &rows));
   EXPECT_EQ(0u, rows);
   EXPECT_TRUE(sql::test::CountTableRows(db, "icon_mapping", &rows));
   EXPECT_EQ(0u, rows);
 }
 
@@ skipping 51 matching lines @@
   }
 
   if (memcmp(favicon_bitmaps[0].bitmap_data->front(),
              expected_icon_contents, expected_icon_contents_size)) {
     ADD_FAILURE() << "failed to match |expected_icon_contents|";
     return false;
   }
   return true;
 }
 
+bool CompareIconMappingIconUrl(const IconMapping& a, const IconMapping& b) {
+  return a.icon_url < b.icon_url;
+}
+
+void SortMappingsByIconUrl(std::vector<IconMapping>* mappings) {
+  std::sort(mappings->begin(), mappings->end(), &CompareIconMappingIconUrl);
+}
+
 }  // namespace
 
 class ThumbnailDatabaseTest : public testing::Test {
  public:
   ThumbnailDatabaseTest() {}
   ~ThumbnailDatabaseTest() override {}
 
   // Initialize a thumbnail database instance from the SQL file at
   // |golden_path| in the "History/" subdirectory of test data.
   std::unique_ptr<ThumbnailDatabase> LoadFromGolden(const char* golden_path) {
@@ skipping 323 matching lines @@
   icon_mappings.clear();
   EXPECT_TRUE(db.GetIconMappingsForPageURL(page_url, &icon_mappings));
 
   EXPECT_EQ(page_url, icon_mappings.front().page_url);
   EXPECT_EQ(id3, icon_mappings.front().icon_id);
   EXPECT_EQ(favicon_base::TOUCH_PRECOMPOSED_ICON,
             icon_mappings.front().icon_type);
   EXPECT_EQ(icon_url, icon_mappings.front().icon_url);
 }
 
-// Test result of GetIconMappingsForPageURL when an icon type is passed in.
-TEST_F(ThumbnailDatabaseTest, GetIconMappingsForPageURLWithIconType) {
+// Test that when multiple icon types are passed to GetIconMappingsForPageURL()
+// that the results are filtered according to the passed in types.
+TEST_F(ThumbnailDatabaseTest, GetIconMappingsForPageURLWithIconTypes) {
   ThumbnailDatabase db(NULL);
   ASSERT_EQ(sql::INIT_OK, db.Init(file_name_));
   db.BeginTransaction();
 
-  GURL url("http://google.com");
-  std::vector<unsigned char> data(kBlob1, kBlob1 + sizeof(kBlob1));
-  scoped_refptr<base::RefCountedBytes> favicon(new base::RefCountedBytes(data));
-  base::Time time = base::Time::Now();
+  GURL page_url("http://www.google.com");
+  AddFaviconAndMapSimple(&db, page_url, kIconUrl1, favicon_base::FAVICON);
+  AddFaviconAndMapSimple(&db, page_url, kIconUrl2, favicon_base::TOUCH_ICON);
+  AddFaviconAndMapSimple(&db, page_url, kIconUrl3, favicon_base::TOUCH_ICON);
+  AddFaviconAndMapSimple(&db, page_url, kIconUrl4,
+                         favicon_base::TOUCH_PRECOMPOSED_ICON);
 
-  favicon_base::FaviconID id1 =
-      db.AddFavicon(url, favicon_base::FAVICON, favicon, time, gfx::Size());
-  EXPECT_NE(0, db.AddIconMapping(url, id1));
-
-  favicon_base::FaviconID id2 =
-      db.AddFavicon(url, favicon_base::TOUCH_ICON, favicon, time, gfx::Size());
-  EXPECT_NE(0, db.AddIconMapping(url, id2));
-
-  favicon_base::FaviconID id3 =
-      db.AddFavicon(url, favicon_base::TOUCH_ICON, favicon, time, gfx::Size());
-  EXPECT_NE(0, db.AddIconMapping(url, id3));
-
-  // Only the mappings for favicons of type TOUCH_ICON should be returned as
-  // TOUCH_ICON is a larger icon type than FAVICON.
+  // Only the mappings for FAVICON and TOUCH_ICON should be returned.
   std::vector<IconMapping> icon_mappings;
   EXPECT_TRUE(db.GetIconMappingsForPageURL(
-      url,
-      favicon_base::FAVICON | favicon_base::TOUCH_ICON |
-          favicon_base::TOUCH_PRECOMPOSED_ICON,
+      page_url, favicon_base::FAVICON | favicon_base::TOUCH_ICON,
       &icon_mappings));
+  SortMappingsByIconUrl(&icon_mappings);
 
-  EXPECT_EQ(2u, icon_mappings.size());
-  if (id2 == icon_mappings[0].icon_id) {
-    EXPECT_EQ(id3, icon_mappings[1].icon_id);
-  } else {
-    EXPECT_EQ(id3, icon_mappings[0].icon_id);
-    EXPECT_EQ(id2, icon_mappings[1].icon_id);
-  }
-
-  icon_mappings.clear();
-  EXPECT_TRUE(db.GetIconMappingsForPageURL(
-      url, favicon_base::TOUCH_ICON, &icon_mappings));
-  if (id2 == icon_mappings[0].icon_id) {
-    EXPECT_EQ(id3, icon_mappings[1].icon_id);
-  } else {
-    EXPECT_EQ(id3, icon_mappings[0].icon_id);
-    EXPECT_EQ(id2, icon_mappings[1].icon_id);
-  }
-
-  icon_mappings.clear();
-  EXPECT_TRUE(
-      db.GetIconMappingsForPageURL(url, favicon_base::FAVICON, &icon_mappings));
-  EXPECT_EQ(1u, icon_mappings.size());
-  EXPECT_EQ(id1, icon_mappings[0].icon_id);
+  EXPECT_EQ(3u, icon_mappings.size());
+  EXPECT_EQ(kIconUrl1, icon_mappings[0].icon_url);
+  EXPECT_EQ(kIconUrl2, icon_mappings[1].icon_url);
+  EXPECT_EQ(kIconUrl3, icon_mappings[2].icon_url);

[mastiz, 2017/04/20 08:55:59]

A more natural way to do this would be to get rid of SortMappingsByIconUrl and
instead introduce MATCHER_P(HasIconUrl) and then do:
EXPECT_THAT(icon_mappings, UnorderedElementsAre(HasIconUrl(kIconUrl1),
HasIconUrl(kIconUrl2), HasIconUrl(kIconUrl3)));

Not feeling strongly though.

[pkotwicz, 2017/04/30 03:49:56]

I chose to not add a GMock dependency to this test for the sake of a smooth
review. Some people like GMock others don't. No clue about sky@'s opinion. I
believe that Ben Goodger dislikes it it

 }
 
 TEST_F(ThumbnailDatabaseTest, HasMappingFor) {
   ThumbnailDatabase db(NULL);
   ASSERT_EQ(sql::INIT_OK, db.Init(file_name_));
   db.BeginTransaction();
 
   std::vector<unsigned char> data(kBlob1, kBlob1 + sizeof(kBlob1));
   scoped_refptr<base::RefCountedBytes> favicon(new base::RefCountedBytes(data));
 
@@ skipping 81 matching lines @@
   // Version 6 is deprecated, the data should all be gone.
   VerifyDatabaseEmpty(&db->db_);
 }
 
 // Test loading version 7 database.
 TEST_F(ThumbnailDatabaseTest, Version7) {
   std::unique_ptr<ThumbnailDatabase> db = LoadFromGolden("Favicons.v7.sql");
   ASSERT_TRUE(db.get() != NULL);
   VerifyTablesAndColumns(&db->db_);
 
-  EXPECT_TRUE(CheckPageHasIcon(db.get(),
-                               kPageUrl1,
-                               favicon_base::FAVICON,
-                               kIconUrl1,
-                               kLargeSize,
-                               sizeof(kBlob1),
-                               kBlob1));
-  EXPECT_TRUE(CheckPageHasIcon(db.get(),
-                               kPageUrl2,
-                               favicon_base::FAVICON,
-                               kIconUrl2,
-                               kLargeSize,
-                               sizeof(kBlob2),
-                               kBlob2));
-  EXPECT_TRUE(CheckPageHasIcon(db.get(),
-                               kPageUrl3,
-                               favicon_base::FAVICON,
-                               kIconUrl1,
-                               kLargeSize,
-                               sizeof(kBlob1),
-                               kBlob1));
-  EXPECT_TRUE(CheckPageHasIcon(db.get(),
-                               kPageUrl3,
-                               favicon_base::TOUCH_ICON,
-                               kIconUrl3,
-                               kLargeSize,
-                               sizeof(kBlob2),
-                               kBlob2));
+  EXPECT_TRUE(CheckPageHasIcon(db.get(), kPageUrl1, favicon_base::FAVICON,
+                               kIconUrl3, kLargeSize, sizeof(kBlob1), kBlob1));
+  EXPECT_TRUE(CheckPageHasIcon(db.get(), kPageUrl2, favicon_base::FAVICON,
+                               kIconUrl5, kLargeSize, sizeof(kBlob2), kBlob2));
+  EXPECT_TRUE(CheckPageHasIcon(db.get(), kPageUrl3, favicon_base::FAVICON,
+                               kIconUrl3, kLargeSize, sizeof(kBlob1), kBlob1));
+  EXPECT_TRUE(CheckPageHasIcon(db.get(), kPageUrl3, favicon_base::TOUCH_ICON,
+                               kIconUrl4, kLargeSize, sizeof(kBlob2), kBlob2));
 }
 
 // Test loading version 8 database.
 TEST_F(ThumbnailDatabaseTest, Version8) {
   std::unique_ptr<ThumbnailDatabase> db = LoadFromGolden("Favicons.v8.sql");
   ASSERT_TRUE(db.get() != NULL);
   VerifyTablesAndColumns(&db->db_);
 
-  EXPECT_TRUE(CheckPageHasIcon(db.get(),
-                               kPageUrl1,
-                               favicon_base::FAVICON,
-                               kIconUrl1,
-                               kLargeSize,
-                               sizeof(kBlob1),
-                               kBlob1));
-  EXPECT_TRUE(CheckPageHasIcon(db.get(),
-                               kPageUrl2,
-                               favicon_base::FAVICON,
-                               kIconUrl2,
-                               kLargeSize,
-                               sizeof(kBlob2),
-                               kBlob2));
-  EXPECT_TRUE(CheckPageHasIcon(db.get(),
-                               kPageUrl3,
-                               favicon_base::FAVICON,
-                               kIconUrl1,
-                               kLargeSize,
-                               sizeof(kBlob1),
-                               kBlob1));
-  EXPECT_TRUE(CheckPageHasIcon(db.get(),
-                               kPageUrl3,
-                               favicon_base::TOUCH_ICON,
-                               kIconUrl3,
-                               kLargeSize,
-                               sizeof(kBlob2),
-                               kBlob2));
+  EXPECT_TRUE(CheckPageHasIcon(db.get(), kPageUrl1, favicon_base::FAVICON,
+                               kIconUrl3, kLargeSize, sizeof(kBlob1), kBlob1));
+  EXPECT_TRUE(CheckPageHasIcon(db.get(), kPageUrl2, favicon_base::FAVICON,
+                               kIconUrl5, kLargeSize, sizeof(kBlob2), kBlob2));
+  EXPECT_TRUE(CheckPageHasIcon(db.get(), kPageUrl3, favicon_base::FAVICON,
+                               kIconUrl3, kLargeSize, sizeof(kBlob1), kBlob1));
+  EXPECT_TRUE(CheckPageHasIcon(db.get(), kPageUrl3, favicon_base::TOUCH_ICON,
+                               kIconUrl4, kLargeSize, sizeof(kBlob2), kBlob2));
 }
 
 TEST_F(ThumbnailDatabaseTest, Recovery) {
   // This code tests the recovery module in concert with Chromium's
   // custom recover virtual table.  Under USE_SYSTEM_SQLITE, this is
   // not available.  This is detected dynamically because corrupt
   // databases still need to be handled, perhaps by Raze(), and the
   // recovery module is an obvious layer to abstract that to.
   // TODO(shess): Handle that case for real!
   if (!sql::Recovery::FullRecoverySupported())
     return;
 
   // Create an example database.
   {
     EXPECT_TRUE(CreateDatabaseFromSQL(file_name_, "Favicons.v8.sql"));
 
     sql::Connection raw_db;
     EXPECT_TRUE(raw_db.Open(file_name_));
     VerifyTablesAndColumns(&raw_db);
   }
 
   // Test that the contents make sense after clean open.
   {
     ThumbnailDatabase db(NULL);
     ASSERT_EQ(sql::INIT_OK, db.Init(file_name_));
 
-    EXPECT_TRUE(CheckPageHasIcon(&db,
-                                 kPageUrl1,
-                                 favicon_base::FAVICON,
-                                 kIconUrl1,
-                                 kLargeSize,
-                                 sizeof(kBlob1),
+    EXPECT_TRUE(CheckPageHasIcon(&db, kPageUrl1, favicon_base::FAVICON,
+                                 kIconUrl3, kLargeSize, sizeof(kBlob1),
                                  kBlob1));
-    EXPECT_TRUE(CheckPageHasIcon(&db,
-                                 kPageUrl2,
-                                 favicon_base::FAVICON,
-                                 kIconUrl2,
-                                 kLargeSize,
-                                 sizeof(kBlob2),
+    EXPECT_TRUE(CheckPageHasIcon(&db, kPageUrl2, favicon_base::FAVICON,
+                                 kIconUrl5, kLargeSize, sizeof(kBlob2),
                                  kBlob2));
   }
 
   // Corrupt the |icon_mapping.page_url| index by deleting an element
   // from the backing table but not the index.
   {
     sql::Connection raw_db;
     EXPECT_TRUE(raw_db.Open(file_name_));
     ASSERT_EQ("ok", sql::test::IntegrityCheck(&raw_db));
   }
@@ skipping 38 matching lines @@
 
   // Database should also be recovered at higher levels.
   {
     ThumbnailDatabase db(NULL);
     ASSERT_EQ(sql::INIT_OK, db.Init(file_name_));
 
     // Now this fails because there is no mapping.
     EXPECT_FALSE(db.GetIconMappingsForPageURL(kPageUrl2, NULL));
 
     // Other data was retained by recovery.
-    EXPECT_TRUE(CheckPageHasIcon(&db,
-                                 kPageUrl1,
-                                 favicon_base::FAVICON,
-                                 kIconUrl1,
-                                 kLargeSize,
-                                 sizeof(kBlob1),
+    EXPECT_TRUE(CheckPageHasIcon(&db, kPageUrl1, favicon_base::FAVICON,
+                                 kIconUrl3, kLargeSize, sizeof(kBlob1),
                                  kBlob1));
   }
 
   // Corrupt the database again by adjusting the header.
   EXPECT_TRUE(sql::test::CorruptSizeInHeader(file_name_));
 
   // Database is unusable at the SQLite level.
   {
     sql::test::ScopedErrorExpecter expecter;
     expecter.ExpectError(SQLITE_CORRUPT);
     sql::Connection raw_db;
     EXPECT_TRUE(raw_db.Open(file_name_));
     EXPECT_FALSE(raw_db.IsSQLValid("PRAGMA integrity_check"));
     ASSERT_TRUE(expecter.SawExpectedErrors());
   }
 
   // Database should be recovered during open.
   {
     sql::test::ScopedErrorExpecter expecter;
     expecter.ExpectError(SQLITE_CORRUPT);
     ThumbnailDatabase db(NULL);
     ASSERT_EQ(sql::INIT_OK, db.Init(file_name_));
 
     EXPECT_FALSE(db.GetIconMappingsForPageURL(kPageUrl2, NULL));
-    EXPECT_TRUE(CheckPageHasIcon(&db,
-                                 kPageUrl1,
-                                 favicon_base::FAVICON,
-                                 kIconUrl1,
-                                 kLargeSize,
-                                 sizeof(kBlob1),
+    EXPECT_TRUE(CheckPageHasIcon(&db, kPageUrl1, favicon_base::FAVICON,
+                                 kIconUrl3, kLargeSize, sizeof(kBlob1),
                                  kBlob1));
 
     ASSERT_TRUE(expecter.SawExpectedErrors());
   }
 }
 
 TEST_F(ThumbnailDatabaseTest, Recovery7) {
   // This code tests the recovery module in concert with Chromium's
   // custom recover virtual table.  Under USE_SYSTEM_SQLITE, this is
   // not available.  This is detected dynamically because corrupt
@@ skipping 56 matching lines @@
 
   // Database should also be recovered at higher levels.
   {
     ThumbnailDatabase db(NULL);
     ASSERT_EQ(sql::INIT_OK, db.Init(file_name_));
 
     // Now this fails because there is no mapping.
     EXPECT_FALSE(db.GetIconMappingsForPageURL(kPageUrl2, NULL));
 
     // Other data was retained by recovery.
-    EXPECT_TRUE(CheckPageHasIcon(&db,
-                                 kPageUrl1,
-                                 favicon_base::FAVICON,
-                                 kIconUrl1,
-                                 kLargeSize,
-                                 sizeof(kBlob1),
+    EXPECT_TRUE(CheckPageHasIcon(&db, kPageUrl1, favicon_base::FAVICON,
+                                 kIconUrl3, kLargeSize, sizeof(kBlob1),
                                  kBlob1));
   }
 
   // Corrupt the database again by adjusting the header.
   EXPECT_TRUE(sql::test::CorruptSizeInHeader(file_name_));
 
   // Database is unusable at the SQLite level.
   {
     sql::test::ScopedErrorExpecter expecter;
     expecter.ExpectError(SQLITE_CORRUPT);
     sql::Connection raw_db;
     EXPECT_TRUE(raw_db.Open(file_name_));
     EXPECT_FALSE(raw_db.IsSQLValid("PRAGMA integrity_check"));
     ASSERT_TRUE(expecter.SawExpectedErrors());
   }
 
   // Database should be recovered during open.
   {
     sql::test::ScopedErrorExpecter expecter;
     expecter.ExpectError(SQLITE_CORRUPT);
     ThumbnailDatabase db(NULL);
     ASSERT_EQ(sql::INIT_OK, db.Init(file_name_));
 
     EXPECT_FALSE(db.GetIconMappingsForPageURL(kPageUrl2, NULL));
-    EXPECT_TRUE(CheckPageHasIcon(&db,
-                                 kPageUrl1,
-                                 favicon_base::FAVICON,
-                                 kIconUrl1,
-                                 kLargeSize,
-                                 sizeof(kBlob1),
+    EXPECT_TRUE(CheckPageHasIcon(&db, kPageUrl1, favicon_base::FAVICON,
+                                 kIconUrl3, kLargeSize, sizeof(kBlob1),
                                  kBlob1));
 
     ASSERT_TRUE(expecter.SawExpectedErrors());
   }
 }
 
 TEST_F(ThumbnailDatabaseTest, Recovery6) {
   // TODO(shess): See comment at top of Recovery test.
   if (!sql::Recovery::FullRecoverySupported())
     return;
@@ skipping 112 matching lines @@
     ThumbnailDatabase db(NULL);
     ASSERT_EQ(sql::INIT_OK, db.Init(db_path));
 
     // Verify that the resulting schema is correct, whether it
     // involved razing the file or fixing things in place.
     VerifyTablesAndColumns(&db.db_);
   }
 }
 
 }  // namespace history