Reland fix for thumbnail generation.

chrome/browser/thumbnails/thumbnail_tab_helper.cc

Index: chrome/browser/thumbnails/thumbnail_tab_helper.cc
diff --git a/chrome/browser/thumbnails/thumbnail_tab_helper.cc b/chrome/browser/thumbnails/thumbnail_tab_helper.cc
index a5de4ee96d333b457f869314119c46252b4aef3a..1f6260d14dfb66b567629857e0209166ca9db063 100644
--- a/chrome/browser/thumbnails/thumbnail_tab_helper.cc
+++ b/chrome/browser/thumbnails/thumbnail_tab_helper.cc
@@ -53,73 +53,11 @@ using thumbnails::ClipResult;
 using thumbnails::ThumbnailingContext;
 using thumbnails::ThumbnailingAlgorithm;
 
-namespace {
-
-// Feed the constructed thumbnail to the thumbnail service.
-void UpdateThumbnail(const ThumbnailingContext& context,
-                     const SkBitmap& thumbnail) {
-  gfx::Image image = gfx::Image::CreateFrom1xBitmap(thumbnail);
-  context.service->SetPageThumbnail(context, image);
-  DVLOG(1) << "Thumbnail taken for " << context.url << ": "
-           << context.score.ToString();
-}
-
-void ProcessCapturedBitmap(scoped_refptr<ThumbnailingContext> context,
-                           scoped_refptr<ThumbnailingAlgorithm> algorithm,
-                           const SkBitmap& bitmap,
-                           content::ReadbackResponse response) {
-  if (response != content::READBACK_SUCCESS)
-    return;
-
-  // On success, we must be on the UI thread (on failure because of shutdown we
-  // are not on the UI thread).
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-
-  algorithm->ProcessBitmap(context, base::Bind(&UpdateThumbnail), bitmap);
-}
-
-void AsyncProcessThumbnail(content::WebContents* web_contents,
-                           scoped_refptr<ThumbnailingContext> context,
-                           scoped_refptr<ThumbnailingAlgorithm> algorithm) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  RenderWidgetHost* render_widget_host =
-      web_contents->GetRenderViewHost()->GetWidget();
-  content::RenderWidgetHostView* view = render_widget_host->GetView();
-  if (!view)
-    return;
-  if (!view->IsSurfaceAvailableForCopy())
-    return;
-
-  gfx::Rect copy_rect = gfx::Rect(view->GetViewBounds().size());
-  // Clip the pixels that will commonly hold a scrollbar, which looks bad in
-  // thumbnails.
-  int scrollbar_size = gfx::scrollbar_size();
-  gfx::Size copy_size;
-  copy_rect.Inset(0, 0, scrollbar_size, scrollbar_size);
-
-  if (copy_rect.IsEmpty())
-    return;
-
-  ui::ScaleFactor scale_factor =
-      ui::GetSupportedScaleFactor(
-          ui::GetScaleFactorForNativeView(view->GetNativeView()));
-  context->clip_result = algorithm->GetCanvasCopyInfo(
-      copy_rect.size(),
-      scale_factor,
-      &copy_rect,
-      &context->requested_copy_size);
-  render_widget_host->CopyFromBackingStore(
-      copy_rect,
-      context->requested_copy_size,
-      base::Bind(&ProcessCapturedBitmap, context, algorithm),
-      kN32_SkColorType);
-}
-
-}  // namespace
-
 ThumbnailTabHelper::ThumbnailTabHelper(content::WebContents* contents)
     : content::WebContentsObserver(contents),
-      load_interrupted_(false) {
+      thumbnailing_context_(nullptr),

[Lei Zhang, 2016/01/09 02:14:24]

no need to explicitly initialize scoped_refptrs.

[shrike, 2016/01/11 19:07:08]

Acknowledged.

+      load_interrupted_(false),
+      weak_factory_(this) {
   // Even though we deal in RenderWidgetHosts, we only care about its
   // subclass, RenderViewHost when it is in a tab. We don't make thumbnails
   // for RenderViewHosts that aren't in tabs, or RenderWidgetHosts that
@@ -172,8 +110,94 @@ void ThumbnailTabHelper::NavigationStopped() {
   load_interrupted_ = true;
 }
 
+// Balance the call to IncrementCapturerCount() from

[Lei Zhang, 2016/01/09 02:14:24]

Comments go in the header.

[shrike, 2016/01/11 19:07:08]

Acknowledged.

+// UpdateThumbnailIfNecessary(), and inform the tab helper that thumbnail
+// generation is complete.
+void ThumbnailTabHelper::DecrementCapturerCount() {
+  if (web_contents()) {
+    web_contents()->DecrementCapturerCount();
+  }
+  thumbnailing_context_ = nullptr;
+}
+
+// Feed the constructed thumbnail to the thumbnail service.
+void ThumbnailTabHelper::UpdateThumbnail(
+    const thumbnails::ThumbnailingContext& context,
+    const SkBitmap& thumbnail) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  gfx::Image image = gfx::Image::CreateFrom1xBitmap(thumbnail);
+      context.service->SetPageThumbnail(context, image);
+  DVLOG(1) << "Thumbnail taken for " << context.url << ": "
+      << context.score.ToString();
+
+  DecrementCapturerCount();
+}
+
+void ThumbnailTabHelper::ProcessCapturedBitmap(
+    scoped_refptr<thumbnails::ThumbnailingAlgorithm> algorithm,
+    const SkBitmap& bitmap,
+    content::ReadbackResponse response) {
+  if (web_contents() && response == content::READBACK_SUCCESS) {
+    // On success, we must be on the UI thread (on failure because of shutdown
+    // we are not on the UI thread).
+    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+    algorithm->ProcessBitmap(
+        thumbnailing_context_,
+        base::Bind(&ThumbnailTabHelper::UpdateThumbnail,
+                   weak_factory_.GetWeakPtr()),
+        bitmap);
+  } else {
+    DecrementCapturerCount();
+  }
+}
+
+void ThumbnailTabHelper::AsyncProcessThumbnail(
+    scoped_refptr<thumbnails::ThumbnailingAlgorithm> algorithm) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  RenderWidgetHost* render_widget_host =
+      web_contents()->GetRenderViewHost()->GetWidget();
+  content::RenderWidgetHostView* view = render_widget_host->GetView();
+  if (!view || !view->IsSurfaceAvailableForCopy()) {
+    thumbnailing_context_ = nullptr;
+    return;
+  }
+
+  gfx::Rect copy_rect = gfx::Rect(view->GetViewBounds().size());
+  // Clip the pixels that will commonly hold a scrollbar, which looks bad in
+  // thumbnails.
+  int scrollbar_size = gfx::scrollbar_size();
+  gfx::Size copy_size;
+  copy_rect.Inset(0, 0, scrollbar_size, scrollbar_size);
+
+  if (copy_rect.IsEmpty()) {
+    thumbnailing_context_ = nullptr;
+    return;
+  }
+
+  ui::ScaleFactor scale_factor =
+      ui::GetSupportedScaleFactor(
+          ui::GetScaleFactorForNativeView(view->GetNativeView()));
+  thumbnailing_context_->clip_result = algorithm->GetCanvasCopyInfo(
+      copy_rect.size(),
+      scale_factor,
+      &copy_rect,
+      &thumbnailing_context_->requested_copy_size);
+  render_widget_host->CopyFromBackingStore(
+      copy_rect,
+      thumbnailing_context_->requested_copy_size,
+      base::Bind(&ThumbnailTabHelper::ProcessCapturedBitmap,
+                 weak_factory_.GetWeakPtr(),
+                 algorithm),
+      kN32_SkColorType);
+}
+
 void ThumbnailTabHelper::UpdateThumbnailIfNecessary(
     WebContents* web_contents) {

[Lei Zhang, 2016/01/09 02:14:25]

BTW, why bother having this? Just call web_contents() as needed.

+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);

[Lei Zhang, 2016/01/09 02:14:24]

I think you should add this everywhere.

[shrike, 2016/01/11 19:07:08]

It pretty much already is everywhere.

+  // Ignore thumbnail update requests if one is already being generated.
+  if (thumbnailing_context_) {
+    return;
+  }
   // Destroying a WebContents may trigger it to be hidden, prompting a snapshot
   // which would be unwise to attempt <http://crbug.com/130097>. If the
   // WebContents is in the middle of destruction, do not risk it.
@@ -196,12 +220,18 @@ void ThumbnailTabHelper::UpdateThumbnailIfNecessary(
     return;
   }
 
+  web_contents->IncrementCapturerCount(gfx::Size());
+  // It turns out that the corresponding DecrementCapturerCount() can trigger a

[Lei Zhang, 2016/01/09 02:14:24]

This comment is about IncrementCapturerCount() right? So it should go above the
call, not below.

[shrike, 2016/01/11 19:07:08]

Yeah. This latest patchset was a checkpoint, not really ready for review. I
uploaded it to get the cl off my machine and saved in the ether.

+  // call to content::WebContentsImpl::WasHidden(), which eventually calls
+  // ThumbnailTabHelper::UpdateThumbnailIfNecessary(), starting the thumbnail
+  // generation process all over again. Break this cycle by noting that we're
+  // currently generating a thumbnail.
   scoped_refptr<thumbnails::ThumbnailingAlgorithm> algorithm(

[Lei Zhang, 2016/01/09 02:14:25]

This can now be done in AsyncProcessThumbnail().

[shrike, 2016/01/11 19:07:08]

Acknowledged.

       thumbnail_service->GetThumbnailingAlgorithm());
 
-  scoped_refptr<ThumbnailingContext> context(new ThumbnailingContext(
-      web_contents, thumbnail_service.get(), load_interrupted_));
-  AsyncProcessThumbnail(web_contents, context, algorithm);
+  thumbnailing_context_ = new ThumbnailingContext(
+      web_contents, thumbnail_service.get(), load_interrupted_);
+  AsyncProcessThumbnail(algorithm);
 }
 
 void ThumbnailTabHelper::RenderViewHostCreated(