Reland of: Make OffscreenCanvas Transferable

Reland of: Make OffscreenCanvas Transferable

The previous CL is here:
https://codereview.chromium.org/1862033002/

Difference:
The OffscreenCanvas class is moved from modules/ to core/, which makes
this CL much simplier than the previous one.

BUG=563845
Committed: https://crrev.com/df4f2a03dc45a80e08d31bf51dab796ed16f7c41
Cr-Commit-Position: refs/heads/master@{#390171}

[xidachen, 2016-04-26 19:13:26]

Description was changed from

==========
Reland of: Make OffscreenCanvas Transferable

The previous CL is here:
https://codereview.chromium.org/1862033002/

Difference:
The OffscreenCanvas class is moved from modules/ to core/, which makes
this CL much simplier than the previous one.

BUG=563845
==========

to

==========
Reland of: Make OffscreenCanvas Transferable

The previous CL is here:
https://codereview.chromium.org/1862033002/

Difference:
The OffscreenCanvas class is moved from modules/ to core/, which makes
this CL much simplier than the previous one.

BUG=563845
==========

[xidachen, 2016-04-26 19:13:26]

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

[xidachen, 2016-04-26 19:17:09]

xidachen@chromium.org changed reviewers:
+ sigbjornf@opera.com

[xidachen, 2016-04-26 19:18:43]

junov@: In this CL, we change it such that an offscreenCanvas doesn't hold a
WeakMember<HTMLCanvasElement>, instead it keeps an int which represent the
DOMNodeId for that HTMLCanvasElement.

[sof, 2016-04-26 19:37:15]

transferables lgtm, this looks a lot better & avoids unnecessary complications
by being core/ based.

https://codereview.chromium.org/1917733004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-transferable-exceptions.html
(right):

https://codereview.chromium.org/1917733004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-transferable-exceptions.html:10:
window.jsTestIsAsync = true;
It looks like you can avoid making this test async.

https://codereview.chromium.org/1917733004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
(right):

https://codereview.chromium.org/1917733004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:159: if
(visited.contains(offscreenCanvases[i].get()))
If it really is legal/expected to have duplicates here, should this check be at
the top/first?

[Justin Novosad, 2016-04-26 20:08:32]

https://codereview.chromium.org/1917733004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-transferable-exceptions.html
(right):

https://codereview.chromium.org/1917733004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-transferable-exceptions.html:21:
try {
make this simpler and more readable by using "shouldThrow"

https://codereview.chromium.org/1917733004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-transferable-exceptions.html:30:
try {
shouldThrow

https://codereview.chromium.org/1917733004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-transferable-exceptions.html:37:
try {
shouldThrow

https://codereview.chromium.org/1917733004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-transferable-exceptions.html:44:
try {
shouldThrow

https://codereview.chromium.org/1917733004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp (right):

https://codereview.chromium.org/1917733004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp:46:
exceptionState.throwDOMException(InvalidStateError, "Cannot transfer an
ImageBitmap from a neutered OffscreenCanvas");
The spec has cleaned up it vocabulare and uses "detached" rather than "neutered"
now. We should do the same.
Message: "OffscreenCanvas object is detached"

https://codereview.chromium.org/1917733004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.h (right):

https://codereview.chromium.org/1917733004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.h:35: void
setAssociatedCanvasId(int canvasId) { m_canvasId = canvasId; }
I do not see any tests that verify that transfered OffscreenCanvas objects
correctly carry over their associated CanvasId.  Should you a unit test for
that, I think.

https://codereview.chromium.org/1917733004/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvasModules.cpp
(right):

https://codereview.chromium.org/1917733004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvasModules.cpp:16:
exceptionState.throwDOMException(InvalidStateError, "Cannot get context of a
neutered OffscreenCanvas");
Message: "OffscreenCanvas object is detached"

[xidachen, 2016-04-26 20:40:59]

https://codereview.chromium.org/1917733004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-transferable-exceptions.html
(right):

https://codereview.chromium.org/1917733004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-transferable-exceptions.html:10:
window.jsTestIsAsync = true;
On 2016/04/26 19:37:15, sof wrote:
> It looks like you can avoid making this test async.

Done.

https://codereview.chromium.org/1917733004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-transferable-exceptions.html:21:
try {
On 2016/04/26 20:08:31, Justin Novosad wrote:
> make this simpler and more readable by using "shouldThrow"

Done.

https://codereview.chromium.org/1917733004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-transferable-exceptions.html:30:
try {
On 2016/04/26 20:08:31, Justin Novosad wrote:
> shouldThrow

Done.

https://codereview.chromium.org/1917733004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-transferable-exceptions.html:37:
try {
On 2016/04/26 20:08:31, Justin Novosad wrote:
> shouldThrow

Done.

https://codereview.chromium.org/1917733004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-transferable-exceptions.html:44:
try {
On 2016/04/26 20:08:31, Justin Novosad wrote:
> shouldThrow

Done.

https://codereview.chromium.org/1917733004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
(right):

https://codereview.chromium.org/1917733004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:159: if
(visited.contains(offscreenCanvases[i].get()))
On 2016/04/26 19:37:15, sof wrote:
> If it really is legal/expected to have duplicates here, should this check be
at
> the top/first? 

Done.

https://codereview.chromium.org/1917733004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp (right):

https://codereview.chromium.org/1917733004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp:46:
exceptionState.throwDOMException(InvalidStateError, "Cannot transfer an
ImageBitmap from a neutered OffscreenCanvas");
On 2016/04/26 20:08:31, Justin Novosad wrote:
> The spec has cleaned up it vocabulare and uses "detached" rather than
"neutered"
> now. We should do the same.
> Message: "OffscreenCanvas object is detached"

Will have another CL that change the word "neutered" --> "detached" for the case
of ImageBitmap as well.

https://codereview.chromium.org/1917733004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.h (right):

https://codereview.chromium.org/1917733004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.h:35: void
setAssociatedCanvasId(int canvasId) { m_canvasId = canvasId; }
On 2016/04/26 20:08:32, Justin Novosad wrote:
> I do not see any tests that verify that transfered OffscreenCanvas objects
> correctly carry over their associated CanvasId.  Should you a unit test for
> that, I think.

Per offline discussion: this ID will be verified once we have the commit() flow
works.

https://codereview.chromium.org/1917733004/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvasModules.cpp
(right):

https://codereview.chromium.org/1917733004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvasModules.cpp:16:
exceptionState.throwDOMException(InvalidStateError, "Cannot get context of a
neutered OffscreenCanvas");
On 2016/04/26 20:08:32, Justin Novosad wrote:
> Message: "OffscreenCanvas object is detached"

Done.

[Justin Novosad, 2016-04-26 20:50:41]

On 2016/04/26 20:40:59, xidachen wrote:
> 
> Will have another CL that change the word "neutered" --> "detached" for the
case
> of ImageBitmap as well.
> 

FWIW, focus on strings that are visible to users. Having "neutered" in the C++
code as variable and function names is not a big deal.

[haraken, 2016-04-27 04:13:19]

This looks much simpler. LGTM on my side.

[sof, 2016-04-27 05:21:04]

https://codereview.chromium.org/1917733004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h (right):

https://codereview.chromium.org/1917733004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h:472: virtual
bool tryGetTransferredOffscreenCanvas(uint32_t index, uint32_t width, uint32_t
height, uint32_t id, v8::Local<v8::Value>*) = 0;
swap up one line so as to consistently sort this group of methods.

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

https://codereview.chromium.org/1917733004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.h:40:
CanvasRenderingContext* renderingContext() { return m_context.get(); }
redundant get()

[Justin Novosad, 2016-04-27 14:37:10]

lgtm core+modules, modulo sof's comments.

[xidachen, 2016-04-27 16:42:56]

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

[commit-bot: I haz the power, 2016-04-27 16:43:16]

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

[xidachen, 2016-04-27 17:26:21]

The CQ bit was unchecked by xidachen@chromium.org

[xidachen, 2016-04-27 20:35:47]

The CQ bit was checked by xidachen@chromium.org

[xidachen, 2016-04-27 20:35:48]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com,
haraken@chromium.org, junov@chromium.org
Link to the patchset: https://codereview.chromium.org/1917733004/#ps80001
(title: "address all comments")

[commit-bot: I haz the power, 2016-04-27 20:36:20]

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

[commit-bot: I haz the power, 2016-04-27 20:41:52]

Description was changed from

==========
Reland of: Make OffscreenCanvas Transferable

The previous CL is here:
https://codereview.chromium.org/1862033002/

Difference:
The OffscreenCanvas class is moved from modules/ to core/, which makes
this CL much simplier than the previous one.

BUG=563845
==========

to

==========
Reland of: Make OffscreenCanvas Transferable

The previous CL is here:
https://codereview.chromium.org/1862033002/

Difference:
The OffscreenCanvas class is moved from modules/ to core/, which makes
this CL much simplier than the previous one.

BUG=563845
==========

[commit-bot: I haz the power, 2016-04-27 20:41:53]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-30 17:12:41]

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

[commit-bot: I haz the power, 2016-04-30 17:51:40]

Description was changed from

==========
Reland of: Make OffscreenCanvas Transferable

The previous CL is here:
https://codereview.chromium.org/1862033002/

Difference:
The OffscreenCanvas class is moved from modules/ to core/, which makes
this CL much simplier than the previous one.

BUG=563845
==========

to

==========
Reland of: Make OffscreenCanvas Transferable

The previous CL is here:
https://codereview.chromium.org/1862033002/

Difference:
The OffscreenCanvas class is moved from modules/ to core/, which makes
this CL much simplier than the previous one.

BUG=563845

Committed: https://crrev.com/df4f2a03dc45a80e08d31bf51dab796ed16f7c41
Cr-Commit-Position: refs/heads/master@{#390171}
==========