<webview>: Fix missing transparency in captureVisibleRegion().

<webview>: Fix missing transparency in captureVisibleRegion().

In output formats that support transparency (png),
WebContentsCaptureClient::EncodeBitmap should propagate the alpha
values of the WebContents if that contents has any.

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

Committed: https://crrev.com/71152bea8837143acecf76d4c662b3482eeab66a
Cr-Commit-Position: refs/heads/master@{#385821}

[avallee, 2016-04-06 01:56:31]

avallee@chromium.org changed reviewers:
+ jam@chromium.org, wjmaclean@chromium.org

[avallee, 2016-04-06 01:56:31]

PTAL.

Sample test case at
https://github.com/avallee/chrome-samples/tree/master/webview_transparency

[wjmaclean, 2016-04-06 12:39:21]

LGTM with nits (see comments below).

Also, perhaps your example could be placed under

chrome/common/extensions/docs/examples/api/tabs/

as there is precedence for that.

https://codereview.chromium.org/1863953002/diff/1/extensions/browser/api/web_...
File extensions/browser/api/web_contents_capture_client.cc (right):

https://codereview.chromium.org/1863953002/diff/1/extensions/browser/api/web_...
extensions/browser/api/web_contents_capture_client.cc:123: /* Don't discard
transparency */
This comment could be re-worded, something like "Don't discard transparency if
client allows transparency."

https://codereview.chromium.org/1863953002/diff/1/extensions/browser/api/web_...
File extensions/browser/api/web_contents_capture_client.h (right):

https://codereview.chromium.org/1863953002/diff/1/extensions/browser/api/web_...
extensions/browser/api/web_contents_capture_client.h:31: virtual bool
ContentsHasTransparency() = 0;
This name is a bit misleading, since it may return true even if the contents
doesn't have transparency (e.g. a WebView with allowsTransparency set, but
displaying content without transparency).

Perhaps "ClientAllowsTransparency"?

[avallee, 2016-04-06 14:12:27]

https://codereview.chromium.org/1863953002/diff/1/extensions/browser/api/web_...
File extensions/browser/api/web_contents_capture_client.cc (right):

https://codereview.chromium.org/1863953002/diff/1/extensions/browser/api/web_...
extensions/browser/api/web_contents_capture_client.cc:123: /* Don't discard
transparency */
On 2016/04/06 12:39:20, wjmaclean wrote:
> This comment could be re-worded, something like "Don't discard transparency if
> client allows transparency."

Done.

https://codereview.chromium.org/1863953002/diff/1/extensions/browser/api/web_...
File extensions/browser/api/web_contents_capture_client.h (right):

https://codereview.chromium.org/1863953002/diff/1/extensions/browser/api/web_...
extensions/browser/api/web_contents_capture_client.h:31: virtual bool
ContentsHasTransparency() = 0;
On 2016/04/06 12:39:21, wjmaclean wrote:
> This name is a bit misleading, since it may return true even if the contents
> doesn't have transparency (e.g. a WebView with allowsTransparency set, but
> displaying content without transparency).
> 
> Perhaps "ClientAllowsTransparency"?

Done.

[jam, 2016-04-06 17:13:45]

not sure why i'm added as a reviewer.

please look for owners in these directories

[avallee, 2016-04-06 17:17:59]

Description was changed from

==========
<webview>: Fix missing transparency in captureVisibleRegion().

In output formats that support transparency (png),
WebContentsCaptureClient::EncodeBitmap should propagate the alpha
values of the WebContents if that contents has any.

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

to

==========
<webview>: Fix missing transparency in captureVisibleRegion().

In output formats that support transparency (png),
WebContentsCaptureClient::EncodeBitmap should propagate the alpha
values of the WebContents if that contents has any.

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

[avallee, 2016-04-06 17:17:59]

avallee@chromium.org changed reviewers:
- jam@chromium.org

[avallee, 2016-04-06 17:18:43]

avallee@chromium.org changed reviewers:
+ finnur@chromium.org

[Finnur, 2016-04-07 10:08:33]

This almost slipped through the cracks in my review queue. In the future, please
include a message when you add new reviewers. :)

LGTM with nits.

https://codereview.chromium.org/1863953002/diff/20001/extensions/browser/api/...
File extensions/browser/api/guest_view/web_view/web_view_internal_api.h (right):

https://codereview.chromium.org/1863953002/diff/20001/extensions/browser/api/...
extensions/browser/api/guest_view/web_view/web_view_internal_api.h:46:
WebViewInternalCaptureVisibleRegionFunction() {}
is_guest_transparent_(false) ... or something.

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

https://codereview.chromium.org/1863953002/diff/20001/extensions/browser/api/...
extensions/browser/api/web_contents_capture_client.cc:124: * client allows
transparency. */
Please avoid multi-line comments. This comment doesn't really help with
anything, so I would just remove it.

[avallee, 2016-04-07 14:21:16]

https://codereview.chromium.org/1863953002/diff/20001/extensions/browser/api/...
File extensions/browser/api/guest_view/web_view/web_view_internal_api.h (right):

https://codereview.chromium.org/1863953002/diff/20001/extensions/browser/api/...
extensions/browser/api/guest_view/web_view/web_view_internal_api.h:46:
WebViewInternalCaptureVisibleRegionFunction() {}
On 2016/04/07 10:08:33, Finnur wrote:
> is_guest_transparent_(false) ... or something.

Done.

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

https://codereview.chromium.org/1863953002/diff/20001/extensions/browser/api/...
extensions/browser/api/web_contents_capture_client.cc:124: * client allows
transparency. */
On 2016/04/07 10:08:33, Finnur wrote:
> Please avoid multi-line comments. This comment doesn't really help with
> anything, so I would just remove it.

Replaced with a variable that documents what's going on.

[avallee, 2016-04-07 18:06:20]

The CQ bit was checked by avallee@chromium.org

[avallee, 2016-04-07 18:06:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
wjmaclean@chromium.org, finnur@chromium.org
Link to the patchset: https://codereview.chromium.org/1863953002/#ps60001
(title: "fix build break")

[commit-bot: I haz the power, 2016-04-07 18:06:47]

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

[commit-bot: I haz the power, 2016-04-07 18:50:45]

Description was changed from

==========
<webview>: Fix missing transparency in captureVisibleRegion().

In output formats that support transparency (png),
WebContentsCaptureClient::EncodeBitmap should propagate the alpha
values of the WebContents if that contents has any.

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

to

==========
<webview>: Fix missing transparency in captureVisibleRegion().

In output formats that support transparency (png),
WebContentsCaptureClient::EncodeBitmap should propagate the alpha
values of the WebContents if that contents has any.

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

[commit-bot: I haz the power, 2016-04-07 18:50:46]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-07 18:55:00]

Description was changed from

==========
<webview>: Fix missing transparency in captureVisibleRegion().

In output formats that support transparency (png),
WebContentsCaptureClient::EncodeBitmap should propagate the alpha
values of the WebContents if that contents has any.

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

to

==========
<webview>: Fix missing transparency in captureVisibleRegion().

In output formats that support transparency (png),
WebContentsCaptureClient::EncodeBitmap should propagate the alpha
values of the WebContents if that contents has any.

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

Committed: https://crrev.com/71152bea8837143acecf76d4c662b3482eeab66a
Cr-Commit-Position: refs/heads/master@{#385821}
==========

[commit-bot: I haz the power, 2016-04-07 18:55:01]

Patchset 4 (id:??) landed as
https://crrev.com/71152bea8837143acecf76d4c662b3482eeab66a
Cr-Commit-Position: refs/heads/master@{#385821}