Revert 87264 - Add load_completed property to ThumbnailScore.

The change broke the tsan (ThreadSanitizer) build:

http://build.chromium.org/p/chromium.memory/builders/Linux%20Tests%20%28tsan%29/builds/3164

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

TBR=satorux@chromium.org
Review URL: http://codereview.chromium.org/7094002

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

10 files changed, 40 insertions, 171 deletions

diff --git a/chrome/browser/history/top_sites_database.cc b/chrome/browser/history/top_sites_database.cc
index 8a3d715..4639494 100644
--- a/chrome/browser/history/top_sites_database.cc
+++ b/chrome/browser/history/top_sites_database.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2009 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,9 +14,7 @@
 
 namespace history {
 
-// 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;
+static const int kVersionNumber = 1;
 
 TopSitesDatabase::TopSitesDatabase() : may_need_history_migration_(false) {
 }
@@ -53,32 +51,16 @@ 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;
 }
 
@@ -93,8 +75,7 @@ bool TopSitesDatabase::InitThumbnailTable() {
                       "boring_score DOUBLE DEFAULT 1.0, "
                       "good_clipping INTEGER DEFAULT 0, "
                       "at_top INTEGER DEFAULT 0, "
-                      "last_updated INTEGER DEFAULT 0, "
-                      "load_completed INTEGER DEFAULT 0) ")) {
+                      "last_updated INTEGER DEFAULT 0) ")) {
       LOG(WARNING) << db_->GetErrorMessage();
       return false;
     }
@@ -102,23 +83,12 @@ 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, load_completed "
+      "boring_score, good_clipping, at_top, last_updated "
       "FROM thumbnails ORDER BY url_rank "));
 
   if (!statement) {
@@ -148,7 +118,6 @@ 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;
   }
@@ -194,8 +163,7 @@ void TopSitesDatabase::UpdatePageThumbnail(
       SQL_FROM_HERE,
       "UPDATE thumbnails SET "
       "title = ?, thumbnail = ?, redirects = ?, "
-      "boring_score = ?, good_clipping = ?, at_top = ?, last_updated = ?, "
-      "load_completed = ? "
+      "boring_score = ?, good_clipping = ?, at_top = ?, last_updated = ? "
       "WHERE url = ? "));
   if (!statement)
     return;
@@ -211,8 +179,7 @@ void TopSitesDatabase::UpdatePageThumbnail(
   statement.BindBool(4, score.good_clipping);
   statement.BindBool(5, score.at_top);
   statement.BindInt64(6, score.time_at_snapshot.ToInternalValue());
-  statement.BindBool(7, score.load_completed);
-  statement.BindString(8, url.url.spec());
+  statement.BindString(7, url.url.spec());
   if (!statement.Run())
     NOTREACHED() << db_->GetErrorMessage();
 }
@@ -226,8 +193,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, load_completed) "
-      "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)"));
+      "boring_score, good_clipping, at_top, last_updated) "
+      "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)"));
   if (!statement)
     return;
 
@@ -244,7 +211,6 @@ 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 624feeb..d136798 100644
--- a/chrome/browser/history/top_sites_database.h
+++ b/chrome/browser/history/top_sites_database.h
@@ -10,7 +10,6 @@
 #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.
@@ -67,16 +66,10 @@ 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
deleted file mode 100644
index abd681e..0000000
--- a/chrome/browser/history/top_sites_database_unittest.cc
+++ /dev/null
@@ -1,56 +0,0 @@
-// 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 105e5142..257d2bd 100644
--- a/chrome/browser/tab_contents/thumbnail_generator.cc
+++ b/chrome/browser/tab_contents/thumbnail_generator.cc
@@ -120,9 +120,7 @@ struct ThumbnailGenerator::AsyncRequestInfo {
   RenderWidgetHost* renderer;  // Not owned.
 };
 
-ThumbnailGenerator::ThumbnailGenerator()
-    : tab_contents_observer_registrar_(this),
-      load_interrupted_(false) {
+ThumbnailGenerator::ThumbnailGenerator() : tab_contents_(NULL) {
   // The BrowserProcessImpl creates this non-lazily. If you add nontrivial
   // stuff here, be sure to convert it to being lazily created.
   //
@@ -134,7 +132,7 @@ ThumbnailGenerator::~ThumbnailGenerator() {
 }
 
 void ThumbnailGenerator::StartThumbnailing(TabContents* tab_contents) {
-  tab_contents_observer_registrar_.Observe(tab_contents);
+  tab_contents_ = tab_contents;
 
   if (registrar_.IsEmpty()) {
     // Even though we deal in RenderWidgetHosts, we only care about its
@@ -142,9 +140,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_));
   }
 }
 
@@ -341,9 +339,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) {
@@ -447,7 +445,6 @@ 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();
@@ -481,13 +478,3 @@ 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 62c91847..c44a952 100644
--- a/chrome/browser/tab_contents/thumbnail_generator.h
+++ b/chrome/browser/tab_contents/thumbnail_generator.h
@@ -15,7 +15,6 @@
 #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"
 
@@ -29,8 +28,7 @@ namespace history {
 class TopSites;
 }
 
-class ThumbnailGenerator : public NotificationObserver,
-                           public TabContentsObserver {
+class ThumbnailGenerator : NotificationObserver {
  public:
   typedef Callback1<const SkBitmap&>::Type ThumbnailReadyCallback;
   // The result of clipping. This can be used to determine if the
@@ -123,10 +121,6 @@ class ThumbnailGenerator : public NotificationObserver,
                                     history::TopSites* top_sites,
                                     const GURL& url);
 
-  // TabContentsObserver overrides.
-  virtual void DidStartLoading();
-  virtual void StopNavigation();
-
  private:
   virtual void WidgetDidReceivePaintAtSizeAck(
       RenderWidgetHost* widget,
@@ -153,9 +147,7 @@ class ThumbnailGenerator : public NotificationObserver,
                    linked_ptr<AsyncRequestInfo> > ThumbnailCallbackMap;
   ThumbnailCallbackMap callback_map_;
 
-  TabContentsObserver::Registrar tab_contents_observer_registrar_;
-
-  bool load_interrupted_;
+  TabContents* tab_contents_;
 
   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 243d93b..1c8ba8c 100644
--- a/chrome/browser/tab_contents/thumbnail_generator_unittest.cc
+++ b/chrome/browser/tab_contents/thumbnail_generator_unittest.cc
@@ -387,7 +387,6 @@ 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 497e83c..c106ab0 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -1406,7 +1406,6 @@
         '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 055429d..37c942a 100644
--- a/chrome/common/thumbnail_score.cc
+++ b/chrome/common/thumbnail_score.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2009 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,25 +15,27 @@ const double ThumbnailScore::kThumbnailMaximumBoringness = 0.94;
 const double ThumbnailScore::kThumbnailDegradePerHour = 0.01;
 
 // Calculates a numeric score from traits about where a snapshot was
-// 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;
+// 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;
+  }
 }
 
 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) {
 }
@@ -42,7 +44,6 @@ 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) {
 }
@@ -52,7 +53,6 @@ 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,20 +73,19 @@ 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);
-  int replacement_type = GetThumbnailType(replacement);
+  int current_type = GetThumbnailType(current.good_clipping, current.at_top);
+  int replacement_type = GetThumbnailType(replacement.good_clipping,
+                                          replacement.at_top);
   if (replacement_type < current_type) {
     // If we have a better class of thumbnail, add it if it meets
     // certain minimum boringness.
@@ -132,7 +131,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 && load_completed) {
+      good_clipping && at_top) {
     // 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 0a0650b..d35d7d8 100644
--- a/chrome/common/thumbnail_score.h
+++ b/chrome/common/thumbnail_score.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2009 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,11 +57,6 @@ 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 5e3151f..2fe6632 100644
--- a/chrome/common/thumbnail_score_unittest.cc
+++ b/chrome/common/thumbnail_score_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2009 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,13 +66,8 @@ TEST(ThumbnailScoreTest, ShouldConsiderUpdating) {
   score.good_clipping = true;
   EXPECT_TRUE(score.ShouldConsiderUpdating());
 
-  // at_top is important, but still not enough.
+  // at_top is important. Finally, the thumbnail is new and interesting 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