Add load_completed property to ThumbnailScore.

Thumbnails taken while page loading may only contain partial contents,
hence these are not desirable.

BUG=65936
TEST=with --enable-in-browser-thumbnailing --vmodule=thumbnail_generator=1, confirm that thumbnails taken while page loading were replaced with new ones properly.

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

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

10 files changed, 171 insertions, 40 deletions

diff --git a/chrome/browser/history/top_sites_database.cc b/chrome/browser/history/top_sites_database.cc
index 4639494..8a3d715 100644
--- a/chrome/browser/history/top_sites_database.cc
+++ b/chrome/browser/history/top_sites_database.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -14,7 +14,9 @@
 
 namespace history {
 
-static const int kVersionNumber = 1;
+// From the version 1 to 2, one column was added. Old versions of Chrome
+// should be able to read version 2 files just fine.
+static const int kVersionNumber = 2;
 
 TopSitesDatabase::TopSitesDatabase() : may_need_history_migration_(false) {
 }
@@ -51,16 +53,32 @@ bool TopSitesDatabase::Init(const FilePath& db_name) {
       return false;
   }
 
+  // Scope initialization in a transaction so we can't be partially
+  // initialized.
+  sql::Transaction transaction(db_.get());
+  transaction.Begin();
+
   if (!meta_table_.Init(db_.get(), kVersionNumber, kVersionNumber))
     return false;
 
   if (!InitThumbnailTable())
     return false;
 
+  if (meta_table_.GetVersionNumber() == 1) {
+    if (!UpgradeToVersion2()) {
+      LOG(WARNING) << "Unable to upgrade top sites database to version 2.";
+      return false;
+    }
+  }
+
   // Version check.
   if (meta_table_.GetVersionNumber() != kVersionNumber)
     return false;
 
+  // Initialization is complete.
+  if (!transaction.Commit())
+    return false;
+
   return true;
 }
 
@@ -75,7 +93,8 @@ bool TopSitesDatabase::InitThumbnailTable() {
                       "boring_score DOUBLE DEFAULT 1.0, "
                       "good_clipping INTEGER DEFAULT 0, "
                       "at_top INTEGER DEFAULT 0, "
-                      "last_updated INTEGER DEFAULT 0) ")) {
+                      "last_updated INTEGER DEFAULT 0, "
+                      "load_completed INTEGER DEFAULT 0) ")) {
       LOG(WARNING) << db_->GetErrorMessage();
       return false;
     }
@@ -83,12 +102,23 @@ bool TopSitesDatabase::InitThumbnailTable() {
   return true;
 }
 
+bool TopSitesDatabase::UpgradeToVersion2() {
+  // Add 'load_completed' column.
+  if (!db_->Execute(
+          "ALTER TABLE thumbnails ADD load_completed INTEGER DEFAULT 0")) {
+    NOTREACHED();
+    return false;
+  }
+  meta_table_.SetVersionNumber(2);
+  return true;
+}
+
 void TopSitesDatabase::GetPageThumbnails(MostVisitedURLList* urls,
-                                             URLToImagesMap* thumbnails) {
+                                         URLToImagesMap* thumbnails) {
   sql::Statement statement(db_->GetCachedStatement(
       SQL_FROM_HERE,
       "SELECT url, url_rank, title, thumbnail, redirects, "
-      "boring_score, good_clipping, at_top, last_updated "
+      "boring_score, good_clipping, at_top, last_updated, load_completed "
       "FROM thumbnails ORDER BY url_rank "));
 
   if (!statement) {
@@ -118,6 +148,7 @@ void TopSitesDatabase::GetPageThumbnails(MostVisitedURLList* urls,
     thumbnail.thumbnail_score.at_top = statement.ColumnBool(7);
     thumbnail.thumbnail_score.time_at_snapshot =
         base::Time::FromInternalValue(statement.ColumnInt64(8));
+    thumbnail.thumbnail_score.load_completed = statement.ColumnBool(9);
 
     (*thumbnails)[gurl] = thumbnail;
   }
@@ -163,7 +194,8 @@ void TopSitesDatabase::UpdatePageThumbnail(
       SQL_FROM_HERE,
       "UPDATE thumbnails SET "
       "title = ?, thumbnail = ?, redirects = ?, "
-      "boring_score = ?, good_clipping = ?, at_top = ?, last_updated = ? "
+      "boring_score = ?, good_clipping = ?, at_top = ?, last_updated = ?, "
+      "load_completed = ? "
       "WHERE url = ? "));
   if (!statement)
     return;
@@ -179,7 +211,8 @@ void TopSitesDatabase::UpdatePageThumbnail(
   statement.BindBool(4, score.good_clipping);
   statement.BindBool(5, score.at_top);
   statement.BindInt64(6, score.time_at_snapshot.ToInternalValue());
-  statement.BindString(7, url.url.spec());
+  statement.BindBool(7, score.load_completed);
+  statement.BindString(8, url.url.spec());
   if (!statement.Run())
     NOTREACHED() << db_->GetErrorMessage();
 }
@@ -193,8 +226,8 @@ void TopSitesDatabase::AddPageThumbnail(const MostVisitedURL& url,
       SQL_FROM_HERE,
       "INSERT OR REPLACE INTO thumbnails "
       "(url, url_rank, title, thumbnail, redirects, "
-      "boring_score, good_clipping, at_top, last_updated) "
-      "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)"));
+      "boring_score, good_clipping, at_top, last_updated, load_completed) "
+      "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)"));
   if (!statement)
     return;
 
@@ -211,6 +244,7 @@ void TopSitesDatabase::AddPageThumbnail(const MostVisitedURL& url,
   statement.BindBool(6, score.good_clipping);
   statement.BindBool(7, score.at_top);
   statement.BindInt64(8, score.time_at_snapshot.ToInternalValue());
+  statement.BindBool(9, score.load_completed);
   if (!statement.Run())
     NOTREACHED() << db_->GetErrorMessage();
 
diff --git a/chrome/browser/history/top_sites_database.h b/chrome/browser/history/top_sites_database.h
index d136798..624feeb 100644
--- a/chrome/browser/history/top_sites_database.h
+++ b/chrome/browser/history/top_sites_database.h
@@ -10,6 +10,7 @@
 #include <string>
 
 #include "app/sql/meta_table.h"
+#include "base/gtest_prod_util.h"
 #include "base/memory/ref_counted.h"
 #include "chrome/browser/history/history_types.h"
 #include "chrome/browser/history/url_database.h"  // For DBCloseScoper.
@@ -66,10 +67,16 @@ class TopSitesDatabase {
   bool RemoveURL(const MostVisitedURL& url);
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(TopSitesDatabaseTest, UpgradeToVersion2);
+
   // Creates the thumbnail table, returning true if the table already exists
   // or was successfully created.
   bool InitThumbnailTable();
 
+  // Upgrades the thumbnail table to version 2, returning true if the
+  // upgrade was successful.
+  bool UpgradeToVersion2();
+
   // Adds a new URL to the database.
   void AddPageThumbnail(const MostVisitedURL& url,
                         int new_rank,
diff --git a/chrome/browser/history/top_sites_database_unittest.cc b/chrome/browser/history/top_sites_database_unittest.cc
new file mode 100644
index 0000000..abd681e
--- /dev/null
+++ b/chrome/browser/history/top_sites_database_unittest.cc
@@ -0,0 +1,56 @@
+// Copyright (c) 2011 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 "base/file_path.h"
+#include "base/file_util.h"
+#include "base/scoped_temp_dir.h"
+#include "chrome/browser/history/top_sites_database.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace history {
+
+class TopSitesDatabaseTest : public testing::Test {
+ protected:
+  virtual void SetUp() {
+    // Get a temporary directory for the test DB files.
+    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+    file_name_ = temp_dir_.path().AppendASCII("TestTopSites.db");
+  }
+
+  ScopedTempDir temp_dir_;
+  FilePath file_name_;
+};
+
+TEST_F(TopSitesDatabaseTest, UpgradeToVersion2) {
+  TopSitesDatabase db;
+  ASSERT_TRUE(db.Init(file_name_));
+
+  // Create a version 1 table. SQLite doesn't support DROP COLUMN with
+  // ALTER TABLE. Hence, we recreate a table here.
+  ASSERT_TRUE(db.db_->Execute("DROP TABLE IF EXISTS thumbnails"));
+  ASSERT_TRUE(db.db_->Execute("CREATE TABLE thumbnails ("
+                              "url LONGVARCHAR PRIMARY KEY,"
+                              "url_rank INTEGER ,"
+                              "title LONGVARCHAR,"
+                              "thumbnail BLOB,"
+                              "redirects LONGVARCHAR,"
+                              "boring_score DOUBLE DEFAULT 1.0, "
+                              "good_clipping INTEGER DEFAULT 0, "
+                              "at_top INTEGER DEFAULT 0, "
+                              "last_updated INTEGER DEFAULT 0)"));
+  db.meta_table_.SetVersionNumber(1);
+
+  // Make sure the table meets the version 1 criteria.
+  ASSERT_EQ(1, db.meta_table_.GetVersionNumber());
+  ASSERT_FALSE(db.db_->DoesColumnExist("thumbnails", "load_completed"));
+
+  // Upgrade to version 2.
+  ASSERT_TRUE(db.UpgradeToVersion2());
+
+  // Make sure the table meets the version 2 criteria.
+  ASSERT_EQ(2, db.meta_table_.GetVersionNumber());
+  ASSERT_TRUE(db.db_->DoesColumnExist("thumbnails", "load_completed"));
+}
+
+}  // namespace history
diff --git a/chrome/browser/tab_contents/thumbnail_generator.cc b/chrome/browser/tab_contents/thumbnail_generator.cc
index 257d2bd..105e5142 100644
--- a/chrome/browser/tab_contents/thumbnail_generator.cc
+++ b/chrome/browser/tab_contents/thumbnail_generator.cc
@@ -120,7 +120,9 @@ struct ThumbnailGenerator::AsyncRequestInfo {
   RenderWidgetHost* renderer;  // Not owned.
 };
 
-ThumbnailGenerator::ThumbnailGenerator() : tab_contents_(NULL) {
+ThumbnailGenerator::ThumbnailGenerator()
+    : tab_contents_observer_registrar_(this),
+      load_interrupted_(false) {
   // The BrowserProcessImpl creates this non-lazily. If you add nontrivial
   // stuff here, be sure to convert it to being lazily created.
   //
@@ -132,7 +134,7 @@ ThumbnailGenerator::~ThumbnailGenerator() {
 }
 
 void ThumbnailGenerator::StartThumbnailing(TabContents* tab_contents) {
-  tab_contents_ = tab_contents;
+  tab_contents_observer_registrar_.Observe(tab_contents);
 
   if (registrar_.IsEmpty()) {
     // Even though we deal in RenderWidgetHosts, we only care about its
@@ -140,9 +142,9 @@ void ThumbnailGenerator::StartThumbnailing(TabContents* tab_contents) {
     // for RenderViewHosts that aren't in tabs, or RenderWidgetHosts that
     // aren't views like select popups.
     registrar_.Add(this, NotificationType::RENDER_VIEW_HOST_CREATED_FOR_TAB,
-                   Source<TabContents>(tab_contents_));
+                   Source<TabContents>(tab_contents));
     registrar_.Add(this, NotificationType::TAB_CONTENTS_DISCONNECTED,
-                   Source<TabContents>(tab_contents_));
+                   Source<TabContents>(tab_contents));
   }
 }
 
@@ -339,9 +341,9 @@ void ThumbnailGenerator::WidgetHidden(RenderWidgetHost* widget) {
   // tab_contents_ can be NULL, if StartThumbnailing() is not called, but
   // MonitorRenderer() is called. The use case is found in
   // chrome/test/ui_test_utils.cc.
-  if (!tab_contents_)
+  if (!tab_contents())
     return;
-  UpdateThumbnailIfNecessary(tab_contents_);
+  UpdateThumbnailIfNecessary(tab_contents());
 }
 
 void ThumbnailGenerator::TabContentsDisconnected(TabContents* contents) {
@@ -445,6 +447,7 @@ void ThumbnailGenerator::UpdateThumbnailIfNecessary(
   score.good_clipping =
       (clip_result == ThumbnailGenerator::kTallerThanWide ||
        clip_result == ThumbnailGenerator::kNotClipped);
+  score.load_completed = (!tab_contents->is_loading() && !load_interrupted_);
 
   top_sites->SetPageThumbnail(url, thumbnail, score);
   VLOG(1) << "Thumbnail taken for " << url << ": " << score.ToString();
@@ -478,3 +481,13 @@ bool ThumbnailGenerator::ShouldUpdateThumbnail(Profile* profile,
 
   return true;
 }
+
+void ThumbnailGenerator::DidStartLoading() {
+  load_interrupted_ = false;
+}
+
+void ThumbnailGenerator::StopNavigation() {
+  // This function gets called when the page loading is interrupted by the
+  // stop button.
+  load_interrupted_ = true;
+}
diff --git a/chrome/browser/tab_contents/thumbnail_generator.h b/chrome/browser/tab_contents/thumbnail_generator.h
index c44a952..62c91847 100644
--- a/chrome/browser/tab_contents/thumbnail_generator.h
+++ b/chrome/browser/tab_contents/thumbnail_generator.h
@@ -15,6 +15,7 @@
 #include "base/memory/linked_ptr.h"
 #include "base/timer.h"
 #include "content/browser/renderer_host/backing_store.h"
+#include "content/browser/tab_contents/tab_contents_observer.h"
 #include "content/common/notification_observer.h"
 #include "content/common/notification_registrar.h"
 
@@ -28,7 +29,8 @@ namespace history {
 class TopSites;
 }
 
-class ThumbnailGenerator : NotificationObserver {
+class ThumbnailGenerator : public NotificationObserver,
+                           public TabContentsObserver {
  public:
   typedef Callback1<const SkBitmap&>::Type ThumbnailReadyCallback;
   // The result of clipping. This can be used to determine if the
@@ -121,6 +123,10 @@ class ThumbnailGenerator : NotificationObserver {
                                     history::TopSites* top_sites,
                                     const GURL& url);
 
+  // TabContentsObserver overrides.
+  virtual void DidStartLoading();
+  virtual void StopNavigation();
+
  private:
   virtual void WidgetDidReceivePaintAtSizeAck(
       RenderWidgetHost* widget,
@@ -147,7 +153,9 @@ class ThumbnailGenerator : NotificationObserver {
                    linked_ptr<AsyncRequestInfo> > ThumbnailCallbackMap;
   ThumbnailCallbackMap callback_map_;
 
-  TabContents* tab_contents_;
+  TabContentsObserver::Registrar tab_contents_observer_registrar_;
+
+  bool load_interrupted_;
 
   DISALLOW_COPY_AND_ASSIGN(ThumbnailGenerator);
 };
diff --git a/chrome/browser/tab_contents/thumbnail_generator_unittest.cc b/chrome/browser/tab_contents/thumbnail_generator_unittest.cc
index 1c8ba8c..243d93b 100644
--- a/chrome/browser/tab_contents/thumbnail_generator_unittest.cc
+++ b/chrome/browser/tab_contents/thumbnail_generator_unittest.cc
@@ -387,6 +387,7 @@ TEST(ThumbnailGeneratorSimpleTest, ShouldUpdateThumbnail) {
   good_score.at_top = true;
   good_score.good_clipping = true;
   good_score.boring_score = 0.0;
+  good_score.load_completed = true;
   top_sites->AddKnownURL(kGoodURL, good_score);
 
   // Should be false, as the existing thumbnail is good enough (i.e. don't
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index c106ab0..497e83c 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -1406,6 +1406,7 @@
         'browser/history/text_database_manager_unittest.cc',
         'browser/history/text_database_unittest.cc',
         'browser/history/thumbnail_database_unittest.cc',
+        'browser/history/top_sites_database_unittest.cc',
         'browser/history/top_sites_unittest.cc',
         'browser/history/url_database_unittest.cc',
         'browser/history/visit_database_unittest.cc',
diff --git a/chrome/common/thumbnail_score.cc b/chrome/common/thumbnail_score.cc
index 37c942a..055429d 100644
--- a/chrome/common/thumbnail_score.cc
+++ b/chrome/common/thumbnail_score.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -15,27 +15,25 @@ const double ThumbnailScore::kThumbnailMaximumBoringness = 0.94;
 const double ThumbnailScore::kThumbnailDegradePerHour = 0.01;
 
 // Calculates a numeric score from traits about where a snapshot was
-// taken. We store the raw components in the database because I'm sure
-// this will evolve and I don't want to break databases.
-static int GetThumbnailType(bool good_clipping, bool at_top) {
-  if (good_clipping && at_top) {
-    return 0;
-  } else if (good_clipping && !at_top) {
-    return 1;
-  } else if (!good_clipping && at_top) {
-    return 2;
-  } else if (!good_clipping && !at_top) {
-    return 3;
-  } else {
-    NOTREACHED();
-    return -1;
-  }
+// taken. The lower the better. We store the raw components in the
+// database because I'm sure this will evolve and I don't want to break
+// databases.
+static int GetThumbnailType(const ThumbnailScore& score) {
+  int type = 0;
+  if (!score.at_top)
+    type += 1;
+  if (!score.good_clipping)
+    type += 2;
+  if (!score.load_completed)
+    type += 3;
+  return type;
 }
 
 ThumbnailScore::ThumbnailScore()
     : boring_score(1.0),
       good_clipping(false),
       at_top(false),
+      load_completed(false),
       time_at_snapshot(Time::Now()),
       redirect_hops_from_dest(0) {
 }
@@ -44,6 +42,7 @@ ThumbnailScore::ThumbnailScore(double score, bool clipping, bool top)
     : boring_score(score),
       good_clipping(clipping),
       at_top(top),
+      load_completed(false),
       time_at_snapshot(Time::Now()),
       redirect_hops_from_dest(0) {
 }
@@ -53,6 +52,7 @@ ThumbnailScore::ThumbnailScore(double score, bool clipping, bool top,
     : boring_score(score),
       good_clipping(clipping),
       at_top(top),
+      load_completed(false),
       time_at_snapshot(time),
       redirect_hops_from_dest(0) {
 }
@@ -73,19 +73,20 @@ bool ThumbnailScore::Equals(const ThumbnailScore& rhs) const {
 
 std::string ThumbnailScore::ToString() const {
   return StringPrintf("boring_score: %f, at_top %d, good_clipping %d, "
+                      "load_completed: %d, "
                       "time_at_snapshot: %f, redirect_hops_from_dest: %d",
                       boring_score,
                       at_top,
                       good_clipping,
+                      load_completed,
                       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);
-  int replacement_type = GetThumbnailType(replacement.good_clipping,
-                                          replacement.at_top);
+  int current_type = GetThumbnailType(current);
+  int replacement_type = GetThumbnailType(replacement);
   if (replacement_type < current_type) {
     // If we have a better class of thumbnail, add it if it meets
     // certain minimum boringness.
@@ -131,7 +132,7 @@ bool ShouldReplaceThumbnailWith(const ThumbnailScore& current,
 bool ThumbnailScore::ShouldConsiderUpdating() {
   const TimeDelta time_elapsed = Time::Now() - time_at_snapshot;
   if (time_elapsed < kUpdateThumbnailTime &&
-      good_clipping && at_top) {
+      good_clipping && at_top && load_completed) {
     // The current thumbnail is new and has good properties.
     return false;
   }
diff --git a/chrome/common/thumbnail_score.h b/chrome/common/thumbnail_score.h
index d35d7d8..0a0650b 100644
--- a/chrome/common/thumbnail_score.h
+++ b/chrome/common/thumbnail_score.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -57,6 +57,11 @@ struct ThumbnailScore {
   // thumbnails with |at_top| set to false.
   bool at_top;
 
+  // Whether this thumbnail was taken after load was completed.
+  // Thumbnails taken while page loading may only contain partial
+  // contents.
+  bool load_completed;
+
   // Record the time when a thumbnail was taken. This is used to make
   // sure thumbnails are kept fresh.
   base::Time time_at_snapshot;
diff --git a/chrome/common/thumbnail_score_unittest.cc b/chrome/common/thumbnail_score_unittest.cc
index 2fe6632..5e3151f 100644
--- a/chrome/common/thumbnail_score_unittest.cc
+++ b/chrome/common/thumbnail_score_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -66,8 +66,13 @@ TEST(ThumbnailScoreTest, ShouldConsiderUpdating) {
   score.good_clipping = true;
   EXPECT_TRUE(score.ShouldConsiderUpdating());
 
-  // at_top is important. Finally, the thumbnail is new and interesting enough.
+  // at_top is important, but still not enough.
   score.at_top = true;
+  EXPECT_TRUE(score.ShouldConsiderUpdating());
+
+  // load_completed is important. Finally, the thumbnail is new and
+  // interesting enough.
+  score.load_completed = true;
   EXPECT_FALSE(score.ShouldConsiderUpdating());
 
   // Make it very boring, but it won't change the result. The boring score