Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(16)

Issue 1862033002: Make OffscreenCanvas Transferable (Closed)

Created:
2 years, 7 months ago by xidachen
Modified:
2 years, 6 months ago
CC:
chromium-reviews, blink-reviews, haraken, blink-reviews-bindings_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Patch Set 2 : include newly added files #

Patch Set 3 : serialization part is done #

Patch Set 4 : needs a layout test #

Patch Set 5 : layout test timed out, needs debug #

Patch Set 6 : include newly added files #

Patch Set 7 : just to debug #

Patch Set 8 : working code, layout test passes #

Patch Set 9 : rebase, code compile but broke some layout tests #

Patch Set 10 : layout tests should all pass #

Patch Set 11 : update layout test results #

Total comments: 14

Patch Set 12 : address junov@'s comments #

Patch Set 13 : rebase, compiles but crashes some layout tests #

Total comments: 9

Patch Set 14 : all layout tests under fast/canvas passes, needs to look at diff and address comments #

Patch Set 15 : address haraken's comments #

Patch Set 16 : should not crash, needs bot to verify #

Patch Set 17 : address haraken's comments, need bots to test #

Total comments: 23

Patch Set 18 : Should work, register TransfersForModules is not done, DONOT commit this PS #

Patch Set 19 : register in ModuleBindingsInitializer #

Patch Set 20 : no need to register, taking the same approach as extractTransferables #

Unified diffs Side-by-side diffs Delta from patch set Stats (+415 lines, -72 lines) Patch
A third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-transferable.html View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +37 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-transferable-exceptions.html View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +54 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-transferable-exceptions-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +15 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-transferable-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +15 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/canvas/resources/OffscreenCanvas-transferable.js View 1 2 3 4 5 1 chunk +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.h View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +3 lines, -1 line 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/SerializationTag.h View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +3 lines, -36 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/SerializedScriptValueFactory.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +3 lines, -1 line 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/SerializedScriptValueFactory.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 4 chunks +48 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/Transferables.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +4 lines, -5 lines 0 comments Download
A third_party/WebKit/Source/bindings/core/v8/Transferables.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +20 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +4 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/v8.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/bindings/modules/v8/ScriptValueSerializerForModules.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +8 lines, -1 line 0 comments Download
M third_party/WebKit/Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 6 chunks +70 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/bindings/modules/v8/SerializedScriptValueForModulesFactory.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +5 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/bindings/modules/v8/SerializedScriptValueForModulesFactory.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 4 chunks +47 lines, -1 line 0 comments Download
A third_party/WebKit/Source/bindings/modules/v8/TransferablesForModules.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +31 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/bindings/modules/v8/v8.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/bindings/templates/methods.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +4 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +4 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/modules/canvas/HTMLCanvasElementModule.cpp View 1 2 3 4 5 6 7 2 chunks +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/modules/canvas/HTMLCanvasElementModuleTest.cpp View 1 2 3 4 5 6 7 2 chunks +3 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +7 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.cpp View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +19 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.idl View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 32 (15 generated)
xidachen
PTAL.
2 years, 7 months ago (2016-04-12 17:19:32 UTC) #5
Justin Novosad
https://codereview.chromium.org/1862033002/diff/200001/third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-transferable.html File third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-transferable.html (right): https://codereview.chromium.org/1862033002/diff/200001/third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-transferable.html#newcode44 third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-transferable.html:44: worker.postMessage({data: newOffscreenCanvas}, [newOffscreenCanvas]); This test nested in a test ...
2 years, 7 months ago (2016-04-12 18:10:23 UTC) #7
xidachen
https://codereview.chromium.org/1862033002/diff/200001/third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-transferable.html File third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-transferable.html (right): https://codereview.chromium.org/1862033002/diff/200001/third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-transferable.html#newcode44 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: ...
2 years, 7 months ago (2016-04-12 19:35:14 UTC) #9
Justin Novosad
lgtm for modules/canvas and LayoutTests
2 years, 7 months ago (2016-04-18 19:46:27 UTC) #10
haraken
https://codereview.chromium.org/1862033002/diff/240001/third_party/WebKit/Source/bindings/core/v8/TransferableExtractor.h File third_party/WebKit/Source/bindings/core/v8/TransferableExtractor.h (right): https://codereview.chromium.org/1862033002/diff/240001/third_party/WebKit/Source/bindings/core/v8/TransferableExtractor.h#newcode16 third_party/WebKit/Source/bindings/core/v8/TransferableExtractor.h:16: class CORE_EXPORT TransferableExtractor { Do you have a plan ...
2 years, 7 months ago (2016-04-19 00:48:11 UTC) #11
haraken
+Sigbjorn
2 years, 7 months ago (2016-04-19 06:22:04 UTC) #13
xidachen
On 2016/04/19 06:22:04, haraken wrote: > +Sigbjorn This CL is still in progress. After sof's ...
2 years, 7 months ago (2016-04-19 10:59:14 UTC) #14
xidachen
PS#15 should work. https://codereview.chromium.org/1862033002/diff/240001/third_party/WebKit/Source/bindings/core/v8/TransferableExtractor.h File third_party/WebKit/Source/bindings/core/v8/TransferableExtractor.h (right): https://codereview.chromium.org/1862033002/diff/240001/third_party/WebKit/Source/bindings/core/v8/TransferableExtractor.h#newcode16 third_party/WebKit/Source/bindings/core/v8/TransferableExtractor.h:16: class CORE_EXPORT TransferableExtractor { On 2016/04/19 ...
2 years, 7 months ago (2016-04-19 15:29:51 UTC) #15
haraken
https://codereview.chromium.org/1862033002/diff/240001/third_party/WebKit/Source/bindings/core/v8/TransferableExtractor.h File third_party/WebKit/Source/bindings/core/v8/TransferableExtractor.h (right): https://codereview.chromium.org/1862033002/diff/240001/third_party/WebKit/Source/bindings/core/v8/TransferableExtractor.h#newcode16 third_party/WebKit/Source/bindings/core/v8/TransferableExtractor.h:16: class CORE_EXPORT TransferableExtractor { On 2016/04/19 15:29:50, xidachen wrote: ...
2 years, 7 months ago (2016-04-20 01:18:24 UTC) #16
xidachen
https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp (right): https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp#newcode235 third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:235: if (!SerializedScriptValueFactory::instance().extractTransferables(isolate, transferables, exceptionState, transferableObject, i)) { Becaseu extractTransferables() ...
2 years, 7 months ago (2016-04-20 17:16:28 UTC) #17
haraken
LGTM with comments. https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp (right): https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp#newcode235 third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:235: if (!SerializedScriptValueFactory::instance().extractTransferables(isolate, transferables, exceptionState, transferableObject, i)) ...
2 years, 7 months ago (2016-04-21 00:29:03 UTC) #18
commit-bot: I haz the power
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
2 years, 7 months ago (2016-04-21 14:34:34 UTC) #20
xidachen
haraken@: all comments are addressed. Bots seem to be happy. Committing now. https://codereview.chromium.org/1862033002/diff/320001/third_party/WebKit/Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp File third_party/WebKit/Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp ...
2 years, 7 months ago (2016-04-21 15:32:49 UTC) #22
commit-bot: I haz the power
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
2 years, 7 months ago (2016-04-21 15:56:32 UTC) #25
commit-bot: I haz the power
Committed patchset #20 (id:380001)
2 years, 7 months ago (2016-04-21 16:02:47 UTC) #27
xidachen
A revert of this CL (patchset #20 id:380001) has been created in https://codereview.chromium.org/1904973003/ by xidachen@chromium.org. ...
2 years, 7 months ago (2016-04-21 16:46:17 UTC) #28
commit-bot: I haz the power
2 years, 7 months ago (2016-04-22 19:34:59 UTC) #31
Message was sent while issue was closed.
Patchset 20 (id:??) landed as
https://crrev.com/47d06d097019e79f4f9a56068e086033ac72dba0
Cr-Commit-Position: refs/heads/master@{#388780}

Powered by Google App Engine
This is Rietveld 408576698