Add TopSites::ProcessPendingCallbacks and call it after ReadDatabase.

Fix the test and unmark as FLAKY.

BUG=51721
TEST=unit_tests, ui_tests

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

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

3 files changed, 60 insertions, 45 deletions

diff --git a/chrome/browser/history/top_sites.cc b/chrome/browser/history/top_sites.cc
index 1a8e300..97a2155 100644
--- a/chrome/browser/history/top_sites.cc
+++ b/chrome/browser/history/top_sites.cc
@@ -387,8 +387,8 @@ void TopSites::MigratePinnedURLs() {
 
 void TopSites::ApplyBlacklistAndPinnedURLs(const MostVisitedURLList& urls,
                                            MostVisitedURLList* out) {
-  MostVisitedURLList urls_copy;
   lock_.AssertAcquired();
+  MostVisitedURLList urls_copy;
   for (size_t i = 0; i < urls.size(); i++) {
     if (!IsBlacklisted(urls[i].url))
       urls_copy.push_back(urls[i]);
@@ -575,34 +575,47 @@ void TopSites::GenerateCanonicalURLs() {
 
 void TopSites::StoreMostVisited(MostVisitedURLList* most_visited) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::DB));
-  AutoLock lock(lock_);
-  top_sites_.clear();
-  // Take ownership of the most visited data.
-  top_sites_.swap(*most_visited);
-  waiting_for_results_ = false;
+  MostVisitedURLList filtered_urls;
+  PendingCallbackSet callbacks;
+  {
+    AutoLock lock(lock_);
+    top_sites_.clear();
+    // Take ownership of the most visited data.
+    top_sites_.swap(*most_visited);
+    waiting_for_results_ = false;
 
-  // Save the redirect information for quickly mapping to the canonical URLs.
-  GenerateCanonicalURLs();
+    // Save the redirect information for quickly mapping to the canonical URLs.
+    GenerateCanonicalURLs();
 
-  for (size_t i = 0; i < top_sites_.size(); i++) {
-    const MostVisitedURL& mv = top_sites_[i];
-    std::map<GURL, Images>::iterator it = temp_thumbnails_map_.begin();
-    GURL canonical_url = GetCanonicalURL(mv.url);
-    for (; it != temp_thumbnails_map_.end(); it++) {
-      // Must map all temp URLs to canonical ones.
-      // temp_thumbnails_map_ contains non-canonical URLs, because
-      // when we add a temp thumbnail, redirect chain is not known.
-      // This is slow, but temp_thumbnails_map_ should have very few URLs.
-      if (canonical_url == GetCanonicalURL(it->first)) {
-        SetPageThumbnailEncoded(mv.url, it->second.thumbnail,
-                                it->second.thumbnail_score);
-        temp_thumbnails_map_.erase(it);
-        break;
+    for (size_t i = 0; i < top_sites_.size(); i++) {
+      const MostVisitedURL& mv = top_sites_[i];
+      std::map<GURL, Images>::iterator it = temp_thumbnails_map_.begin();
+      GURL canonical_url = GetCanonicalURL(mv.url);
+      for (; it != temp_thumbnails_map_.end(); it++) {
+        // Must map all temp URLs to canonical ones.
+        // temp_thumbnails_map_ contains non-canonical URLs, because
+        // when we add a temp thumbnail, redirect chain is not known.
+        // This is slow, but temp_thumbnails_map_ should have very few URLs.
+        if (canonical_url == GetCanonicalURL(it->first)) {
+          SetPageThumbnailEncoded(mv.url, it->second.thumbnail,
+                                  it->second.thumbnail_score);
+          temp_thumbnails_map_.erase(it);
+          break;
+        }
       }
     }
-  }
-  if (top_sites_.size() >= kTopSitesNumber)
-    temp_thumbnails_map_.clear();
+    if (top_sites_.size() >= kTopSitesNumber)
+      temp_thumbnails_map_.clear();
+
+    if (pending_callbacks_.empty())
+      return;
+
+    ApplyBlacklistAndPinnedURLs(top_sites_, &filtered_urls);
+    callbacks.swap(pending_callbacks_);
+  }  // lock_ is released.
+  // Process callbacks outside the lock - ForwardResults may cause
+  // thread switches.
+  ProcessPendingCallbacks(callbacks, filtered_urls);
 }
 
 void TopSites::StoreRedirectChain(const RedirectList& redirects,
@@ -817,23 +830,19 @@ void TopSites::OnTopSitesAvailable(
   AddPrepopulatedPages(&pages);
   ChromeThread::PostTask(ChromeThread::DB, FROM_HERE, NewRunnableMethod(
       this, &TopSites::UpdateMostVisited, pages));
+}
 
-  if (!pending_callbacks_.empty()) {
-    MostVisitedURLList filtered_urls;
-    {
-      AutoLock lock(lock_);
-      ApplyBlacklistAndPinnedURLs(pages, &filtered_urls);
-    }
-
-    PendingCallbackSet::iterator i;
-    for (i = pending_callbacks_.begin();
-         i != pending_callbacks_.end(); ++i) {
-      scoped_refptr<CancelableRequest<GetTopSitesCallback> > request = *i;
-      if (!request->canceled())
-        request->ForwardResult(GetTopSitesCallback::TupleType(filtered_urls));
-    }
-    pending_callbacks_.clear();
-  }
+// static
+void TopSites::ProcessPendingCallbacks(PendingCallbackSet pending_callbacks,
+                                       const MostVisitedURLList& urls) {
+  PendingCallbackSet::iterator i;
+  for (i = pending_callbacks.begin();
+       i != pending_callbacks.end(); ++i) {
+    scoped_refptr<CancelableRequest<GetTopSitesCallback> > request = *i;
+    if (!request->canceled())
+      request->ForwardResult(GetTopSitesCallback::TupleType(urls));
+  }
+  pending_callbacks.clear();
 }
 
 void TopSites::OnThumbnailAvailable(CancelableRequestProvider::Handle handle,
diff --git a/chrome/browser/history/top_sites.h b/chrome/browser/history/top_sites.h
index f664a8b..8c11681 100644
--- a/chrome/browser/history/top_sites.h
+++ b/chrome/browser/history/top_sites.h
@@ -82,7 +82,9 @@ class TopSites :
                         const ThumbnailScore& score);
 
   // Callback for GetMostVisitedURLs.
-  typedef Callback1<const MostVisitedURLList&>::Type GetTopSitesCallback;
+  typedef Callback1<MostVisitedURLList>::Type GetTopSitesCallback;
+  typedef std::set<scoped_refptr<CancelableRequest<GetTopSitesCallback> > >
+      PendingCallbackSet;
 
   // Returns a list of most visited URLs via a callback.
   // NOTE: the callback may be called immediately if we have the data cached.
@@ -179,6 +181,10 @@ class TopSites :
   void OnTopSitesAvailable(CancelableRequestProvider::Handle handle,
                            MostVisitedURLList data);
 
+  // Returns a list of urls to each pending callback.
+  static void ProcessPendingCallbacks(PendingCallbackSet pending_callbacks,
+                                      const MostVisitedURLList& urls);
+
   // Called when history service returns a thumbnail.
   void OnThumbnailAvailable(CancelableRequestProvider::Handle handle,
                             scoped_refptr<RefCountedBytes> thumbnail);
@@ -332,8 +338,6 @@ class TopSites :
   // The map of requests for the top sites list. Can only be
   // non-empty at startup. After we read the top sites from the DB, we'll
   // always have a cached list.
-  typedef std::set<scoped_refptr<CancelableRequest<GetTopSitesCallback> > >
-      PendingCallbackSet;
   PendingCallbackSet pending_callbacks_;
 
   // Are we waiting for the top sites from HistoryService?
diff --git a/chrome/browser/history/top_sites_unittest.cc b/chrome/browser/history/top_sites_unittest.cc
index c7c2e91..d6b7ae0 100644
--- a/chrome/browser/history/top_sites_unittest.cc
+++ b/chrome/browser/history/top_sites_unittest.cc
@@ -82,7 +82,7 @@ class TopSitesTest : public testing::Test {
   }
 
   // Callback for TopSites::GetMostVisitedURLs.
-  void OnTopSitesAvailable(const history::MostVisitedURLList& data) {
+  void OnTopSitesAvailable(history::MostVisitedURLList data) {
     urls_ = data;
     number_of_callbacks_++;
   }
@@ -976,6 +976,7 @@ TEST_F(TopSitesTest, QueueingRequestsForTopSites) {
 }
 
 TEST_F(TopSitesTest, CancelingRequestsForTopSites) {
+  ChromeThread db_loop(ChromeThread::DB, MessageLoop::current());
   CancelableRequestConsumer c1;
   CancelableRequestConsumer c2;
   top_sites().GetMostVisitedURLs(
@@ -1010,6 +1011,7 @@ TEST_F(TopSitesTest, CancelingRequestsForTopSites) {
   pages.push_back(url);
 
   top_sites().OnTopSitesAvailable(0, pages);
+  MessageLoop::current()->RunAllPending();
 
   // 1 request was canceled.
   EXPECT_EQ(2u, number_of_callbacks());