Consistent CopyFromSurface() API, consolidated to RWHV

Consistent CopyFromSurface() API, consolidated to RWHV

A clean-up to consolidate the multiple "copy from RenderWidget surface"
methods across a few classes into the RenderWidgetHostView class.
Deletes the legacy RWH::CopyFromBackingStore() API and migrates all
callers to use RWHV::CopyFromSurface(). Also, made changes to ensure the
semantics of the arguments is consistent among all callers and among all
platform impls; and place less burdeon on callers to compute rects
correctly. Added a few TODO comments, where size data is being used
incorrectly, to be addressed in later changes relevant to crbug 73362.

BUG=73362
TEST=Manually tested what did not have obvious unit testing in place (e.g., devtools color picker).
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2702093002
Cr-Commit-Position: refs/heads/master@{#454098}
Committed: https://chromium.googlesource.com/chromium/src/+/515321aa08c29e910a720af8b3752ab52826df22

[miu, 2017-02-19 01:17:43]

Description was changed from

==========
Consistent CopyFromSurface() API and always try to go to Surface.

A clean-up to consolidate the multiple "copy from RenderWidget surface"
methods across RenderWidgetHost and RenderWidgetHostView into the two
main variants in the latter class. Deletes the legacy
RWH::CopyFromBackingStore() API and migrates all callers to use
RWHV::CopyFromSurface(). Also, made changes to ensure the semantics of
the arguments is consistent among all callers and among all platform
impls.

This change also makes a small performance tweak in DelegatedFrameHost:
All copy requests will try to copy from the surface, to avoid extra
browser composites. This code was already written for the
FrameSubscriber use case, but there's no reason why it shouldn't also
apply to "one-off" copy requests.

BUG=73362
==========

to

==========
Consistent CopyFromSurface() API and always try to go to Surface.

A clean-up to consolidate the multiple "copy from RenderWidget surface"
methods across RenderWidgetHost and RenderWidgetHostView into the two
main variants in the latter class. Deletes the legacy
RWH::CopyFromBackingStore() API and migrates all callers to use
RWHV::CopyFromSurface(). Also, made changes to ensure the semantics of
the arguments is consistent among all callers and among all platform
impls.

This change also makes a small performance tweak in DelegatedFrameHost:
All copy requests will try to copy from the surface, to avoid extra
browser composites. This code was already written for the
FrameSubscriber use case, but there's no reason why it shouldn't also
apply to "one-off" copy requests.

BUG=73362
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[miu, 2017-02-19 01:19:00]

Description was changed from

==========
Consistent CopyFromSurface() API and always try to go to Surface.

A clean-up to consolidate the multiple "copy from RenderWidget surface"
methods across RenderWidgetHost and RenderWidgetHostView into the two
main variants in the latter class. Deletes the legacy
RWH::CopyFromBackingStore() API and migrates all callers to use
RWHV::CopyFromSurface(). Also, made changes to ensure the semantics of
the arguments is consistent among all callers and among all platform
impls.

This change also makes a small performance tweak in DelegatedFrameHost:
All copy requests will try to copy from the surface, to avoid extra
browser composites. This code was already written for the
FrameSubscriber use case, but there's no reason why it shouldn't also
apply to "one-off" copy requests.

BUG=73362
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Consistent CopyFromSurface() API and always try to go to Surface.

A clean-up to consolidate the multiple "copy from RenderWidget surface"
methods across RenderWidgetHost and RenderWidgetHostView into the two
main variants in the latter class. Deletes the legacy
RWH::CopyFromBackingStore() API and migrates all callers to use
RWHV::CopyFromSurface(). Also, made changes to ensure the semantics of
the arguments is consistent among all callers and among all platform
impls.

This change also makes a small performance tweak in DelegatedFrameHost:
All copy requests will try to copy from the surface, to avoid extra
browser composites. This code was already written for the
FrameSubscriber use case, but there's no reason why it shouldn't also
apply to "one-off" copy requests.

BUG=73362
TEST=Manually tested what did not have obvious unit testing in place (e.g.,
devtools color picker).
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[miu, 2017-02-19 01:19:16]

The CQ bit was checked by miu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-19 01:19:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-19 01:31:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-19 01:31:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[miu, 2017-02-19 03:05:15]

The CQ bit was checked by miu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-19 03:05:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[miu, 2017-02-19 03:16:01]

Description was changed from

==========
Consistent CopyFromSurface() API and always try to go to Surface.

A clean-up to consolidate the multiple "copy from RenderWidget surface"
methods across RenderWidgetHost and RenderWidgetHostView into the two
main variants in the latter class. Deletes the legacy
RWH::CopyFromBackingStore() API and migrates all callers to use
RWHV::CopyFromSurface(). Also, made changes to ensure the semantics of
the arguments is consistent among all callers and among all platform
impls.

This change also makes a small performance tweak in DelegatedFrameHost:
All copy requests will try to copy from the surface, to avoid extra
browser composites. This code was already written for the
FrameSubscriber use case, but there's no reason why it shouldn't also
apply to "one-off" copy requests.

BUG=73362
TEST=Manually tested what did not have obvious unit testing in place (e.g.,
devtools color picker).
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Consistent CopyFromSurface() API, consolidated to RWHV

A clean-up to consolidate the multiple "copy from RenderWidget surface"
methods across RenderWidgetHost and RenderWidgetHostView into the two
main variants in the latter class. Deletes the legacy
RWH::CopyFromBackingStore() API and migrates all callers to use
RWHV::CopyFromSurface(). Also, made changes to ensure the semantics of
the arguments is consistent among all callers and among all platform
impls.

BUG=73362
TEST=Manually tested what did not have obvious unit testing in place (e.g.,
devtools color picker).
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2017-02-19 04:00:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-19 04:00:04]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[miu, 2017-02-28 02:14:54]

Description was changed from

==========
Consistent CopyFromSurface() API, consolidated to RWHV

A clean-up to consolidate the multiple "copy from RenderWidget surface"
methods across RenderWidgetHost and RenderWidgetHostView into the two
main variants in the latter class. Deletes the legacy
RWH::CopyFromBackingStore() API and migrates all callers to use
RWHV::CopyFromSurface(). Also, made changes to ensure the semantics of
the arguments is consistent among all callers and among all platform
impls.

BUG=73362
TEST=Manually tested what did not have obvious unit testing in place (e.g.,
devtools color picker).
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Consistent CopyFromSurface() API, consolidated to RWHV

A clean-up to consolidate the multiple "copy from RenderWidget surface"
methods across a few classes into the RenderWidgetHostView class.
Deletes the legacy RWH::CopyFromBackingStore() API and migrates all
callers to use RWHV::CopyFromSurface(). Also, made changes to ensure the
semantics of the arguments is consistent among all callers and among all
platform impls; and place less burdeon on callers to compute rects
correctly. Added a few TODO comments, where size data is being used
incorrectly, to be addressed in later changes relevant to crbug 73362.

BUG=73362
TEST=Manually tested what did not have obvious unit testing in place (e.g.,
devtools color picker).
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[miu, 2017-02-28 02:21:35]

miu@chromium.org changed reviewers:
+ alexmos@chromium.org, changwan@chromium.org, dgozman@chromium.org,
nick@chromium.org, rdevlin.cronin@chromium.org, sky@chromium.org

[miu, 2017-02-28 02:21:36]

Fairly trivial content public API cleanup here. PTAL. 

alexmos: content/browser/frame_host/*

changwan: chrome/browser/android/compositor/tab_content_manager.cc

dgozman: content/browser/devtools/*

nick:
 content/browser/media/capture/*
 content/browser/renderer_host/*
 content/public/browser/*
 content/test/*

rdevlin.cronin@chromium.org:
 chrome/browser/extensions/*
 extensions/*

sky: chrome/browser/* (except extensions)

[miu, 2017-02-28 02:21:46]

The CQ bit was checked by miu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 02:22:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 04:08:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-28 04:08:54]

Dry run: This issue passed the CQ dry run.

[sky, 2017-02-28 04:13:51]

c/b LGTM (I didn't look at extensions or the android file, which you have other
owners for).

[Devlin, 2017-02-28 14:03:02]

lgtm % question

https://codereview.chromium.org/2702093002/diff/60001/extensions/browser/api/...
File extensions/browser/api/web_contents_capture_client.cc (right):

https://codereview.chromium.org/2702093002/diff/60001/extensions/browser/api/...
extensions/browser/api/web_contents_capture_client.cc:57:
view->CopyFromSurface(gfx::Rect(),  // Copy entire surface area.
Should we be checking IsSurfaceAvailableForCopy()?

[dgozman, 2017-02-28 19:13:52]

https://codereview.chromium.org/2702093002/diff/60001/content/browser/devtool...
File content/browser/devtools/protocol/color_picker.cc (right):

https://codereview.chromium.org/2702093002/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/color_picker.cc:86: // TODO(miu): This is the
wrong size. It's the size of the view on-screen, and
Does cc::CompositorFrameMetadata work? We have it in OnSwapCompositorFrame (just
not passed here from PageHandler class).

https://codereview.chromium.org/2702093002/diff/60001/content/browser/devtool...
File content/browser/devtools/protocol/page_handler.cc (right):

https://codereview.chromium.org/2702093002/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/page_handler.cc:473: // TODO(miu): RWHV to
provide an API to provide actual rendering size.
I think that would help in devtools_frame_trace_recorder as well.

Ideally, the clients would not have to do all the calculations. For example, 30
lines here try to express "copy everything but scale to not larger than |size|".

[alexmos, 2017-02-28 19:44:09]

https://codereview.chromium.org/2702093002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_screenshot_manager.cc (right):

https://codereview.chromium.org/2702093002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_screenshot_manager.cc:118: if (auto*
view = host->GetWidget()->GetView()) {
Do you need this check?  It looks like
NavigationEntryScreenshotManager::TakeScreenshot already null-checks this before
calling TakeScreenshotImpl, so it doesn't seem like the else clause would ever
be reached.  If you want to keep the null case handling here, perhaps remove the
null check from TakeScreenshot?

https://codereview.chromium.org/2702093002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2702093002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:607: const
gfx::Rect& src_subrect,
nit: the header has this as src_rect, make them the same?

[miu, 2017-03-01 01:17:02]

https://codereview.chromium.org/2702093002/diff/60001/content/browser/devtool...
File content/browser/devtools/protocol/color_picker.cc (right):

https://codereview.chromium.org/2702093002/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/color_picker.cc:86: // TODO(miu): This is the
wrong size. It's the size of the view on-screen, and
On 2017/02/28 19:13:52, dgozman wrote:
> Does cc::CompositorFrameMetadata work? We have it in OnSwapCompositorFrame
(just
> not passed here from PageHandler class).

Possibly. Here, I'm not making any functional change. The TODO I added here is
just like the one in PageHandler: It'd be best for there to be a simple API for
getting the size, rather than burden callers with grabbing
CompositorFrameMetadata and doing a bunch of math themselves.

https://codereview.chromium.org/2702093002/diff/60001/content/browser/devtool...
File content/browser/devtools/protocol/page_handler.cc (right):

https://codereview.chromium.org/2702093002/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/page_handler.cc:473: // TODO(miu): RWHV to
provide an API to provide actual rendering size.
On 2017/02/28 19:13:52, dgozman wrote:
> I think that would help in devtools_frame_trace_recorder as well.
> 
> Ideally, the clients would not have to do all the calculations. For example,
30
> lines here try to express "copy everything but scale to not larger than
|size|".

Acknowledged.

https://codereview.chromium.org/2702093002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_screenshot_manager.cc (right):

https://codereview.chromium.org/2702093002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_screenshot_manager.cc:118: if (auto*
view = host->GetWidget()->GetView()) {
On 2017/02/28 19:44:09, alexmos wrote:
> Do you need this check?  It looks like
> NavigationEntryScreenshotManager::TakeScreenshot already null-checks this
before
> calling TakeScreenshotImpl, so it doesn't seem like the else clause would ever
> be reached.  If you want to keep the null case handling here, perhaps remove
the
> null check from TakeScreenshot?

Done. Plus, things were simpler when I just dumped the remaining code at the end
of TakeScreenshot() (i.e., no need for a separate TakeScreenshotImpl() method).

https://codereview.chromium.org/2702093002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2702093002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:607: const
gfx::Rect& src_subrect,
On 2017/02/28 19:44:09, alexmos wrote:
> nit: the header has this as src_rect, make them the same?

Done.

https://codereview.chromium.org/2702093002/diff/60001/extensions/browser/api/...
File extensions/browser/api/web_contents_capture_client.cc (right):

https://codereview.chromium.org/2702093002/diff/60001/extensions/browser/api/...
extensions/browser/api/web_contents_capture_client.cc:57:
view->CopyFromSurface(gfx::Rect(),  // Copy entire surface area.
On 2017/02/28 14:03:02, Devlin wrote:
> Should we be checking IsSurfaceAvailableForCopy()?

It's actually perfectly acceptable not to. One of the changes I made in the RWHV
impls was to guarantee the callback is always run. So, if the surface is not
available, the callback will be run with a non-success flag.

[alexmos, 2017-03-01 01:30:05]

https://codereview.chromium.org/2702093002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_screenshot_manager.cc (right):

https://codereview.chromium.org/2702093002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_screenshot_manager.cc:118: if (auto*
view = host->GetWidget()->GetView()) {
On 2017/03/01 01:17:02, miu wrote:
> On 2017/02/28 19:44:09, alexmos wrote:
> > Do you need this check?  It looks like
> > NavigationEntryScreenshotManager::TakeScreenshot already null-checks this
> before
> > calling TakeScreenshotImpl, so it doesn't seem like the else clause would
ever
> > be reached.  If you want to keep the null case handling here, perhaps remove
> the
> > null check from TakeScreenshot?
> 
> Done. Plus, things were simpler when I just dumped the remaining code at the
end
> of TakeScreenshot() (i.e., no need for a separate TakeScreenshotImpl()
method).

Sounds good, but looks like there's another test in
web_contents_view_aura_browsertest.cc that overrides TakeScreenshotImpl, so do
you need to fix that test as well?

[Changwan Ryu, 2017-03-01 07:01:10]

On 2017/03/01 01:30:05, alexmos wrote:
>
https://codereview.chromium.org/2702093002/diff/60001/content/browser/frame_h...
> File content/browser/frame_host/navigation_entry_screenshot_manager.cc
(right):
> 
>
https://codereview.chromium.org/2702093002/diff/60001/content/browser/frame_h...
> content/browser/frame_host/navigation_entry_screenshot_manager.cc:118: if
(auto*
> view = host->GetWidget()->GetView()) {
> On 2017/03/01 01:17:02, miu wrote:
> > On 2017/02/28 19:44:09, alexmos wrote:
> > > Do you need this check?  It looks like
> > > NavigationEntryScreenshotManager::TakeScreenshot already null-checks this
> > before
> > > calling TakeScreenshotImpl, so it doesn't seem like the else clause would
> ever
> > > be reached.  If you want to keep the null case handling here, perhaps
remove
> > the
> > > null check from TakeScreenshot?
> > 
> > Done. Plus, things were simpler when I just dumped the remaining code at the
> end
> > of TakeScreenshot() (i.e., no need for a separate TakeScreenshotImpl()
> method).
> 
> Sounds good, but looks like there's another test in
> web_contents_view_aura_browsertest.cc that overrides TakeScreenshotImpl, so do
> you need to fix that test as well?

tab_content_manager.cc lgtm

[ncarter (slow), 2017-03-01 18:34:26]

lgtm, wonderful

https://codereview.chromium.org/2702093002/diff/80001/content/browser/media/c...
File content/browser/media/capture/web_contents_video_capture_device_unittest.cc
(right):

https://codereview.chromium.org/2702093002/diff/80001/content/browser/media/c...
content/browser/media/capture/web_contents_video_capture_device_unittest.cc:466:
// CopyFromSurfaceToVideoFrame functionality into TestRenderViewHostView
TestRenderViewHostView -> TestRenderWidgetHostView.

https://codereview.chromium.org/2702093002/diff/80001/content/public/browser/...
File content/public/browser/render_widget_host.h (right):

https://codereview.chromium.org/2702093002/diff/80001/content/public/browser/...
content/public/browser/render_widget_host.h:159: 
Really glad to see this vestigial nomenclature go: BackingStores haven't existed
in a very very long time. Thanks!

Looks like the second two paragraphs of the class comment here should probably
just be deleted; if you're updating this CL anyway feel free to delete them:
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid...

https://codereview.chromium.org/2702093002/diff/80001/content/test/test_rende...
File content/test/test_render_view_host.cc (right):

https://codereview.chromium.org/2702093002/diff/80001/content/test/test_rende...
content/test/test_render_view_host.cc:151: 
with the removal of the overrides, TestRenderWidgetHostView just now uses the
versions from RenderWidgetHostViewBase, is that right? And those have a
NOTIMPLEMENTED().

Was it the case that these are not actually called in any unittest?

https://codereview.chromium.org/2702093002/diff/80001/extensions/browser/api/...
File extensions/browser/api/web_contents_capture_client.cc (right):

https://codereview.chromium.org/2702093002/diff/80001/extensions/browser/api/...
extensions/browser/api/web_contents_capture_client.cc:56: }
It is hard for me to verify that the transformation you do here is equivalent to
the old behavior, given that there are several implementations of
CopyFromSurface.

(the above is not an objection, just a caution. you should triple-check that
there's no behavior change here; if there is, you might want to split this part
out into its own patch).

[dgozman, 2017-03-01 19:05:18]

lgtm

[miu, 2017-03-01 21:57:18]

alexmos: PTAL.

https://codereview.chromium.org/2702093002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_screenshot_manager.cc (right):

https://codereview.chromium.org/2702093002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_screenshot_manager.cc:118: if (auto*
view = host->GetWidget()->GetView()) {
On 2017/03/01 01:30:05, alexmos wrote:
> Sounds good, but looks like there's another test in
> web_contents_view_aura_browsertest.cc that overrides TakeScreenshotImpl, so do
> you need to fix that test as well?

Ah, yes. Fixed. I had to add back a "notification" method for the test to
override.

[miu, 2017-03-01 21:57:24]

The CQ bit was checked by miu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 21:58:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alexmos, 2017-03-01 22:02:03]

Thanks, LGTM

[miu, 2017-03-01 22:18:24]

https://codereview.chromium.org/2702093002/diff/80001/content/browser/media/c...
File content/browser/media/capture/web_contents_video_capture_device_unittest.cc
(right):

https://codereview.chromium.org/2702093002/diff/80001/content/browser/media/c...
content/browser/media/capture/web_contents_video_capture_device_unittest.cc:466:
// CopyFromSurfaceToVideoFrame functionality into TestRenderViewHostView
On 2017/03/01 18:34:26, ncarter wrote:
> TestRenderViewHostView -> TestRenderWidgetHostView.

Done.

https://codereview.chromium.org/2702093002/diff/80001/content/public/browser/...
File content/public/browser/render_widget_host.h (right):

https://codereview.chromium.org/2702093002/diff/80001/content/public/browser/...
content/public/browser/render_widget_host.h:159: 
On 2017/03/01 18:34:26, ncarter wrote:
> Really glad to see this vestigial nomenclature go: BackingStores haven't
existed
> in a very very long time. Thanks!
> 
> Looks like the second two paragraphs of the class comment here should probably
> just be deleted; if you're updating this CL anyway feel free to delete them:
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid...

Done. PTAL at render_widget_helper.h to make sure I understood your comment
correctly. FWICT, it seems everything about "backing stores" and "DIBs" in the
class comments can be deleted.

https://codereview.chromium.org/2702093002/diff/80001/content/test/test_rende...
File content/test/test_render_view_host.cc (right):

https://codereview.chromium.org/2702093002/diff/80001/content/test/test_rende...
content/test/test_render_view_host.cc:151: 
On 2017/03/01 18:34:26, ncarter wrote:
> with the removal of the overrides, TestRenderWidgetHostView just now uses the
> versions from RenderWidgetHostViewBase, is that right? And those have a
> NOTIMPLEMENTED().
> 
> Was it the case that these are not actually called in any unittest?

Yes...Only the WCVideoCaptureDevice unit tests would call them, *except* those
tests override CopyFromSurfaceToVideoFrame() with their own impl (and do not use
CopyFromSurface()).

Otherwise, tests that use this API are running as browser tests, and are using
the normal platform impls instead of a TestRenderWidgetHostView.

https://codereview.chromium.org/2702093002/diff/80001/extensions/browser/api/...
File extensions/browser/api/web_contents_capture_client.cc (right):

https://codereview.chromium.org/2702093002/diff/80001/extensions/browser/api/...
extensions/browser/api/web_contents_capture_client.cc:56: }
On 2017/03/01 18:34:26, ncarter wrote:
> It is hard for me to verify that the transformation you do here is equivalent
to
> the old behavior, given that there are several implementations of
> CopyFromSurface.
> 
> (the above is not an objection, just a caution. you should triple-check that
> there's no behavior change here; if there is, you might want to split this
part
> out into its own patch).

I'm certain this is correct. I've worked on this code myself before, so I'm
familiar with the intention here: The intention is to capture the entire view,
with the resulting SkBitmap having "all available detail" (e.g., on Retina, the
bitmap resolution will be 2X the source size because of the device scale
factor).

At this point, there shouldn't be a behavior change because the view's on-screen
size generally matches its size in view-coordinates. If there *is* actually a
behavior change, the new code would be fixing a bug in the old code. :)

[miu, 2017-03-01 22:39:19]

The CQ bit was checked by miu@chromium.org

[miu, 2017-03-01 22:39:20]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
rdevlin.cronin@chromium.org, nick@chromium.org, changwan@chromium.org,
dgozman@chromium.org, alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/2702093002/#ps140001
(title: "REBASE")

[commit-bot: I haz the power, 2017-03-01 22:39:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 23:46:01]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1488407959321350,
"parent_rev": "1a15baef08261f53209cd171e4ea200e221fc162", "commit_rev":
"515321aa08c29e910a720af8b3752ab52826df22"}

[commit-bot: I haz the power, 2017-03-01 23:46:42]

Description was changed from

==========
Consistent CopyFromSurface() API, consolidated to RWHV

A clean-up to consolidate the multiple "copy from RenderWidget surface"
methods across a few classes into the RenderWidgetHostView class.
Deletes the legacy RWH::CopyFromBackingStore() API and migrates all
callers to use RWHV::CopyFromSurface(). Also, made changes to ensure the
semantics of the arguments is consistent among all callers and among all
platform impls; and place less burdeon on callers to compute rects
correctly. Added a few TODO comments, where size data is being used
incorrectly, to be addressed in later changes relevant to crbug 73362.

BUG=73362
TEST=Manually tested what did not have obvious unit testing in place (e.g.,
devtools color picker).
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Consistent CopyFromSurface() API, consolidated to RWHV

A clean-up to consolidate the multiple "copy from RenderWidget surface"
methods across a few classes into the RenderWidgetHostView class.
Deletes the legacy RWH::CopyFromBackingStore() API and migrates all
callers to use RWHV::CopyFromSurface(). Also, made changes to ensure the
semantics of the arguments is consistent among all callers and among all
platform impls; and place less burdeon on callers to compute rects
correctly. Added a few TODO comments, where size data is being used
incorrectly, to be addressed in later changes relevant to crbug 73362.

BUG=73362
TEST=Manually tested what did not have obvious unit testing in place (e.g.,
devtools color picker).
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2702093002
Cr-Commit-Position: refs/heads/master@{#454098}
Committed:
https://chromium.googlesource.com/chromium/src/+/515321aa08c29e910a720af8b375...
==========

[commit-bot: I haz the power, 2017-03-01 23:46:44]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/515321aa08c29e910a720af8b375...