Add heuristics to skip thumbnail generation when it's unnecessary.

More specifically, skip thumbnail generation in the following occasions:
- The browser is in the off-the-record mode.
- The URL is not valid for top sites (ex. new tab pages, etc.)
- The existing thumbnail is new and interesting enough.

This patch should reduce the number of thumbnail generations significantly.

BUG=65936
TEST=add unit tests. confirm that the thumbnails are updated as expected.

Review URL: http://codereview.chromium.org/6389001

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@72763 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/history/top_sites.cc

@@ -127,7 +127,7 @@ class LoadThumbnailsFromHistoryTask : public HistoryDBTask {
 }  // namespace
 
 TopSites::TopSites(Profile* profile)
-    : backend_(new TopSitesBackend()),
+    : backend_(NULL),
       cache_(new TopSitesCache()),
       thread_safe_cache_(new TopSitesCache()),
       profile_(profile),
@@ -154,6 +154,9 @@ TopSites::TopSites(Profile* profile)
 }
 
 void TopSites::Init(const FilePath& db_name) {
+  // Create the backend here, rather than in the constructor, so that
+  // unit tests that do not need the backend can run without a problem.
+  backend_ = new TopSitesBackend;
   backend_->Init(db_name);
   backend_->GetMostVisitedThumbnails(
       &cancelable_consumer_,
@@ -181,8 +184,8 @@ bool TopSites::SetPageThumbnail(const GURL& url,
   }
 
   bool add_temp_thumbnail = false;
-  if (!cache_->IsKnownURL(url)) {
-    if (cache_->top_sites().size() < kTopSitesNumber) {
+  if (!IsKnownURL(url)) {
+    if (!IsFull()) {
       add_temp_thumbnail = true;
     } else {
       return false;  // This URL is not known to us.
@@ -237,6 +240,13 @@ bool TopSites::GetPageThumbnail(const GURL& url,
   return thread_safe_cache_->GetPageThumbnail(url, bytes);
 }
 
+bool TopSites::GetPageThumbnailScore(const GURL& url,
+                                     ThumbnailScore* score) {
+  // WARNING: this may be invoked on any thread.
+  base::AutoLock lock(lock_);
+  return thread_safe_cache_->GetPageThumbnailScore(url, score);
+}
+
 // Returns the index of |url| in |urls|, or -1 if not found.
 static int IndexOf(const MostVisitedURLList& urls, const GURL& url) {
   for (size_t i = 0; i < urls.size(); i++) {
@@ -449,6 +459,14 @@ CancelableRequestProvider::Handle TopSites::StartQueryForMostVisited() {
   return 0;
 }
 
+bool TopSites::IsKnownURL(const GURL& url) {
+  return loaded_ && cache_->IsKnownURL(url);
+}
+
+bool TopSites::IsFull() {
+  return loaded_ && cache_->top_sites().size() >= kTopSitesNumber;
+}
+
 TopSites::~TopSites() {
 }
 
@@ -724,7 +742,7 @@ void TopSites::Observe(NotificationType type,
     }
     StartQueryForMostVisited();
   } else if (type == NotificationType::NAV_ENTRY_COMMITTED) {
-    if (cache_->top_sites().size() < kTopSitesNumber) {
+    if (!IsFull()) {
       NavigationController::LoadCommittedDetails* load_details =
           Details<NavigationController::LoadCommittedDetails>(details).ptr();
       if (!load_details)

chrome/browser/history/top_sites.h

@@ -79,6 +79,11 @@ class TopSites
   bool GetPageThumbnail(const GURL& url,
                         scoped_refptr<RefCountedBytes>* bytes);
 
+  // Get a thumbnail score for a given page. Returns true iff we have the
+  // thumbnail score.  This may be invoked on any thread. The score will
+  // be copied to |score|.
+  virtual bool GetPageThumbnailScore(const GURL& url, ThumbnailScore* score);
+
   // Invoked from History if migration is needed. If this is invoked it will
   // be before HistoryLoaded is invoked.
   void MigrateFromHistory();
@@ -144,6 +149,19 @@ class TopSites
 
   bool loaded() const { return loaded_; }
 
+  // Returns true if the given URL is known to the top sites service.
+  // This function also returns false if TopSites isn't loaded yet.
+  virtual bool IsKnownURL(const GURL& url);
+
+  // Returns true if the top sites list is full (i.e. we already have the
+  // maximum number of top sites).  This function also returns false if
+  // TopSites isn't loaded yet.
+  virtual bool IsFull();
+
+ protected:
+  // For allowing inheritance.
+  virtual ~TopSites();
+
  private:
   friend class base::RefCountedThreadSafe<TopSites>;
   friend class TopSitesTest;
@@ -177,8 +195,6 @@ class TopSites
     TOP_SITES_LOADED
   };
 
-  ~TopSites();
-
   // Sets the thumbnail without writing to the database. Useful when
   // reading last known top sites from the DB.
   // Returns true if the thumbnail was set, false if the existing one is better.

chrome/browser/history/top_sites_cache.cc

@@ -47,6 +47,17 @@ bool TopSitesCache::GetPageThumbnail(const GURL& url,
   return false;
 }
 
+bool TopSitesCache::GetPageThumbnailScore(const GURL& url,
+                                          ThumbnailScore* score) {
+  std::map<GURL, Images>::const_iterator found =
+      images_.find(GetCanonicalURL(url));
+  if (found != images_.end()) {
+    *score = found->second.thumbnail_score;
+    return true;
+  }
+  return false;
+}
+
 GURL TopSitesCache::GetCanonicalURL(const GURL& url) {
   CanonicalURLs::iterator i = TopSitesCache::GetCanonicalURLsIterator(url);
   return i == canonical_urls_.end() ? url : i->first.first->url;

chrome/browser/history/top_sites_cache.h

@@ -45,6 +45,10 @@ class TopSitesCache {
   bool GetPageThumbnail(const GURL& url,
                         scoped_refptr<RefCountedBytes>* bytes);
 
+  // Fetches the thumbnail score for the specified url. Returns true if
+  // there is a thumbnail score for the specified url.
+  bool GetPageThumbnailScore(const GURL& url, ThumbnailScore* score);
+
   // Returns the canonical URL for |url|.
   GURL GetCanonicalURL(const GURL& url);
 

chrome/browser/tab_contents/thumbnail_generator.cc

@@ -632,11 +632,10 @@ SkBitmap ThumbnailGenerator::GetClippedBitmap(const SkBitmap& bitmap,
 
 void ThumbnailGenerator::UpdateThumbnailIfNecessary(
     TabContents* tab_contents, const GURL& url) {
-  if (tab_contents->profile()->IsOffTheRecord() ||
-      (url.SchemeIs("chrome") && url.host() == "newtab"))
+  history::TopSites* top_sites = tab_contents->profile()->GetTopSites();
+  // Skip if we don't need to update the thumbnail.
+  if (!ShouldUpdateThumbnail(tab_contents->profile(), top_sites, url))
     return;
-  // TODO(satorux): Add more conditions here to avoid unnecessary
-  // thumbnail generation.
 
   ThumbnailGenerator* generator = g_browser_process->GetThumbnailGenerator();
   const int options = ThumbnailGenerator::kClippedThumbnail;
@@ -656,10 +655,31 @@ void ThumbnailGenerator::UpdateThumbnailIfNecessary(
       (clip_result == ThumbnailGenerator::kTallerThanWide ||
        clip_result == ThumbnailGenerator::kNotClipped);
 
-  history::TopSites* top_sites = tab_contents->profile()->GetTopSites();
   top_sites->SetPageThumbnail(url, thumbnail, score);
-  VLOG(1) << "Thumbnail taken for " << url
-          << ", at_top: " << score.at_top
-          << ", boring_score: " << score.boring_score
-          << ", good_clipping: " << score.good_clipping;
+  VLOG(1) << "Thumbnail taken for " << url << ": " << score.ToString();
+}
+
+bool ThumbnailGenerator::ShouldUpdateThumbnail(Profile* profile,
+                                               history::TopSites* top_sites,
+                                               const GURL& url) {
+  if (!profile || !top_sites)
+    return false;
+  // Skip if it's in the off-the-record mode.
+  if (profile->IsOffTheRecord())
+    return false;
+  // Skip if the given URL is not appropriate for history.
+  if (!HistoryService::CanAddURL(url))
+    return false;
+  // Skip if the top sites list is full, and the URL is not known.
+  const bool is_known = top_sites->IsKnownURL(url);
+  if (top_sites->IsFull() && !is_known)
+    return false;
+  // Skip if we don't have to udpate the existing thumbnail.
+  ThumbnailScore current_score;
+  if (is_known &&
+      top_sites->GetPageThumbnailScore(url, &current_score) &&
+      !current_score.ShouldConsiderUpdating())
+    return false;
+
+  return true;
 }

chrome/browser/tab_contents/thumbnail_generator.h

@@ -19,10 +19,15 @@
 #include "chrome/common/notification_registrar.h"
 
 class GURL;
+class Profile;
 class RenderWidgetHost;
 class SkBitmap;
 class TabContents;
 
+namespace history {
+class TopSites;
+}
+
 class ThumbnailGenerator : NotificationObserver {
  public:
   typedef Callback1<const SkBitmap&>::Type ThumbnailReadyCallback;
@@ -121,6 +126,11 @@ class ThumbnailGenerator : NotificationObserver {
   static void UpdateThumbnailIfNecessary(TabContents* tab_contents,
                                          const GURL& url);
 
+  // Returns true if we should update the thumbnail of the given URL.
+  static bool ShouldUpdateThumbnail(Profile* profile,
+                                    history::TopSites* top_sites,
+                                    const GURL& url);
+
  private:
   // RenderWidgetHostPaintingObserver implementation.
   virtual void WidgetWillDestroyBackingStore(RenderWidgetHost* widget,

chrome/browser/tab_contents/thumbnail_generator_unittest.cc

@@ -4,6 +4,8 @@
 
 #include "app/surface/transport_dib.h"
 #include "base/basictypes.h"
+#include "base/stringprintf.h"
+#include "chrome/browser/history/top_sites.h"
 #include "chrome/browser/renderer_host/backing_store_manager.h"
 #include "chrome/browser/renderer_host/mock_render_process_host.h"
 #include "chrome/browser/renderer_host/test/test_render_view_host.h"
@@ -278,3 +280,100 @@ TEST(ThumbnailGeneratorSimpleTest, GetClippedBitmap_NonSquareOutput) {
   // The input was taller than wide.
   EXPECT_EQ(ThumbnailGenerator::kTallerThanWide, clip_result);
 }
+
+// A mock version of TopSites, used for testing ShouldUpdateThumbnail().
+class MockTopSites : public history::TopSites {
+ public:
+  MockTopSites(Profile* profile)
+      : history::TopSites(profile),
+        capacity_(1) {
+  }
+
+  // history::TopSites overrides.
+  virtual bool IsFull() {
+    return known_url_map_.size() >= capacity_;
+  }
+  virtual bool IsKnownURL(const GURL& url) {
+    return known_url_map_.find(url.spec()) != known_url_map_.end();
+  }
+  virtual bool GetPageThumbnailScore(const GURL& url, ThumbnailScore* score) {
+    std::map<std::string, ThumbnailScore>::const_iterator iter =
+        known_url_map_.find(url.spec());
+    if (iter == known_url_map_.end()) {
+      return false;
+    } else {
+      *score = iter->second;
+      return true;
+    }
+  }
+
+  // Adds a known URL with the associated thumbnail score.
+  void AddKnownURL(const GURL& url, const ThumbnailScore& score) {
+    known_url_map_[url.spec()] = score;
+  }
+
+ private:
+  virtual ~MockTopSites() {}
+  size_t capacity_;
+  std::map<std::string, ThumbnailScore> known_url_map_;
+};
+
+TEST(ThumbnailGeneratorSimpleTest, ShouldUpdateThumbnail) {
+  const GURL kGoodURL("http://www.google.com/");
+  const GURL kBadURL("chrome://newtab");
+
+  // Set up the profile.
+  TestingProfile profile;
+
+  // Set up the top sites service.
+  ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  scoped_refptr<MockTopSites> top_sites(new MockTopSites(&profile));
+
+  // Should be false because it's a bad URL.
+  EXPECT_FALSE(ThumbnailGenerator::ShouldUpdateThumbnail(
+      &profile, top_sites.get(), kBadURL));
+
+  // Should be true, as it's a good URL.
+  EXPECT_TRUE(ThumbnailGenerator::ShouldUpdateThumbnail(
+      &profile, top_sites.get(), kGoodURL));
+
+  // Should be false, if it's in the off-the-record mode.
+  profile.set_off_the_record(true);
+  EXPECT_FALSE(ThumbnailGenerator::ShouldUpdateThumbnail(
+      &profile, top_sites.get(), kGoodURL));
+
+  // Should be true again, once turning off the off-the-record mode.
+  profile.set_off_the_record(false);
+  EXPECT_TRUE(ThumbnailGenerator::ShouldUpdateThumbnail(
+      &profile, top_sites.get(), kGoodURL));
+
+  // Add a known URL. This makes the top sites data full.
+  ThumbnailScore bad_score;
+  bad_score.time_at_snapshot = base::Time::UnixEpoch();  // Ancient time stamp.
+  top_sites->AddKnownURL(kGoodURL, bad_score);
+  ASSERT_TRUE(top_sites->IsFull());
+
+  // Should be false, as the top sites data is full, and the new URL is
+  // not known.
+  const GURL kAnotherGoodURL("http://www.youtube.com/");
+  EXPECT_FALSE(ThumbnailGenerator::ShouldUpdateThumbnail(
+      &profile, top_sites.get(), kAnotherGoodURL));
+
+  // Should be true, as the existing thumbnail is bad (i.e need a better one).
+  EXPECT_TRUE(ThumbnailGenerator::ShouldUpdateThumbnail(
+      &profile, top_sites.get(), kGoodURL));
+
+  // Replace the thumbnail score with a really good one.
+  ThumbnailScore good_score;
+  good_score.time_at_snapshot = base::Time::Now();  // Very new.
+  good_score.at_top = true;
+  good_score.good_clipping = true;
+  good_score.boring_score = 0.0;
+  top_sites->AddKnownURL(kGoodURL, good_score);
+
+  // Should be false, as the existing thumbnail is good enough (i.e. don't
+  // need to replace the existing thumbnail which is new and good).
+  EXPECT_FALSE(ThumbnailGenerator::ShouldUpdateThumbnail(
+      &profile, top_sites.get(), kGoodURL));
+}

chrome/common/thumbnail_score.cc

@@ -5,12 +5,15 @@
 #include "chrome/common/thumbnail_score.h"
 
 #include "base/logging.h"
+#include "base/stringprintf.h"
 
 using base::Time;
 using base::TimeDelta;
 
 const TimeDelta ThumbnailScore::kUpdateThumbnailTime = TimeDelta::FromDays(1);
 const double ThumbnailScore::kThumbnailMaximumBoringness = 0.94;
+// Per crbug.com/65936#c4, 91.83% of thumbnail scores are less than 0.70.
+const double ThumbnailScore::kThumbnailInterestingEnoughBoringness = 0.70;
 const double ThumbnailScore::kThumbnailDegradePerHour = 0.01;
 
 // Calculates a numeric score from traits about where a snapshot was
@@ -70,6 +73,16 @@ bool ThumbnailScore::Equals(const ThumbnailScore& rhs) const {
       redirect_hops_from_dest == rhs.redirect_hops_from_dest;
 }
 
+std::string ThumbnailScore::ToString() const {
+  return StringPrintf("boring_score: %f, at_top %d, good_clipping %d, "
+                      "time_at_snapshot: %f, redirect_hops_from_dest: %d",
+                      boring_score,
+                      at_top,
+                      good_clipping,
+                      time_at_snapshot.ToDoubleT(),
+                      redirect_hops_from_dest);
+}
+
 bool ShouldReplaceThumbnailWith(const ThumbnailScore& current,
                                 const ThumbnailScore& replacement) {
   int current_type = GetThumbnailType(current.good_clipping, current.at_top);
@@ -116,3 +129,16 @@ bool ShouldReplaceThumbnailWith(const ThumbnailScore& current,
   return current.boring_score >= ThumbnailScore::kThumbnailMaximumBoringness &&
       replacement.boring_score < ThumbnailScore::kThumbnailMaximumBoringness;
 }
+
+bool ThumbnailScore::ShouldConsiderUpdating() {
+  const TimeDelta time_elapsed = Time::Now() - time_at_snapshot;
+  // Consider the current thumbnail to be new and interesting enough if
+  // the following critera are met.
+  const bool new_and_interesting_enough =
+      (time_elapsed < kUpdateThumbnailTime &&
+       good_clipping && at_top &&
+       boring_score < kThumbnailInterestingEnoughBoringness);
+  // We want to generate a new thumbnail when the current thumbnail is
+  // sufficiently old or uninteresting.
+  return !new_and_interesting_enough;
+}

chrome/common/thumbnail_score.h

@@ -6,6 +6,7 @@
 #define CHROME_COMMON_THUMBNAIL_SCORE_H_
 #pragma once
 
+#include <string>
 #include "base/time.h"
 
 // A set of metadata about a Thumbnail.
@@ -27,6 +28,9 @@ struct ThumbnailScore {
   // Tests for equivalence between two ThumbnailScore objects.
   bool Equals(const ThumbnailScore& rhs) const;
 
+  // Returns string representation of this object.
+  std::string ToString() const;
+
   // How "boring" a thumbnail is. The boring score is the 0,1 ranged
   // percentage of pixels that are the most common luma. Higher boring
   // scores indicate that a higher percentage of a bitmap are all the
@@ -67,6 +71,10 @@ struct ThumbnailScore {
   // How bad a thumbnail needs to be before we completely ignore it.
   static const double kThumbnailMaximumBoringness;
 
+  // We consider a thumbnail interesting enough if the boring score is
+  // lower than this.
+  static const double kThumbnailInterestingEnoughBoringness;
+
   // Time before we take a worse thumbnail (subject to
   // kThumbnailMaximumBoringness) over what's currently in the database
   // for freshness.
@@ -74,6 +82,11 @@ struct ThumbnailScore {
 
   // Penalty of how much more boring a thumbnail should be per hour.
   static const double kThumbnailDegradePerHour;
+
+  // Checks whether we should consider updating a new thumbnail based on
+  // this score. For instance, we don't have to update a new thumbnail
+  // if the current thumbnail is new and interesting enough.
+  bool ShouldConsiderUpdating();
 };
 
 // Checks whether we should replace one thumbnail with another.

chrome/common/thumbnail_score_unittest.cc

@@ -52,3 +52,27 @@ TEST(ThumbnailScoreTest, RedirectCount) {
   lotsa_redirects.redirect_hops_from_dest = 4;
   EXPECT_FALSE(ShouldReplaceThumbnailWith(no_redirects, lotsa_redirects));
 }
+
+TEST(ThumbnailScoreTest, ShouldConsiderUpdating) {
+  ThumbnailScore score;
+  // By default, the score is 1.0, meaning very boring, thus we should
+  // generate a new thumbnail.
+  EXPECT_DOUBLE_EQ(1.0, score.boring_score);
+  EXPECT_TRUE(score.ShouldConsiderUpdating());
+
+  // Make it very interesting, but this is not enough.
+  score.boring_score = 0.0;
+  EXPECT_TRUE(score.ShouldConsiderUpdating());
+
+  // good_clipping is important, but sill not enough.
+  score.good_clipping = true;
+  EXPECT_TRUE(score.ShouldConsiderUpdating());
+
+  // at_top is important. Finally, the thumbnail is new and interesting enough.
+  score.at_top = true;
+  EXPECT_FALSE(score.ShouldConsiderUpdating());
+
+  // Make it old. Then, it's no longer new enough.
+  score.time_at_snapshot -= ThumbnailScore::kUpdateThumbnailTime;
+  EXPECT_TRUE(score.ShouldConsiderUpdating());
+}