Reland fix for thumbnail generation.

chrome/browser/thumbnails/thumbnail_tab_helper.cc

 // Copyright (c) 2012 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 "chrome/browser/thumbnails/thumbnail_tab_helper.h"
 
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/thumbnails/thumbnail_service.h"
 #include "chrome/browser/thumbnails/thumbnail_service_factory.h"
@@ skipping 40 matching lines @@
 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();
+  context.service()->SetPageThumbnail(context, image);
+  DVLOG(1) << "Thumbnail taken for " << context.GetURL() << ": "
+           << context.score().ToString();
+
+  // Now that we're completely finished with the web contents, balance the
+  // IncrementCapturerCount() from UpdateThumbnailIfNecessary().
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);

[Lei Zhang, 2015/11/18 18:06:43]

This generally should be at the top of the function. The one below on like 87 is
a special case since it has to deal with the UI thread "going away" on shutdown.

[shrike, 2015/11/18 18:20:16]

I actually wasn't sure if it was needed at all (but I was thinking the
DecrementCapturerCount() needs to run from the UI thread). Should I move it to
the top or remove the check completely?

[Lei Zhang, 2015/11/18 18:24:00]

Move it to the top.

+  context.web_contents()->DecrementCapturerCount();
 }
 
 void ProcessCapturedBitmap(scoped_refptr<ThumbnailingContext> context,
                            scoped_refptr<ThumbnailingAlgorithm> algorithm,
                            const SkBitmap& bitmap,
                            content::ReadbackResponse response) {
-  if (response != content::READBACK_SUCCESS)
+  // Was the web contents destroyed before the thumbnail could be generated?
+  if (!context->web_contents())
     return;
 
+  if (response != content::READBACK_SUCCESS) {
+    // Balance the IncrementCapturerCount() from UpdateThumbnailIfNecessary().
+    context->web_contents()->DecrementCapturerCount();
+    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,
+void AsyncProcessThumbnail(scoped_refptr<ThumbnailingContext> context,
                            scoped_refptr<ThumbnailingAlgorithm> algorithm) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   RenderWidgetHost* render_widget_host =
-      web_contents->GetRenderViewHost()->GetWidget();
+      context->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(
+  gfx::Size requested_copy_size;
+  context->set_clip_result(algorithm->GetCanvasCopyInfo(
       copy_rect.size(),
       scale_factor,
       &copy_rect,
-      &context->requested_copy_size);
+      &requested_copy_size));
+  context->set_requested_copy_size(requested_copy_size);
   render_widget_host->CopyFromBackingStore(
       copy_rect,
-      context->requested_copy_size,
+      requested_copy_size,
       base::Bind(&ProcessCapturedBitmap, context, algorithm),
       kN32_SkColorType);
 }
 
 }  // namespace
 
 ThumbnailTabHelper::ThumbnailTabHelper(content::WebContents* contents)
     : content::WebContentsObserver(contents),
       load_interrupted_(false) {
   // Even though we deal in RenderWidgetHosts, we only care about its
@@ skipping 65 matching lines @@
 
   scoped_refptr<thumbnails::ThumbnailService> thumbnail_service =
       ThumbnailServiceFactory::GetForProfile(profile);
 
   // Skip if we don't need to update the thumbnail.
   if (thumbnail_service.get() == NULL ||
       !thumbnail_service->ShouldAcquirePageThumbnail(url)) {
     return;
   }
 
+  web_contents->IncrementCapturerCount(gfx::Size());
+  // Stop responding to notifications. It turns out that the corresponding
+  // DecrementCapturerCount() can trigger a call to
+  // content::WebContentsImpl::WasHidden(), which eventually calls
+  // ThumbnailTabHelper::UpdateThumbnailIfNecessary(), starting the thumbnail
+  // generation process all over again.
+  registrar_.RemoveAll();

[Lei Zhang, 2015/11/18 18:06:43]

Is ThumbnailTabHelper one-shot? If not, then this breaks thumbnailing for future
RenderViews / on future hidden events.

[shrike, 2015/11/18 18:20:16]

It appears to be one per web contents (tab_helpers.cc:205).

[Lei Zhang, 2015/11/18 18:24:00]

That I understand. My question is, will a ThumbnailTabHelper make one snapshot
in its lifetime, or more than one? By clearing the notification registrar, a
ThumbnailTabHelper instance will only make one.

[shrike, 2015/11/18 19:18:06]

Sorry (need more coffee).

My assumption in turning off all notifications at this point was that the user
is switching away from the page, which is when the generator needs to generate a
thumbnail. I was thinking at that point you're done with thumbnail generation,
but I realize that the web contents may not go away completely? and the user
might switch back to its page, which means we'll want another thumbnail
generation attempt if the user switches away again. So I don't think it's a good
idea to disable notifications for good like this.

My goal is to prevent or ignore notifications that result from the call to
DecrementCapturerCount(), so perhaps a better idea is to disable this particular
notification here and to reenable it after the call to IncrementCapturerCount()?

+
   scoped_refptr<thumbnails::ThumbnailingAlgorithm> algorithm(
       thumbnail_service->GetThumbnailingAlgorithm());
 
   scoped_refptr<ThumbnailingContext> context(new ThumbnailingContext(
       web_contents, thumbnail_service.get(), load_interrupted_));
-  AsyncProcessThumbnail(web_contents, context, algorithm);
+  AsyncProcessThumbnail(context, algorithm);
 }
 
 void ThumbnailTabHelper::RenderViewHostCreated(
     content::RenderViewHost* renderer) {
   // NOTIFICATION_WEB_CONTENTS_RENDER_VIEW_HOST_CREATED is really a new
   // RenderView, not RenderViewHost, and there is no good way to get
   // notifications of RenderViewHosts. So just be tolerant of re-registrations.
   bool registered = registrar_.IsRegistered(
       this, content::NOTIFICATION_RENDER_WIDGET_VISIBILITY_CHANGED,
       content::Source<RenderWidgetHost>(renderer->GetWidget()));
   if (!registered) {
     registrar_.Add(this, content::NOTIFICATION_RENDER_WIDGET_VISIBILITY_CHANGED,
                    content::Source<RenderWidgetHost>(renderer->GetWidget()));
   }
 }
 
 void ThumbnailTabHelper::WidgetHidden(RenderWidgetHost* widget) {
   UpdateThumbnailIfNecessary(web_contents());
 }