Make OffscreenCanvas Transferable

Make OffscreenCanvas Transferable

At this moment, an offscreenCanvas could have an associated HTMLCanvasElement,
and we maintain a WeakPtr to that. Per discussion with esprehn@,
we change the strategy such that an offscreenCanvas holds a int which
is the DOMNodeId for its associated HTMLCanvasElement.

This change makes offscreenCanvas transferable much easier. In particular,
we just need to pass three int to deserializer, which are width, height
and DOMNodeId of for an offscreenCanvas.

A kayout test is included to test its correctness. The layout test
includes several things:
1). Transfer an offscreenCanvas to worker and transfer it back to main,
the width and height attributes should not change.
2). After transfer, the width/height should be 0.
3). Transfer a neutered offscreenCanvas should throw an exception.
4). After getting the offscreenCanvas back from the worker, call getContext('2d')
should work just fine.
5). Transfer an offscreenCanvas after calling getContext('2d')
should throw an exception.

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

[xidachen, 2016-04-06 12:49:20]

Description was changed from

==========
Make OffscreenCanvas Transferable

Will put in more detailed description here

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

to

==========
WIP: Make OffscreenCanvas Transferable

Will put in more detailed description here

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

[xidachen, 2016-04-12 14:20:06]

Description was changed from

==========
WIP: Make OffscreenCanvas Transferable

Will put in more detailed description here

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

to

==========
Make OffscreenCanvas Transferable

At this moment, an offscreenCanvas always has an associated HTMLCanvasElement,
and we maintain a WeakPtr to that. Per discussion with esprehn@, 
we change the strategy such that an offscreenCanvas holds a int which
is the DOMNodeId for its associated HTMLCanvasElement.

This change makes offscreenCanvas transferable much easier. In particular,
we just need to pass three int to deserializer, which are width, height
and DOMNodeId of for an offscreenCanvas.

Layout tests are included to test its correctness.

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

[xidachen, 2016-04-12 14:20:06]

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

[xidachen, 2016-04-12 15:59:01]

Description was changed from

==========
Make OffscreenCanvas Transferable

At this moment, an offscreenCanvas always has an associated HTMLCanvasElement,
and we maintain a WeakPtr to that. Per discussion with esprehn@, 
we change the strategy such that an offscreenCanvas holds a int which
is the DOMNodeId for its associated HTMLCanvasElement.

This change makes offscreenCanvas transferable much easier. In particular,
we just need to pass three int to deserializer, which are width, height
and DOMNodeId of for an offscreenCanvas.

Layout tests are included to test its correctness.

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

to

==========
Make OffscreenCanvas Transferable

At this moment, an offscreenCanvas always has an associated HTMLCanvasElement,
and we maintain a WeakPtr to that. Per discussion with esprehn@, 
we change the strategy such that an offscreenCanvas holds a int which
is the DOMNodeId for its associated HTMLCanvasElement.

This change makes offscreenCanvas transferable much easier. In particular,
we just need to pass three int to deserializer, which are width, height
and DOMNodeId of for an offscreenCanvas.

A kayout test is included to test its correctness. The layout test
includes several things:
1). Transfer an offscreenCanvas to worker and transfer it back to main,
the width and height attributes should not change.
2). After transfer, the width/height should be 0.
3). Transfer a neutered offscreenCanvas should throw an exception.
4). After getting the offscreenCanvas back from the worker, call
getContext('2d')
should work just fine.
5). Transfer an offscreenCanvas after calling getContext('2d')
should throw an exception.

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

[xidachen, 2016-04-12 17:19:32]

PTAL.

[xidachen, 2016-04-12 17:21:37]

Description was changed from

==========
Make OffscreenCanvas Transferable

At this moment, an offscreenCanvas always has an associated HTMLCanvasElement,
and we maintain a WeakPtr to that. Per discussion with esprehn@, 
we change the strategy such that an offscreenCanvas holds a int which
is the DOMNodeId for its associated HTMLCanvasElement.

This change makes offscreenCanvas transferable much easier. In particular,
we just need to pass three int to deserializer, which are width, height
and DOMNodeId of for an offscreenCanvas.

A kayout test is included to test its correctness. The layout test
includes several things:
1). Transfer an offscreenCanvas to worker and transfer it back to main,
the width and height attributes should not change.
2). After transfer, the width/height should be 0.
3). Transfer a neutered offscreenCanvas should throw an exception.
4). After getting the offscreenCanvas back from the worker, call
getContext('2d')
should work just fine.
5). Transfer an offscreenCanvas after calling getContext('2d')
should throw an exception.

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

to

==========
Make OffscreenCanvas Transferable

At this moment, an offscreenCanvas could have an associated HTMLCanvasElement,
and we maintain a WeakPtr to that. Per discussion with esprehn@, 
we change the strategy such that an offscreenCanvas holds a int which
is the DOMNodeId for its associated HTMLCanvasElement.

This change makes offscreenCanvas transferable much easier. In particular,
we just need to pass three int to deserializer, which are width, height
and DOMNodeId of for an offscreenCanvas.

A kayout test is included to test its correctness. The layout test
includes several things:
1). Transfer an offscreenCanvas to worker and transfer it back to main,
the width and height attributes should not change.
2). After transfer, the width/height should be 0.
3). Transfer a neutered offscreenCanvas should throw an exception.
4). After getting the offscreenCanvas back from the worker, call
getContext('2d')
should work just fine.
5). Transfer an offscreenCanvas after calling getContext('2d')
should throw an exception.

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

[Justin Novosad, 2016-04-12 18:10:23]

https://codereview.chromium.org/1862033002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-transferable.html
(right):

https://codereview.chromium.org/1862033002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-transferable.html:44:
worker.postMessage({data: newOffscreenCanvas}, [newOffscreenCanvas]);
This test nested in a test is hard to follow.  I do not see why this need to be
invoked from within the onmessage handler, which is like a weird recursion,
except that it isn't.

https://codereview.chromium.org/1862033002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.cpp
(right):

https://codereview.chromium.org/1862033002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.cpp:41:
m_isNeutered = true;
Add "ASSERT(!m_context);"

https://codereview.chromium.org/1862033002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.cpp:42:
m_size.setWidth(0);
Why do we need to reset m_size? This has observable side-effects in JS.

https://codereview.chromium.org/1862033002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.cpp:46:
OffscreenCanvasRenderingContext2D* OffscreenCanvas::getContext(const String& id,
const CanvasContextCreationAttributes& attributes)
This method need to throw an InvalidStateError exception if the object is
neutered. And there should be a test to cover this case.

https://codereview.chromium.org/1862033002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.cpp:73:
ImageBitmap* OffscreenCanvas::transferToImageBitmap(ExceptionState&
exceptionState)
Same here: throw if neutered + test.

https://codereview.chromium.org/1862033002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.cpp:87:
OffscreenCanvasRenderingContext2D* OffscreenCanvas::renderingContext() const
ASSERT(!m_isNeutered);

https://codereview.chromium.org/1862033002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.h
(right):

https://codereview.chromium.org/1862033002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.h:59: int
m_canvasId;
This field needs an initial value since it is not set by the constructor.
Ideally it should be a named constant that is not a valid node id.

[xidachen, 2016-04-12 19:35:13]

xidachen@chromium.org changed reviewers:
+ ajuma@chromium.org, xlai@chromium.org

[xidachen, 2016-04-12 19:35:14]

https://codereview.chromium.org/1862033002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-transferable.html
(right):

https://codereview.chromium.org/1862033002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-transferable.html:44:
worker.postMessage({data: newOffscreenCanvas}, [newOffscreenCanvas]);
On 2016/04/12 18:10:23, Justin Novosad wrote:
> This test nested in a test is hard to follow.  I do not see why this need to
be
> invoked from within the onmessage handler, which is like a weird recursion,
> except that it isn't.

Good point. The postMessage is not appropriate here. I made changes to this
layout test such that it doesn't include any exception case. All the exception
cases are tested in a separate layout test.

https://codereview.chromium.org/1862033002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.cpp
(right):

https://codereview.chromium.org/1862033002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.cpp:41:
m_isNeutered = true;
On 2016/04/12 18:10:23, Justin Novosad wrote:
> Add "ASSERT(!m_context);"

Done.

https://codereview.chromium.org/1862033002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.cpp:42:
m_size.setWidth(0);
On 2016/04/12 18:10:23, Justin Novosad wrote:
> Why do we need to reset m_size? This has observable side-effects in JS.

When an offscreenCanvas gets transferred to worker, it is neutered, shouldn't
its size be 0?

https://codereview.chromium.org/1862033002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.cpp:46:
OffscreenCanvasRenderingContext2D* OffscreenCanvas::getContext(const String& id,
const CanvasContextCreationAttributes& attributes)
On 2016/04/12 18:10:23, Justin Novosad wrote:
> This method need to throw an InvalidStateError exception if the object is
> neutered. And there should be a test to cover this case.

Done.

https://codereview.chromium.org/1862033002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.cpp:73:
ImageBitmap* OffscreenCanvas::transferToImageBitmap(ExceptionState&
exceptionState)
On 2016/04/12 18:10:23, Justin Novosad wrote:
> Same here: throw if neutered + test.

Done.

https://codereview.chromium.org/1862033002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.cpp:87:
OffscreenCanvasRenderingContext2D* OffscreenCanvas::renderingContext() const
On 2016/04/12 18:10:23, Justin Novosad wrote:
> ASSERT(!m_isNeutered);

Done.

https://codereview.chromium.org/1862033002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.h
(right):

https://codereview.chromium.org/1862033002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.h:59: int
m_canvasId;
On 2016/04/12 18:10:23, Justin Novosad wrote:
> This field needs an initial value since it is not set by the constructor.
> Ideally it should be a named constant that is not a valid node id.

This is actually initialized in the constructor. But it makes more sense to
initialize here.

[Justin Novosad, 2016-04-18 19:46:27]

lgtm for modules/canvas and LayoutTests

[haraken, 2016-04-19 00:48:11]

https://codereview.chromium.org/1862033002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/TransferableExtractor.h (right):

https://codereview.chromium.org/1862033002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/TransferableExtractor.h:16: class
CORE_EXPORT TransferableExtractor {

Do you have a plan to add more extractors? It looks a bit overkilling to
introduce this class only for TransferableOffscreenCanvasExtractor.

If you only need the TransferableOffscreenCanvasExtractor, can you register the
extractor on SerializedScriptValueForModules (and use the extractor in
SerializedScriptValueForModules)?

https://codereview.chromium.org/1862033002/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp
(right):

https://codereview.chromium.org/1862033002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp:77:
static v8::Local<v8::Object> toV8Object(OffscreenCanvas* impl,
v8::Local<v8::Object> creationContext, v8::Isolate* isolate)

Why do you need this helper method? Can't you simply use toV8()?

https://codereview.chromium.org/1862033002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp:704:
m_offscreenCanvases.append(result);

Where are the appended results used?

https://codereview.chromium.org/1862033002/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/modules/v8/SerializedScriptValueForModulesFactory.cpp
(right):

https://codereview.chromium.org/1862033002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/SerializedScriptValueForModulesFactory.cpp:71:
for (size_t i = 0; i < offscreenCanvases.size(); i++) {

Can we avoid iterating over offscreenCanvases twice?

[haraken, 2016-04-19 06:22:03]

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

[haraken, 2016-04-19 06:22:04]

+Sigbjorn

[xidachen, 2016-04-19 10:59:14]

On 2016/04/19 06:22:04, haraken wrote:
> +Sigbjorn

This CL is still in progress.

After sof's CL, I had to make more changes. In particular, we cannot do stack
allocation like this:
Transferables transferables;

Because this statement could be in core/, and transferables has no info about
OffscreenCanvas which lives under modules/.

[xidachen, 2016-04-19 15:29:51]

PS#15 should work.

https://codereview.chromium.org/1862033002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/TransferableExtractor.h (right):

https://codereview.chromium.org/1862033002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/TransferableExtractor.h:16: class
CORE_EXPORT TransferableExtractor {
On 2016/04/19 00:48:10, haraken wrote:
> 
> Do you have a plan to add more extractors? It looks a bit overkilling to
> introduce this class only for TransferableOffscreenCanvasExtractor.
> 
> If you only need the TransferableOffscreenCanvasExtractor, can you register
the
> extractor on SerializedScriptValueForModules (and use the extractor in
> SerializedScriptValueForModules)?


The reason to have this class here is that we cannot foresee the future. Just
like when we add the ArrayBuffer and ImageBitmap to be transferable, we should
have created a Transferables class at that time.

As worker gets more and more popular, I certainly believe that there will be
other transferable types in the future because transfer an object involves zero
copy. These transferable types could live under modules/. So this
TransferableExtractor provides a place holder for all types in modules that are
to be "transferable" in the future.

https://codereview.chromium.org/1862033002/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp
(right):

https://codereview.chromium.org/1862033002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp:77:
static v8::Local<v8::Object> toV8Object(OffscreenCanvas* impl,
v8::Local<v8::Object> creationContext, v8::Isolate* isolate)
On 2016/04/19 00:48:10, haraken wrote:
> 
> Why do you need this helper method? Can't you simply use toV8()?

Simplified. This helper function is now removed.

https://codereview.chromium.org/1862033002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp:704:
m_offscreenCanvases.append(result);
On 2016/04/19 00:48:10, haraken wrote:
> 
> Where are the appended results used?
> 

Turns out m_offscreenCanvases isn't needed, removed. Thanks.

https://codereview.chromium.org/1862033002/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/modules/v8/SerializedScriptValueForModulesFactory.cpp
(right):

https://codereview.chromium.org/1862033002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/SerializedScriptValueForModulesFactory.cpp:71:
for (size_t i = 0; i < offscreenCanvases.size(); i++) {
On 2016/04/19 00:48:10, haraken wrote:
> 
> Can we avoid iterating over offscreenCanvases twice?

Done.

[haraken, 2016-04-20 01:18:24]

https://codereview.chromium.org/1862033002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/TransferableExtractor.h (right):

https://codereview.chromium.org/1862033002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/TransferableExtractor.h:16: class
CORE_EXPORT TransferableExtractor {
On 2016/04/19 15:29:50, xidachen wrote:
> On 2016/04/19 00:48:10, haraken wrote:
> > 
> > Do you have a plan to add more extractors? It looks a bit overkilling to
> > introduce this class only for TransferableOffscreenCanvasExtractor.
> > 
> > If you only need the TransferableOffscreenCanvasExtractor, can you register
> the
> > extractor on SerializedScriptValueForModules (and use the extractor in
> > SerializedScriptValueForModules)?
> 
> 
> The reason to have this class here is that we cannot foresee the future. Just
> like when we add the ArrayBuffer and ImageBitmap to be transferable, we should
> have created a Transferables class at that time.
> 
> As worker gets more and more popular, I certainly believe that there will be
> other transferable types in the future because transfer an object involves
zero
> copy. These transferable types could live under modules/. So this
> TransferableExtractor provides a place holder for all types in modules that
are
> to be "transferable" in the future.

If you don't have a concrete plan now, I'd prefer keeping the code as simple as
possible. We can generalize the mechanism when we actually need it.

[xidachen, 2016-04-20 17:16:28]

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
(right):

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:235: if
(!SerializedScriptValueFactory::instance().extractTransferables(isolate,
transferables, exceptionState, transferableObject, i)) {
Becaseu extractTransferables() is a static method so I cannot make it virtual.

I took the same approach as SerializedScriptValue::deserialize(), which calls
SerializedScriptValueFactory::instance().deserialize(). In here, I call
SerializedScriptValueFactory::instance().extractTransferables().

With that, the extractor and offscreenCanvasExtractor classes are removed.

[haraken, 2016-04-21 00:29:03]

LGTM with comments.

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
(right):

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:235: if
(!SerializedScriptValueFactory::instance().extractTransferables(isolate,
transferables, exceptionState, transferableObject, i)) {
On 2016/04/20 17:16:28, xidachen wrote:
> Becaseu extractTransferables() is a static method so I cannot make it virtual.
> 
> I took the same approach as SerializedScriptValue::deserialize(), which calls
> SerializedScriptValueFactory::instance().deserialize(). In here, I call
> SerializedScriptValueFactory::instance().extractTransferables().
> 
> With that, the extractor and offscreenCanvasExtractor classes are removed.

Nice refactoring.

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/Transferables.h (right):

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/Transferables.h:35: static void
registerFactory(Factory f) { s_registeredFactory = f; }

f => factory

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp
(right):

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp:87:
v8::Local<v8::Object> v8OffscreenCanvas = v8::Local<v8::Object>();

' = v8::Local<v8::Object>()' is not needed.

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp:89:
v8::Local<v8::Value> wrapper = toV8(offscreenCanvases[i].get(), creationContext,
isolate());

You can use:

  toV8(offscreenCanvases[i].get(), scriptState);

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp:91:
v8OffscreenCanvas = wrapper.As<v8::Object>();

if (wrapper.IsEmpty()) {
  v8OffscreenCanvas = wrapper.As<v8::Object>();
  if (!m_transferredOffscreenCanvas.contains(v8OffscreenCanvas))
    m_transferredOffscreenCanvas.set(v8OffscreenCanvas, i);
}

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp:347:
return 0;

nullptr

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp:353:
return 0;

nullptr

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp:696:
result = toV8(offscreenCanvas, creationContext, isolate);

Use:

  toV8(offscreenCanvas, scriptState);

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp:700:
*object = result;

*object = toV8(...) would be enough. result won't be needed.

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/modules/v8/SerializedScriptValueForModulesFactory.cpp
(right):

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/SerializedScriptValueForModulesFactory.cpp:105:
bool SerializedScriptValueForModulesFactory::extractTransferables(v8::Isolate*
isolate, Transferables& transferables, ExceptionState& exceptionState,
v8::Local<v8::Value>& transferableObject, unsigned i)

i => index

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/modules/v8/SerializedScriptValueForModulesFactory.h
(right):

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/SerializedScriptValueForModulesFactory.h:28:
void transferData(SerializedScriptValue*, SerializedScriptValueWriter&,
Transferables*, ExceptionState&, v8::Isolate*) override;

Make the parameter order be consistent with extractTransferables.

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/ModulesInitializer.cpp (right):

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/ModulesInitializer.cpp:58:
Transferables::registerFactory([]() -> Transferables* {

Move this method into ModuleBindingsInitializer::init.

Also use the same pattern as SerializedScriptValueFactory::initialize (and avoid
using a lambda function).

[xidachen, 2016-04-21 14:34:04]

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

[commit-bot: I haz the power, 2016-04-21 14:34:34]

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

[xidachen, 2016-04-21 15:31:49]

The CQ bit was unchecked by xidachen@chromium.org

[xidachen, 2016-04-21 15:32:49]

haraken@: all comments are addressed. Bots seem to be happy. Committing now.

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp
(right):

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp:87:
v8::Local<v8::Object> v8OffscreenCanvas = v8::Local<v8::Object>();
On 2016/04/21 00:29:03, haraken wrote:
> 
> ' = v8::Local<v8::Object>()' is not needed.

Done.

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp:89:
v8::Local<v8::Value> wrapper = toV8(offscreenCanvases[i].get(), creationContext,
isolate());
On 2016/04/21 00:29:02, haraken wrote:
> 
> You can use:
> 
>   toV8(offscreenCanvases[i].get(), scriptState);

Done.

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp:91:
v8OffscreenCanvas = wrapper.As<v8::Object>();
On 2016/04/21 00:29:03, haraken wrote:
> 
> if (wrapper.IsEmpty()) {
>   v8OffscreenCanvas = wrapper.As<v8::Object>();
>   if (!m_transferredOffscreenCanvas.contains(v8OffscreenCanvas))
>     m_transferredOffscreenCanvas.set(v8OffscreenCanvas, i);
> }

Done.

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp:347:
return 0;
On 2016/04/21 00:29:02, haraken wrote:
> 
> nullptr

Done.

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp:353:
return 0;
On 2016/04/21 00:29:02, haraken wrote:
> 
> nullptr

Done.

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp:696:
result = toV8(offscreenCanvas, creationContext, isolate);
On 2016/04/21 00:29:03, haraken wrote:
> 
> Use:
> 
>   toV8(offscreenCanvas, scriptState);
> 

Done.

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp:700:
*object = result;
On 2016/04/21 00:29:03, haraken wrote:
> 
> *object = toV8(...) would be enough. result won't be needed.

Done.

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/modules/v8/SerializedScriptValueForModulesFactory.cpp
(right):

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/SerializedScriptValueForModulesFactory.cpp:105:
bool SerializedScriptValueForModulesFactory::extractTransferables(v8::Isolate*
isolate, Transferables& transferables, ExceptionState& exceptionState,
v8::Local<v8::Value>& transferableObject, unsigned i)
On 2016/04/21 00:29:03, haraken wrote:
> 
> i => index

Done.

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/modules/v8/SerializedScriptValueForModulesFactory.h
(right):

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/SerializedScriptValueForModulesFactory.h:28:
void transferData(SerializedScriptValue*, SerializedScriptValueWriter&,
Transferables*, ExceptionState&, v8::Isolate*) override;
On 2016/04/21 00:29:03, haraken wrote:
> 
> Make the parameter order be consistent with extractTransferables.

Done.

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/ModulesInitializer.cpp (right):

https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/ModulesInitializer.cpp:58:
Transferables::registerFactory([]() -> Transferables* {
On 2016/04/21 00:29:03, haraken wrote:
> 
> Move this method into ModuleBindingsInitializer::init.
> 
> Also use the same pattern as SerializedScriptValueFactory::initialize (and
avoid
> using a lambda function).

Done.

[xidachen, 2016-04-21 15:56:21]

The CQ bit was checked by xidachen@chromium.org

[xidachen, 2016-04-21 15:56:22]

The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1862033002/#ps380001
(title: "no need to register, taking the same approach as extractTransferables")

[commit-bot: I haz the power, 2016-04-21 15:56:32]

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

[commit-bot: I haz the power, 2016-04-21 16:02:46]

Description was changed from

==========
Make OffscreenCanvas Transferable

At this moment, an offscreenCanvas could have an associated HTMLCanvasElement,
and we maintain a WeakPtr to that. Per discussion with esprehn@, 
we change the strategy such that an offscreenCanvas holds a int which
is the DOMNodeId for its associated HTMLCanvasElement.

This change makes offscreenCanvas transferable much easier. In particular,
we just need to pass three int to deserializer, which are width, height
and DOMNodeId of for an offscreenCanvas.

A kayout test is included to test its correctness. The layout test
includes several things:
1). Transfer an offscreenCanvas to worker and transfer it back to main,
the width and height attributes should not change.
2). After transfer, the width/height should be 0.
3). Transfer a neutered offscreenCanvas should throw an exception.
4). After getting the offscreenCanvas back from the worker, call
getContext('2d')
should work just fine.
5). Transfer an offscreenCanvas after calling getContext('2d')
should throw an exception.

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

to

==========
Make OffscreenCanvas Transferable

At this moment, an offscreenCanvas could have an associated HTMLCanvasElement,
and we maintain a WeakPtr to that. Per discussion with esprehn@, 
we change the strategy such that an offscreenCanvas holds a int which
is the DOMNodeId for its associated HTMLCanvasElement.

This change makes offscreenCanvas transferable much easier. In particular,
we just need to pass three int to deserializer, which are width, height
and DOMNodeId of for an offscreenCanvas.

A kayout test is included to test its correctness. The layout test
includes several things:
1). Transfer an offscreenCanvas to worker and transfer it back to main,
the width and height attributes should not change.
2). After transfer, the width/height should be 0.
3). Transfer a neutered offscreenCanvas should throw an exception.
4). After getting the offscreenCanvas back from the worker, call
getContext('2d')
should work just fine.
5). Transfer an offscreenCanvas after calling getContext('2d')
should throw an exception.

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

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

Committed patchset #20 (id:380001)

[xidachen, 2016-04-21 16:46:17]

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

The reason for reverting is: Could be causing a failure to some unit_tests here:
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20x64%20Bu....

[xidachen, 2016-04-21 16:54:39]

Description was changed from

==========
Make OffscreenCanvas Transferable

At this moment, an offscreenCanvas could have an associated HTMLCanvasElement,
and we maintain a WeakPtr to that. Per discussion with esprehn@, 
we change the strategy such that an offscreenCanvas holds a int which
is the DOMNodeId for its associated HTMLCanvasElement.

This change makes offscreenCanvas transferable much easier. In particular,
we just need to pass three int to deserializer, which are width, height
and DOMNodeId of for an offscreenCanvas.

A kayout test is included to test its correctness. The layout test
includes several things:
1). Transfer an offscreenCanvas to worker and transfer it back to main,
the width and height attributes should not change.
2). After transfer, the width/height should be 0.
3). Transfer a neutered offscreenCanvas should throw an exception.
4). After getting the offscreenCanvas back from the worker, call
getContext('2d')
should work just fine.
5). Transfer an offscreenCanvas after calling getContext('2d')
should throw an exception.

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

to

==========
Make OffscreenCanvas Transferable

At this moment, an offscreenCanvas could have an associated HTMLCanvasElement,
and we maintain a WeakPtr to that. Per discussion with esprehn@, 
we change the strategy such that an offscreenCanvas holds a int which
is the DOMNodeId for its associated HTMLCanvasElement.

This change makes offscreenCanvas transferable much easier. In particular,
we just need to pass three int to deserializer, which are width, height
and DOMNodeId of for an offscreenCanvas.

A kayout test is included to test its correctness. The layout test
includes several things:
1). Transfer an offscreenCanvas to worker and transfer it back to main,
the width and height attributes should not change.
2). After transfer, the width/height should be 0.
3). Transfer a neutered offscreenCanvas should throw an exception.
4). After getting the offscreenCanvas back from the worker, call
getContext('2d')
should work just fine.
5). Transfer an offscreenCanvas after calling getContext('2d')
should throw an exception.

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

[commit-bot: I haz the power, 2016-04-22 19:34:58]

Description was changed from

==========
Make OffscreenCanvas Transferable

At this moment, an offscreenCanvas could have an associated HTMLCanvasElement,
and we maintain a WeakPtr to that. Per discussion with esprehn@, 
we change the strategy such that an offscreenCanvas holds a int which
is the DOMNodeId for its associated HTMLCanvasElement.

This change makes offscreenCanvas transferable much easier. In particular,
we just need to pass three int to deserializer, which are width, height
and DOMNodeId of for an offscreenCanvas.

A kayout test is included to test its correctness. The layout test
includes several things:
1). Transfer an offscreenCanvas to worker and transfer it back to main,
the width and height attributes should not change.
2). After transfer, the width/height should be 0.
3). Transfer a neutered offscreenCanvas should throw an exception.
4). After getting the offscreenCanvas back from the worker, call
getContext('2d')
should work just fine.
5). Transfer an offscreenCanvas after calling getContext('2d')
should throw an exception.

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

to

==========
Make OffscreenCanvas Transferable

At this moment, an offscreenCanvas could have an associated HTMLCanvasElement,
and we maintain a WeakPtr to that. Per discussion with esprehn@,
we change the strategy such that an offscreenCanvas holds a int which
is the DOMNodeId for its associated HTMLCanvasElement.

This change makes offscreenCanvas transferable much easier. In particular,
we just need to pass three int to deserializer, which are width, height
and DOMNodeId of for an offscreenCanvas.

A kayout test is included to test its correctness. The layout test
includes several things:
1). Transfer an offscreenCanvas to worker and transfer it back to main,
the width and height attributes should not change.
2). After transfer, the width/height should be 0.
3). Transfer a neutered offscreenCanvas should throw an exception.
4). After getting the offscreenCanvas back from the worker, call
getContext('2d')
should work just fine.
5). Transfer an offscreenCanvas after calling getContext('2d')
should throw an exception.

BUG=563845

Committed: https://crrev.com/47d06d097019e79f4f9a56068e086033ac72dba0
Cr-Commit-Position: refs/heads/master@{#388780}
==========

[commit-bot: I haz the power, 2016-04-22 19:34:59]

Patchset 20 (id:??) landed as
https://crrev.com/47d06d097019e79f4f9a56068e086033ac72dba0
Cr-Commit-Position: refs/heads/master@{#388780}

[xidachen, 2016-04-26 17:48:54]

Description was changed from

==========
Make OffscreenCanvas Transferable

At this moment, an offscreenCanvas could have an associated HTMLCanvasElement,
and we maintain a WeakPtr to that. Per discussion with esprehn@,
we change the strategy such that an offscreenCanvas holds a int which
is the DOMNodeId for its associated HTMLCanvasElement.

This change makes offscreenCanvas transferable much easier. In particular,
we just need to pass three int to deserializer, which are width, height
and DOMNodeId of for an offscreenCanvas.

A kayout test is included to test its correctness. The layout test
includes several things:
1). Transfer an offscreenCanvas to worker and transfer it back to main,
the width and height attributes should not change.
2). After transfer, the width/height should be 0.
3). Transfer a neutered offscreenCanvas should throw an exception.
4). After getting the offscreenCanvas back from the worker, call
getContext('2d')
should work just fine.
5). Transfer an offscreenCanvas after calling getContext('2d')
should throw an exception.

BUG=563845

Committed: https://crrev.com/47d06d097019e79f4f9a56068e086033ac72dba0
Cr-Commit-Position: refs/heads/master@{#388780}
==========

to

==========
Make OffscreenCanvas Transferable

At this moment, an offscreenCanvas could have an associated HTMLCanvasElement,
and we maintain a WeakPtr to that. Per discussion with esprehn@,
we change the strategy such that an offscreenCanvas holds a int which
is the DOMNodeId for its associated HTMLCanvasElement.

This change makes offscreenCanvas transferable much easier. In particular,
we just need to pass three int to deserializer, which are width, height
and DOMNodeId of for an offscreenCanvas.

A kayout test is included to test its correctness. The layout test
includes several things:
1). Transfer an offscreenCanvas to worker and transfer it back to main,
the width and height attributes should not change.
2). After transfer, the width/height should be 0.
3). Transfer a neutered offscreenCanvas should throw an exception.
4). After getting the offscreenCanvas back from the worker, call
getContext('2d')
should work just fine.
5). Transfer an offscreenCanvas after calling getContext('2d')
should throw an exception.

BUG=563845

Committed: https://crrev.com/47d06d097019e79f4f9a56068e086033ac72dba0
Cr-Commit-Position: refs/heads/master@{#388780}
==========