gesture nav: Encode screenshots in a worker thread.

Screenshots can be large, and png-encoding them in the UI thread is
just a very bad idea (because it can take a long time).

BUG=235739
R=piman@chromium.org

Review URL: https://codereview.chromium.org/14335003

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

6 files changed, 133 insertions, 19 deletions

diff --git a/content/browser/web_contents/navigation_controller_impl_unittest.cc b/content/browser/web_contents/navigation_controller_impl_unittest.cc
index 320c8aa..a8433e7 100644
--- a/content/browser/web_contents/navigation_controller_impl_unittest.cc
+++ b/content/browser/web_contents/navigation_controller_impl_unittest.cc
@@ -34,6 +34,7 @@
 #include "content/public/browser/web_contents_delegate.h"
 #include "content/public/test/mock_render_process_host.h"
 #include "content/public/test/test_notification_tracker.h"
+#include "content/public/test/test_utils.h"
 #include "content/test/test_web_contents.h"
 #include "net/base/net_util.h"
 #include "skia/ext/platform_canvas.h"
@@ -73,7 +74,8 @@ bool DoImagesMatch(const gfx::Image& a, const gfx::Image& b) {
 class MockScreenshotManager : public content::WebContentsScreenshotManager {
  public:
   explicit MockScreenshotManager(content::NavigationControllerImpl* owner)
-      : content::WebContentsScreenshotManager(owner) {
+      : content::WebContentsScreenshotManager(owner),
+        encoding_screenshot_in_progress_(false) {
   }
 
   virtual ~MockScreenshotManager() {
@@ -84,13 +86,22 @@ class MockScreenshotManager : public content::WebContentsScreenshotManager {
     bitmap.setConfig(SkBitmap::kARGB_8888_Config, 1, 1);
     bitmap.allocPixels();
     bitmap.eraseRGB(0, 0, 0);
+    encoding_screenshot_in_progress_ = true;
     OnScreenshotTaken(entry->GetUniqueID(), true, bitmap);
+    WaitUntilScreenshotIsReady();
   }
 
   int GetScreenshotCount() {
     return content::WebContentsScreenshotManager::GetScreenshotCount();
   }
 
+  void WaitUntilScreenshotIsReady() {
+    if (!encoding_screenshot_in_progress_)
+      return;
+    message_loop_runner_ = new content::MessageLoopRunner;
+    message_loop_runner_->Run();
+  }
+
  private:
   // Overridden from content::WebContentsScreenshotManager:
   virtual void TakeScreenshotImpl(
@@ -98,6 +109,16 @@ class MockScreenshotManager : public content::WebContentsScreenshotManager {
       content::NavigationEntryImpl* entry) OVERRIDE {
   }
 
+  virtual void OnScreenshotSet(content::NavigationEntryImpl* entry) OVERRIDE {
+    encoding_screenshot_in_progress_ = false;
+    WebContentsScreenshotManager::OnScreenshotSet(entry);
+    if (message_loop_runner_)
+      message_loop_runner_->Quit();
+  }
+
+  scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
+  bool encoding_screenshot_in_progress_;
+
   DISALLOW_COPY_AND_ASSIGN(MockScreenshotManager);
 };
 
diff --git a/content/browser/web_contents/navigation_entry_impl.cc b/content/browser/web_contents/navigation_entry_impl.cc
index dafc2e0..b3b6f78 100644
--- a/content/browser/web_contents/navigation_entry_impl.cc
+++ b/content/browser/web_contents/navigation_entry_impl.cc
@@ -310,8 +310,8 @@ void NavigationEntryImpl::ClearExtraData(const std::string& key) {
 }
 
 void NavigationEntryImpl::SetScreenshotPNGData(
-    const std::vector<unsigned char>& png_data) {
-  screenshot_ = png_data.empty() ? NULL : new base::RefCountedBytes(png_data);
+    scoped_refptr<base::RefCountedBytes> png_data) {
+  screenshot_ = png_data;
   if (screenshot_)
     UMA_HISTOGRAM_MEMORY_KB("Overscroll.ScreenshotSize", screenshot_->size());
 }
diff --git a/content/browser/web_contents/navigation_entry_impl.h b/content/browser/web_contents/navigation_entry_impl.h
index 62234bf..c5d8c49 100644
--- a/content/browser/web_contents/navigation_entry_impl.h
+++ b/content/browser/web_contents/navigation_entry_impl.h
@@ -185,7 +185,7 @@ class CONTENT_EXPORT NavigationEntryImpl
     should_replace_entry_ = should_replace_entry;
   }
 
-  void SetScreenshotPNGData(const std::vector<unsigned char>& png_data);
+  void SetScreenshotPNGData(scoped_refptr<base::RefCountedBytes> png_data);
   const scoped_refptr<base::RefCountedBytes> screenshot() const {
     return screenshot_;
   }
diff --git a/content/browser/web_contents/web_contents_screenshot_manager.cc b/content/browser/web_contents/web_contents_screenshot_manager.cc
index 9b8750f..49cb0c6 100644
--- a/content/browser/web_contents/web_contents_screenshot_manager.cc
+++ b/content/browser/web_contents/web_contents_screenshot_manager.cc
@@ -5,6 +5,7 @@
 #include "content/browser/web_contents/web_contents_screenshot_manager.h"
 
 #include "base/command_line.h"
+#include "base/threading/worker_pool.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
 #include "content/browser/web_contents/navigation_controller_impl.h"
 #include "content/browser/web_contents/navigation_entry_impl.h"
@@ -23,10 +24,45 @@ const int kMinScreenshotIntervalMS = 1000;
 
 namespace content {
 
+// Encodes an SkBitmap to PNG data in a worker thread.
+class ScreenshotData : public base::RefCountedThreadSafe<ScreenshotData> {
+ public:
+  ScreenshotData() {
+  }
+
+  void EncodeScreenshot(const SkBitmap& bitmap, base::Closure callback) {
+    if (!base::WorkerPool::PostTaskAndReply(FROM_HERE,
+            base::Bind(&ScreenshotData::EncodeOnWorker,
+                       this,
+                       bitmap),
+            callback,
+            true)) {
+      callback.Run();
+    }
+  }
+
+  scoped_refptr<base::RefCountedBytes> data() const { return data_; }
+
+ private:
+  friend class base::RefCountedThreadSafe<ScreenshotData>;
+  virtual ~ScreenshotData() {
+  }
+
+  void EncodeOnWorker(const SkBitmap& bitmap) {
+    std::vector<unsigned char> data;
+    if (gfx::PNGCodec::EncodeBGRASkBitmap(bitmap, true, &data))
+      data_ = new base::RefCountedBytes(data);
+  }
+
+  scoped_refptr<base::RefCountedBytes> data_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScreenshotData);
+};
+
 WebContentsScreenshotManager::WebContentsScreenshotManager(
     NavigationControllerImpl* owner)
     : owner_(owner),
-      ALLOW_THIS_IN_INITIALIZER_LIST(take_screenshot_factory_(this)),
+      screenshot_factory_(this),
       min_screenshot_interval_ms_(kMinScreenshotIntervalMS) {
 }
 
@@ -85,7 +121,7 @@ void WebContentsScreenshotManager::TakeScreenshotImpl(
   host->CopyFromBackingStore(gfx::Rect(),
       host->GetView()->GetViewBounds().size(),
       base::Bind(&WebContentsScreenshotManager::OnScreenshotTaken,
-                 take_screenshot_factory_.GetWeakPtr(),
+                 screenshot_factory_.GetWeakPtr(),
                  entry->GetUniqueID()));
 }
 
@@ -117,13 +153,13 @@ void WebContentsScreenshotManager::OnScreenshotTaken(int unique_id,
     return;
   }
 
-  std::vector<unsigned char> data;
-  if (gfx::PNGCodec::EncodeBGRASkBitmap(bitmap, true, &data)) {
-    entry->SetScreenshotPNGData(data);
-    PurgeScreenshotsIfNecessary();
-  } else {
-    ClearScreenshot(entry);
-  }
+  scoped_refptr<ScreenshotData> screenshot = new ScreenshotData();
+  screenshot->EncodeScreenshot(
+      bitmap,
+      base::Bind(&WebContentsScreenshotManager::OnScreenshotEncodeComplete,
+                 screenshot_factory_.GetWeakPtr(),
+                 unique_id,
+                 screenshot));
 }
 
 int WebContentsScreenshotManager::GetScreenshotCount() const {
@@ -138,11 +174,33 @@ int WebContentsScreenshotManager::GetScreenshotCount() const {
   return screenshot_count;
 }
 
+void WebContentsScreenshotManager::OnScreenshotEncodeComplete(
+    int unique_id,
+    scoped_refptr<ScreenshotData> screenshot) {
+  NavigationEntryImpl* entry = NULL;
+  int entry_count = owner_->GetEntryCount();
+  for (int i = 0; i < entry_count; ++i) {
+    NavigationEntry* iter = owner_->GetEntryAtIndex(i);
+    if (iter->GetUniqueID() == unique_id) {
+      entry = NavigationEntryImpl::FromNavigationEntry(iter);
+      break;
+    }
+  }
+  if (!entry)
+    return;
+  entry->SetScreenshotPNGData(screenshot->data());
+  OnScreenshotSet(entry);
+}
+
+void WebContentsScreenshotManager::OnScreenshotSet(NavigationEntryImpl* entry) {
+  PurgeScreenshotsIfNecessary();
+}
+
 bool WebContentsScreenshotManager::ClearScreenshot(NavigationEntryImpl* entry) {
   if (!entry->screenshot())
     return false;
 
-  entry->SetScreenshotPNGData(std::vector<unsigned char>());
+  entry->SetScreenshotPNGData(NULL);
   return true;
 }
 
diff --git a/content/browser/web_contents/web_contents_screenshot_manager.h b/content/browser/web_contents/web_contents_screenshot_manager.h
index 54e58ed..2162cf4 100644
--- a/content/browser/web_contents/web_contents_screenshot_manager.h
+++ b/content/browser/web_contents/web_contents_screenshot_manager.h
@@ -17,6 +17,7 @@ namespace content {
 class NavigationControllerImpl;
 class NavigationEntryImpl;
 class RenderViewHost;
+class ScreenshotData;
 
 // WebContentsScreenshotManager takes care of taking image-captures for the
 // current navigation entry of a NavigationControllerImpl, and managing these
@@ -37,6 +38,10 @@ class CONTENT_EXPORT WebContentsScreenshotManager {
   virtual void TakeScreenshotImpl(RenderViewHost* host,
                                   NavigationEntryImpl* entry);
 
+  // Called after a screenshot has been set on an NavigationEntryImpl.
+  // Overridden in tests to get notified of when a screenshot is set.
+  virtual void OnScreenshotSet(NavigationEntryImpl* entry);
+
   NavigationControllerImpl* owner() { return owner_; }
 
   void SetMinScreenshotIntervalMS(int interval_ms);
@@ -51,6 +56,11 @@ class CONTENT_EXPORT WebContentsScreenshotManager {
   int GetScreenshotCount() const;
 
  private:
+  // This is called when the screenshot data has beene encoded to PNG in a
+  // worker thread.
+  void OnScreenshotEncodeComplete(int unique_id,
+                                  scoped_refptr<ScreenshotData> data);
+
   // Removes the screenshot for the entry, returning true if the entry had a
   // screenshot.
   bool ClearScreenshot(NavigationEntryImpl* entry);
@@ -63,10 +73,10 @@ class CONTENT_EXPORT WebContentsScreenshotManager {
   // The navigation controller that owns this screenshot-manager.
   NavigationControllerImpl* owner_;
 
-  // Taking a screenshot can be async. So use a weakptr for the callback to make
-  // sure that the screenshot completion callback does not trigger on a
-  // destroyed WebContentsScreenshotManager.
-  base::WeakPtrFactory<WebContentsScreenshotManager> take_screenshot_factory_;
+  // Taking a screenshot and encoding them can be async. So use a weakptr for
+  // the callback to make sure that the screenshot/encoding completion callback
+  // does not trigger on a destroyed WebContentsScreenshotManager.
+  base::WeakPtrFactory<WebContentsScreenshotManager> screenshot_factory_;
 
   base::Time last_screenshot_time_;
   int min_screenshot_interval_ms_;
diff --git a/content/browser/web_contents/web_contents_view_aura_browsertest.cc b/content/browser/web_contents/web_contents_view_aura_browsertest.cc
index a18b994..a6cdfe3 100644
--- a/content/browser/web_contents/web_contents_view_aura_browsertest.cc
+++ b/content/browser/web_contents/web_contents_view_aura_browsertest.cc
@@ -33,7 +33,8 @@ class ScreenshotTracker : public WebContentsScreenshotManager {
  public:
   explicit ScreenshotTracker(NavigationControllerImpl* controller)
       : WebContentsScreenshotManager(controller),
-        screenshot_taken_for_(NULL) {
+        screenshot_taken_for_(NULL),
+        waiting_for_screenshots_(0) {
   }
 
   virtual ~ScreenshotTracker() {
@@ -49,15 +50,32 @@ class ScreenshotTracker : public WebContentsScreenshotManager {
     SetMinScreenshotIntervalMS(interval_ms);
   }
 
+  void WaitUntilScreenshotIsReady() {
+    if (!waiting_for_screenshots_)
+      return;
+    message_loop_runner_ = new content::MessageLoopRunner;
+    message_loop_runner_->Run();
+  }
+
  private:
   // Overridden from WebContentsScreenshotManager:
   virtual void TakeScreenshotImpl(RenderViewHost* host,
                                   NavigationEntryImpl* entry) OVERRIDE {
+    ++waiting_for_screenshots_;
     screenshot_taken_for_ = host;
     WebContentsScreenshotManager::TakeScreenshotImpl(host, entry);
   }
 
+  virtual void OnScreenshotSet(NavigationEntryImpl* entry) OVERRIDE {
+    --waiting_for_screenshots_;
+    WebContentsScreenshotManager::OnScreenshotSet(entry);
+    if (waiting_for_screenshots_ == 0 && message_loop_runner_)
+      message_loop_runner_->Quit();
+  }
+
   RenderViewHost* screenshot_taken_for_;
+  scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
+  int waiting_for_screenshots_;
 
   DISALLOW_COPY_AND_ASSIGN(ScreenshotTracker);
 };
@@ -321,6 +339,7 @@ IN_PROC_BROWSER_TEST_F(WebContentsViewAuraTest,
   EXPECT_EQ(1, GetCurrentIndex());
   ExecuteSyncJSFunction(view_host, "navigate_next()");
   EXPECT_EQ(2, GetCurrentIndex());
+  screenshot_manager()->WaitUntilScreenshotIsReady();
 
   // The current entry won't have any screenshots. But the entries in the
   // history should now have screenshots.
@@ -339,6 +358,7 @@ IN_PROC_BROWSER_TEST_F(WebContentsViewAuraTest,
   // Navigate again. Index 2 should now have a screenshot.
   ExecuteSyncJSFunction(view_host, "navigate_next()");
   EXPECT_EQ(3, GetCurrentIndex());
+  screenshot_manager()->WaitUntilScreenshotIsReady();
 
   entry = NavigationEntryImpl::FromNavigationEntry(
       web_contents->GetController().GetEntryAtIndex(2));
@@ -364,6 +384,7 @@ IN_PROC_BROWSER_TEST_F(WebContentsViewAuraTest,
     string16 actual_title = title_watcher.WaitAndGetTitle();
     EXPECT_EQ(expected_title, actual_title);
     EXPECT_EQ(2, GetCurrentIndex());
+    screenshot_manager()->WaitUntilScreenshotIsReady();
     entry = NavigationEntryImpl::FromNavigationEntry(
         web_contents->GetController().GetEntryAtIndex(3));
     EXPECT_TRUE(entry->screenshot().get());
@@ -374,6 +395,7 @@ IN_PROC_BROWSER_TEST_F(WebContentsViewAuraTest,
   EXPECT_EQ(3, GetCurrentIndex());
   ExecuteSyncJSFunction(view_host, "navigate_next()");
   EXPECT_EQ(4, GetCurrentIndex());
+  screenshot_manager()->WaitUntilScreenshotIsReady();
   entry = NavigationEntryImpl::FromNavigationEntry(
       web_contents->GetController().GetEntryAtIndex(4));
   EXPECT_FALSE(entry->screenshot().get());
@@ -386,6 +408,7 @@ IN_PROC_BROWSER_TEST_F(WebContentsViewAuraTest,
     string16 actual_title = title_watcher.WaitAndGetTitle();
     EXPECT_EQ(expected_title, actual_title);
     EXPECT_EQ(3, GetCurrentIndex());
+    screenshot_manager()->WaitUntilScreenshotIsReady();
     entry = NavigationEntryImpl::FromNavigationEntry(
         web_contents->GetController().GetEntryAtIndex(4));
     EXPECT_TRUE(entry->screenshot().get());
@@ -431,6 +454,7 @@ IN_PROC_BROWSER_TEST_F(WebContentsViewAuraTest,
     RenderViewHost* old_host = web_contents->GetRenderViewHost();
     web_contents->GetController().LoadURLWithParams(params);
     WaitForLoadStop(web_contents);
+    screenshot_manager()->WaitUntilScreenshotIsReady();
 
     EXPECT_NE(old_host, web_contents->GetRenderViewHost())
         << navigations[i].url.spec();
@@ -455,6 +479,7 @@ IN_PROC_BROWSER_TEST_F(WebContentsViewAuraTest,
   params.transition_type = PageTransitionFromInt(navigations[0].transition);
   web_contents->GetController().LoadURLWithParams(params);
   WaitForLoadStop(web_contents);
+  screenshot_manager()->WaitUntilScreenshotIsReady();
 
   EXPECT_EQ(NULL, screenshot_manager()->screenshot_taken_for());
 }