Spec: https://html.spec.whatwg.org/multipage/canvas.html#the-offscreencanvas-interface
Created attachment 380630 [details] Patch
Created attachment 385745 [details] Patch
Comment on attachment 385745 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385745&action=review > Source/WebCore/html/OffscreenCanvas.cpp:284 > + return std::unique_ptr<DetachedOffscreenCanvas>(new DetachedOffscreenCanvas(takeImageBuffer(), size(), originClean())); Please use makeUnique<>
Comment on attachment 385745 [details] Patch r- due to build failures.
Created attachment 385954 [details] Patch
argh, I think I've missed some OffscreenCanvas guards, will fix...
Created attachment 385965 [details] Patch
I don't think the style test will pass without reformatting a not-trivial amount of the affected file - note, my additions are in keeping with what's in the file already.
Created attachment 388978 [details] Patch
Comment on attachment 388978 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388978&action=review > Source/WebCore/bindings/js/SerializedScriptValue.cpp:3153 > +#if ENABLE(OFFSCREEN_CANVAS) > + { }, > +#endif > #if ENABLE(WEBASSEMBLY) > nullptr, > #endif Maybe CloneDeserializer could have default argument values to avoid these? > Source/WebCore/bindings/js/SerializedScriptValue.cpp:3574 > return adoptRef(*new SerializedScriptValue(WTFMove(buffer), blobURLs, nullptr, nullptr, { } > +#if ENABLE(OFFSCREEN_CANVAS) > + , { } > +#endif > #if ENABLE(WEBASSEMBLY) > , nullptr > #endif Maybe SerializedScriptValue could have default argument values to avoid these? > Source/WebCore/bindings/js/SerializedScriptValue.cpp:3589 > +static bool offscreenCanvasesCanDetach(const Vector<RefPtr<OffscreenCanvas>>& offscreenCanvases) The usual style is to start boolean functions with verb like is/are/has etc., maybe areAllOffscreenCanvasesDetachable or something like that. > Source/WebCore/bindings/js/SerializedScriptValue.cpp:3596 > + if (!visited.add(offscreenCanvas.get())) > + return false; What is this test about? Maybe add a comment?
Created attachment 388986 [details] again
Comment on attachment 388986 [details] again oops wrong bug
Created attachment 389010 [details] Patch
Created attachment 389015 [details] Patch
Comment on attachment 389015 [details] Patch Clearing flags on attachment: 389015 Committed r255315: <https://trac.webkit.org/changeset/255315>
All reviewed patches have been landed. Closing bug.
<rdar://problem/58970553>
Comment on attachment 389015 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389015&action=review > Source/WebCore/bindings/js/SerializedScriptValue.cpp:567 > static SerializationReturnCode serialize(JSGlobalObject* lexicalGlobalObject, JSValue value, Vector<RefPtr<MessagePort>>& messagePorts, Vector<RefPtr<JSC::ArrayBuffer>>& arrayBuffers, const Vector<RefPtr<ImageBitmap>>& imageBitmaps, > +#if ENABLE(OFFSCREEN_CANVAS) > + const Vector<RefPtr<OffscreenCanvas>>& offscreenCanvases, > +#endif > #if ENABLE(WEBASSEMBLY) > WasmModuleArray& wasmModules, > #endif > Vector<String>& blobURLs, Vector<uint8_t>& out, SerializationContext context, ArrayBufferContentsArray& sharedBuffers) Seems to me that long arguments lists like these with compiler-conditioonal arguments in them are not a great pattern. We should look for something more elegant and easy to read. Maybe a structure would help in some way? > Source/WebCore/bindings/js/SerializedScriptValue.cpp:969 > + void dumpOffscreenCanvas(JSObject* obj, SerializationReturnCode& code) WebKit coding style says use a word here, object, not an abbreviation, obj. Also, we like to use references for things that can’t be null. Although I guess JavaScript programmers are holdouts on that one? > Source/WebCore/bindings/js/SerializedScriptValue.cpp:3595 > + Vector<RefPtr<OffscreenCanvas>> offscreenCanvases; I’d think Vector<Ref> would be the right type for these vectors. Seems unlikely we need to store nulls in them. > Source/WebCore/bindings/js/SerializedScriptValue.h:52 > +#if ENABLE(OFFSCREEN_CANVAS) > +class DetachedOffscreenCanvas; > +#endif Two coding style thoughts: 1) I’m not sure it’s worthwhile to put forward declarations inside #if blocks. I think it’s probably a reasonable practice to define them even if they aren’t used. 2) If we are going to put them in #if blocks, then the #if block should be a separate paragraph and not interspersed with other forward declarations. This is the same rule we follow for includes. > Source/WebCore/html/OffscreenCanvas.cpp:90 > + if (m_detached) > + return 0; Can we just set m_size to 0 as part of the detaching process? Then we can let width remain a non-virtual function. > Source/WebCore/html/OffscreenCanvas.cpp:97 > + if (m_detached) > + return 0; Ditto.