Make OffscreenCanvas Webgl Context work with preserveDrawingBuffer flag

Make OffscreenCanvas Webgl Context work with preserveDrawingBuffer flag

BUG=646864
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2627793003
Cr-Commit-Position: refs/heads/master@{#443374}
Committed: https://chromium.googlesource.com/chromium/src/+/6a05eae35091da67cadd2458ffe0c5c87a3cf1ef

[xlai (Olivia), 2017-01-11 20:59:36]

Description was changed from

==========
Make OffscreenCanvas Webgl Context work with preserveDrawingBuffer flag

BUG=646864
==========

to

==========
Make OffscreenCanvas Webgl Context work with preserveDrawingBuffer flag

BUG=646864
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[xlai (Olivia), 2017-01-11 21:02:42]

xlai@chromium.org changed reviewers:
+ junov@chromium.org, kbr@chromium.org

[xlai (Olivia), 2017-01-11 21:02:42]

kbr@: Please take a look at WebglRenderingContext. Basically I just extract that
part of code from the newly implemented toImageData function.

junov@: Please take a look at the test.

Thanks!

[xlai (Olivia), 2017-01-11 21:08:43]

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

[commit-bot: I haz the power, 2017-01-11 21:09:23]

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

[Justin Novosad, 2017-01-11 21:40:04]

LGTM with nits.

https://codereview.chromium.org/2627793003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-webgl-preserveDrawingBuffer.html
(right):

https://codereview.chromium.org/2627793003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-webgl-preserveDrawingBuffer.html:2:
<canvas id="withPreserve" width="50" height="50"></canvas>
You don't need these.

https://codereview.chromium.org/2627793003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-webgl-preserveDrawingBuffer.html:8:
var canvas = document.getElementById(placeHolderCanvasId);
Just do:
var canvas = document.createElement('canvas');

https://codereview.chromium.org/2627793003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2627793003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4138: if
(canvas()) {
The way this is written, canvas() will get called on builds that have DCHECKs
disabled.  Either use #if or ternary operator inside DCHECK.

https://codereview.chromium.org/2627793003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h
(right):

https://codereview.chromium.org/2627793003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:1658:
sk_sp<SkImage> makeImageSnapshotCopy(SkImageInfo&);
I think the "Copy" part of the name is not necessary.

[commit-bot: I haz the power, 2017-01-11 22:50:55]

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

[commit-bot: I haz the power, 2017-01-11 22:50:56]

Dry run: This issue passed the CQ dry run.

[Ken Russell (switch to Gerrit), 2017-01-12 02:01:45]

LGTM with junov's comments addressed. Thanks for the good fix.

It would be ideal to add more tests to the WebGL conformance suite for this, so
that this functionality is verified on all browsers. Could you work on that at
some point?

https://codereview.chromium.org/2627793003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2627793003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4138: if
(canvas()) {
On 2017/01/11 21:40:04, Justin Novosad wrote:
> The way this is written, canvas() will get called on builds that have DCHECKs
> disabled.  Either use #if or ternary operator inside DCHECK.

Yes, let's not call this in release builds. You could use:

#if DCHECK_IS_ON()
...
#endif

but the ternary operator seems reasonable in this case.

[xlai (Olivia), 2017-01-12 20:04:48]

On 2017/01/12 02:01:45, Ken Russell wrote:
> LGTM with junov's comments addressed. Thanks for the good fix.
> 
> It would be ideal to add more tests to the WebGL conformance suite for this,
so
> that this functionality is verified on all browsers. Could you work on that at
> some point?

I have created a tracking issue
https://bugs.chromium.org/p/chromium/issues/detail?id=680635 and would work on
that at some point.

https://codereview.chromium.org/2627793003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-webgl-preserveDrawingBuffer.html
(right):

https://codereview.chromium.org/2627793003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-webgl-preserveDrawingBuffer.html:2:
<canvas id="withPreserve" width="50" height="50"></canvas>
On 2017/01/11 21:40:04, Justin Novosad wrote:
> You don't need these.

Done.

https://codereview.chromium.org/2627793003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-webgl-preserveDrawingBuffer.html:8:
var canvas = document.getElementById(placeHolderCanvasId);
On 2017/01/11 21:40:04, Justin Novosad wrote:
> Just do:
> var canvas = document.createElement('canvas');

Done.

https://codereview.chromium.org/2627793003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2627793003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4138: if
(canvas()) {
On 2017/01/12 02:01:45, Ken Russell wrote:
> On 2017/01/11 21:40:04, Justin Novosad wrote:
> > The way this is written, canvas() will get called on builds that have
DCHECKs
> > disabled.  Either use #if or ternary operator inside DCHECK.
> 
> Yes, let's not call this in release builds. You could use:
> 
> #if DCHECK_IS_ON()
> ...
> #endif
> 
> but the ternary operator seems reasonable in this case.

Done.

https://codereview.chromium.org/2627793003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h
(right):

https://codereview.chromium.org/2627793003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:1658:
sk_sp<SkImage> makeImageSnapshotCopy(SkImageInfo&);
On 2017/01/11 21:40:04, Justin Novosad wrote:
> I think the "Copy" part of the name is not necessary.

Done.

[xlai (Olivia), 2017-01-12 20:07:24]

The CQ bit was checked by xlai@chromium.org

[xlai (Olivia), 2017-01-12 20:07:25]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
junov@chromium.org
Link to the patchset: https://codereview.chromium.org/2627793003/#ps20001
(title: "fix based on reviewer feedback")

[commit-bot: I haz the power, 2017-01-12 20:08:05]

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-01-12 21:52:17]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484251644912930,
"parent_rev": "5d55e69ee9f4e13adf75cc6e45d1475600e95385", "commit_rev":
"6a05eae35091da67cadd2458ffe0c5c87a3cf1ef"}

[commit-bot: I haz the power, 2017-01-12 21:52:50]

Description was changed from

==========
Make OffscreenCanvas Webgl Context work with preserveDrawingBuffer flag

BUG=646864
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Make OffscreenCanvas Webgl Context work with preserveDrawingBuffer flag

BUG=646864
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2627793003
Cr-Commit-Position: refs/heads/master@{#443374}
Committed:
https://chromium.googlesource.com/chromium/src/+/6a05eae35091da67cadd2458ffe0...
==========

[commit-bot: I haz the power, 2017-01-12 21:52:51]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/6a05eae35091da67cadd2458ffe0...