Reland fix for thumbnail generation.

chrome/browser/thumbnails/thumbnailing_context.h

Index: chrome/browser/thumbnails/thumbnailing_context.h
diff --git a/chrome/browser/thumbnails/thumbnailing_context.h b/chrome/browser/thumbnails/thumbnailing_context.h
index 1cfe67178aeb2a2270510a0da26cb12058efddb8..763ef8b7ed8c2d9a2f9d8f0ce164fb0189b12d21 100644
--- a/chrome/browser/thumbnails/thumbnailing_context.h
+++ b/chrome/browser/thumbnails/thumbnailing_context.h
@@ -7,11 +7,15 @@
 
 #include "base/basictypes.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
 #include "chrome/browser/thumbnails/thumbnail_service.h"
 #include "components/history/core/common/thumbnail_score.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_observer.h"
 #include "ui/gfx/geometry/size.h"
 
+class ThumbnailTabHelper;
+
 namespace thumbnails {
 
 // The result of clipping. This can be used to determine if the
@@ -34,9 +38,13 @@ enum ClipResult {
 };
 
 // Holds the information needed for processing a thumbnail.
-struct ThumbnailingContext : base::RefCountedThreadSafe<ThumbnailingContext> {
+class ThumbnailingContext
+    : public base::RefCountedThreadSafe<ThumbnailingContext>,

[Lei Zhang, 2015/11/20 20:01:04]

BTW, I don't think ThumbnailingContext needs to be ref counted, and it is never
passed between threads, so it doesn't need to be thread safe either. Likely the
original author did this just to be on the safe side, and to make passing it
around easier?

[shrike, 2016/01/08 18:32:36]

I think it does. At the very least the context is passed to the thumbnail
algorithm, and there are no guarantees about the thread doing the work there.

+      public content::WebContentsObserver {
+ public:
   ThumbnailingContext(content::WebContents* web_contents,
                       ThumbnailService* receiving_service,
+                      base::WeakPtr<ThumbnailTabHelper> source_tab_helper,

[Lei Zhang, 2015/11/20 20:01:04]

Isn't the ThumbnailTabHelper also a WebContentsUserData subclass for the same
WebContents?

Doesn't that mean if |web_contents| becomes invalid, we already know
|sorce_tab_helper| is invalid as well?

[shrike, 2016/01/08 18:32:36]

Yes and yes. Having a link to the tab helper is important in the case where the
web contents is *not* invalid. The helper sets a flag to know that thumbnail
generation is in progress and the context needs to reset that flag once
generation is complete (otherwise if thumbnail generation fails, say, the tab
helper will never try again to generate a thumbnail as long as it remains
instantiated). But, I have figured out another way to get this information back
to the helper that does not involve the context having a reference to the
helper.

                       bool load_interrupted);
 
   // Create an instance for use with unit tests.
@@ -44,17 +52,34 @@ struct ThumbnailingContext : base::RefCountedThreadSafe<ThumbnailingContext> {
     return new ThumbnailingContext();
   }
 
-  scoped_refptr<ThumbnailService> service;
-  GURL url;
-  ClipResult clip_result;
-  gfx::Size requested_copy_size;
-  ThumbnailScore score;
+  const scoped_refptr<ThumbnailService>& service() const;
+
+  const GURL& GetURL() const;
+
+  ClipResult clip_result() const;
+  void set_clip_result(ClipResult result);
+
+  gfx::Size requested_copy_size();
+  void set_requested_copy_size(const gfx::Size& requested_size);
+
+  ThumbnailScore score() const;
+  void SetBoringScore(double score);
+  void SetGoodClipping(bool is_good_clipping);
+  base::WeakPtr<ThumbnailTabHelper> source_tab_helper() const {
+    return source_tab_helper_;
+  }
 
  private:
   ThumbnailingContext();
-  ~ThumbnailingContext();
+  ~ThumbnailingContext() override;
 
   friend class base::RefCountedThreadSafe<ThumbnailingContext>;
+
+  scoped_refptr<ThumbnailService> service_;
+  ClipResult clip_result_;
+  gfx::Size requested_copy_size_;
+  ThumbnailScore score_;
+  base::WeakPtr<ThumbnailTabHelper> source_tab_helper_;
 };
 
 }  // namespace thumbnails