Fix set_area sizes for RequestCopyOfOutput, and related tests.

NOTE: This is a revival/continuation of https://codereview.chromium.org/333723004, which was started by another developer.

The cc:CopyOutputRequest object assumes the src_subrect region is
in DIP space.  Thus, all calling code should provide the same.

Fixed tests in render_widget_host_view_browsertest.cc that were
completing successfully, even though the implementation was broken
for the high DPI scenarios.

In addition, I updated a number of interface comments to clarify the
difference between src_subrect being in DIP coordinates versus the
dest_size being in physical pixel coordinates.

Dependent on: https://codereview.chromium.org/311253004 (landed at r276139)

BUG=397708,399349

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

Cr-Commit-Position: refs/heads/master@{#289509}
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@289509 0039d316-1c4b-4281-b951-d872f2087c98

content/browser/compositor/delegated_frame_host.cc

@@ -183,9 +183,7 @@ void DelegatedFrameHost::CopyFromCompositingSurfaceToVideoFrame(
           subscriber_texture,
           target,
           callback));
-  gfx::Rect src_subrect_in_pixel =
-      ConvertRectToPixel(client_->CurrentDeviceScaleFactor(), src_subrect);
-  request->set_area(src_subrect_in_pixel);
+  request->set_area(src_subrect);
   if (subscriber_texture.get()) {
     request->SetTextureMailbox(
         cc::TextureMailbox(subscriber_texture->mailbox(),

content/browser/media/capture/desktop_capture_device_aura.cc

@@ -278,10 +278,8 @@ void DesktopVideoCaptureMachine::Capture(bool dirty) {
         cc::CopyOutputRequest::CreateRequest(
             base::Bind(&DesktopVideoCaptureMachine::DidCopyOutput,
                        AsWeakPtr(), frame, start_time, capture_frame_cb));
-    gfx::Rect window_rect =
-        ui::ConvertRectToPixel(desktop_window_->layer(),
-                               gfx::Rect(desktop_window_->bounds().width(),
-                                         desktop_window_->bounds().height()));
+    gfx::Rect window_rect = gfx::Rect(desktop_window_->bounds().width(),
+                                      desktop_window_->bounds().height());
     request->set_area(window_rect);
     desktop_window_->layer()->RequestCopyOfOutput(request.Pass());
   }

content/browser/renderer_host/render_widget_host_view_base.h

@@ -243,27 +243,27 @@ class CONTENT_EXPORT RenderWidgetHostViewBase : public RenderWidgetHostView,
   // Notifies the view that the scroll offset has changed.
   virtual void ScrollOffsetChanged() = 0;
 
-  // Copies the contents of the compositing surface into the given
-  // (uninitialized) PlatformCanvas if any.
-  // The rectangle region specified with |src_subrect| is copied from the
-  // contents, scaled to |dst_size|, and written to |output|.
-  // |callback| is invoked with true on success, false otherwise. |output| can
-  // be initialized even on failure.
-  // A smaller region than |src_subrect| may be copied if the underlying surface
-  // is smaller than |src_subrect|.
-  // NOTE: |callback| is called asynchronously.
+  // Copies the contents of the compositing surface, providing a new SkBitmap
+  // result via an asynchronously-run |callback|. |src_subrect| is specified in
+  // layer space coordinates for the current platform (e.g., DIP for Aura/Mac,
+  // physical for Android), and is the region to be copied from this view. The
+  // copy is then scaled to a SkBitmap of size |dst_size|. |callback| is run
+  // with true on success, false otherwise. A smaller region than |src_subrect|
+  // may be copied if the underlying surface is smaller than |src_subrect|.
   virtual void CopyFromCompositingSurface(
       const gfx::Rect& src_subrect,
       const gfx::Size& dst_size,
       const base::Callback<void(bool, const SkBitmap&)>& callback,
       const SkColorType color_type) = 0;
 
-  // Copies a given subset of the compositing surface's content into a YV12
-  // VideoFrame, and invokes a callback with a success/fail parameter. |target|
-  // must contain an allocated, YV12 video frame of the intended size. If the
-  // copy rectangle does not match |target|'s size, the copied content will be
-  // scaled and letterboxed with black borders. The copy will happen
-  // asynchronously. This operation will fail if there is no available
+  // Copies the contents of the compositing surface, populating the given
+  // |target| with YV12 image data. |src_subrect| is specified in layer space
+  // coordinates for the current platform (e.g., DIP for Aura/Mac, physical for
+  // Android), and is the region to be copied from this view. The copy is then
+  // scaled and letterboxed with black borders to fit |target|. Finally,
+  // |callback| is asynchronously run with true/false for
+  // success/failure. |target| must point to an allocated, YV12 video frame of
+  // the intended size. This operation will fail if there is no available
   // compositing surface.
   virtual void CopyFromCompositingSurfaceToVideoFrame(
       const gfx::Rect& src_subrect,

content/browser/renderer_host/render_widget_host_view_browsertest.cc

@@ -29,7 +29,7 @@
 #include "third_party/skia/include/core/SkCanvas.h"
 #include "ui/base/layout.h"
 #include "ui/base/ui_base_switches.h"
-#include "ui/gfx/size_conversions.h"
+#include "ui/gfx/geometry/size_conversions.h"
 #include "ui/gfx/switches.h"
 #include "ui/gl/gl_switches.h"
 
@@ -523,15 +523,12 @@ class CompositingRenderWidgetHostViewBrowserTestTabCapture
   // Loads a page two boxes side-by-side, each half the width of
   // |html_rect_size|, and with different background colors. The test then
   // copies from |copy_rect| region of the page into a bitmap of size
-  // |output_size|, and compares that with a bitmap of size
-  // |expected_bitmap_size|.
+  // |output_size|, and examines the resulting bitmap/VideoFrame.
   // Note that |output_size| may not have the same size as |copy_rect| (e.g.
-  // when the output is scaled). Also note that |expected_bitmap_size| may not
-  // be the same as |output_size| (e.g. when the device scale factor is not 1).
+  // when the output is scaled).
   void PerformTestWithLeftRightRects(const gfx::Size& html_rect_size,
                                      const gfx::Rect& copy_rect,
                                      const gfx::Size& output_size,
-                                     const gfx::Size& expected_bitmap_size,
                                      bool video_frame) {
     const gfx::Size box_size(html_rect_size.width() / 2,
                              html_rect_size.height());
@@ -592,7 +589,7 @@ class CompositingRenderWidgetHostViewBrowserTestTabCapture
       GiveItSomeTime();
 
     SkBitmap expected_bitmap;
-    SetupLeftRightBitmap(expected_bitmap_size, &expected_bitmap);
+    SetupLeftRightBitmap(output_size, &expected_bitmap);
     SetExpectedCopyFromCompositingSurfaceResult(true, expected_bitmap);
 
     base::RunLoop run_loop;
@@ -605,9 +602,9 @@ class CompositingRenderWidgetHostViewBrowserTestTabCapture
 
       scoped_refptr<media::VideoFrame> video_frame =
           media::VideoFrame::CreateFrame(media::VideoFrame::YV12,
-                                         expected_bitmap_size,
-                                         gfx::Rect(expected_bitmap_size),
-                                         expected_bitmap_size,
+                                         output_size,
+                                         gfx::Rect(output_size),
+                                         output_size,
                                          base::TimeDelta());
 
       base::Callback<void(bool success)> callback =
@@ -679,13 +676,11 @@ IN_PROC_BROWSER_TEST_P(CompositingRenderWidgetHostViewBrowserTestTabCapture,
                        CopyFromCompositingSurface_Origin_Unscaled) {
   gfx::Rect copy_rect(400, 300);
   gfx::Size output_size = copy_rect.size();
-  gfx::Size expected_bitmap_size = output_size;
   gfx::Size html_rect_size(400, 300);
   bool video_frame = false;
   PerformTestWithLeftRightRects(html_rect_size,
                                 copy_rect,
                                 output_size,
-                                expected_bitmap_size,
                                 video_frame);
 }
 
@@ -693,13 +688,11 @@ IN_PROC_BROWSER_TEST_P(CompositingRenderWidgetHostViewBrowserTestTabCapture,
                        CopyFromCompositingSurface_Origin_Scaled) {
   gfx::Rect copy_rect(400, 300);
   gfx::Size output_size(200, 100);
-  gfx::Size expected_bitmap_size = output_size;
   gfx::Size html_rect_size(400, 300);
   bool video_frame = false;
   PerformTestWithLeftRightRects(html_rect_size,
                                 copy_rect,
                                 output_size,
-                                expected_bitmap_size,
                                 video_frame);
 }
 
@@ -710,13 +703,11 @@ IN_PROC_BROWSER_TEST_P(CompositingRenderWidgetHostViewBrowserTestTabCapture,
   copy_rect = gfx::Rect(copy_rect.CenterPoint() - gfx::Vector2d(30, 30),
                         gfx::Size(60, 60));
   gfx::Size output_size = copy_rect.size();
-  gfx::Size expected_bitmap_size = output_size;
   gfx::Size html_rect_size(400, 300);
   bool video_frame = false;
   PerformTestWithLeftRightRects(html_rect_size,
                                 copy_rect,
                                 output_size,
-                                expected_bitmap_size,
                                 video_frame);
 }
 
@@ -727,13 +718,11 @@ IN_PROC_BROWSER_TEST_P(CompositingRenderWidgetHostViewBrowserTestTabCapture,
   copy_rect = gfx::Rect(copy_rect.CenterPoint() - gfx::Vector2d(30, 30),
                         gfx::Size(60, 60));
   gfx::Size output_size(20, 10);
-  gfx::Size expected_bitmap_size = output_size;
   gfx::Size html_rect_size(400, 300);
   bool video_frame = false;
   PerformTestWithLeftRightRects(html_rect_size,
                                 copy_rect,
                                 output_size,
-                                expected_bitmap_size,
                                 video_frame);
 }
 
@@ -744,13 +733,11 @@ IN_PROC_BROWSER_TEST_P(CompositingRenderWidgetHostViewBrowserTestTabCapture,
   copy_rect = gfx::Rect(copy_rect.CenterPoint() - gfx::Vector2d(45, 30),
                         gfx::Size(90, 60));
   gfx::Size output_size = copy_rect.size();
-  gfx::Size expected_bitmap_size = output_size;
   gfx::Size html_rect_size(400, 300);
   bool video_frame = true;
   PerformTestWithLeftRightRects(html_rect_size,
                                 copy_rect,
                                 output_size,
-                                expected_bitmap_size,
                                 video_frame);
 }
 
@@ -762,101 +749,174 @@ IN_PROC_BROWSER_TEST_P(CompositingRenderWidgetHostViewBrowserTestTabCapture,
                         gfx::Size(90, 60));
   // Scale to 30 x 20 (preserve aspect ratio).
   gfx::Size output_size(30, 20);
-  gfx::Size expected_bitmap_size = output_size;
   gfx::Size html_rect_size(400, 300);
   bool video_frame = true;
   PerformTestWithLeftRightRects(html_rect_size,
                                 copy_rect,
                                 output_size,
-                                expected_bitmap_size,
                                 video_frame);
 }
 
-class CompositingRenderWidgetHostViewTabCaptureHighDPI
+class CompositingRenderWidgetHostViewBrowserTestTabCaptureHighDPI
     : public CompositingRenderWidgetHostViewBrowserTestTabCapture {
  public:
-  CompositingRenderWidgetHostViewTabCaptureHighDPI() : kScale(2.f) {}
+  CompositingRenderWidgetHostViewBrowserTestTabCaptureHighDPI() {}
 
-  virtual void SetUpOnMainThread() OVERRIDE {
-    base::CommandLine* cmd = base::CommandLine::ForCurrentProcess();
+ protected:
+  virtual void SetUpCommandLine(base::CommandLine* cmd) OVERRIDE {
+    CompositingRenderWidgetHostViewBrowserTestTabCapture::SetUpCommandLine(cmd);
     cmd->AppendSwitchASCII(switches::kForceDeviceScaleFactor,
                            base::StringPrintf("%f", scale()));
-#if defined(OS_WIN)
-    gfx::ForceHighDPISupportForTesting(scale());
-    gfx::EnableHighDPISupport();
-#endif
   }
 
-  float scale() const { return kScale; }
-
- private:
   virtual bool ShouldContinueAfterTestURLLoad() OVERRIDE {
     // Short-circuit a pass for platforms where setting up high-DPI fails.
-    if (ui::GetScaleForScaleFactor(ui::GetSupportedScaleFactor(
-            GetScaleFactorForView(GetRenderWidgetHostView()))) != scale()) {
-      LOG(WARNING) << "Blindly passing this test: failed to set up "
-          "scale factor: " << scale();
+    const float actual_scale_factor =
+        GetScaleFactorForView(GetRenderWidgetHostView());
+    if (actual_scale_factor != scale()) {
+      LOG(WARNING) << "Blindly passing this test; unable to force device scale "
+                   << "factor: seems to be " << actual_scale_factor
+                   << " but expected " << scale();
       return false;
     }
+    VLOG(1) << ("Successfully forced device scale factor.  Moving forward with "
+                "this test!  :-)");
     return true;
   }
 
-  const float kScale;
+  static float scale() { return 2.0f; }
 
-  DISALLOW_COPY_AND_ASSIGN(CompositingRenderWidgetHostViewTabCaptureHighDPI);
+ private:
+  DISALLOW_COPY_AND_ASSIGN(
+      CompositingRenderWidgetHostViewBrowserTestTabCaptureHighDPI);
 };
 
-IN_PROC_BROWSER_TEST_P(CompositingRenderWidgetHostViewTabCaptureHighDPI,
-                       CopyFromCompositingSurface) {
+// ImageSkia (related to ResourceBundle) implementation crashes the process on
+// Windows when this content_browsertest forces a device scale factor.
+// http://crbug.com/399349
+#if defined(OS_WIN)
+#define MAYBE_CopyToBitmap_EntireRegion DISABLED_CopyToBitmap_EntireRegion
+#define MAYBE_CopyToBitmap_CenterRegion DISABLED_CopyToBitmap_CenterRegion
+#define MAYBE_CopyToBitmap_ScaledResult DISABLED_CopyToBitmap_ScaledResult
+#define MAYBE_CopyToVideoFrame_EntireRegion \
+            DISABLED_CopyToVideoFrame_EntireRegion
+#define MAYBE_CopyToVideoFrame_CenterRegion \
+            DISABLED_CopyToVideoFrame_CenterRegion
+#define MAYBE_CopyToVideoFrame_ScaledResult \
+            DISABLED_CopyToVideoFrame_ScaledResult
+#else
+#define MAYBE_CopyToBitmap_EntireRegion CopyToBitmap_EntireRegion
+#define MAYBE_CopyToBitmap_CenterRegion CopyToBitmap_CenterRegion
+#define MAYBE_CopyToBitmap_ScaledResult CopyToBitmap_ScaledResult
+#define MAYBE_CopyToVideoFrame_EntireRegion CopyToVideoFrame_EntireRegion
+#define MAYBE_CopyToVideoFrame_CenterRegion CopyToVideoFrame_CenterRegion
+#define MAYBE_CopyToVideoFrame_ScaledResult CopyToVideoFrame_ScaledResult
+#endif
+
+IN_PROC_BROWSER_TEST_P(
+    CompositingRenderWidgetHostViewBrowserTestTabCaptureHighDPI,
+    MAYBE_CopyToBitmap_EntireRegion) {
+  gfx::Size html_rect_size(200, 150);
   gfx::Rect copy_rect(200, 150);
-  gfx::Size output_size = copy_rect.size();
-  gfx::Size expected_bitmap_size =
-      gfx::ToFlooredSize(gfx::ScaleSize(output_size, scale(), scale()));
+  // Scale the output size so that, internally, scaling is not occurring.
+  gfx::Size output_size =
+      gfx::ToRoundedSize(gfx::ScaleSize(copy_rect.size(), scale()));
+  bool video_frame = false;
+  PerformTestWithLeftRightRects(html_rect_size,
+                                copy_rect,
+                                output_size,
+                                video_frame);
+}
+
+IN_PROC_BROWSER_TEST_P(
+    CompositingRenderWidgetHostViewBrowserTestTabCaptureHighDPI,
+    MAYBE_CopyToBitmap_CenterRegion) {
   gfx::Size html_rect_size(200, 150);
+  // Grab 90x60 pixels from the center of the tab contents.
+  gfx::Rect copy_rect =
+      gfx::Rect(gfx::Rect(html_rect_size).CenterPoint() - gfx::Vector2d(45, 30),
+                gfx::Size(90, 60));
+  // Scale the output size so that, internally, scaling is not occurring.
+  gfx::Size output_size =
+      gfx::ToRoundedSize(gfx::ScaleSize(copy_rect.size(), scale()));
+  bool video_frame = false;
+  PerformTestWithLeftRightRects(html_rect_size,
+                                copy_rect,
+                                output_size,
+                                video_frame);
+}
+
+IN_PROC_BROWSER_TEST_P(
+    CompositingRenderWidgetHostViewBrowserTestTabCaptureHighDPI,
+    MAYBE_CopyToBitmap_ScaledResult) {
+  gfx::Size html_rect_size(200, 100);
+  gfx::Rect copy_rect(200, 100);
+  // Output is being down-scaled since output_size is in phyiscal pixels.
+  gfx::Size output_size(200, 100);
   bool video_frame = false;
   PerformTestWithLeftRightRects(html_rect_size,
                                 copy_rect,
                                 output_size,
-                                expected_bitmap_size,
                                 video_frame);
 }
 
-IN_PROC_BROWSER_TEST_P(CompositingRenderWidgetHostViewTabCaptureHighDPI,
-                       CopyFromCompositingSurfaceVideoFrame) {
+IN_PROC_BROWSER_TEST_P(
+    CompositingRenderWidgetHostViewBrowserTestTabCaptureHighDPI,
+    MAYBE_CopyToVideoFrame_EntireRegion) {
+  gfx::Size html_rect_size(200, 150);
+  gfx::Rect copy_rect(200, 150);
+  // Scale the output size so that, internally, scaling is not occurring.
+  gfx::Size output_size =
+      gfx::ToRoundedSize(gfx::ScaleSize(copy_rect.size(), scale()));
+  bool video_frame = true;
+  PerformTestWithLeftRightRects(html_rect_size,
+                                copy_rect,
+                                output_size,
+                                video_frame);
+}
+
+IN_PROC_BROWSER_TEST_P(
+    CompositingRenderWidgetHostViewBrowserTestTabCaptureHighDPI,
+    MAYBE_CopyToVideoFrame_CenterRegion) {
   gfx::Size html_rect_size(200, 150);
   // Grab 90x60 pixels from the center of the tab contents.
   gfx::Rect copy_rect =
       gfx::Rect(gfx::Rect(html_rect_size).CenterPoint() - gfx::Vector2d(45, 30),
                 gfx::Size(90, 60));
-  gfx::Size output_size = copy_rect.size();
-  gfx::Size expected_bitmap_size =
-      gfx::ToFlooredSize(gfx::ScaleSize(output_size, scale(), scale()));
+  // Scale the output size so that, internally, scaling is not occurring.
+  gfx::Size output_size =
+      gfx::ToRoundedSize(gfx::ScaleSize(copy_rect.size(), scale()));
   bool video_frame = true;
   PerformTestWithLeftRightRects(html_rect_size,
                                 copy_rect,
                                 output_size,
-                                expected_bitmap_size,
                                 video_frame);
 }
 
-#if !defined(USE_AURA) && !defined(OS_MACOSX)
-// TODO(danakj): Remove this case when GTK linux is no more and move the
-// values inline to testing::Values() below.
-static const CompositingMode kAllCompositingModes[] = {GL_COMPOSITING};
-#else
-static const CompositingMode kAllCompositingModes[] = {GL_COMPOSITING,
-                                                       SOFTWARE_COMPOSITING};
-#endif
+IN_PROC_BROWSER_TEST_P(
+    CompositingRenderWidgetHostViewBrowserTestTabCaptureHighDPI,
+    MAYBE_CopyToVideoFrame_ScaledResult) {
+  gfx::Size html_rect_size(200, 100);
+  gfx::Rect copy_rect(200, 100);
+  // Output is being down-scaled since output_size is in phyiscal pixels.
+  gfx::Size output_size(200, 100);
+  bool video_frame = true;
+  PerformTestWithLeftRightRects(html_rect_size,
+                                copy_rect,
+                                output_size,
+                                video_frame);
+}
 
 INSTANTIATE_TEST_CASE_P(GLAndSoftwareCompositing,
                         CompositingRenderWidgetHostViewBrowserTest,
-                        testing::ValuesIn(kAllCompositingModes));
+                        testing::Values(GL_COMPOSITING, SOFTWARE_COMPOSITING));
 INSTANTIATE_TEST_CASE_P(GLAndSoftwareCompositing,
                         CompositingRenderWidgetHostViewBrowserTestTabCapture,
-                        testing::ValuesIn(kAllCompositingModes));
-INSTANTIATE_TEST_CASE_P(GLAndSoftwareCompositing,
-                        CompositingRenderWidgetHostViewTabCaptureHighDPI,
-                        testing::ValuesIn(kAllCompositingModes));
+                        testing::Values(GL_COMPOSITING, SOFTWARE_COMPOSITING));
+INSTANTIATE_TEST_CASE_P(
+    GLAndSoftwareCompositing,
+    CompositingRenderWidgetHostViewBrowserTestTabCaptureHighDPI,
+    testing::Values(GL_COMPOSITING, SOFTWARE_COMPOSITING));
 
 #endif  // !defined(OS_ANDROID) && !defined(OS_IOS)
 

ui/snapshot/snapshot.h

@@ -41,10 +41,12 @@ SNAPSHOT_EXPORT bool GrabViewSnapshot(
     std::vector<unsigned char>* png_representation,
     const gfx::Rect& snapshot_bounds);
 
+
+// These functions take a snapshot of |source_rect|, specified in layer space
+// coordinates (DIP for desktop, physical pixels for Android), and scale the
+// snapshot to |target_size| (in physical pixels), asynchronously.
 typedef base::Callback<void(const gfx::Image& snapshot)>
     GrabWindowSnapshotAsyncCallback;
-// GrabWindowSnapshotAndScaleAsync() copies snapshot of |source_rect| from
-// window and scales it to |target_size| asynchronously.
 SNAPSHOT_EXPORT void GrabWindowSnapshotAndScaleAsync(
     gfx::NativeWindow window,
     const gfx::Rect& source_rect,