Part of implementing WebSocket.send(Blob).
Created attachment 105297 [details] Patch
Attachment 105297 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/websockets/WebSocketChannel.h:48: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
CCing jianli and kinuko for Blob/FileReaderLoader usage.
Comment on attachment 105297 [details] Patch Attachment 105297 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9513869
Comment on attachment 105297 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=105297&action=review >> Source/WebCore/websockets/WebSocketChannel.h:48 >> + class Blob; > > Code inside a namespace should not be indented. [whitespace/indent] [4] Indentation will be fixed independently.
(In reply to comment #4) > (From update of attachment 105297 [details]) > Attachment 105297 [details] did not pass efl-ews (efl): > Output: http://queues.webkit.org/results/9513869 I forgot to guard FileReaderLoader with #if ENABLE(BLOB). Will be fixed in the next version.
Comment on attachment 105297 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=105297&action=review > Source/WebCore/websockets/WebSocketChannel.cpp:858 > + if (m_blobLoaderStatus == BlobLoaderNotStarted) { We had better use switch(m_blobLoaderStatus). > Source/WebCore/websockets/WebSocketChannel.cpp:860 > + m_blobLoader = adoptPtr(new FileReaderLoader(FileReaderLoader::ReadAsArrayBuffer, this)); should we check ASSERT(!m_blobLoader)? Is it possible to enqueue multiple blob frames?
Created attachment 105329 [details] Patch v2
Comment on attachment 105297 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=105297&action=review >> Source/WebCore/websockets/WebSocketChannel.cpp:858 >> + if (m_blobLoaderStatus == BlobLoaderNotStarted) { > > We had better use switch(m_blobLoaderStatus). Fixed in patch v2. >> Source/WebCore/websockets/WebSocketChannel.cpp:860 >> + m_blobLoader = adoptPtr(new FileReaderLoader(FileReaderLoader::ReadAsArrayBuffer, this)); > > should we check ASSERT(!m_blobLoader)? > Is it possible to enqueue multiple blob frames? Added ASSERT. Re reading multiple Blob, it's technically possible and probably better, but I don't want to do it in this first version because adding many moving parts makes debugging hard and makes the code more fragile. I'll probably do it later. Added a FIXME comment in patch v2.
Comment on attachment 105329 [details] Patch v2 I think the patch is ok except FileReaderLoader, which I'm not familiar with.
Comment on attachment 105329 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=105329&action=review Changes related to FileReaderLoader looks good to me. > Source/WebCore/websockets/WebSocketChannel.cpp:913 > + m_blobLoader->cancel(); nit: no need to clear m_blobLoader here?
Created attachment 105741 [details] Patch v3
Comment on attachment 105329 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=105329&action=review >> Source/WebCore/websockets/WebSocketChannel.cpp:913 >> + m_blobLoader->cancel(); > > nit: no need to clear m_blobLoader here? Good catch, thanks! Added didFail() in patch v3.
FileReaderLoader related changes looks good to me.
Comment on attachment 105741 [details] Patch v3 ok
Comment on attachment 105741 [details] Patch v3 Clearing flags on attachment: 105741 Committed r94162: <http://trac.webkit.org/changeset/94162>
All reviewed patches have been landed. Closing bug.