Implement HTMLCanvasElement's transferControlToOffscreen

Implement HTMLCanvasElement's transferControlToOffscreen

To summarize, this function throws an exception when the canvas has an
associated context. A new OffscreenCanvas is created and it has a pointer
pointing to the originated canvas element.

A layout test is also added to make sure that there is no exception when
the original canvas doesn't have a context, and throws an exception when
the canvas does have a context.

BUG=598753
Committed: https://crrev.com/45e63d4ce6f4eb4c05fa27e1ece44ace00941924
Cr-Commit-Position: refs/heads/master@{#384627}

[xidachen, 2016-03-29 19:33:21]

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

[xidachen, 2016-03-29 19:33:22]

PTAL

[Justin Novosad, 2016-03-29 20:32:58]

https://codereview.chromium.org/1836283002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/canvas/HTMLCanvasElementModule.cpp
(right):

https://codereview.chromium.org/1836283002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/canvas/HTMLCanvasElementModule.cpp:23:
exceptionState.throwDOMException(InvalidStateError, "Cannot transfer control
from a canvas with context");
"with context" -> "that has a rendering context."  (don't forget the period)

https://codereview.chromium.org/1836283002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/canvas/HTMLCanvasElementModule.cpp:27:
offscreenCanvas->setOriginatedCanvas(&canvas);
Please add a unit test for this.  Test should create an HTMLCanvasElement (it's
a bit tricky see the first "paragraph" of CanvasRenderingContext2DTest::SetUp),
call this method and verify m_canvas.

https://codereview.chromium.org/1836283002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.h
(right):

https://codereview.chromium.org/1836283002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.h:42: void
setOriginatedCanvas(HTMLCanvasElement* canvas) { m_canvas = canvas; }
Originated -> Associated

[xidachen, 2016-03-30 13:32:30]

PTAL.

https://codereview.chromium.org/1836283002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/canvas/HTMLCanvasElementModule.cpp
(right):

https://codereview.chromium.org/1836283002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/canvas/HTMLCanvasElementModule.cpp:23:
exceptionState.throwDOMException(InvalidStateError, "Cannot transfer control
from a canvas with context");
On 2016/03/29 20:32:58, Justin Novosad wrote:
> "with context" -> "that has a rendering context."  (don't forget the period)

Done.

https://codereview.chromium.org/1836283002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/canvas/HTMLCanvasElementModule.cpp:27:
offscreenCanvas->setOriginatedCanvas(&canvas);
On 2016/03/29 20:32:58, Justin Novosad wrote:
> Please add a unit test for this.  Test should create an HTMLCanvasElement
(it's
> a bit tricky see the first "paragraph" of
CanvasRenderingContext2DTest::SetUp),
> call this method and verify m_canvas.

Done.

https://codereview.chromium.org/1836283002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.h
(right):

https://codereview.chromium.org/1836283002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.h:42: void
setOriginatedCanvas(HTMLCanvasElement* canvas) { m_canvas = canvas; }
On 2016/03/29 20:32:58, Justin Novosad wrote:
> Originated -> Associated

Done.

[Justin Novosad, 2016-03-30 19:51:06]

https://codereview.chromium.org/1836283002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp
(right):

https://codereview.chromium.org/1836283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp:724:
TEST_F(CanvasRenderingContext2DTest, TransferControlToOffscreen)
Good test, but wrong place for it. It has nothing to do with 2D contexts.  I
think a new source file is in order. Ideally it should be in the same module as
the code it tests.
third_party/WebKit/Source/modules/canvas/HTMLCanvasElementModuleTest.cpp would
make sense IMHO

[xidachen, 2016-03-31 13:35:57]

PTAL

[Justin Novosad, 2016-03-31 19:53:21]

lgtm

[xidachen, 2016-03-31 19:54:57]

The CQ bit was checked by xidachen@chromium.org

[commit-bot: I haz the power, 2016-03-31 19:55:50]

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

[commit-bot: I haz the power, 2016-03-31 20:13:30]

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

[commit-bot: I haz the power, 2016-03-31 20:13:31]

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

[xidachen, 2016-03-31 20:39:44]

xidachen@chromium.org changed reviewers:
+ haraken@chromium.org

[xidachen, 2016-03-31 20:39:45]

haraken@:
The bot is complaining that this CL is missing LGTM from an owner of
"modules/canvas", could you please take a look?

[haraken, 2016-03-31 23:24:24]

https://codereview.chromium.org/1836283002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.h
(right):

https://codereview.chromium.org/1836283002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.h:57:
WeakMember<HTMLCanvasElement> m_canvas;

Would you elaborate on why this is WeakMember rather than Member?

Is it possible that OffscreenCanvas outlives HTMLCanvasElement?

[xidachen, 2016-03-31 23:56:15]

Description was changed from

==========
Implement HTMLCanvasElement's transferControlToOffscreen

To summarize, this function throws an exception when the canvas has an
associated context. A new OffscreenCanvas is created and it has a pointer
pointing to the originated canvas element.

A layout test is also added to make sure that there is no exception when
the original canvas doesn't have a context, and throws an exception when
the canvas does have a context.

BUG=598753
==========

to

==========
Implement HTMLCanvasElement's transferControlToOffscreen

To summarize, this function throws an exception when the canvas has an
associated context. A new OffscreenCanvas is created and it has a pointer
pointing to the originated canvas element.

A layout test is also added to make sure that there is no exception when
the original canvas doesn't have a context, and throws an exception when
the canvas does have a context.

BUG=598753
==========

[xidachen, 2016-03-31 23:56:15]

xidachen@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org, sigbjornf@opera.com

[xidachen, 2016-04-01 00:01:02]

On 2016/03/31 23:24:24, haraken wrote:
>
https://codereview.chromium.org/1836283002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.h
> (right):
> 
>
https://codereview.chromium.org/1836283002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.h:57:
> WeakMember<HTMLCanvasElement> m_canvas;
> 
> Would you elaborate on why this is WeakMember rather than Member?
> 
> Is it possible that OffscreenCanvas outlives HTMLCanvasElement?

Very good question, I am adding sof@ and oilpan-reviewers to get more feedback.

Basically after this CL, I want to make OffscreenCanvas transferable, which is
part of the proposal here: https://wiki.whatwg.org/wiki/OffscreenCanvas

In my opinion, the way this would work is that we would destroy the
OffscreenCanvas object on the main thread, and construct a new OffscreenCanvas
object on worker. This newly constructed OffscreenCanvas object should have a
weakptr pointing to the HTMLCanvasElement that lives on the main thread. That is
why I am setting it as a WeakMember instead of Member because I believe a
WeakPtr is thread-safe, is that correct?

sof@ and oilpan-reviewers, comments please.

[haraken, 2016-04-01 00:03:45]

On 2016/04/01 00:01:02, xidachen wrote:
> On 2016/03/31 23:24:24, haraken wrote:
> >
>
https://codereview.chromium.org/1836283002/diff/60001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.h
> > (right):
> > 
> >
>
https://codereview.chromium.org/1836283002/diff/60001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.h:57:
> > WeakMember<HTMLCanvasElement> m_canvas;
> > 
> > Would you elaborate on why this is WeakMember rather than Member?
> > 
> > Is it possible that OffscreenCanvas outlives HTMLCanvasElement?
> 
> Very good question, I am adding sof@ and oilpan-reviewers to get more
feedback.
> 
> Basically after this CL, I want to make OffscreenCanvas transferable, which is
> part of the proposal here: https://wiki.whatwg.org/wiki/OffscreenCanvas
> 
> In my opinion, the way this would work is that we would destroy the
> OffscreenCanvas object on the main thread, and construct a new OffscreenCanvas
> object on worker. This newly constructed OffscreenCanvas object should have a
> weakptr pointing to the HTMLCanvasElement that lives on the main thread. That
is
> why I am setting it as a WeakMember instead of Member because I believe a
> WeakPtr is thread-safe, is that correct?
> 
> sof@ and oilpan-reviewers, comments please.

Both WeakMembers and Members are thread-safe. The key question is whether the
OffscreenCanvas can outlive the HTMLMediaElement or not.

[xidachen, 2016-04-01 00:09:38]

On 2016/04/01 00:03:45, haraken wrote:
> On 2016/04/01 00:01:02, xidachen wrote:
> > On 2016/03/31 23:24:24, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/1836283002/diff/60001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.h
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1836283002/diff/60001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.h:57:
> > > WeakMember<HTMLCanvasElement> m_canvas;
> > > 
> > > Would you elaborate on why this is WeakMember rather than Member?
> > > 
> > > Is it possible that OffscreenCanvas outlives HTMLCanvasElement?
> > 
> > Very good question, I am adding sof@ and oilpan-reviewers to get more
> feedback.
> > 
> > Basically after this CL, I want to make OffscreenCanvas transferable, which
is
> > part of the proposal here: https://wiki.whatwg.org/wiki/OffscreenCanvas
> > 
> > In my opinion, the way this would work is that we would destroy the
> > OffscreenCanvas object on the main thread, and construct a new
OffscreenCanvas
> > object on worker. This newly constructed OffscreenCanvas object should have
a
> > weakptr pointing to the HTMLCanvasElement that lives on the main thread.
That
> is
> > why I am setting it as a WeakMember instead of Member because I believe a
> > WeakPtr is thread-safe, is that correct?
> > 
> > sof@ and oilpan-reviewers, comments please.
> 
> Both WeakMembers and Members are thread-safe. The key question is whether the
> OffscreenCanvas can outlive the HTMLMediaElement or not.

I believe that OffscreenCanvas should not outlive its associated
HTMLCanvasElement, but I am not 100% certain. 
junov@: are there cases that OffscreenCanvas would outlive its
HTMLCanvasElement?

[Justin Novosad, 2016-04-01 14:17:05]

On 2016/04/01 00:09:38, xidachen wrote:

> I believe that OffscreenCanvas should not outlive its associated
> HTMLCanvasElement, but I am not 100% certain. 
> junov@: are there cases that OffscreenCanvas would outlive its
> HTMLCanvasElement?

I disagree. Once an HTMLCanvasElement is no longer referenced (by the DOM or in
JavaScript), the OffscreenCanvas may still be referenced. We have no control
over that, it is up to the web developer. When this happens, there is no reason
for the OffscreenCanvas to force the HTMLCanvasElement to stay alive through a
strong reference. Since the HTMLCanvasElement is no longer a useful placeholder
for the OffscreenCanvas's content, the reference has become useless. I think
WeakMember is the way to go here.  There are no user-observable side-effects to
this because the OffscreenCanvas interface does not expose its associated canvas
element.

This is in contrast to the relation that exists between RenderingContexts and
canvas elements where both objects keep the other alive, which is necessary as
per the spec. That constraint does not exist with OffscreenCanvas.

[haraken, 2016-04-01 14:28:44]

On 2016/04/01 14:17:05, Justin Novosad wrote:
> On 2016/04/01 00:09:38, xidachen wrote:
> 
> > I believe that OffscreenCanvas should not outlive its associated
> > HTMLCanvasElement, but I am not 100% certain. 
> > junov@: are there cases that OffscreenCanvas would outlive its
> > HTMLCanvasElement?
> 
> I disagree. Once an HTMLCanvasElement is no longer referenced (by the DOM or
in
> JavaScript), the OffscreenCanvas may still be referenced. We have no control
> over that, it is up to the web developer. When this happens, there is no
reason
> for the OffscreenCanvas to force the HTMLCanvasElement to stay alive through a
> strong reference. Since the HTMLCanvasElement is no longer a useful
placeholder
> for the OffscreenCanvas's content, the reference has become useless. I think
> WeakMember is the way to go here.  There are no user-observable side-effects
to
> this because the OffscreenCanvas interface does not expose its associated
canvas
> element.
> 
> This is in contrast to the relation that exists between RenderingContexts and
> canvas elements where both objects keep the other alive, which is necessary as
> per the spec. That constraint does not exist with OffscreenCanvas.

Makes sense.

https://codereview.chromium.org/1836283002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.h
(right):

https://codereview.chromium.org/1836283002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.h:43:
HTMLCanvasElement* getAssociatedCanvas() const { return m_canvas; }

Is this method used only by testing code? Or do you plan to use the method in
some places in a follow-up CL?

[xidachen, 2016-04-01 14:46:02]

https://codereview.chromium.org/1836283002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.h
(right):

https://codereview.chromium.org/1836283002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.h:43:
HTMLCanvasElement* getAssociatedCanvas() const { return m_canvas; }
On 2016/04/01 14:28:44, haraken wrote:
> 
> Is this method used only by testing code? Or do you plan to use the method in
> some places in a follow-up CL?

I believe that this method will be needed in a future CL when making the
OffscreenCanvas transferable. What I have in mind is something like
HeapVector<WeakMember<HTMLCanvasElement>, 1> and this HeapVector stores pointers
to the m_canvas. Does that make sense?

[haraken, 2016-04-01 14:57:56]

Makes sense. LGTM.

https://codereview.chromium.org/1836283002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.h
(right):

https://codereview.chromium.org/1836283002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.h:43:
HTMLCanvasElement* getAssociatedCanvas() const { return m_canvas; }
On 2016/04/01 14:46:02, xidachen wrote:
> On 2016/04/01 14:28:44, haraken wrote:
> > 
> > Is this method used only by testing code? Or do you plan to use the method
in
> > some places in a follow-up CL?
> 
> I believe that this method will be needed in a future CL when making the
> OffscreenCanvas transferable. What I have in mind is something like
> HeapVector<WeakMember<HTMLCanvasElement>, 1> and this HeapVector stores
pointers
> to the m_canvas. Does that make sense?

You need to HeapHashMap since HeapVector cannot contain weak members though.

[xidachen, 2016-04-01 15:32:00]

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

[commit-bot: I haz the power, 2016-04-01 15:32:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1836283002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1836283002/80001

[xidachen, 2016-04-01 16:16:08]

The CQ bit was unchecked by xidachen@chromium.org

[xidachen, 2016-04-01 16:16:35]

The CQ bit was checked by xidachen@chromium.org

[xidachen, 2016-04-01 16:16:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, junov@chromium.org
Link to the patchset: https://codereview.chromium.org/1836283002/#ps80001
(title: "update interface layout tests result")

[commit-bot: I haz the power, 2016-04-01 16:16:47]

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

[commit-bot: I haz the power, 2016-04-01 17:59:51]

Description was changed from

==========
Implement HTMLCanvasElement's transferControlToOffscreen

To summarize, this function throws an exception when the canvas has an
associated context. A new OffscreenCanvas is created and it has a pointer
pointing to the originated canvas element.

A layout test is also added to make sure that there is no exception when
the original canvas doesn't have a context, and throws an exception when
the canvas does have a context.

BUG=598753
==========

to

==========
Implement HTMLCanvasElement's transferControlToOffscreen

To summarize, this function throws an exception when the canvas has an
associated context. A new OffscreenCanvas is created and it has a pointer
pointing to the originated canvas element.

A layout test is also added to make sure that there is no exception when
the original canvas doesn't have a context, and throws an exception when
the canvas does have a context.

BUG=598753
==========

[commit-bot: I haz the power, 2016-04-01 17:59:52]

Committed patchset #5 (id:80001)

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

Description was changed from

==========
Implement HTMLCanvasElement's transferControlToOffscreen

To summarize, this function throws an exception when the canvas has an
associated context. A new OffscreenCanvas is created and it has a pointer
pointing to the originated canvas element.

A layout test is also added to make sure that there is no exception when
the original canvas doesn't have a context, and throws an exception when
the canvas does have a context.

BUG=598753
==========

to

==========
Implement HTMLCanvasElement's transferControlToOffscreen

To summarize, this function throws an exception when the canvas has an
associated context. A new OffscreenCanvas is created and it has a pointer
pointing to the originated canvas element.

A layout test is also added to make sure that there is no exception when
the original canvas doesn't have a context, and throws an exception when
the canvas does have a context.

BUG=598753

Committed: https://crrev.com/45e63d4ce6f4eb4c05fa27e1ece44ace00941924
Cr-Commit-Position: refs/heads/master@{#384627}
==========

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

Patchset 5 (id:??) landed as
https://crrev.com/45e63d4ce6f4eb4c05fa27e1ece44ace00941924
Cr-Commit-Position: refs/heads/master@{#384627}