Bug 78682 - [WebSocket] Move WebSocketChannel::FrameData into a separate header file
Summary: [WebSocket] Move WebSocketChannel::FrameData into a separate header file
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kenichi Ishibashi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-15 01:34 PST by Kenichi Ishibashi
Modified: 2012-02-20 06:55 PST (History)
5 users (show)

See Also:


Attachments
WIP patch (22.82 KB, patch)
2012-02-15 01:37 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch (30.50 KB, patch)
2012-02-19 18:30 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch (31.01 KB, patch)
2012-02-20 03:37 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenichi Ishibashi 2012-02-15 01:34:45 PST
This brings flexibility to add classes which want to do something for incoming/outgoing frames (e.g. compression/decompression).
Comment 1 Kenichi Ishibashi 2012-02-15 01:37:37 PST
Created attachment 127133 [details]
WIP patch
Comment 2 Kenichi Ishibashi 2012-02-15 01:45:33 PST
(In reply to comment #1)
> Created an attachment (id=127133) [details]
> WIP patch

Yuta-san, could you take a look?

Some notes:
- Should WebSocketFrame be a class (not struct) and should I add setter/getter methods?
- I removed frameEnd field and added m_currentFrameEnd to WebSocketChannel. I ran layout tests, but not sure it is safe or not.
Comment 3 Yuta Kitamura 2012-02-15 03:09:37 PST
Comment on attachment 127133 [details]
WIP patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127133&action=review

I think your patch is in the right direction.

Re struct vs class, I guess current WebSocketFrame is simple enough to leave it as a struct.

> Source/WebCore/websockets/WebSocketChannel.cpp:606
> +    m_currentFrameEnd = p + maskingKeyLength + payloadLength;

Ending position of a frame has nothing to do with the channel's state, so it sounds strange to have it as a member variable.

Rather, I prefer to have it as an output parameter of parseFrame() function:

    parseFrame(WebSocketFrame& frame, const char*& frameEnd) { ... }

> Source/WebCore/websockets/WebSocketChannel.cpp:978
> +    frame.opCode = opCode;
> +    frame.final = true;
> +    frame.masked = true;
> +    frame.payload = data;
> +    frame.payloadLength = dataLength;

It's a bit risky to initialize a struct this way (consider the situation where you have added a new member but forget to fix here).

I think it's okay for structs to have a constructor. There are many examples in WebKit code base.

> Source/WebCore/websockets/WebSocketFrame.h:46
> +    static const OpCode OpCodeContinuation = 0x0;
> +    static const OpCode OpCodeText = 0x1;
> +    static const OpCode OpCodeBinary = 0x2;
> +    static const OpCode OpCodeClose = 0x8;
> +    static const OpCode OpCodePing = 0x9;
> +    static const OpCode OpCodePong = 0xA;

You probably need to have defitions of these variables in .cpp file.

(According to my vague memory, Visual Studio didn't like to have a constant definition in class declaration, but I may be wrong. Try EWS if you are curious.)
Comment 4 Kenichi Ishibashi 2012-02-15 05:20:13 PST
Thank you for comments. I'll address your comment next week.

(In reply to comment #3)
> (From update of attachment 127133 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127133&action=review
> 
> I think your patch is in the right direction.
> 
> Re struct vs class, I guess current WebSocketFrame is simple enough to leave it as a struct.
> 
> > Source/WebCore/websockets/WebSocketChannel.cpp:606
> > +    m_currentFrameEnd = p + maskingKeyLength + payloadLength;
> 
> Ending position of a frame has nothing to do with the channel's state, so it sounds strange to have it as a member variable.
> 
> Rather, I prefer to have it as an output parameter of parseFrame() function:
> 
>     parseFrame(WebSocketFrame& frame, const char*& frameEnd) { ... }
> 
> > Source/WebCore/websockets/WebSocketChannel.cpp:978
> > +    frame.opCode = opCode;
> > +    frame.final = true;
> > +    frame.masked = true;
> > +    frame.payload = data;
> > +    frame.payloadLength = dataLength;
> 
> It's a bit risky to initialize a struct this way (consider the situation where you have added a new member but forget to fix here).
> 
> I think it's okay for structs to have a constructor. There are many examples in WebKit code base.
> 
> > Source/WebCore/websockets/WebSocketFrame.h:46
> > +    static const OpCode OpCodeContinuation = 0x0;
> > +    static const OpCode OpCodeText = 0x1;
> > +    static const OpCode OpCodeBinary = 0x2;
> > +    static const OpCode OpCodeClose = 0x8;
> > +    static const OpCode OpCodePing = 0x9;
> > +    static const OpCode OpCodePong = 0xA;
> 
> You probably need to have defitions of these variables in .cpp file.
> 
> (According to my vague memory, Visual Studio didn't like to have a constant definition in class declaration, but I may be wrong. Try EWS if you are curious.)
Comment 5 Kenichi Ishibashi 2012-02-19 18:30:43 PST
Created attachment 127749 [details]
Patch
Comment 6 Kenichi Ishibashi 2012-02-19 18:36:46 PST
Comment on attachment 127133 [details]
WIP patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127133&action=review

>>> Source/WebCore/websockets/WebSocketChannel.cpp:606
>>> +    m_currentFrameEnd = p + maskingKeyLength + payloadLength;
>> 
>> Ending position of a frame has nothing to do with the channel's state, so it sounds strange to have it as a member variable.
>> 
>> Rather, I prefer to have it as an output parameter of parseFrame() function:
>> 
>>     parseFrame(WebSocketFrame& frame, const char*& frameEnd) { ... }
> 
> 

Done.

>> Source/WebCore/websockets/WebSocketChannel.cpp:978
>> +    frame.payloadLength = dataLength;
> 
> It's a bit risky to initialize a struct this way (consider the situation where you have added a new member but forget to fix here).
> 
> I think it's okay for structs to have a constructor. There are many examples in WebKit code base.

Done.

>> Source/WebCore/websockets/WebSocketFrame.h:46
>> +    static const OpCode OpCodePong = 0xA;
> 
> You probably need to have defitions of these variables in .cpp file.
> 
> (According to my vague memory, Visual Studio didn't like to have a constant definition in class declaration, but I may be wrong. Try EWS if you are curious.)

It seems that I need to use enum hack here because g++ complained like below when I defined these variables in .cpp file:

WebKit/Source/WebCore/websockets/WebSocketChannel.cpp:654: error: 'WebCore::WebSocketFrame::OpCodeContinuation' cannot appear in a constant-expression
Comment 7 Kenichi Ishibashi 2012-02-20 01:19:11 PST
Hi Kent-san, Alexey,

Could you review the patch?
Comment 8 Kent Tamura 2012-02-20 02:31:11 PST
Comment on attachment 127749 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127749&action=review

> Source/WebCore/websockets/WebSocketChannel.cpp:975
> +    // Mask the frame.

The comment is not so helpful.
How about making a function to make a frame data if you'd like to improve the readability of the code?

> Source/WebCore/websockets/WebSocketChannel.cpp:1005
> -    // Mask the frame.
> -    size_t maskingKeyStart = frame.size();
> -    frame.grow(frame.size() + maskingKeyWidthInBytes); // Add placeholder for masking key. Will be overwritten.
> -    size_t payloadStart = frame.size();
> -    frame.append(data, dataLength);
> +    size_t maskingKeyStart = frameData.size();
> +    frameData.grow(frameData.size() + maskingKeyWidthInBytes); // Add placeholder for masking key. Will be overwritten.
> +    size_t payloadStart = frameData.size();
> +    frameData.append(frame.payload, frame.payloadLength);
>  
> -    cryptographicallyRandomValues(frame.data() + maskingKeyStart, maskingKeyWidthInBytes);
> -    for (size_t i = 0; i < dataLength; ++i)
> -        frame[payloadStart + i] ^= frame[maskingKeyStart + i % maskingKeyWidthInBytes];
> +    cryptographicallyRandomValues(frameData.data() + maskingKeyStart, maskingKeyWidthInBytes);
> +    for (size_t i = 0; i < frame.payloadLength; ++i)
> +        frameData[payloadStart + i] ^= frameData[maskingKeyStart + i % maskingKeyWidthInBytes];

Also, making a function to mask the frame data would improve readability.
Comment 9 Kenichi Ishibashi 2012-02-20 03:37:32 PST
Created attachment 127800 [details]
Patch
Comment 10 Kenichi Ishibashi 2012-02-20 03:46:28 PST
Comment on attachment 127749 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127749&action=review

Kent-san,

Thank you for review. I added two functions. I have though about making WebSocketFrame to be a class and put these function as its member functions, but I just added them as static functions for now.

>> Source/WebCore/websockets/WebSocketChannel.cpp:975
>> +    // Mask the frame.
> 
> The comment is not so helpful.
> How about making a function to make a frame data if you'd like to improve the readability of the code?

I put the comment on wrong place.. Anyway, added makeFrameData() function.

>> Source/WebCore/websockets/WebSocketChannel.cpp:1005
>> +        frameData[payloadStart + i] ^= frameData[maskingKeyStart + i % maskingKeyWidthInBytes];
> 
> Also, making a function to mask the frame data would improve readability.

Added a function as appendMaskedFramePayload().
Comment 11 WebKit Review Bot 2012-02-20 06:55:06 PST
Comment on attachment 127800 [details]
Patch

Clearing flags on attachment: 127800

Committed r108240: <http://trac.webkit.org/changeset/108240>
Comment 12 WebKit Review Bot 2012-02-20 06:55:11 PST
All reviewed patches have been landed.  Closing bug.