Implement webview.captureVisibleRegion()

Implement webview.captureVisibleRegion()

This CL implements  webview.captureVisibleRegion(), an extension/apps
API to allow WebView users to capture screenshots of the contents
displayed in a WebView. The surfaces contents capture has been plumbed
via RenderWidgetHostViewChildFrame so this implementation should not
require changes when WebView switches to using OOPIF.

As part of the implementation, there are two notable refactors:

1) CaptureWebContentsFunction has been refactored into
WebContentsCaptureClient to remove the extensions::AsyncExtensionFunction
dependence so that this code can be used by both tabs.captureVisibleTab
and webview.captureVisibleRegion, and

2) common code from DelegatedFrameHost has ben moved to
content/browser/compositor/surface_utils.* in order to avoid duplication
as both DelegatedFrameHost and RenderWidgetHostViewChildFrame now
use the code.

Finally, this CL adds a surface-drawn callback to
RenderWidgetHostViewChildFrame, to allow deferring a
screen capture request until a frame has actually been
drawn. This callback can be used to simplify some
existing tests.

BUG=326755
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/f89035216b627283b79731c3e6a7957707ed9034
Cr-Commit-Position: refs/heads/master@{#372799}

[wjmaclean, 2016-01-25 15:59:56]

Description was changed from

==========
Implement webview.captureVisibleRegion()

This CL implements  webview.captureVisibleRegion(), an extension/apps
API to allow WebView users to capture screenshots of the contents
displayed in a WebView. The surfaces contents capture has been plumbed
via RenderWidgetHostViewChildFrame so this implementation should not
require changes when WebView switches to using OOPIF.

As part of the implementation, there are two notable refactors:

1) CaptureWebContentsFunction has been refactored into
WebContentsCaptureClient to remove the extensions::AsyncExtensionFunction
dependence so that this code can be used by both tabs.captureVisibleTab
and webview.captureVisibleRegion, and

2) common code from DelegatedFrameHost has ben moved to
content/browser/compositor/surface_utils.* in order to avoid duplication
as both DelegatedFrameHost and RenderWidgetHostViewChildFrame now
use the code.

BUG=326755
==========

to

==========
Implement webview.captureVisibleRegion()

This CL implements  webview.captureVisibleRegion(), an extension/apps
API to allow WebView users to capture screenshots of the contents
displayed in a WebView. The surfaces contents capture has been plumbed
via RenderWidgetHostViewChildFrame so this implementation should not
require changes when WebView switches to using OOPIF.

As part of the implementation, there are two notable refactors:

1) CaptureWebContentsFunction has been refactored into
WebContentsCaptureClient to remove the extensions::AsyncExtensionFunction
dependence so that this code can be used by both tabs.captureVisibleTab
and webview.captureVisibleRegion, and

2) common code from DelegatedFrameHost has ben moved to
content/browser/compositor/surface_utils.* in order to avoid duplication
as both DelegatedFrameHost and RenderWidgetHostViewChildFrame now
use the code.

BUG=326755
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[wjmaclean, 2016-01-25 16:01:37]

Description was changed from

==========
Implement webview.captureVisibleRegion()

This CL implements  webview.captureVisibleRegion(), an extension/apps
API to allow WebView users to capture screenshots of the contents
displayed in a WebView. The surfaces contents capture has been plumbed
via RenderWidgetHostViewChildFrame so this implementation should not
require changes when WebView switches to using OOPIF.

As part of the implementation, there are two notable refactors:

1) CaptureWebContentsFunction has been refactored into
WebContentsCaptureClient to remove the extensions::AsyncExtensionFunction
dependence so that this code can be used by both tabs.captureVisibleTab
and webview.captureVisibleRegion, and

2) common code from DelegatedFrameHost has ben moved to
content/browser/compositor/surface_utils.* in order to avoid duplication
as both DelegatedFrameHost and RenderWidgetHostViewChildFrame now
use the code.

BUG=326755
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Implement webview.captureVisibleRegion()

This CL implements  webview.captureVisibleRegion(), an extension/apps
API to allow WebView users to capture screenshots of the contents
displayed in a WebView. The surfaces contents capture has been plumbed
via RenderWidgetHostViewChildFrame so this implementation should not
require changes when WebView switches to using OOPIF.

As part of the implementation, there are two notable refactors:

1) CaptureWebContentsFunction has been refactored into
WebContentsCaptureClient to remove the extensions::AsyncExtensionFunction
dependence so that this code can be used by both tabs.captureVisibleTab
and webview.captureVisibleRegion, and

2) common code from DelegatedFrameHost has ben moved to
content/browser/compositor/surface_utils.* in order to avoid duplication
as both DelegatedFrameHost and RenderWidgetHostViewChildFrame now
use the code.

BUG=326755
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[wjmaclean, 2016-01-25 16:01:37]

wjmaclean@chromium.org changed reviewers:
+ creis@chromium.org, fsamuel@chromium.org, kenrb@chromium.org,
nick@chromium.org, rockot@chromium.org

[wjmaclean, 2016-01-25 16:05:51]

The first attempt at landing this got reverted due to the new test failing
sometime. It seems that sometime the test was requesting pixel readback before
the first frame had rendered, so I've modified the test to avoid that.

I opened a new issue to make it easier to track l-g-t-m's, but I've also
uploaded the revised patch on the old issue to make it easy to see what changed
(not a lot). Look for Patch Set 10 at
https://codereview.chromium.org/1582053002.

** Sadly there were some formatting changes from git CL format, sorry for the
noise.

Please take a look.

[Fady Samuel, 2016-01-25 16:13:23]

Could you please put this in the previous issue so we can focus on the diff?

>> It seems that sometime the test was requesting pixel readback before the
first frame had rendered, so I've modified the test to avoid that.

Couldn't this be a problem in practice as well? Based on your description of the
problem, it seems like it's catching a real problem?

[Fady Samuel, 2016-01-25 16:13:54]

On 2016/01/25 16:13:23, Fady Samuel wrote:
> Could you please put this in the previous issue so we can focus on the diff?
> 
> >> It seems that sometime the test was requesting pixel readback before the
> first frame had rendered, so I've modified the test to avoid that.
> 
> Couldn't this be a problem in practice as well? Based on your description of
the
> problem, it seems like it's catching a real problem?

Oops sorry, I didn't read your full description. Looks like you already updated
the previous issue. Thanks!

[wjmaclean, 2016-01-25 17:00:12]

On 2016/01/25 16:13:23, Fady Samuel wrote:
> Could you please put this in the previous issue so we can focus on the diff?
> 
> >> It seems that sometime the test was requesting pixel readback before the
> first frame had rendered, so I've modified the test to avoid that.
> 
> Couldn't this be a problem in practice as well? Based on your description of
the
> problem, it seems like it's catching a real problem?

I guess I had figured that a user of the API would know that one possible cause
of a failed readback would be a lack of pixels, and possibly retry.

I can try and trigger a composite on attempted readback ... not sure if there
are any edge cases but I'll think about that.

[wjmaclean, 2016-01-27 17:59:14]

wjmaclean@chromium.org changed reviewers:
+ piman@chromium.org

[wjmaclean, 2016-01-27 18:04:21]

+piman@ for the SurfaceDrawn placement of the callback notification.

I've updated the CL so that if a screen capture is requested when no surface has
yet been drawn, it defers the readback request until after the next SurfaceDrawn
ack. This ack is pre-existing, so this CL has not had to add any extra IPCs.

I've restored the original version of the test, in which no explicit check is
made for whether the surface has been drawn yet.

Currently this is only in RenderWidgetHostViewChildFrame, though the mechanism
could be added to other view classes.

Please take a look.

[wjmaclean, 2016-01-27 18:04:28]

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

[commit-bot: I haz the power, 2016-01-27 18:05:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1635513003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1635513003/20001

[wjmaclean, 2016-01-27 18:06:00]

Description was changed from

==========
Implement webview.captureVisibleRegion()

This CL implements  webview.captureVisibleRegion(), an extension/apps
API to allow WebView users to capture screenshots of the contents
displayed in a WebView. The surfaces contents capture has been plumbed
via RenderWidgetHostViewChildFrame so this implementation should not
require changes when WebView switches to using OOPIF.

As part of the implementation, there are two notable refactors:

1) CaptureWebContentsFunction has been refactored into
WebContentsCaptureClient to remove the extensions::AsyncExtensionFunction
dependence so that this code can be used by both tabs.captureVisibleTab
and webview.captureVisibleRegion, and

2) common code from DelegatedFrameHost has ben moved to
content/browser/compositor/surface_utils.* in order to avoid duplication
as both DelegatedFrameHost and RenderWidgetHostViewChildFrame now
use the code.

BUG=326755
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Implement webview.captureVisibleRegion()

This CL implements  webview.captureVisibleRegion(), an extension/apps
API to allow WebView users to capture screenshots of the contents
displayed in a WebView. The surfaces contents capture has been plumbed
via RenderWidgetHostViewChildFrame so this implementation should not
require changes when WebView switches to using OOPIF.

As part of the implementation, there are two notable refactors:

1) CaptureWebContentsFunction has been refactored into
WebContentsCaptureClient to remove the extensions::AsyncExtensionFunction
dependence so that this code can be used by both tabs.captureVisibleTab
and webview.captureVisibleRegion, and

2) common code from DelegatedFrameHost has ben moved to
content/browser/compositor/surface_utils.* in order to avoid duplication
as both DelegatedFrameHost and RenderWidgetHostViewChildFrame now
use the code.

Finally, this CL adds a surface-drawn callback to
RenderWidgetHostViewChildFrame, to allow deferring a
screen capture request until a frame has actually been
drawn. This callback can be used to simplify some
existing tests.

BUG=326755
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-01-27 20:06:44]

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

[commit-bot: I haz the power, 2016-01-27 20:06:45]

Dry run: This issue passed the CQ dry run.

[wjmaclean, 2016-01-27 20:18:39]

On 2016/01/27 20:06:45, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

Here's a list of some possible cleanups from having the surface-drawn callback:

1) Calls to RendererFrameNumber() in tests like:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/si...
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/si...

2) It's possible RendererFrameNumber() itself can be removed (I need to look at
all the call sites to verify).

[ncarter (slow), 2016-01-27 21:08:25]

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

https://codereview.chromium.org/1635513003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:446: if
(!IsSurfaceAvailableForCopy()) {
This behavior goes somewhat against the intended contract of
CopyFromCompositingSurface (which is a conversation between RenderWidgetHostImpl
and its View).

RenderWidgetHost's callers are supposed to be able to tell when a copy would not
succeed due to a lack of a surface, by querying
RenderWidgetHost::CanCopyFromBackingStore prior to calling
RenderWidgetHost::CopyFromBackingStore.

I wonder if there's a satisfactory way to implement "wait until frame ready"
using existing primitives of the content public api? Maybe by installing a
FrameSubscriber to be notified when frames are available? It's not clear how
well this will work for the RWHVChildFrame case.

https://codereview.chromium.org/1635513003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/1635513003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.h:66: // types,
such as RenderWidgetHostViewAura.
This seems fairly similar in purpose to RenderWidgetHostViewFrameSubscriber,
though the abilities of that interface is currently narrowly scoped to what
WebContentsVideoCaptureDevice needs (only one subscriber is supported, it
assumes a stream of frames, and it assumes you want things like YV12 that are
appropriate for dumping into a video encode).

https://codereview.chromium.org/1635513003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.h:67: void
RegisterFrameDrawnCallback(scoped_ptr<base::Closure> callback);
In the webview case, is there really necessarily a "frame drawn" event in the
RWHVChildFrame for every visible change to the pixel content (I'm thinking about
when there are oopifs or, I dunno, flash or something, that could cause an
additional delegated layer under the current one)?

If these were OOPIFs, then in the A(B(C)) case, if we're capturing B, will
reading from B's RWHVChildFrame actually include the pixel content from C?

[ncarter (slow), 2016-01-27 21:10:47]

Also, +miu for tab capture expertise.

[ncarter (slow), 2016-01-27 21:11:03]

nick@chromium.org changed reviewers:
+ miu@chromium.org

[wjmaclean, 2016-01-27 21:28:01]

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

https://codereview.chromium.org/1635513003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:446: if
(!IsSurfaceAvailableForCopy()) {
On 2016/01/27 21:08:24, ncarter wrote:
> This behavior goes somewhat against the intended contract of
> CopyFromCompositingSurface (which is a conversation between
RenderWidgetHostImpl
> and its View).
> 
> RenderWidgetHost's callers are supposed to be able to tell when a copy would
not
> succeed due to a lack of a surface, by querying
> RenderWidgetHost::CanCopyFromBackingStore prior to calling
> RenderWidgetHost::CopyFromBackingStore.
> 
> I wonder if there's a satisfactory way to implement "wait until frame ready"
> using existing primitives of the content public api? Maybe by installing a
> FrameSubscriber to be notified when frames are available? It's not clear how
> well this will work for the RWHVChildFrame case.

I guess having a surface ID may not be enough, but I'm not sure.

Presumably one could simply, each time a new surface is created, keep track of
the next draw ack to know it actually has pixels, though also those pixels could
presumably disappear if we're not careful (I was willing to just let the
existing system indicate that readback failed in that case, or we could force a
re-composite).

https://codereview.chromium.org/1635513003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/1635513003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.h:66: // types,
such as RenderWidgetHostViewAura.
On 2016/01/27 21:08:25, ncarter wrote:
> This seems fairly similar in purpose to RenderWidgetHostViewFrameSubscriber,
> though the abilities of that interface is currently narrowly scoped to what
> WebContentsVideoCaptureDevice needs (only one subscriber is supported, it
> assumes a stream of frames, and it assumes you want things like YV12 that are
> appropriate for dumping into a video encode).

Are you suggesting we re-write RenderWidgetHostViewFrameSubscriber then to be
more general?

https://codereview.chromium.org/1635513003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.h:67: void
RegisterFrameDrawnCallback(scoped_ptr<base::Closure> callback);
On 2016/01/27 21:08:25, ncarter wrote:
> In the webview case, is there really necessarily a "frame drawn" event in the
> RWHVChildFrame for every visible change to the pixel content (I'm thinking
about
> when there are oopifs or, I dunno, flash or something, that could cause an
> additional delegated layer under the current one)?
> 
> If these were OOPIFs, then in the A(B(C)) case, if we're capturing B, will
> reading from B's RWHVChildFrame actually include the pixel content from C?

I'm not entirely sure, though I suspect the surface for B would include it. It's
easy enough to test.

[ncarter (slow), 2016-01-28 20:49:59]

https://codereview.chromium.org/1635513003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/1635513003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.h:66: // types,
such as RenderWidgetHostViewAura.
On 2016/01/27 21:28:01, wjmaclean wrote:
> On 2016/01/27 21:08:25, ncarter wrote:
> > This seems fairly similar in purpose to RenderWidgetHostViewFrameSubscriber,
> > though the abilities of that interface is currently narrowly scoped to what
> > WebContentsVideoCaptureDevice needs (only one subscriber is supported, it
> > assumes a stream of frames, and it assumes you want things like YV12 that
are
> > appropriate for dumping into a video encode).
> 
> Are you suggesting we re-write RenderWidgetHostViewFrameSubscriber then to be
> more general?

My main intent was to call attention to the RenderWidgetHostViewFrameSubscriber
methods in RWHVBase, which seems like it has some level of functional overlap
with this -- which I thought was potentially worth mentioning in the TODO.

Having done a little more thinking, I am now wondering whether an even better
long-term approach might be for RenderWidgetHostViewFrameSubscriber to be
eliminated, and have WebContentsVideoCaptureDevice's FrameSubscriber be
implemented in terms of something simpler like RegisterFrameDrawnCallback.
RenderWidgetHostViewFrameSubscriber originally had some really gnarly threading
behaviors, and had to be driven substantially differently between platforms. I
don't think any of these are concerns anymore: DelegatedFrameHost drives the
FrameSubscriber on all platforms, and WCVCD's FrameSubscriber is in content (and
thus can do content-impl things), so if we provided some kind of hook at
compositor-swap time (i.e. RegisterFrameDrawnCallback), maybe the rest of the
FrameSubscriber-specific behavior could be moved out of DelegatedFrameHost.

Anyhow: the upshot of this is that I'm on board with RWHVBase having a
RegisterFrameDrawnCallback.

https://codereview.chromium.org/1635513003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.h:67: void
RegisterFrameDrawnCallback(scoped_ptr<base::Closure> callback);
On 2016/01/27 21:28:01, wjmaclean wrote:
> On 2016/01/27 21:08:25, ncarter wrote:
> > In the webview case, is there really necessarily a "frame drawn" event in
the
> > RWHVChildFrame for every visible change to the pixel content (I'm thinking
> about
> > when there are oopifs or, I dunno, flash or something, that could cause an
> > additional delegated layer under the current one)?
> > 
> > If these were OOPIFs, then in the A(B(C)) case, if we're capturing B, will
> > reading from B's RWHVChildFrame actually include the pixel content from C?
> 
> I'm not entirely sure, though I suspect the surface for B would include it.
It's
> easy enough to test.

OK.

[ncarter (slow), 2016-01-28 21:22:38]

I'm giving a high-level lgtm on the changes here

[piman, 2016-01-29 04:29:46]

https://codereview.chromium.org/1635513003/diff/40001/content/browser/composi...
File content/browser/compositor/surface_utils.cc (right):

https://codereview.chromium.org/1635513003/diff/40001/content/browser/composi...
content/browser/compositor/surface_utils.cc:62: // implement it here.
See the code in
content/browser/renderer_host/render_widget_host_view_android.cc, which was
heavily inspired (cargo-culted?) from the aura code (used to be in RWHVAura, but
moved to DelegatedFrameHost). The implementation of
PrepareTextureCopyOutputResult and CopyFromCompositingSurfaceFinished are
virtually identical, except for how you get to the GLHelper. It would be good to
merge.

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

https://codereview.chromium.org/1635513003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:449:
RegisterFrameDrawnCallback(make_scoped_ptr(new base::Closure(base::Bind(
Why waiting for the frame to be *drawn* (SurfaceDrawn), as opposed to *received*
(OnSwapCompositorFrame)?

Once you received the frame from the renderer, it should be fine to add the copy
request, which will be processed when compositing the next browser frame.

If you wait untilthe next browser frame is drawn, then:
- you incur an extra frame of latency
- you'll kick off yet another browser frame to process the copy request, when
possibly none would be have needed otherwise.

[wjmaclean, 2016-01-29 21:01:57]

piman@ - PTAL? I left a TODO with your ldap on it, which is a question I hope
you can answer.

https://codereview.chromium.org/1635513003/diff/40001/content/browser/composi...
File content/browser/compositor/surface_utils.cc (right):

https://codereview.chromium.org/1635513003/diff/40001/content/browser/composi...
content/browser/compositor/surface_utils.cc:62: // implement it here.
On 2016/01/29 04:29:46, piman (OOO back 01-28) wrote:
> See the code in
> content/browser/renderer_host/render_widget_host_view_android.cc, which was
> heavily inspired (cargo-culted?) from the aura code (used to be in RWHVAura,
but
> moved to DelegatedFrameHost). The implementation of
> PrepareTextureCopyOutputResult and CopyFromCompositingSurfaceFinished are
> virtually identical, except for how you get to the GLHelper. It would be good
to
> merge.

I agree, but in comparing the relevant code, it seems like there's anough
difference that we might be best to push the merging into it's own CL. Is it ok
to do that, and include a TODO for it here (and in RenderWidgetHostViewAndroid)?

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

https://codereview.chromium.org/1635513003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:449:
RegisterFrameDrawnCallback(make_scoped_ptr(new base::Closure(base::Bind(
On 2016/01/29 04:29:46, piman (OOO back 01-28) wrote:
> Why waiting for the frame to be *drawn* (SurfaceDrawn), as opposed to
*received*
> (OnSwapCompositorFrame)?
> 
> Once you received the frame from the renderer, it should be fine to add the
copy
> request, which will be processed when compositing the next browser frame.
> 
> If you wait untilthe next browser frame is drawn, then:
> - you incur an extra frame of latency
> - you'll kick off yet another browser frame to process the copy request, when
> possibly none would be have needed otherwise.

I didn't realize that would happen, thanks for pointing it out! I'll revise and
upload a new CL.

[wjmaclean, 2016-01-29 21:02:27]

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

[commit-bot: I haz the power, 2016-01-29 21:06:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1635513003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1635513003/60001

[piman, 2016-01-29 22:28:54]

https://codereview.chromium.org/1635513003/diff/40001/content/browser/composi...
File content/browser/compositor/surface_utils.cc (right):

https://codereview.chromium.org/1635513003/diff/40001/content/browser/composi...
content/browser/compositor/surface_utils.cc:62: // implement it here.
On 2016/01/29 21:01:57, wjmaclean wrote:
> On 2016/01/29 04:29:46, piman (OOO back 01-28) wrote:
> > See the code in
> > content/browser/renderer_host/render_widget_host_view_android.cc, which was
> > heavily inspired (cargo-culted?) from the aura code (used to be in RWHVAura,
> but
> > moved to DelegatedFrameHost). The implementation of
> > PrepareTextureCopyOutputResult and CopyFromCompositingSurfaceFinished are
> > virtually identical, except for how you get to the GLHelper. It would be
good
> to
> > merge.
> 
> I agree, but in comparing the relevant code, it seems like there's anough
> difference that we might be best to push the merging into it's own CL. Is it
ok
> to do that, and include a TODO for it here (and in
RenderWidgetHostViewAndroid)?

Ok, can you file a bug too, and reference from the TODOs?

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

https://codereview.chromium.org/1635513003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:457:
host_->ScheduleComposite();
Is there a reason why the renderer would not generate a frame on its own? I
assume if you don't have a frame yet, it's because the renderer hasn't done the
first layout/commit/composite yet, but as soon as it will, you'll get a frame
back.
Keep in mind that ScheduleComposite can only track whether or not the browser
forced a frame, but can't know whether the renderer is in the process of doing
one on its own.


ScheduleComposite is virtually only used in tests, so I'm a bit reluctant to add
a production use case.

[commit-bot: I haz the power, 2016-01-30 00:35:53]

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

[commit-bot: I haz the power, 2016-01-30 00:35:54]

Dry run: This issue passed the CQ dry run.

[wjmaclean, 2016-02-01 15:36:04]

Comments addressed, PTAL?

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

https://codereview.chromium.org/1635513003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:457:
host_->ScheduleComposite();
On 2016/01/29 22:28:54, piman wrote:
> Is there a reason why the renderer would not generate a frame on its own? I
> assume if you don't have a frame yet, it's because the renderer hasn't done
the
> first layout/commit/composite yet, but as soon as it will, you'll get a frame
> back.
> Keep in mind that ScheduleComposite can only track whether or not the browser
> forced a frame, but can't know whether the renderer is in the process of doing
> one on its own.
> 
> 
> ScheduleComposite is virtually only used in tests, so I'm a bit reluctant to
add
> a production use case.

Ok, I've removed this. There was some discussion as to whether a frame would be
produced if the guest content was not currently visible in the tab, but if that
turns out to be an issue we can deal with it later.

[wjmaclean, 2016-02-01 15:36:12]

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

[commit-bot: I haz the power, 2016-02-01 15:37:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1635513003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1635513003/80001

[piman, 2016-02-01 16:42:49]

lgtm

[wjmaclean, 2016-02-01 16:47:26]

rockot@ - ptal for extensions LGTM? (I don't think anything in extensions has
really changed at all since the first time we landed this)

creis@ - did you want to take another look?

[ncarter (slow), 2016-02-01 19:50:20]

lgtm

[Charlie Reis, 2016-02-01 19:52:22]

On 2016/02/01 16:47:26, wjmaclean wrote:
> rockot@ - ptal for extensions LGTM? (I don't think anything in extensions has
> really changed at all since the first time we landed this)
> 
> creis@ - did you want to take another look?

Thanks.  I'll defer to Nick, so rubber stamp LGTM.

[Ken Rockot(use gerrit already), 2016-02-01 21:27:09]

rs lgtm

[wjmaclean, 2016-02-01 21:27:39]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2016-02-01 21:29:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1635513003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1635513003/100001

[commit-bot: I haz the power, 2016-02-01 22:54:56]

Description was changed from

==========
Implement webview.captureVisibleRegion()

This CL implements  webview.captureVisibleRegion(), an extension/apps
API to allow WebView users to capture screenshots of the contents
displayed in a WebView. The surfaces contents capture has been plumbed
via RenderWidgetHostViewChildFrame so this implementation should not
require changes when WebView switches to using OOPIF.

As part of the implementation, there are two notable refactors:

1) CaptureWebContentsFunction has been refactored into
WebContentsCaptureClient to remove the extensions::AsyncExtensionFunction
dependence so that this code can be used by both tabs.captureVisibleTab
and webview.captureVisibleRegion, and

2) common code from DelegatedFrameHost has ben moved to
content/browser/compositor/surface_utils.* in order to avoid duplication
as both DelegatedFrameHost and RenderWidgetHostViewChildFrame now
use the code.

Finally, this CL adds a surface-drawn callback to
RenderWidgetHostViewChildFrame, to allow deferring a
screen capture request until a frame has actually been
drawn. This callback can be used to simplify some
existing tests.

BUG=326755
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Implement webview.captureVisibleRegion()

This CL implements  webview.captureVisibleRegion(), an extension/apps
API to allow WebView users to capture screenshots of the contents
displayed in a WebView. The surfaces contents capture has been plumbed
via RenderWidgetHostViewChildFrame so this implementation should not
require changes when WebView switches to using OOPIF.

As part of the implementation, there are two notable refactors:

1) CaptureWebContentsFunction has been refactored into
WebContentsCaptureClient to remove the extensions::AsyncExtensionFunction
dependence so that this code can be used by both tabs.captureVisibleTab
and webview.captureVisibleRegion, and

2) common code from DelegatedFrameHost has ben moved to
content/browser/compositor/surface_utils.* in order to avoid duplication
as both DelegatedFrameHost and RenderWidgetHostViewChildFrame now
use the code.

Finally, this CL adds a surface-drawn callback to
RenderWidgetHostViewChildFrame, to allow deferring a
screen capture request until a frame has actually been
drawn. This callback can be used to simplify some
existing tests.

BUG=326755
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-02-01 22:54:57]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-01 22:56:25]

Description was changed from

==========
Implement webview.captureVisibleRegion()

This CL implements  webview.captureVisibleRegion(), an extension/apps
API to allow WebView users to capture screenshots of the contents
displayed in a WebView. The surfaces contents capture has been plumbed
via RenderWidgetHostViewChildFrame so this implementation should not
require changes when WebView switches to using OOPIF.

As part of the implementation, there are two notable refactors:

1) CaptureWebContentsFunction has been refactored into
WebContentsCaptureClient to remove the extensions::AsyncExtensionFunction
dependence so that this code can be used by both tabs.captureVisibleTab
and webview.captureVisibleRegion, and

2) common code from DelegatedFrameHost has ben moved to
content/browser/compositor/surface_utils.* in order to avoid duplication
as both DelegatedFrameHost and RenderWidgetHostViewChildFrame now
use the code.

Finally, this CL adds a surface-drawn callback to
RenderWidgetHostViewChildFrame, to allow deferring a
screen capture request until a frame has actually been
drawn. This callback can be used to simplify some
existing tests.

BUG=326755
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Implement webview.captureVisibleRegion()

This CL implements  webview.captureVisibleRegion(), an extension/apps
API to allow WebView users to capture screenshots of the contents
displayed in a WebView. The surfaces contents capture has been plumbed
via RenderWidgetHostViewChildFrame so this implementation should not
require changes when WebView switches to using OOPIF.

As part of the implementation, there are two notable refactors:

1) CaptureWebContentsFunction has been refactored into
WebContentsCaptureClient to remove the extensions::AsyncExtensionFunction
dependence so that this code can be used by both tabs.captureVisibleTab
and webview.captureVisibleRegion, and

2) common code from DelegatedFrameHost has ben moved to
content/browser/compositor/surface_utils.* in order to avoid duplication
as both DelegatedFrameHost and RenderWidgetHostViewChildFrame now
use the code.

Finally, this CL adds a surface-drawn callback to
RenderWidgetHostViewChildFrame, to allow deferring a
screen capture request until a frame has actually been
drawn. This callback can be used to simplify some
existing tests.

BUG=326755
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/f89035216b627283b79731c3e6a7957707ed9034
Cr-Commit-Position: refs/heads/master@{#372799}
==========

[commit-bot: I haz the power, 2016-02-01 22:56:26]

Patchset 6 (id:??) landed as
https://crrev.com/f89035216b627283b79731c3e6a7957707ed9034
Cr-Commit-Position: refs/heads/master@{#372799}

[miu, 2016-02-04 20:19:11]

Sorry...some unpublished drafts I thought I had sent:

https://codereview.chromium.org/1635513003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/1635513003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.h:66: // types,
such as RenderWidgetHostViewAura.
On 2016/01/28 20:49:58, ncarter wrote:
> On 2016/01/27 21:28:01, wjmaclean wrote:
> > On 2016/01/27 21:08:25, ncarter wrote:
> > > This seems fairly similar in purpose to
RenderWidgetHostViewFrameSubscriber,
>
> Having done a little more thinking, I am now wondering whether an even better
> long-term approach might be for RenderWidgetHostViewFrameSubscriber to be
> eliminated...

Agreed.  I have it on my roadmap this year to do some significant refactoring in
this area.  In particular, FrameSubscriber was solving problems in the
"pre-Uber-Compositor world" and has suffered some rather weird code factoring
issues over the past 2+ years.  In one case (that I was not aware of at the
time), someone threw up their arms and just copy-and-pasted a bunch of code from
DelegatedFrameHost in order to implement screen capture on CrOS.  That's not
good.

So, for the future, I think it makes sense for the Compositor to provide some
kind of "frame observer" interface instead of the RenderWidgetHostView*
providing a "frame subscriber" interface.  I have a number of ideas that I plan
to prototype-out before proposing a new design.

> Anyhow: the upshot of this is that I'm on board with RWHVBase having a
> RegisterFrameDrawnCallback.

I'm not sure it will stay that way for long, but I don't see this as a bad idea
now, nor would it seem to make my future refactoring efforts any more difficult.

https://codereview.chromium.org/1635513003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.h:67: void
RegisterFrameDrawnCallback(scoped_ptr<base::Closure> callback);
On 2016/01/28 20:49:58, ncarter wrote:
> On 2016/01/27 21:28:01, wjmaclean wrote:
> > On 2016/01/27 21:08:25, ncarter wrote:
> > > In the webview case, is there really necessarily a "frame drawn" event in
> the
> > > RWHVChildFrame for every visible change to the pixel content (I'm thinking
> > about
> > > when there are oopifs or, I dunno, flash or something, that could cause an
> > > additional delegated layer under the current one)?
> > > 
> > > If these were OOPIFs, then in the A(B(C)) case, if we're capturing B, will
> > > reading from B's RWHVChildFrame actually include the pixel content from C?
> > 
> > I'm not entirely sure, though I suspect the surface for B would include it.
> It's
> > easy enough to test.
> 
> OK.

My understanding here is that the Compositor will execute a "copy request on
layer B" on the result from drawing "B(C)".  In other words, I'd expect to see
the pixels for B and C.