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.

BUG=326755
Committed: https://crrev.com/5b8cdcc82fc9da7bab15fd3dbf65df843dc73589
Cr-Commit-Position: refs/heads/master@{#370565}

[wjmaclean, 2016-01-13 17: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
CaptureWebContents 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/readback_request_helpers.* in order to avoid duplication
as both DelegatedFrameHost and RenderWidgetHostViewChildFrame now
use the code. Note: the duplicate code in DelegatedFrameHost will be
removed in a follow CL.

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
CaptureWebContents 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/readback_request_helpers.* in order to avoid duplication
as both DelegatedFrameHost and RenderWidgetHostViewChildFrame now
use the code. Note: the duplicate code in DelegatedFrameHost will be
removed in a follow CL.

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

[wjmaclean, 2016-01-13 17:54:57]

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

[wjmaclean, 2016-01-13 17:55:31]

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

[wjmaclean, 2016-01-13 17:56:33]

wjmaclean@chromium.org changed reviewers:
+ asvitkine@google.com

[wjmaclean, 2016-01-13 18:12:53]

wjmaclean@chromium.org changed reviewers:
- asvitkine@google.com

[wjmaclean, 2016-01-13 18:13:07]

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

[commit-bot: I haz the power, 2016-01-13 18:13:47]

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

[wjmaclean, 2016-01-13 18:14:46]

kenrb@ - could you take a look at the content/ changes?

rockot@ - please look at changes in *extensions* ?

https://codereview.chromium.org/1582053002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1582053002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:63229: +  <int value="1103"
label="WEBVIEWINTERNAL_CAPTUREVISIBLEREGION"/>
Ooops, this is an error ... I'll fix this!

[wjmaclean, 2016-01-13 18:18:35]

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
CaptureWebContents 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/readback_request_helpers.* in order to avoid duplication
as both DelegatedFrameHost and RenderWidgetHostViewChildFrame now
use the code. Note: the duplicate code in DelegatedFrameHost will be
removed in a follow CL.

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
CaptureWebContents 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/readback_request_helpers.* in order to avoid duplication
as both DelegatedFrameHost and RenderWidgetHostViewChildFrame now
use the code. Note: the duplicate code in DelegatedFrameHost will be
removed in a follow-on CL.

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

[commit-bot: I haz the power, 2016-01-13 18:31:41]

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

[commit-bot: I haz the power, 2016-01-13 18:31:41]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Ken Rockot(use gerrit already), 2016-01-13 22:41:35]

looks good overall

https://codereview.chromium.org/1582053002/diff/20001/extensions/browser/api/...
File extensions/browser/api/capture_web_contents.h (right):

https://codereview.chromium.org/1582053002/diff/20001/extensions/browser/api/...
extensions/browser/api/capture_web_contents.h:23: class CaptureWebContents {
It's unusual to have a verb phrase for a typename. Maybe this should be
something like a WebContentsCaptureDelegate - or maybe WebContentsCaptureClient
since it's not strictly a delegate interface and has some base functionality.

https://codereview.chromium.org/1582053002/diff/20001/extensions/browser/api/...
extensions/browser/api/capture_web_contents.h:38:
api::extension_types::ImageDetails* image_detail,
If you want to pass ownership of |image_detail| this should be a scoped_ptr. It
doesn't look like you need to though, so in that case maybe this should be a
const ref?

https://codereview.chromium.org/1582053002/diff/20001/extensions/browser/api/...
File
extensions/browser/api/guest_view/extension_view/extension_view_internal_api.h
(right):

https://codereview.chromium.org/1582053002/diff/20001/extensions/browser/api/...
extensions/browser/api/guest_view/extension_view/extension_view_internal_api.h:9:
//#include "extensions/browser/api/capture_web_contents.h"
remove or uncomment?

[wjmaclean, 2016-01-18 16:20:12]

rockot@ - PTAL?

https://codereview.chromium.org/1582053002/diff/20001/extensions/browser/api/...
File extensions/browser/api/capture_web_contents.h (right):

https://codereview.chromium.org/1582053002/diff/20001/extensions/browser/api/...
extensions/browser/api/capture_web_contents.h:23: class CaptureWebContents {
On 2016/01/13 22:41:35, Ken Rockot wrote:
> It's unusual to have a verb phrase for a typename. Maybe this should be
> something like a WebContentsCaptureDelegate - or maybe
WebContentsCaptureClient
> since it's not strictly a delegate interface and has some base functionality.

Done.

https://codereview.chromium.org/1582053002/diff/20001/extensions/browser/api/...
extensions/browser/api/capture_web_contents.h:38:
api::extension_types::ImageDetails* image_detail,
On 2016/01/13 22:41:35, Ken Rockot wrote:
> If you want to pass ownership of |image_detail| this should be a scoped_ptr.
It
> doesn't look like you need to though, so in that case maybe this should be a
> const ref?

There won't always be an ImageDetails object (it comes from an optional
parameter), and so a pointer is used to be able to check whether it exists or
not. Ownership remains with the caller, which seems reasonable.

What if we make it const *, since we will never modify it?

https://codereview.chromium.org/1582053002/diff/20001/extensions/browser/api/...
File
extensions/browser/api/guest_view/extension_view/extension_view_internal_api.h
(right):

https://codereview.chromium.org/1582053002/diff/20001/extensions/browser/api/...
extensions/browser/api/guest_view/extension_view/extension_view_internal_api.h:9:
//#include "extensions/browser/api/capture_web_contents.h"
On 2016/01/13 22:41:35, Ken Rockot wrote:
> remove or uncomment?

Done.

[kenrb, 2016-01-18 16:36:43]

lgtm

[Charlie Reis, 2016-01-19 22:45:34]

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

[Charlie Reis, 2016-01-19 22:45:36]

[+nick for ideas on how to lay out this code]

I'm not thrilled with either the code duplication or putting these methods
straight into the content/ namespace.  I wonder if there's a better way to share
them, as noted below.

https://codereview.chromium.org/1582053002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_api.h (right):

https://codereview.chromium.org/1582053002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.h:208: bool RunAsync() override;
This is deprecated (as afakhry mentioned to me the other day):
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

I don't know the background or what code uses the new approach.  @rockot, any
tips?

https://codereview.chromium.org/1582053002/diff/80001/content/browser/composi...
File content/browser/compositor/delegated_frame_host.cc (right):

https://codereview.chromium.org/1582053002/diff/80001/content/browser/composi...
content/browser/compositor/delegated_frame_host.cc:595: // TODO(wjmaclean):
Remove this and get from readback_request_helpers.h instead.
Normally I'd be fine with keeping this CL small and following up, but this
leaves around duplicated code that could quickly get out of sync.  Is there a
reason we can't share the code now?

https://codereview.chromium.org/1582053002/diff/80001/content/browser/readbac...
File content/browser/readback_request_helpers.h (right):

https://codereview.chromium.org/1582053002/diff/80001/content/browser/readbac...
content/browser/readback_request_helpers.h:6: #define
CONTENT_BROWSER_READBACK_REQUEST_HELPERS_H_
As a non-graphics person, "readback" isn't an obvious term.  Seems like this
file should have some comments saying what this is for.

https://codereview.chromium.org/1582053002/diff/80001/content/browser/readbac...
content/browser/readback_request_helpers.h:21: namespace content {
Having this float around the content namespace seems weird to me.  For one thing
(as Nick pointed out to me), it would be tough to know where to look for this
code without doing a codesearch.

Is there a reason that RWHVCF can't call these methods on DelegatedFrameHost if
they were made public?  Or perhaps they should be in utils somewhere?

[ncarter (slow), 2016-01-20 00:16:19]

https://codereview.chromium.org/1582053002/diff/80001/content/browser/readbac...
File content/browser/readback_request_helpers.h (right):

https://codereview.chromium.org/1582053002/diff/80001/content/browser/readbac...
content/browser/readback_request_helpers.h:6: #define
CONTENT_BROWSER_READBACK_REQUEST_HELPERS_H_
On 2016/01/19 22:45:36, Charlie Reis wrote:
> As a non-graphics person, "readback" isn't an obvious term.  Seems like this
> file should have some comments saying what this is for.

'Readback' as a term is already part of the content public api (in the form of
ReadbackRequestCallback and ReadbackResponse, defined in readback_types.h).

https://codereview.chromium.org/1582053002/diff/80001/content/browser/readbac...
content/browser/readback_request_helpers.h:23: void
CopyFromCompositingSurfaceFinished(
This is just a helper used to implement PrepareTextureCopyOutputResult, right?
It's confusing for that to be exposed in the header file.

https://codereview.chromium.org/1582053002/diff/80001/content/browser/readbac...
content/browser/readback_request_helpers.h:28: bool result);
Functions in a big namespace like 'content' hurt readability, since you
basically have to use codesearch or grep to find where they are. That's why we
usually either put them in a class and make them static methods, or put them in
a util namespace, so that call sites give some hint as to where to look to find
the definition.

But an even better option, perhaps, would be to make this actually a stateful
class (e.g. 'class ReadbackRequestHelper', which DelegatedFrameHost has a
member?) I have no idea if that's a suitable solution or not, these copy paths
are a twisty maze of chained callbacks, but I think it's worth the due diligence
to try to arrange these in OO style if possible.

https://codereview.chromium.org/1582053002/diff/80001/content/browser/readbac...
content/browser/readback_request_helpers.h:30: void
PrepareTextureCopyOutputResult(const gfx::Size& dst_size_in_pixel,
Similar to CopyFromCompositingSurfaceFinished, PrepareTextureCopyOutputResult
and PrepareBitmapCopyOutputResult seem like they're just helpers used to
implement CopyFromCompositingSurfaceHasResult. Is that right? If so I'd look for
a way to restrict their visibility so that it's clearer that
CopyFromCompositingSurfaceHasResult is the bit of code we're actually trying to
share.

https://codereview.chromium.org/1582053002/diff/80001/content/browser/readbac...
content/browser/readback_request_helpers.h:40: void
CopyFromCompositingSurfaceHasResult(
My interpretation is that the reusable functionality here, is all about bridging
between cc::CopyOutputResult and the content-public ReadbackRequestCallback. We
can't declare this helper in cc because of their content/public dependency, so
they need to be in content/browser somewhere.

Why not a _util file in content/browser/compositor, then? Isn't that a natural
place to put code that's converting cc-isms into content-isms?

https://codereview.chromium.org/1582053002/diff/80001/content/browser/readbac...
content/browser/readback_request_helpers.h:44: scoped_ptr<cc::CopyOutputResult>
result);
+= blank line

https://codereview.chromium.org/1582053002/diff/80001/content/browser/readbac...
content/browser/readback_request_helpers.h:45: }
}  // namespace content

[kenrb, 2016-01-20 01:43:04]

https://codereview.chromium.org/1582053002/diff/80001/content/browser/readbac...
File content/browser/readback_request_helpers.h (right):

https://codereview.chromium.org/1582053002/diff/80001/content/browser/readbac...
content/browser/readback_request_helpers.h:21: namespace content {
On 2016/01/19 22:45:36, Charlie Reis wrote:
> Having this float around the content namespace seems weird to me.  For one
thing
> (as Nick pointed out to me), it would be tough to know where to look for this
> code without doing a codesearch.
> 
> Is there a reason that RWHVCF can't call these methods on DelegatedFrameHost
if
> they were made public?  Or perhaps they should be in utils somewhere?

DelegatedFrameHost manages compositor frame state for top-level
RenderWidgetHostViews, and interacts with the compositor on their behalf.
RenderWidgetHostViewChildFrame does not have access to one.

[wjmaclean, 2016-01-20 15:27:31]

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

[commit-bot: I haz the power, 2016-01-20 15:27:39]

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

[wjmaclean, 2016-01-20 15:32:37]

Hi all, PTAL?

I'll admit, I wasn't entirely sure of where to put the ReadbackRequestUtils
stuff, but based on Nick's comments about content/browser/compositor, and taking
a look inside surface_utils.cc, I think that might be a good home for several
reasons:

1) both of the consumers already import this,
2) all the util code is surface-related, and
3) the android-specific differences seem to getting handled here for other
functions (I still need to figure out if there's an existing Android path for
the readback ... DelegatedFrameHost seems to be an Aura concept, so I don't know
if there is an equivalent pathway for Android, or if that needs to be developed
separately).

Please see my responses to various comments below.

https://codereview.chromium.org/1582053002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_api.h (right):

https://codereview.chromium.org/1582053002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.h:208: bool RunAsync() override;
On 2016/01/19 22:45:36, Charlie Reis wrote:
> This is deprecated (as afakhry mentioned to me the other day):
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
> 
> I don't know the background or what code uses the new approach.  @rockot, any
> tips?

In the derived WebView function I used RunAsyncSafe(), but I wasn't sure about
the plumbing for the existing class, so I kept it the same here
(CaptureWebContentsFunction used RunAsync() as opposed to RunAsyncSafe()).

https://codereview.chromium.org/1582053002/diff/80001/content/browser/composi...
File content/browser/compositor/delegated_frame_host.cc (right):

https://codereview.chromium.org/1582053002/diff/80001/content/browser/composi...
content/browser/compositor/delegated_frame_host.cc:595: // TODO(wjmaclean):
Remove this and get from readback_request_helpers.h instead.
On 2016/01/19 22:45:36, Charlie Reis wrote:
> Normally I'd be fine with keeping this CL small and following up, but this
> leaves around duplicated code that could quickly get out of sync.  Is there a
> reason we can't share the code now?

I was mostly hoping to limit the damage if something went wrong (and the size of
the subsequent revert), but I've removed these now.

That being said, there are also a set of similar, but independent, functions for
video that likely should also be moved at some point, but I'd rather not get
into that in this CL as the codepath is distinct.

Done.

https://codereview.chromium.org/1582053002/diff/80001/content/browser/readbac...
File content/browser/readback_request_helpers.h (right):

https://codereview.chromium.org/1582053002/diff/80001/content/browser/readbac...
content/browser/readback_request_helpers.h:6: #define
CONTENT_BROWSER_READBACK_REQUEST_HELPERS_H_
On 2016/01/20 00:16:18, ncarter wrote:
> On 2016/01/19 22:45:36, Charlie Reis wrote:
> > As a non-graphics person, "readback" isn't an obvious term.  Seems like this
> > file should have some comments saying what this is for.
> 
> 'Readback' as a term is already part of the content public api (in the form of
> ReadbackRequestCallback and ReadbackResponse, defined in readback_types.h).

Acknowledged.

https://codereview.chromium.org/1582053002/diff/80001/content/browser/readbac...
content/browser/readback_request_helpers.h:6: #define
CONTENT_BROWSER_READBACK_REQUEST_HELPERS_H_
On 2016/01/19 22:45:36, Charlie Reis wrote:
> As a non-graphics person, "readback" isn't an obvious term.  Seems like this
> file should have some comments saying what this is for.

Acknowledged.

https://codereview.chromium.org/1582053002/diff/80001/content/browser/readbac...
content/browser/readback_request_helpers.h:21: namespace content {
On 2016/01/19 22:45:36, Charlie Reis wrote:
> Having this float around the content namespace seems weird to me.  For one
thing
> (as Nick pointed out to me), it would be tough to know where to look for this
> code without doing a codesearch.
> 
> Is there a reason that RWHVCF can't call these methods on DelegatedFrameHost
if
> they were made public?  Or perhaps they should be in utils somewhere?

We could consider making content::surface_utils or content::compositor::utils.
Since there are other unrelated functions in surface_utils that would be
affected by this, perhaps the namespace change should be done separately?

https://codereview.chromium.org/1582053002/diff/80001/content/browser/readbac...
content/browser/readback_request_helpers.h:23: void
CopyFromCompositingSurfaceFinished(
On 2016/01/20 00:16:18, ncarter wrote:
> This is just a helper used to implement PrepareTextureCopyOutputResult, right?
> It's confusing for that to be exposed in the header file.

Done.

https://codereview.chromium.org/1582053002/diff/80001/content/browser/readbac...
content/browser/readback_request_helpers.h:28: bool result);
On 2016/01/20 00:16:18, ncarter wrote:
> Functions in a big namespace like 'content' hurt readability, since you
> basically have to use codesearch or grep to find where they are. That's why we
> usually either put them in a class and make them static methods, or put them
in
> a util namespace, so that call sites give some hint as to where to look to
find
> the definition.
> 
> But an even better option, perhaps, would be to make this actually a stateful
> class (e.g. 'class ReadbackRequestHelper', which DelegatedFrameHost has a
> member?) I have no idea if that's a suitable solution or not, these copy paths
> are a twisty maze of chained callbacks, but I think it's worth the due
diligence
> to try to arrange these in OO style if possible.

The functions all appear to be stateless, though they could still be grouped
into a class as static functions? For now I've moved them into
content/browser/compositor/surface_utils.{h|cc} with some related surface
functionality.

https://codereview.chromium.org/1582053002/diff/80001/content/browser/readbac...
content/browser/readback_request_helpers.h:40: void
CopyFromCompositingSurfaceHasResult(
On 2016/01/20 00:16:19, ncarter wrote:
> My interpretation is that the reusable functionality here, is all about
bridging
> between cc::CopyOutputResult and the content-public ReadbackRequestCallback.
We
> can't declare this helper in cc because of their content/public dependency, so
> they need to be in content/browser somewhere.
> 
> Why not a _util file in content/browser/compositor, then? Isn't that a natural
> place to put code that's converting cc-isms into content-isms?

Acknowledged.

https://codereview.chromium.org/1582053002/diff/80001/content/browser/readbac...
content/browser/readback_request_helpers.h:44: scoped_ptr<cc::CopyOutputResult>
result);
On 2016/01/20 00:16:18, ncarter wrote:
> += blank line

Done.

https://codereview.chromium.org/1582053002/diff/80001/content/browser/readbac...
content/browser/readback_request_helpers.h:45: }
On 2016/01/20 00:16:18, ncarter wrote:
> }  // namespace content

Done.

[commit-bot: I haz the power, 2016-01-20 15:43:20]

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

[commit-bot: I haz the power, 2016-01-20 15:43:21]

Dry run: Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

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

https://codereview.chromium.org/1582053002/diff/120001/content/browser/compos...
File content/browser/compositor/delegated_frame_host.cc (right):

https://codereview.chromium.org/1582053002/diff/120001/content/browser/compos...
content/browser/compositor/delegated_frame_host.cc:903: else {
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

"""However, if one part of an if-else statement uses curly braces, the other
part must too"""

https://codereview.chromium.org/1582053002/diff/120001/content/browser/compos...
File content/browser/compositor/surface_utils.cc (right):

https://codereview.chromium.org/1582053002/diff/120001/content/browser/compos...
content/browser/compositor/surface_utils.cc:157: }  // namesspace
typo

https://codereview.chromium.org/1582053002/diff/120001/content/browser/compos...
File content/browser/compositor/surface_utils.h (right):

https://codereview.chromium.org/1582053002/diff/120001/content/browser/compos...
content/browser/compositor/surface_utils.h:13: class SkAutoLockPixels;
Seems unused.

https://codereview.chromium.org/1582053002/diff/120001/content/browser/compos...
content/browser/compositor/surface_utils.h:14: class SkBitmap;
Seems unused.

https://codereview.chromium.org/1582053002/diff/120001/content/browser/compos...
content/browser/compositor/surface_utils.h:18: class SingleReleaseCallback;
Seems unused.

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

https://codereview.chromium.org/1582053002/diff/120001/extensions/browser/api...
extensions/browser/api/web_contents_capture_client.cc:92: std::string&
base64_result) {
Return params ought to be pointers, not mutable refs.

https://google.github.io/styleguide/cppguide.html#Reference_Arguments

https://codereview.chromium.org/1582053002/diff/120001/extensions/browser/api...
File extensions/browser/api/web_contents_capture_client.h (right):

https://codereview.chromium.org/1582053002/diff/120001/extensions/browser/api...
extensions/browser/api/web_contents_capture_client.h:21: // Base class for
capturing visibile area of a WebContents.
visibile -> visible

[wjmaclean, 2016-01-20 21:45:19]

Minor errors fixed, ptal?

https://codereview.chromium.org/1582053002/diff/120001/content/browser/compos...
File content/browser/compositor/delegated_frame_host.cc (right):

https://codereview.chromium.org/1582053002/diff/120001/content/browser/compos...
content/browser/compositor/delegated_frame_host.cc:903: else {
On 2016/01/20 21:20:49, ncarter wrote:
>
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...
> 
> """However, if one part of an if-else statement uses curly braces, the other
> part must too"""

Done.

https://codereview.chromium.org/1582053002/diff/120001/content/browser/compos...
File content/browser/compositor/surface_utils.cc (right):

https://codereview.chromium.org/1582053002/diff/120001/content/browser/compos...
content/browser/compositor/surface_utils.cc:157: }  // namesspace
On 2016/01/20 21:20:49, ncarter wrote:
> typo

Done.

https://codereview.chromium.org/1582053002/diff/120001/content/browser/compos...
File content/browser/compositor/surface_utils.h (right):

https://codereview.chromium.org/1582053002/diff/120001/content/browser/compos...
content/browser/compositor/surface_utils.h:13: class SkAutoLockPixels;
On 2016/01/20 21:20:49, ncarter wrote:
> Seems unused.

Done.

https://codereview.chromium.org/1582053002/diff/120001/content/browser/compos...
content/browser/compositor/surface_utils.h:14: class SkBitmap;
On 2016/01/20 21:20:49, ncarter wrote:
> Seems unused.

Done.

https://codereview.chromium.org/1582053002/diff/120001/content/browser/compos...
content/browser/compositor/surface_utils.h:18: class SingleReleaseCallback;
On 2016/01/20 21:20:49, ncarter wrote:
> Seems unused.

Done.

Thanks for spotting these unused cases!

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

https://codereview.chromium.org/1582053002/diff/120001/extensions/browser/api...
extensions/browser/api/web_contents_capture_client.cc:92: std::string&
base64_result) {
On 2016/01/20 21:20:49, ncarter wrote:
> Return params ought to be pointers, not mutable refs.
> 
> https://google.github.io/styleguide/cppguide.html#Reference_Arguments

Done.

https://codereview.chromium.org/1582053002/diff/120001/extensions/browser/api...
File extensions/browser/api/web_contents_capture_client.h (right):

https://codereview.chromium.org/1582053002/diff/120001/extensions/browser/api...
extensions/browser/api/web_contents_capture_client.h:21: // Base class for
capturing visibile area of a WebContents.
On 2016/01/20 21:20:49, ncarter wrote:
> visibile -> visible

Done.

[wjmaclean, 2016-01-20 21:45:27]

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

[Ken Rockot(use gerrit already), 2016-01-20 21:46:24]

lgtm sorry for the delay, forgot to send earlier

[commit-bot: I haz the power, 2016-01-20 21:46:28]

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

[Fady Samuel, 2016-01-20 21:50:21]

fsamuel@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Fady Samuel, 2016-01-20 21:50:22]

https://codereview.chromium.org/1582053002/diff/140001/extensions/common/api/...
File extensions/common/api/_api_features.json (right):

https://codereview.chromium.org/1582053002/diff/140001/extensions/common/api/...
extensions/common/api/_api_features.json:469: "internal": true,
Drive by: I think we turn on experimental features for these WebUI pages but we
don't need to do that anymore, right? I don't want to expose experimental
<webview> features to WebUI if we don't need to.

[wjmaclean, 2016-01-20 21:58:13]

I've disable experimental WebView APIs on WebUI.

https://codereview.chromium.org/1582053002/diff/140001/extensions/common/api/...
File extensions/common/api/_api_features.json (right):

https://codereview.chromium.org/1582053002/diff/140001/extensions/common/api/...
extensions/common/api/_api_features.json:469: "internal": true,
On 2016/01/20 21:50:22, Fady Samuel wrote:
> Drive by: I think we turn on experimental features for these WebUI pages but
we
> don't need to do that anymore, right? I don't want to expose experimental
> <webview> features to WebUI if we don't need to.

Done.

I'm happy enough to turn this off. Is there any danger of anything breaking when
we make the switch to non-experimental later? I can't think of any WebUI pages
that would be trying to use this particular API ...

[wjmaclean, 2016-01-20 21:59:58]

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
CaptureWebContents 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/readback_request_helpers.* in order to avoid duplication
as both DelegatedFrameHost and RenderWidgetHostViewChildFrame now
use the code. Note: the duplicate code in DelegatedFrameHost will be
removed in a follow-on CL.

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
CaptureWebContents 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
==========

[wjmaclean, 2016-01-20 22:00:54]

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
CaptureWebContents 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
==========

[ncarter (slow), 2016-01-20 22:03:17]

lgtm

[Charlie Reis, 2016-01-20 22:09:36]

Much better, thanks!  content/ LGTM too.

[wjmaclean, 2016-01-20 23:40:06]

The CQ bit was checked by wjmaclean@chromium.org

[wjmaclean, 2016-01-20 23:40:06]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org,
rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/1582053002/#ps160001
(title: "Don't enable experimental WebView APIs for WebUI.")

[commit-bot: I haz the power, 2016-01-20 23:42:24]

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

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

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
==========

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

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-01-21 02:17:45]

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

Committed: https://crrev.com/5b8cdcc82fc9da7bab15fd3dbf65df843dc73589
Cr-Commit-Position: refs/heads/master@{#370565}
==========

[commit-bot: I haz the power, 2016-01-21 02:17:46]

Patchset 9 (id:??) landed as
https://crrev.com/5b8cdcc82fc9da7bab15fd3dbf65df843dc73589
Cr-Commit-Position: refs/heads/master@{#370565}

[tsergeant, 2016-01-21 06:49:05]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/1614703003/ by tsergeant@chromium.org.

The reason for reverting is: The test WebViewAPITest.TestCaptureVisibleRegion is
failing on several windows bots:

https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2...
https://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/build...
https://build.chromium.org/p/chromium.win/builders/Vista%20Tests%20%281%29/bu....

[Fady Samuel, 2016-01-25 16:20:39]

This seems like it could be a problem in practice as well, rather than a testing
bug. I think the API implementation should internally request a new frame if one
isn't available yet and wait until the new frame becomes available.

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

https://codereview.chromium.org/1582053002/diff/180001/extensions/browser/api...
extensions/browser/api/web_contents_capture_client.cc:85: // TODO(wjmaclean):
Improve error reporting. Why aren't we passing more
Remove this now?

https://codereview.chromium.org/1582053002/diff/180001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_view_apitest.cc (right):

https://codereview.chromium.org/1582053002/diff/180001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_view_apitest.cc:758:
WaitForFirstFrame(guest_web_contents);
This seems like a broken API if we have to do this.