Bug 67013

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.