Bug 67013 – WebSocket: Load Blob in WebSocketChannel

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Yuta Kitamura]

Part of implementing WebSocket.send(Blob).

[Yuta Kitamura]

Created attachment 105297 [details]
Patch

[WebKit Review Bot]

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.

[Yuta Kitamura]

CCing jianli and kinuko for Blob/FileReaderLoader usage.

[Gyuyoung Kim]

Comment on attachment 105297 [details]
Patch

Attachment 105297 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9513869

[Yuta Kitamura]

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.

[Yuta Kitamura]

(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.

[Kent Tamura]

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?

[Yuta Kitamura]

Created attachment 105329 [details]
Patch v2

[Yuta Kitamura]

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.

[Kent Tamura]

Comment on attachment 105329 [details]
Patch v2

I think the patch is ok except FileReaderLoader, which I'm not familiar with.

[Kinuko Yasuda]

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?

[Yuta Kitamura]

Created attachment 105741 [details]
Patch v3

[Yuta Kitamura]

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.

[Kinuko Yasuda]

FileReaderLoader related changes looks good to me.

[Kent Tamura]

Comment on attachment 105741 [details]
Patch v3

ok

[WebKit Review Bot]

Comment on attachment 105741 [details]
Patch v3

Clearing flags on attachment: 105741

Committed r94162: <http://trac.webkit.org/changeset/94162>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.