Resolving drawImage issue with canvases that contain bitmap images

Adding proper check for canvas type to
HTMLCanvasElement::getSourceImageForCanvas() such that
drawImage can correctly draw canvases with bitmap images.

BUG=639841
Committed: https://crrev.com/4d29ebec048d83aac139ec5edcbe22d107351b7b
Committed: https://crrev.com/d161245684a7adab144318deec691ad6fdc6251e
Cr-Original-Commit-Position: refs/heads/master@{#413731}
Cr-Commit-Position: refs/heads/master@{#413828}

[zakerinasab, 2016-08-22 17:16:26]

Description was changed from

==========
Addressing the bug in which improper use of ImageBuffer in
HTMLCanvasElement::getSourceImageForCanvas() was led to not drawning the
canvases with image bitmap in drawImage().

BUG=639841
==========

to

==========
Addressing the bug in which improper use of ImageBuffer in
HTMLCanvasElement::getSourceImageForCanvas() was led to not drawning the
canvases with image bitmap in drawImage().

BUG=639841
==========

[zakerinasab, 2016-08-22 18:10:18]

Patchset #1 (id:1) has been deleted

[zakerinasab, 2016-08-22 18:12:01]

zakerinasab@chromium.org changed reviewers:
+ junov@chromium.org

[zakerinasab, 2016-08-22 18:12:02]

When returning m_image from ImageBitmapRenderingContext, I tried to return
RefPtr<Image> frist, but then presubmit check was failed and asked me to use
PassRefPtr. I wasn't sure if it is safe to move the pointer ownership, so I
returned Image* instead. Please let me know if this needs to change.

[Justin Novosad, 2016-08-22 19:28:23]

For the code review description: this will become the commit comment when the
change lands.  The first line should be a short summary, and the body of the
description should use at most 72 characters per line.

[zakerinasab, 2016-08-22 19:33:38]

Description was changed from

==========
Addressing the bug in which improper use of ImageBuffer in
HTMLCanvasElement::getSourceImageForCanvas() was led to not drawning the
canvases with image bitmap in drawImage().

BUG=639841
==========

to

==========
Adding proper check for canvas type to
HTMLCanvasElement::getSourceImageForCanvas() such that
drawImage can correctly draw canvases with bitmap
images.


BUG=639841
==========

[zakerinasab, 2016-08-22 19:33:59]

On 2016/08/22 19:28:23, Justin Novosad wrote:
> For the code review description: this will become the commit comment when the
> change lands.  The first line should be a short summary, and the body of the
> description should use at most 72 characters per line.

Done.

[zakerinasab, 2016-08-22 19:34:18]

Description was changed from

==========
Adding proper check for canvas type to
HTMLCanvasElement::getSourceImageForCanvas() such that
drawImage can correctly draw canvases with bitmap
images.


BUG=639841
==========

to

==========
Adding proper check for canvas type to
HTMLCanvasElement::getSourceImageForCanvas() such that
drawImage can correctly draw canvases with bitmap images.


BUG=639841
==========

[Justin Novosad, 2016-08-22 19:42:25]

https://codereview.chromium.org/2267653005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/2267653005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1105: if
(renderingContext()->getContextType() ==
CanvasRenderingContext::ContextImageBitmap)
Avoid explicit type checks when possible. In this case, you made getImage
virtual, so the type check is not necessary.
Should be:
RefPtr<SkImage> skImage;
RefPtr<Blink::Image> image = renderingContext()->getImage();
if (image)
   skImage = image->imageForCurrentFrame();
else
...

https://codereview.chromium.org/2267653005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h
(right):

https://codereview.chromium.org/2267653005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h:130: virtual
Image* getImage() const { return nullptr; }
Image is a ref counted type. You should not be returning a raw pointer, but
rather a PassRefPtr<Image>

https://codereview.chromium.org/2267653005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/imagebitmap/ImageBitmapRenderingContext.h
(right):

https://codereview.chromium.org/2267653005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/imagebitmap/ImageBitmapRenderingContext.h:40:
Image* getImage() const override { return m_image.get(); }
override -> final

[zakerinasab, 2016-08-22 19:55:50]

https://codereview.chromium.org/2267653005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/2267653005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1105: if
(renderingContext()->getContextType() ==
CanvasRenderingContext::ContextImageBitmap)
On 2016/08/22 19:42:25, Justin Novosad wrote:
> Avoid explicit type checks when possible. In this case, you made getImage
> virtual, so the type check is not necessary.
> Should be:
> RefPtr<SkImage> skImage;
> RefPtr<Blink::Image> image = renderingContext()->getImage();
> if (image)
>    skImage = image->imageForCurrentFrame();
> else
> ...

Done.

https://codereview.chromium.org/2267653005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h
(right):

https://codereview.chromium.org/2267653005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h:130: virtual
Image* getImage() const { return nullptr; }
On 2016/08/22 19:42:25, Justin Novosad wrote:
> Image is a ref counted type. You should not be returning a raw pointer, but
> rather a PassRefPtr<Image>

Done.

https://codereview.chromium.org/2267653005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/imagebitmap/ImageBitmapRenderingContext.h
(right):

https://codereview.chromium.org/2267653005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/imagebitmap/ImageBitmapRenderingContext.h:40:
Image* getImage() const override { return m_image.get(); }
On 2016/08/22 19:42:25, Justin Novosad wrote:
> override -> final

Done.

[Justin Novosad, 2016-08-22 20:03:02]

https://codereview.chromium.org/2267653005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/2267653005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1109: skImage =
buffer()->newSkImageSnapshot(hint, reason);
Just realized this code may inadvertently allocate a permanent ImageBuffer,
which should be avoided if possible.  Try this:
skImage = hasImageBuffer() ? buffer()->newSkImageSnapshot(hint, reason) :
createTransparentImage(size());

[zakerinasab, 2016-08-22 20:19:10]

https://codereview.chromium.org/2267653005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/2267653005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1109: skImage =
buffer()->newSkImageSnapshot(hint, reason);
On 2016/08/22 20:03:02, Justin Novosad wrote:
> Just realized this code may inadvertently allocate a permanent ImageBuffer,
> which should be avoided if possible.  Try this:
> skImage = hasImageBuffer() ? buffer()->newSkImageSnapshot(hint, reason) :
> createTransparentImage(size());

createTransparentImage(size()) returns PassRefPtr<Image>, so I added the call to
imageForCurrentFrame(). Regarding hasImageBuffer(), what is the expected
behavior in our case that we don't have an ImageBuffer to use and we get the
info from m_context? It still returns true.

[Justin Novosad, 2016-08-22 20:38:26]

On 2016/08/22 20:19:10, zakerinasab wrote:
>
https://codereview.chromium.org/2267653005/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):
> 
>
https://codereview.chromium.org/2267653005/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1109: skImage =
> buffer()->newSkImageSnapshot(hint, reason);
> On 2016/08/22 20:03:02, Justin Novosad wrote:
> > Just realized this code may inadvertently allocate a permanent ImageBuffer,
> > which should be avoided if possible.  Try this:
> > skImage = hasImageBuffer() ? buffer()->newSkImageSnapshot(hint, reason) :
> > createTransparentImage(size());
> 
> createTransparentImage(size()) returns PassRefPtr<Image>, so I added the call
to
> imageForCurrentFrame(). Regarding hasImageBuffer(), what is the expected
> behavior in our case that we don't have an ImageBuffer to use and we get the
> info from m_context? It still returns true.

The cases where hasImageBuffer will return false here are: 1) a 2D context that
is blanks (has not been drawn to), 2) an ImageBitmapRenderingContext that has no
image (transferFromImageBitmap was not yet called).
In these case we want to produce a transparent black image.

[Justin Novosad, 2016-08-22 20:38:39]

lgtm

[zakerinasab, 2016-08-22 20:41:00]

The CQ bit was checked by zakerinasab@chromium.org

[commit-bot: I haz the power, 2016-08-22 20:42:16]

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, 2016-08-22 22:45:20]

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

[commit-bot: I haz the power, 2016-08-22 22:45:20]

Try jobs failed on following builders:
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[zakerinasab, 2016-08-23 13:57:49]

The CQ bit was checked by zakerinasab@chromium.org

[commit-bot: I haz the power, 2016-08-23 13:58:04]

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

[zakerinasab, 2016-08-23 14:08:59]

The CQ bit was unchecked by zakerinasab@chromium.org

[zakerinasab, 2016-08-23 14:18:41]

The CQ bit was checked by zakerinasab@chromium.org

[commit-bot: I haz the power, 2016-08-23 14:18:58]

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, 2016-08-23 14:23:12]

Description was changed from

==========
Adding proper check for canvas type to
HTMLCanvasElement::getSourceImageForCanvas() such that
drawImage can correctly draw canvases with bitmap images.


BUG=639841
==========

to

==========
Adding proper check for canvas type to
HTMLCanvasElement::getSourceImageForCanvas() such that
drawImage can correctly draw canvases with bitmap images.


BUG=639841
==========

[commit-bot: I haz the power, 2016-08-23 14:23:14]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-08-23 14:25:16]

Description was changed from

==========
Adding proper check for canvas type to
HTMLCanvasElement::getSourceImageForCanvas() such that
drawImage can correctly draw canvases with bitmap images.


BUG=639841
==========

to

==========
Adding proper check for canvas type to
HTMLCanvasElement::getSourceImageForCanvas() such that
drawImage can correctly draw canvases with bitmap images.

BUG=639841

Committed: https://crrev.com/4d29ebec048d83aac139ec5edcbe22d107351b7b
Cr-Commit-Position: refs/heads/master@{#413731}
==========

[commit-bot: I haz the power, 2016-08-23 14:25:17]

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

[xidachen, 2016-08-23 14:53:16]

A revert of this CL (patchset #3 id:60001) has been created in
https://codereview.chromium.org/2269073002/ by xidachen@chromium.org.

The reason for reverting is: Causing this DCHECK:
m_context->getContextType() != CanvasRenderingContext::ContextImageBitmap

on this test:
transferFromImageBitmap-drawImage.html

.

[zakerinasab, 2016-08-23 17:07:07]

New patch submitted.

[zakerinasab, 2016-08-23 17:11:18]

Patchset #4 (id:80001) has been deleted

[zakerinasab, 2016-08-23 17:15:44]

Patchset #4 (id:100001) has been deleted

[zakerinasab, 2016-08-23 17:34:15]

Description was changed from

==========
Adding proper check for canvas type to
HTMLCanvasElement::getSourceImageForCanvas() such that
drawImage can correctly draw canvases with bitmap images.

BUG=639841

Committed: https://crrev.com/4d29ebec048d83aac139ec5edcbe22d107351b7b
Cr-Commit-Position: refs/heads/master@{#413731}
==========

to

==========
Adding proper check for canvas type to
HTMLCanvasElement::getSourceImageForCanvas() such that
drawImage can correctly draw canvases with bitmap images.

BUG=639841

Committed: https://crrev.com/4d29ebec048d83aac139ec5edcbe22d107351b7b
Cr-Commit-Position: refs/heads/master@{#413731}
==========

[Justin Novosad, 2016-08-23 18:47:48]

On 2016/08/23 17:07:07, zakerinasab wrote:
> New patch submitted.

lgtm

[zakerinasab, 2016-08-23 18:51:30]

The CQ bit was checked by zakerinasab@chromium.org

[commit-bot: I haz the power, 2016-08-23 18:51:56]

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, 2016-08-23 20:44:33]

Description was changed from

==========
Adding proper check for canvas type to
HTMLCanvasElement::getSourceImageForCanvas() such that
drawImage can correctly draw canvases with bitmap images.

BUG=639841

Committed: https://crrev.com/4d29ebec048d83aac139ec5edcbe22d107351b7b
Cr-Commit-Position: refs/heads/master@{#413731}
==========

to

==========
Adding proper check for canvas type to
HTMLCanvasElement::getSourceImageForCanvas() such that
drawImage can correctly draw canvases with bitmap images.

BUG=639841

Committed: https://crrev.com/4d29ebec048d83aac139ec5edcbe22d107351b7b
Cr-Commit-Position: refs/heads/master@{#413731}
==========

[commit-bot: I haz the power, 2016-08-23 20:44:35]

Committed patchset #4 (id:120001)

[commit-bot: I haz the power, 2016-08-23 20:46:43]

Description was changed from

==========
Adding proper check for canvas type to
HTMLCanvasElement::getSourceImageForCanvas() such that
drawImage can correctly draw canvases with bitmap images.

BUG=639841

Committed: https://crrev.com/4d29ebec048d83aac139ec5edcbe22d107351b7b
Cr-Commit-Position: refs/heads/master@{#413731}
==========

to

==========
Adding proper check for canvas type to
HTMLCanvasElement::getSourceImageForCanvas() such that
drawImage can correctly draw canvases with bitmap images.

BUG=639841

Committed: https://crrev.com/4d29ebec048d83aac139ec5edcbe22d107351b7b
Committed: https://crrev.com/d161245684a7adab144318deec691ad6fdc6251e
Cr-Original-Commit-Position: refs/heads/master@{#413731}
Cr-Commit-Position: refs/heads/master@{#413828}
==========

[commit-bot: I haz the power, 2016-08-23 20:46:44]

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