Split DumpPixelsAsync in pixel_dump.h into more granular functions.

content/shell/test_runner/pixel_dump.cc

Index: content/shell/test_runner/pixel_dump.cc
diff --git a/content/shell/test_runner/pixel_dump.cc b/content/shell/test_runner/pixel_dump.cc
index fe3459b63ea38e15b55f89d93f79d4c81f5ffb39..f80e258ce50ca2c464f791aa8c10e9ed9a81b458 100644
--- a/content/shell/test_runner/pixel_dump.cc
+++ b/content/shell/test_runner/pixel_dump.cc
@@ -24,33 +24,19 @@
 #include "third_party/WebKit/public/platform/WebMockClipboard.h"
 #include "third_party/WebKit/public/platform/WebPoint.h"
 #include "third_party/WebKit/public/web/WebFrame.h"
+#include "third_party/WebKit/public/web/WebFrameWidget.h"
 #include "third_party/WebKit/public/web/WebLocalFrame.h"
 #include "third_party/WebKit/public/web/WebPagePopup.h"
 #include "third_party/WebKit/public/web/WebPrintParams.h"
-#include "third_party/WebKit/public/web/WebView.h"
 #include "ui/gfx/geometry/point.h"
 
 namespace test_runner {
 
 namespace {
 
-struct PixelsDumpRequest {
-  PixelsDumpRequest(blink::WebView* web_view,
-                    const LayoutTestRuntimeFlags& layout_test_runtime_flags,
-                    const base::Callback<void(const SkBitmap&)>& callback)
-      : web_view(web_view),
-        layout_test_runtime_flags(layout_test_runtime_flags),
-        callback(callback) {}
-
-  blink::WebView* web_view;
-  const LayoutTestRuntimeFlags& layout_test_runtime_flags;
-  base::Callback<void(const SkBitmap&)> callback;
-};
-
 class CaptureCallback : public blink::WebCompositeAndReadbackAsyncCallback {
  public:
-  explicit CaptureCallback(
-      const base::Callback<void(const SkBitmap&)>& callback);
+  explicit CaptureCallback(base::OnceCallback<void(const SkBitmap&)> callback);
   virtual ~CaptureCallback();
 
   void set_wait_for_popup(bool wait) { wait_for_popup_ = wait; }
@@ -62,32 +48,18 @@ class CaptureCallback : public blink::WebCompositeAndReadbackAsyncCallback {
   void DidCompositeAndReadback(const SkBitmap& bitmap) override;
 
  private:
-  base::Callback<void(const SkBitmap&)> callback_;
+  base::OnceCallback<void(const SkBitmap&)> callback_;
   SkBitmap main_bitmap_;
   bool wait_for_popup_;
   gfx::Point popup_position_;
 };
 
-void DrawSelectionRect(const PixelsDumpRequest& dump_request,
-                       cc::PaintCanvas* canvas) {
-  // See if we need to draw the selection bounds rect. Selection bounds
-  // rect is the rect enclosing the (possibly transformed) selection.
-  // The rect should be drawn after everything is laid out and painted.
-  if (!dump_request.layout_test_runtime_flags.dump_selection_rect())
-    return;
-
-  // TODO(lukasza): https://crbug.com/667551: Support OOPIFs in pixel dumps.
-  CHECK(dump_request.web_view->MainFrame()->IsWebLocalFrame())
-      << "This function cannot be called if the main frame is not a "
-         "local frame.";
-
-  // If there is a selection rect - draw a red 1px border enclosing rect
-  blink::WebRect wr = dump_request.web_view->MainFrame()
-                          ->ToWebLocalFrame()
-                          ->GetSelectionBoundsRectForTesting();
-  if (wr.IsEmpty())
-    return;
+void DrawSelectionRect(
+    blink::WebRect wr,

[alexmos, 2017/07/11 15:01:06]

const &?

[Łukasz Anforowicz, 2017/07/11 16:30:23]

Done.

+    base::OnceCallback<void(const SkBitmap&)> original_callback,
+    const SkBitmap& bitmap) {
   // Render a red rectangle bounding selection rect
+  cc::SkiaPaintCanvas canvas(bitmap);
   cc::PaintFlags flags;
   flags.setColor(0xFFFF0000);  // Fully opaque red
   flags.setStyle(cc::PaintFlags::kStroke_Style);
@@ -95,20 +67,17 @@ void DrawSelectionRect(const PixelsDumpRequest& dump_request,
   flags.setStrokeWidth(1.0f);
   SkIRect rect;  // Bounding rect
   rect.set(wr.x, wr.y, wr.x + wr.width, wr.y + wr.height);
-  canvas->drawIRect(rect, flags);
-}
+  canvas.drawIRect(rect, flags);
 
-void CapturePixelsForPrinting(std::unique_ptr<PixelsDumpRequest> dump_request) {
-  dump_request->web_view->UpdateAllLifecyclePhases();
+  std::move(original_callback).Run(bitmap);
+}
 
-  blink::WebSize page_size_in_pixels = dump_request->web_view->Size();
+void CapturePixelsForPrinting(
+    blink::WebLocalFrame* web_frame,
+    base::OnceCallback<void(const SkBitmap&)> callback) {
+  web_frame->FrameWidget()->UpdateAllLifecyclePhases();
 
-  // TODO(lukasza): https://crbug.com/667551: Support OOPIFs in pixel dumps.
-  CHECK(dump_request->web_view->MainFrame()->IsWebLocalFrame())
-      << "This function cannot be called if the main frame is not a "
-         "local frame.";
-  blink::WebLocalFrame* web_frame =
-      dump_request->web_view->MainFrame()->ToWebLocalFrame();
+  blink::WebSize page_size_in_pixels = web_frame->FrameWidget()->Size();
 
   int page_count = web_frame->PrintBegin(page_size_in_pixels);
   int totalHeight = page_count * (page_size_in_pixels.height + 1) - 1;
@@ -120,7 +89,7 @@ void CapturePixelsForPrinting(std::unique_ptr<PixelsDumpRequest> dump_request) {
                                 is_opaque)) {
     LOG(ERROR) << "Failed to create bitmap width=" << page_size_in_pixels.width
                << " height=" << totalHeight;
-    dump_request->callback.Run(SkBitmap());
+    std::move(callback).Run(SkBitmap());
     return;
   }
 
@@ -128,13 +97,12 @@ void CapturePixelsForPrinting(std::unique_ptr<PixelsDumpRequest> dump_request) {
   web_frame->PrintPagesForTesting(&canvas, page_size_in_pixels);
   web_frame->PrintEnd();
 
-  DrawSelectionRect(*dump_request, &canvas);
-  dump_request->callback.Run(bitmap);
+  std::move(callback).Run(bitmap);
 }
 
 CaptureCallback::CaptureCallback(
-    const base::Callback<void(const SkBitmap&)>& callback)
-    : callback_(callback), wait_for_popup_(false) {}
+    base::OnceCallback<void(const SkBitmap&)> callback)
+    : callback_(std::move(callback)), wait_for_popup_(false) {}
 
 CaptureCallback::~CaptureCallback() {}
 
@@ -142,7 +110,7 @@ void CaptureCallback::DidCompositeAndReadback(const SkBitmap& bitmap) {
   TRACE_EVENT2("shell", "CaptureCallback::didCompositeAndReadback", "x",
                bitmap.info().width(), "y", bitmap.info().height());
   if (!wait_for_popup_) {
-    callback_.Run(bitmap);
+    std::move(callback_).Run(bitmap);
     delete this;
     return;
   }
@@ -155,42 +123,23 @@ void CaptureCallback::DidCompositeAndReadback(const SkBitmap& bitmap) {
   }
   SkCanvas canvas(main_bitmap_);
   canvas.drawBitmap(bitmap, popup_position_.x(), popup_position_.y());
-  callback_.Run(main_bitmap_);
+  std::move(callback_).Run(main_bitmap_);
   delete this;
 }
 
-void DidCapturePixelsAsync(std::unique_ptr<PixelsDumpRequest> dump_request,
-                           const SkBitmap& bitmap) {
-  cc::SkiaPaintCanvas canvas(bitmap);
-  DrawSelectionRect(*dump_request, &canvas);
-  if (!dump_request->callback.is_null())
-    dump_request->callback.Run(bitmap);
-}
-
 }  // namespace
 
-void DumpPixelsAsync(blink::WebView* web_view,
-                     const LayoutTestRuntimeFlags& layout_test_runtime_flags,
+void DumpPixelsAsync(blink::WebLocalFrame* web_frame,
                      float device_scale_factor_for_test,
-                     const base::Callback<void(const SkBitmap&)>& callback) {
-  TRACE_EVENT0("shell", "WebViewTestProxyBase::CapturePixelsAsync");
+                     base::OnceCallback<void(const SkBitmap&)> callback) {
+  DCHECK(web_frame);
+  DCHECK_LT(0.0, device_scale_factor_for_test);
   DCHECK(!callback.is_null());
-  DCHECK(!layout_test_runtime_flags.dump_drag_image());
 
-  std::unique_ptr<PixelsDumpRequest> pixels_request(
-      new PixelsDumpRequest(web_view, layout_test_runtime_flags, callback));
-
-  if (layout_test_runtime_flags.is_printing()) {
-    base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::Bind(&CapturePixelsForPrinting,
-                              base::Passed(std::move(pixels_request))));
-    return;
-  }
-
-  CaptureCallback* capture_callback = new CaptureCallback(base::Bind(
-      &DidCapturePixelsAsync, base::Passed(std::move(pixels_request))));
-  web_view->CompositeAndReadbackAsync(capture_callback);
-  if (blink::WebPagePopup* popup = web_view->GetPagePopup()) {
+  blink::WebWidget* web_widget = web_frame->FrameWidget();
+  CaptureCallback* capture_callback = new CaptureCallback(std::move(callback));
+  web_widget->CompositeAndReadbackAsync(capture_callback);
+  if (blink::WebPagePopup* popup = web_widget->GetPagePopup()) {
     capture_callback->set_wait_for_popup(true);
     blink::WebPoint position = popup->PositionRelativeToOwner();
     position.x *= device_scale_factor_for_test;
@@ -200,30 +149,53 @@ void DumpPixelsAsync(blink::WebView* web_view,
   }
 }
 
+void PrintFrameAsync(blink::WebLocalFrame* web_frame,
+                     base::OnceCallback<void(const SkBitmap&)> callback) {
+  DCHECK(web_frame);
+  DCHECK(!callback.is_null());
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::Bind(&CapturePixelsForPrinting, base::Unretained(web_frame),
+                 base::Passed(std::move(callback))));
+}
+
+base::OnceCallback<void(const SkBitmap&)> CreateSelectionDrawingCallback(
+    blink::WebLocalFrame* web_frame,
+    base::OnceCallback<void(const SkBitmap&)> original_callback) {
+  DCHECK(web_frame);
+  DCHECK(!original_callback.is_null());
+
+  // If there is no selection rect, just return the original callback.
+  blink::WebRect wr = web_frame->GetSelectionBoundsRectForTesting();
+  if (wr.IsEmpty())
+    return original_callback;

[alexmos, 2017/07/11 15:01:06]

does this need std::move?

[Łukasz Anforowicz, 2017/07/11 16:30:23]

1. It compiles, therefore it has to be a move (base::OnceCallback<> doesn't have
a copy constructor).

2. I don't quite understand it myself, but I hear |return foo| is preferrable
over |return std::move(foo)| because the latter can in some cases undesirably
prevent RVO / ellision.  I found a bit of conflicting advise out there, so I am
not sure if it is fair for me to cherry-pick one that fits my narrative, but
maybe I can leave this here:
https://isocpp.org/blog/2013/02/no-really-moving-a-return-value-is-easy-stack...

[alexmos, 2017/07/11 16:48:10]

Ack.  Thanks for the pointers, I was confused about this myself, as I also saw
some conflicting examples in our codebase (e.g.,
https://cs.chromium.org/chromium/src/android_webview/browser/net/aw_cookie_st...).

+
+  return base::Bind(&DrawSelectionRect, wr,
+                    base::Passed(std::move(original_callback)));
+}
+
 void CopyImageAtAndCapturePixels(
-    blink::WebView* web_view,
+    blink::WebLocalFrame* web_frame,
     int x,
     int y,
-    const base::Callback<void(const SkBitmap&)>& callback) {
+    base::OnceCallback<void(const SkBitmap&)> callback) {
   DCHECK(!callback.is_null());
   uint64_t sequence_number =
       blink::Platform::Current()->Clipboard()->SequenceNumber(
           blink::WebClipboard::Buffer());
-  // TODO(lukasza): Support image capture in OOPIFs for
-  // https://crbug.com/477150.
-  web_view->MainFrame()->ToWebLocalFrame()->CopyImageAt(blink::WebPoint(x, y));
+  web_frame->CopyImageAt(blink::WebPoint(x, y));
   if (sequence_number ==
       blink::Platform::Current()->Clipboard()->SequenceNumber(
           blink::WebClipboard::Buffer())) {
     SkBitmap emptyBitmap;
-    callback.Run(emptyBitmap);
+    std::move(callback).Run(emptyBitmap);
     return;
   }
 
   blink::WebImage image = static_cast<blink::WebMockClipboard*>(
                               blink::Platform::Current()->Clipboard())
                               ->ReadRawImage(blink::WebClipboard::Buffer());
-  callback.Run(image.GetSkBitmap());
+  std::move(callback).Run(image.GetSkBitmap());
 }
 
 }  // namespace test_runner