canvas: fix bugs on HTMLCanvasElement::copiedImage()

canvas: fix bugs on HTMLCanvasElement::copiedImage()

HTMLCanvasElement::copiedImage() has two bugs:
1. Must remove copiedImage after swap buffer.
It's because copiedImage(BackBuffer) must return transparent image after swap buffer.

2. Must keep copiedImage of FrontBuffer and BackBuffer separately.
It's because there is a potential bug that HTMLCanvasElement::copiedImage(FrontBuffer)
returns BackBuffer, because m_copiedImages is used to cache both.
However the 2nd bug is hard to be reproduced because copiedImage(FrontBuffer) is
called by only CSSCanvasValue::image() and Editor::copyImage() and
copiedImage(BackBuffer) is called by only ImageBitmap ctor.

TEST=fast/canvas/canvas-createImageBitmap-webgl.html
BUG=438986
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187239

[dshwang, 2014-12-08 19:23:23]

dongseong.hwang@intel.com changed reviewers:
+ junov@chromium.org, kbr@chromium.org

[dshwang, 2014-12-08 19:23:24]

As The behavior change of Window.createImageBitmap(HTMLCanvasElement)
(https://codereview.chromium.org/776293002/) is rejected, I wanna fix
HTMLCanvasElement::copiedImage(SourceDrawingBuffer) bugs which is caused by
copiedImage(BackBuffer)

[Justin Novosad, 2014-12-08 19:54:27]

On 2014/12/08 19:23:24, dshwang wrote:
> As The behavior change of Window.createImageBitmap(HTMLCanvasElement)
> (https://codereview.chromium.org/776293002/) is rejected, I wanna fix
> HTMLCanvasElement::copiedImage(SourceDrawingBuffer) bugs which is caused by
> copiedImage(BackBuffer)

Is it really that helpful to have a vector of copied images as opposed to just
one?

[dshwang, 2014-12-08 20:17:09]

https://codereview.chromium.org/766333003/diff/40001/Source/core/html/HTMLCan...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/766333003/diff/40001/Source/core/html/HTMLCan...
Source/core/html/HTMLCanvasElement.cpp:715: needToUpdate |=
m_context->paintRenderingResultsToCanvas(sourceBuffer);
On 2014/12/08 19:54:27, junov wrote:
> On 2014/12/08 19:23:24, dshwang wrote:
> > As The behavior change of Window.createImageBitmap(HTMLCanvasElement)
> > (https://codereview.chromium.org/776293002/) is rejected, I wanna fix
> > HTMLCanvasElement::copiedImage(SourceDrawingBuffer) bugs which is caused by
> > copiedImage(BackBuffer)
> 
> Is it really that helpful to have a vector of copied images as opposed to just
> one?

That's good question! After re-thinking, vector is not needed because this line
lets HTMLCanvasElement::copiedImage know if it's needed to update m_copiedImage.

One thing to note is that m_copiedImage exists to avoid repeated image creation
in one frame, and code without Vector must create image every time if clients
interleave to call BackBuffer and FrontBuffer although it's very hard on current
code base.

[Justin Novosad, 2014-12-08 20:21:18]

On 2014/12/08 20:17:09, dshwang wrote:
>
https://codereview.chromium.org/766333003/diff/40001/Source/core/html/HTMLCan...
> File Source/core/html/HTMLCanvasElement.cpp (right):
> 
>
https://codereview.chromium.org/766333003/diff/40001/Source/core/html/HTMLCan...
> Source/core/html/HTMLCanvasElement.cpp:715: needToUpdate |=
> m_context->paintRenderingResultsToCanvas(sourceBuffer);
> On 2014/12/08 19:54:27, junov wrote:
> > On 2014/12/08 19:23:24, dshwang wrote:
> > > As The behavior change of Window.createImageBitmap(HTMLCanvasElement)
> > > (https://codereview.chromium.org/776293002/) is rejected, I wanna fix
> > > HTMLCanvasElement::copiedImage(SourceDrawingBuffer) bugs which is caused
by
> > > copiedImage(BackBuffer)
> > 
> > Is it really that helpful to have a vector of copied images as opposed to
just
> > one?
> 
> That's good question! After re-thinking, vector is not needed because this
line
> lets HTMLCanvasElement::copiedImage know if it's needed to update
m_copiedImage.
> 
> One thing to note is that m_copiedImage exists to avoid repeated image
creation
> in one frame, and code without Vector must create image every time if clients
> interleave to call BackBuffer and FrontBuffer although it's very hard on
current
> code base.

Yes, so the question is: In what cases would a canvas alternate between front
and back, and are such cases common or probable in the real world?

[Justin Novosad, 2014-12-08 20:26:04]

I have no further objections with change set 3. Will let kbr have final say.

[dshwang, 2014-12-08 20:35:18]

On 2014/12/08 20:21:18, junov wrote:
> On 2014/12/08 20:17:09, dshwang wrote:
> >
>
https://codereview.chromium.org/766333003/diff/40001/Source/core/html/HTMLCan...
> > File Source/core/html/HTMLCanvasElement.cpp (right):
> > 
> >
>
https://codereview.chromium.org/766333003/diff/40001/Source/core/html/HTMLCan...
> > Source/core/html/HTMLCanvasElement.cpp:715: needToUpdate |=
> > m_context->paintRenderingResultsToCanvas(sourceBuffer);
> > On 2014/12/08 19:54:27, junov wrote:
> > > On 2014/12/08 19:23:24, dshwang wrote:
> > > > As The behavior change of Window.createImageBitmap(HTMLCanvasElement)
> > > > (https://codereview.chromium.org/776293002/) is rejected, I wanna fix
> > > > HTMLCanvasElement::copiedImage(SourceDrawingBuffer) bugs which is caused
> by
> > > > copiedImage(BackBuffer)
> > > 
> > > Is it really that helpful to have a vector of copied images as opposed to
> just
> > > one?
> > 
> > That's good question! After re-thinking, vector is not needed because this
> line
> > lets HTMLCanvasElement::copiedImage know if it's needed to update
> m_copiedImage.
> > 
> > One thing to note is that m_copiedImage exists to avoid repeated image
> creation
> > in one frame, and code without Vector must create image every time if
clients
> > interleave to call BackBuffer and FrontBuffer although it's very hard on
> current
> > code base.
> 
> Yes, so the question is: In what cases would a canvas alternate between front
> and back, and are such cases common or probable in the real world?

As description mentioned, CSSCanvasValue::image() and Editor::copyImage() call
copiedImage(FrontBuffer) while ImageBitmap ctor calls copiedImage(BackBuffer)
So interleaving is hard, because ImageBitmap ctor is hardly called many times in
one frame.

[aandrey, 2014-12-08 20:36:46]

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

[aandrey, 2014-12-08 20:36:46]

FYI

https://codereview.chromium.org/766333003/diff/40001/Source/core/html/HTMLCan...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/766333003/diff/40001/Source/core/html/HTMLCan...
Source/core/html/HTMLCanvasElement.cpp:710: if (m_context->is2d())
Seems like this code has no effect (unless a context can be both 2d and 3d).

[dshwang, 2014-12-08 20:46:25]

https://codereview.chromium.org/766333003/diff/40001/Source/core/html/HTMLCan...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/766333003/diff/40001/Source/core/html/HTMLCan...
Source/core/html/HTMLCanvasElement.cpp:710: if (m_context->is2d())
On 2014/12/08 20:36:46, aandrey wrote:
> Seems like this code has no effect (unless a context can be both 2d and 3d).

That's right. It has no effect, because
CanvasRenderingContext2D::paintRenderingResultsToCanvas does nothing. I add it
for documentation purpose.

[Ken Russell (switch to Gerrit), 2014-12-11 01:35:57]

Thanks, this contribution LGTM overall. One comment. Please feel free to commit
once addressed.

https://codereview.chromium.org/766333003/diff/40001/LayoutTests/fast/canvas/...
File LayoutTests/fast/canvas/canvas-createImageBitmap-webgl.html (right):

https://codereview.chromium.org/766333003/diff/40001/LayoutTests/fast/canvas/...
LayoutTests/fast/canvas/canvas-createImageBitmap-webgl.html:56: debug("Check the
imageBitmap of webgl in the next frame. It must fail.")
Please rephrase this test so that the expectations don't contain failure
messages.

[dshwang, 2014-12-11 12:37:07]

On 2014/12/11 01:35:57, Ken Russell wrote:
> Thanks, this contribution LGTM overall. One comment. Please feel free to
commit
> once addressed.

Thank you for review!
I'll land it after fixing
https://code.google.com/p/chromium/issues/detail?id=440975

>
https://codereview.chromium.org/766333003/diff/40001/LayoutTests/fast/canvas/...
> File LayoutTests/fast/canvas/canvas-createImageBitmap-webgl.html (right):
> 
>
https://codereview.chromium.org/766333003/diff/40001/LayoutTests/fast/canvas/...
> LayoutTests/fast/canvas/canvas-createImageBitmap-webgl.html:56: debug("Check
the
> imageBitmap of webgl in the next frame. It must fail.")
> Please rephrase this test so that the expectations don't contain failure
> messages.

I'll update it.

[dshwang, 2014-12-16 09:40:56]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2014-12-16 09:41:14]

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

[commit-bot: I haz the power, 2014-12-16 10:46:54]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187239