Fix set_area sizes for RequestCopyOfOutput, and related tests.

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
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289509

[miu, 2014-08-13 01:54:02]

This is a revival of an older change (see change desc).  PTAL.

danakj: render_widget_host_view_browsertest.cc

sky: "Clarifying comment" changes in render_widget_host_view_base.h and
snapshot.h

yfriedman: All files with android in the name, plus content_view_core_impl.cc.

already LGTM'ed from crrev.com/333723004: delegated_frame_host.cc,
desktop_capture_device_aura.cc

[danakj, 2014-08-13 14:31:31]

https://codereview.chromium.org/462173002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_browsertest.cc
(right):

https://codereview.chromium.org/462173002/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:768:
CompositingRenderWidgetHostViewBrowserTestTabCaptureHighDPI() {}
can remove this

https://codereview.chromium.org/462173002/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:778: //
Short-circuit a pass for platforms where setting up high-DPI fails.
Do these platforms still exist? Can you try removing this check and see if the
bots will take it?

https://codereview.chromium.org/462173002/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:859:
gfx::Size output_size = copy_rect.size();
if you want something not scaled down, shouldn't the output_size be scaled by
the DSF?

[miu, 2014-08-13 18:11:11]

https://codereview.chromium.org/462173002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_browsertest.cc
(right):

https://codereview.chromium.org/462173002/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:768:
CompositingRenderWidgetHostViewBrowserTestTabCaptureHighDPI() {}
On 2014/08/13 14:31:30, danakj wrote:
> can remove this

Tried that, and clang says:

../../content/browser/renderer_host/render_widget_host_view_browsertest.cc:859:1:
error: constructor for 'content::(anonymous
namespace)::CompositingRenderWidgetHostViewBrowserTestTabCaptureHighDPI_CopyFromCompositingSurfaceToVideoFrame_ScaledResult_Test'
must explicitly initialize the base class 'content::(anonymous
namespace)::CompositingRenderWidgetHostViewBrowserTestTabCaptureHighDPI' which
does not have a default constructor
IN_PROC_BROWSER_TEST_P(
^

https://codereview.chromium.org/462173002/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:778: //
Short-circuit a pass for platforms where setting up high-DPI fails.
On 2014/08/13 14:31:31, danakj wrote:
> Do these platforms still exist? Can you try removing this check and see if the
> bots will take it?

Done.  Cross your fingers!  ;-)

https://codereview.chromium.org/462173002/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:859:
gfx::Size output_size = copy_rect.size();
On 2014/08/13 14:31:30, danakj wrote:
> if you want something not scaled down, shouldn't the output_size be scaled by
> the DSF?

Done.  Good point.

https://codereview.chromium.org/462173002/diff/80001/content/browser/media/ca...
File content/browser/media/capture/desktop_capture_device_aura.cc (right):

https://codereview.chromium.org/462173002/diff/80001/content/browser/media/ca...
content/browser/media/capture/desktop_capture_device_aura.cc:248:
oracle_proxy_->UpdateCaptureSize(ui::ConvertSizeToPixel(
(This change was due to a rebase.)

[danakj, 2014-08-13 18:39:31]

render_widget_host_view_browsertest.cc LGTM

https://codereview.chromium.org/462173002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_browsertest.cc
(right):

https://codereview.chromium.org/462173002/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:768:
CompositingRenderWidgetHostViewBrowserTestTabCaptureHighDPI() {}
On 2014/08/13 18:11:10, miu wrote:
> On 2014/08/13 14:31:30, danakj wrote:
> > can remove this
> 
> Tried that, and clang says:
> 
>
../../content/browser/renderer_host/render_widget_host_view_browsertest.cc:859:1:
> error: constructor for 'content::(anonymous
>
namespace)::CompositingRenderWidgetHostViewBrowserTestTabCaptureHighDPI_CopyFromCompositingSurfaceToVideoFrame_ScaledResult_Test'
> must explicitly initialize the base class 'content::(anonymous
> namespace)::CompositingRenderWidgetHostViewBrowserTestTabCaptureHighDPI' which
> does not have a default constructor
> IN_PROC_BROWSER_TEST_P(
> ^

Oh because of the DISALLOW_COPY_AND_ASSIGN I guess. I'm not used to seeing that
in *test.cc files.

https://codereview.chromium.org/462173002/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_browsertest.cc
(right):

https://codereview.chromium.org/462173002/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:837: //
Output is being down-scaled since output_size is in phyiscal pixels:
nit: s/:/./

https://codereview.chromium.org/462173002/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:848:
MAYBE_CopyToVideoFr_EntireRegion) {
CopyToVideoFrame please, don't drop half the word

[miu, 2014-08-13 18:48:27]

Thanks for the review.

https://codereview.chromium.org/462173002/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_browsertest.cc
(right):

https://codereview.chromium.org/462173002/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:837: //
Output is being down-scaled since output_size is in phyiscal pixels:
On 2014/08/13 18:39:31, danakj wrote:
> nit: s/:/./

Done.

https://codereview.chromium.org/462173002/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:848:
MAYBE_CopyToVideoFr_EntireRegion) {
On 2014/08/13 18:39:31, danakj wrote:
> CopyToVideoFrame please, don't drop half the word

Done.  I had truncated it because of the 80 char limit, but just made the
#define MAYBE... multi-line instead.

[Yaron, 2014-08-13 18:59:46]

newt's probably a better android owner for this

[newt (away), 2014-08-13 19:10:17]

Android changes looks reasonable to me, though I'm not familiar with this
particular code.

[newt (away), 2014-08-13 19:10:18]

Android changes looks reasonable to me, though I'm not familiar with this
particular code.

[newt (away), 2014-08-13 19:11:02]

+aelias who is probably more familiar with this

[miu, 2014-08-13 22:25:45]

Had a chat with aelias@, and discovered that the Android compositor's layer
space is in physical coordinates.  Thus, I've reverted all the Anrdoid-specific
changes.

https://codereview.chromium.org/462173002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_browsertest.cc
(right):

https://codereview.chromium.org/462173002/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:778: //
Short-circuit a pass for platforms where setting up high-DPI fails.
On 2014/08/13 18:11:10, miu wrote:
> On 2014/08/13 14:31:31, danakj wrote:
> > Do these platforms still exist? Can you try removing this check and see if
the
> > bots will take it?
> 
> Done.  Cross your fingers!  ;-)

The bots did not like this, so I put it back.

[miu, 2014-08-13 22:28:44]

sky: Just need OWNERS stamp for the comment clean-up in
render_widget_host_view_base.h.

[sky, 2014-08-13 22:49:28]

LGTM

https://codereview.chromium.org/462173002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_browsertest.cc
(right):

https://codereview.chromium.org/462173002/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:438: //
Exclude the middle 3 columns of pixels from testing since scaling could
Do we really run tests at different scales? If so, wouldn't it be better to
force a particular scale so you don't have to deal with this?

[miu, 2014-08-14 00:18:33]

https://codereview.chromium.org/462173002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_browsertest.cc
(right):

https://codereview.chromium.org/462173002/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:438: //
Exclude the middle 3 columns of pixels from testing since scaling could
On 2014/08/13 22:49:28, sky wrote:
> Do we really run tests at different scales? If so, wouldn't it be better to
> force a particular scale so you don't have to deal with this?

Removed.  Just checked this again, and it looks like it's not needed anymore. 
IIRC, an earlier iteration of the code was doing zooming (rather than
shrinking), which caused the implementation to mix pixels at the boundary
between the two color regions.

[miu, 2014-08-14 00:18:39]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2014-08-14 00:19:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/miu@chromium.org/462173002/200001

[commit-bot: I haz the power, 2014-08-14 06:27:07]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[commit-bot: I haz the power, 2014-08-14 10:03:15]

Committed patchset #7 (200001) as 289509