Fix copyImage for WebGL elements.

Fix copyImage for WebGL elements.

"Copy image" menu item crashes on WebGL elements because the
copiedImage in HTMLCanvasElement gets cleared before it can
be copied to the clipboard.
Also fixed HTMLCanvasElement::copiedImage() so that the copy
is done on the latest frame composited to the screen.

BUG=392765
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178508

[hj.r.chung, 2014-07-14 10:13:39]

PTAL!

[Ken Russell (switch to Gerrit), 2014-07-14 17:59:21]

Thanks, the code changes look fine, but where are the tests? Without a test,
this is going to break again.

[Ken Russell (switch to Gerrit), 2014-07-14 22:13:15]

To be more concrete: if it's not currently possible to write tests for the "Save
image as..." context menu option for canvas elements, that should be fixed. One
way to do so would be to add a new method to TestRunner which takes in a Canvas
element and invokes the code path, saving to a file, or similar. See for example
the implementation of capturePixelsAsyncThen, which is a callback that was added
relatively recently.

https://code.google.com/p/chromium/codesearch#chromium/src/content/shell/rend...

[hj.r.chung, 2014-07-15 02:54:55]

On 2014/07/14 22:13:15, Ken Russell wrote:
> To be more concrete: if it's not currently possible to write tests for the
"Save
> image as..." context menu option for canvas elements, that should be fixed.
One
> way to do so would be to add a new method to TestRunner which takes in a
Canvas
> element and invokes the code path, saving to a file, or similar. See for
example
> the implementation of capturePixelsAsyncThen, which is a callback that was
added
> relatively recently.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/shell/rend...

Thank you for the pointer. I'll take a look and add the tests.

[hj.r.chung, 2014-07-16 11:29:19]

added layout test for copy image
now depends on chromium side change https://codereview.chromium.org/396953002/
PTAL!

[Ken Russell (switch to Gerrit), 2014-07-16 20:43:16]

The code change and this test look good. Please write another layout test that
verifies copyImageAtAndCapturePixelsAsyncThen (see my suggested renaming in the
other CL) in the case where the (x, y) coordinate doesn't contain an image, and
verify that the callback gets (0, 0, null) or similar.

[Ken Russell (switch to Gerrit), 2014-07-17 21:32:31]

Please split off the negative test into another layout test. It can be much
simpler and not even use WebGL. It could be something like
compositing/copy-image-when-no-image-exists.html.

https://codereview.chromium.org/389273002/diff/130001/LayoutTests/compositing...
File LayoutTests/compositing/webgl/webgl-copy-image.html (right):

https://codereview.chromium.org/389273002/diff/130001/LayoutTests/compositing...
LayoutTests/compositing/webgl/webgl-copy-image.html:77: if (width > 0 && height
> 0) {
This doesn't actually verify that the attempt to fetch an image at (400, 400)
results in a width and height of 0. It should.

[hj.r.chung, 2014-07-18 06:31:39]

I uploaded a patch for splitting the tests. PTAL!

https://codereview.chromium.org/389273002/diff/130001/LayoutTests/compositing...
File LayoutTests/compositing/webgl/webgl-copy-image.html (right):

https://codereview.chromium.org/389273002/diff/130001/LayoutTests/compositing...
LayoutTests/compositing/webgl/webgl-copy-image.html:77: if (width > 0 && height
> 0) {
On 2014/07/17 21:32:31, Ken Russell wrote:
> This doesn't actually verify that the attempt to fetch an image at (400, 400)
> results in a width and height of 0. It should.

Done.

[Ken Russell (switch to Gerrit), 2014-07-18 23:33:29]

Very nice. Thank you for persisting with this change. LGTM

https://codereview.chromium.org/389273002/diff/170001/LayoutTests/compositing...
File LayoutTests/compositing/copy-image-when-no-image-exists.html (right):

https://codereview.chromium.org/389273002/diff/170001/LayoutTests/compositing...
LayoutTests/compositing/copy-image-when-no-image-exists.html:34:
shouldBeEqualToNumber("height", 0);
Ideally this would test the value of "snapshot", too.

[Ken Russell (switch to Gerrit), 2014-07-18 23:33:32]

The CQ bit was checked by kbr@chromium.org

[commit-bot: I haz the power, 2014-07-18 23:33:57]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/heejin.r.chung@samsung.com/389273002/1...

[commit-bot: I haz the power, 2014-07-19 00:45:56]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/17833)

[commit-bot: I haz the power, 2014-07-19 02:17:43]

Change committed as 178508

[hj.r.chung, 2014-07-20 15:26:27]

On 2014/07/18 23:33:29, Ken Russell wrote:
> Very nice. Thank you for persisting with this change. LGTM
> 
>
https://codereview.chromium.org/389273002/diff/170001/LayoutTests/compositing...
> File LayoutTests/compositing/copy-image-when-no-image-exists.html (right):
> 
>
https://codereview.chromium.org/389273002/diff/170001/LayoutTests/compositing...
> LayoutTests/compositing/copy-image-when-no-image-exists.html:34:
> shouldBeEqualToNumber("height", 0);
> Ideally this would test the value of "snapshot", too.

Th

[hj.r.chung, 2014-07-20 15:28:05]

On 2014/07/20 15:26:27, hj.r.chung wrote:
> On 2014/07/18 23:33:29, Ken Russell wrote:
> > Very nice. Thank you for persisting with this change. LGTM
> > 
> >
>
https://codereview.chromium.org/389273002/diff/170001/LayoutTests/compositing...
> > File LayoutTests/compositing/copy-image-when-no-image-exists.html (right):
> > 
> >
>
https://codereview.chromium.org/389273002/diff/170001/LayoutTests/compositing...
> > LayoutTests/compositing/copy-image-when-no-image-exists.html:34:
> > shouldBeEqualToNumber("height", 0);
> > Ideally this would test the value of "snapshot", too.
> 
> Th

Oops, this was supposed to be "Thank you! and I'll try to add checking the
snapshot value soon "