[Thumbnails DB] Add functionality to clear unused on-demand favicons.

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"
 #include "base/files/file_path.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/memory/ref_counted_memory.h"
 #include "base/path_service.h"
+#include "base/strings/stringprintf.h"
+#include "build/build_config.h"

[jkrcal, 2017/07/06 17:01:48]

"build/build_config.h" has been enforced by a presubmit check

+#include "components/history/core/browser/history_backend_client.h"
 #include "components/history/core/browser/thumbnail_database.h"
 #include "components/history/core/test/database_test_utils.h"
 #include "sql/connection.h"
 #include "sql/recovery.h"
 #include "sql/test/scoped_error_expecter.h"
 #include "sql/test/test_helpers.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/sqlite/sqlite3.h"
 #include "url/gurl.h"
 
+using testing::StrictMock;
+using testing::Return;
+
 namespace history {
 
 namespace {
 
 // Blobs for the bitmap tests.  These aren't real bitmaps.  Golden
 // database files store the same blobs (see VersionN tests).
 const unsigned char kBlob1[] =
     "12346102356120394751634516591348710478123649165419234519234512349134";
 const unsigned char kBlob2[] =
     "goiwuegrqrcomizqyzkjalitbahxfjytrqvpqeroicxmnlkhlzunacxaneviawrtxcywhgef";
@@ skipping 127 matching lines @@
 }
 
 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);
 }
 
+class MockHistoryBackendClient : public HistoryBackendClient {
+ public:
+  // MOCK_METHOD0(~HistoryBackendClient, void());
+  MOCK_METHOD1(IsBookmarked, bool(const GURL& url));
+  MOCK_METHOD1(GetBookmarks, void(std::vector<URLAndTitle>* bookmarks));
+  MOCK_METHOD0(ShouldReportDatabaseError, bool());
+  MOCK_METHOD1(IsWebSafe, bool(const GURL& url));
+
+#if defined(OS_ANDROID)
+  MOCK_METHOD4(OnHistoryBackendInitialized,
+               void(HistoryBackend* history_backend,
+                    HistoryDatabase* history_database,
+                    ThumbnailDatabase* thumbnail_database,
+                    const base::FilePath& history_dir));
+  MOCK_METHOD2(OnHistoryBackendDestroyed,
+               void(HistoryBackend* history_backend,
+                    const base::FilePath& history_dir));
+#endif  // defined(OS_ANDROID)
+};
+
 }  // 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 177 matching lines @@
       start + base::TimeDelta::FromDays(kFaviconUpdateLastRequestedAfterDays);
   db.TouchOnDemandFavicon(url, end);
 
   base::Time last_updated;
   base::Time last_requested;
   EXPECT_TRUE(db.GetFaviconBitmap(bitmap, &last_updated, &last_requested,
                                   nullptr, nullptr));
   EXPECT_EQ(start, last_updated);           // Does not mess with last_updated.
   EXPECT_EQ(base::Time(), last_requested);  // No update.
 }
 

[pkotwicz, 2017/07/07 00:42:38]

Please add a comment for each test case. For this test case, how about:

"Test that ThumbnailDatabase::GetOldOnDemandFavicons() returns on-demand icons
which were requested prior to the passed in timestamp."

[jkrcal, 2017/07/07 08:45:16]

Done.

+TEST_F(ThumbnailDatabaseTest, GetOldOnDemandFaviconsReturnsOld) {
+  StrictMock<MockHistoryBackendClient> mock_client;
+  ThumbnailDatabase db(&mock_client);

[pkotwicz, 2017/07/07 00:42:39]

Does the test work if you pass in nullptr?

[jkrcal, 2017/07/07 08:45:16]

Ah, sure. It was there to mock being bookmarked. Not needed anymore.

+  ASSERT_EQ(sql::INIT_OK, db.Init(file_name_));
+  db.BeginTransaction();
+
+  base::Time start;
+  ASSERT_TRUE(base::Time::FromUTCExploded({2017, 5, 0, 1, 0, 0, 0, 0}, &start));
+  std::vector<unsigned char> data(kBlob1, kBlob1 + sizeof(kBlob1));
+  scoped_refptr<base::RefCountedBytes> favicon(new base::RefCountedBytes(data));
+
+  // Icon: old unused case.
+  GURL url("http://google.com");
+  favicon_base::FaviconID icon = db.AddFavicon(url, favicon_base::FAVICON);
+  ASSERT_NE(0, icon);
+  FaviconBitmapID bitmap = db.AddFaviconBitmap(
+      icon, favicon, FaviconBitmapType::ON_DEMAND, start, gfx::Size());

[pkotwicz, 2017/07/07 00:42:39]

Nit: Can you use the 6 argument ThumbnailDatabase::AddFavicon() call?

[jkrcal, 2017/07/07 08:45:16]

Done.

+  ASSERT_NE(0, bitmap);
+  ASSERT_NE(0, db.AddIconMapping(url, icon));
+
+  base::Time delete_older_than = start + base::TimeDelta::FromDays(7);

[pkotwicz, 2017/07/07 00:42:39]

Might as well do:

"+ base::TimeDelta::FromSeconds(1)"

[jkrcal, 2017/07/07 08:45:17]

Done.

+  auto list = db.GetOldOnDemandFavicons(delete_older_than);
+
+  // The icon is returned for deletion.

[pkotwicz, 2017/07/07 00:42:39]

Nit: Don't mention deletion. The fact that the id is used for deletion is an
implementation detail of ExpireHistoryBackend

[jkrcal, 2017/07/07 08:45:16]

Done.

+  EXPECT_EQ(1u, list.size());
+  EXPECT_EQ(icon, list[0].first);
+  EXPECT_EQ(url, list[0].second);
+}
+

[pkotwicz, 2017/07/07 00:42:39]

For this test's comment, how about:

"Test that ThumbnailDatabase::GetOldOnDemandFavicons() returns on-visit icons if
the on-visit icons have expired. We need this behavior in order to delete icons
stored via HistoryService::SetOnDemandFavicons() prior to on-demand icons
setting the "last_requested" time."

[jkrcal, 2017/07/07 08:45:16]

Done.

+TEST_F(ThumbnailDatabaseTest, GetOldOnDemandFaviconsReturnsExpired) {
+  StrictMock<MockHistoryBackendClient> mock_client;
+  ThumbnailDatabase db(&mock_client);
+  ASSERT_EQ(sql::INIT_OK, db.Init(file_name_));
+  db.BeginTransaction();
+
+  base::Time start;
+  ASSERT_TRUE(base::Time::FromUTCExploded({2017, 5, 0, 1, 0, 0, 0, 0}, &start));
+  std::vector<unsigned char> data(kBlob1, kBlob1 + sizeof(kBlob1));
+  scoped_refptr<base::RefCountedBytes> favicon(new base::RefCountedBytes(data));
+
+  // Icon: standard favicon (not on-demand) but expired.
+  GURL url("http://google.com");
+  favicon_base::FaviconID icon = db.AddFavicon(url, favicon_base::FAVICON);
+  ASSERT_NE(0, icon);
+  FaviconBitmapID bitmap = db.AddFaviconBitmap(
+      icon, favicon, FaviconBitmapType::ON_VISIT, start, gfx::Size());
+  ASSERT_NE(0, bitmap);
+  ASSERT_NE(0, db.AddIconMapping(url, icon));
+  ASSERT_NE(0, db.SetFaviconOutOfDate(icon));
+
+  // The threshold is ignored for expired icons.
+  auto list = db.GetOldOnDemandFavicons(/*threshold=*/base::Time());

[pkotwicz, 2017/07/07 00:42:39]

Maybe use base::Time::Now() instead?

[jkrcal, 2017/07/07 08:45:16]

Done.

+
+  // The icon is returned for deletion.
+  EXPECT_EQ(1u, list.size());
+  EXPECT_EQ(icon, list[0].first);
+  EXPECT_EQ(url, list[0].second);
+}
+
+TEST_F(ThumbnailDatabaseTest, GetOldOnDemandFaviconsDoesNotReturnFresh) {

[pkotwicz, 2017/07/07 00:42:39]

For this test's comment how about:

"Test that ThumbnailDatabase::GetOldOnDemandFavicons() does not return on-demand
icons
which were requested after to the passed in timestamp."

[jkrcal, 2017/07/07 08:45:16]

Done.

+  StrictMock<MockHistoryBackendClient> mock_client;
+  ThumbnailDatabase db(&mock_client);
+  ASSERT_EQ(sql::INIT_OK, db.Init(file_name_));
+  db.BeginTransaction();
+
+  base::Time start;
+  ASSERT_TRUE(base::Time::FromUTCExploded({2017, 5, 0, 1, 0, 0, 0, 0}, &start));
+  std::vector<unsigned char> data(kBlob1, kBlob1 + sizeof(kBlob1));
+  scoped_refptr<base::RefCountedBytes> favicon(new base::RefCountedBytes(data));
+
+  // Icon: freshly used case.
+  GURL url("http://google.com");
+  favicon_base::FaviconID icon = db.AddFavicon(url, favicon_base::FAVICON);
+  ASSERT_NE(0, icon);
+  FaviconBitmapID bitmap = db.AddFaviconBitmap(
+      icon, favicon, FaviconBitmapType::ON_DEMAND, start, gfx::Size());
+  ASSERT_NE(0, bitmap);
+  ASSERT_NE(0, db.AddIconMapping(url, icon));
+
+  // Touch the icon 3 weeks later.
+  base::Time now = start + base::TimeDelta::FromDays(21);
+  EXPECT_TRUE(db.TouchOnDemandFavicon(url, now));
+
+  base::Time delete_older_than = start + base::TimeDelta::FromDays(7);
+  auto list = db.GetOldOnDemandFavicons(delete_older_than);
+
+  // No icon is returned for deletion.
+  EXPECT_EQ(0u, list.size());

[pkotwicz, 2017/07/07 00:42:39]

Nit:
EXPECT_TRUE(list.empty());

[jkrcal, 2017/07/07 08:45:16]

Done.

+}
+

[pkotwicz, 2017/07/07 00:42:39]

For this test's comment how about:

"Test that ThumbnailDatabase::GetOldOnDemandFavicons() does not return
non-expired on-visit icons."

[jkrcal, 2017/07/07 08:45:16]

Done.

+TEST_F(ThumbnailDatabaseTest, GetOldOnDemandFaviconsDoesNotDeleteStandard) {
+  StrictMock<MockHistoryBackendClient> mock_client;
+  ThumbnailDatabase db(&mock_client);
+  ASSERT_EQ(sql::INIT_OK, db.Init(file_name_));
+  db.BeginTransaction();
+
+  base::Time start;
+  ASSERT_TRUE(base::Time::FromUTCExploded({2017, 5, 0, 1, 0, 0, 0, 0}, &start));
+  std::vector<unsigned char> data(kBlob1, kBlob1 + sizeof(kBlob1));
+  scoped_refptr<base::RefCountedBytes> favicon(new base::RefCountedBytes(data));
+
+  // Icon: standard favicon (not on-demand).
+  GURL url("http://google.com");
+  favicon_base::FaviconID icon = db.AddFavicon(url, favicon_base::FAVICON);
+  ASSERT_NE(0, icon);
+  FaviconBitmapID bitmap = db.AddFaviconBitmap(
+      icon, favicon, FaviconBitmapType::ON_VISIT, start, gfx::Size());
+  ASSERT_NE(0, bitmap);
+  ASSERT_NE(0, db.AddIconMapping(url, icon));
+
+  base::Time delete_older_than = start + base::TimeDelta::FromDays(7);
+  auto list = db.GetOldOnDemandFavicons(delete_older_than);
+
+  // No icon is returned for deletion.
+  EXPECT_EQ(0u, list.size());

[pkotwicz, 2017/07/07 00:42:39]

Nit:
EXPECT_TRUE(list.empty());

[jkrcal, 2017/07/07 08:45:16]

Done.

+}
+
 TEST_F(ThumbnailDatabaseTest, DeleteIconMappings) {
   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));
 
   GURL url("http://google.com");
   favicon_base::FaviconID id = db.AddFavicon(url, favicon_base::TOUCH_ICON);
@@ skipping 801 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