Bug 98475 – [WebSocket] ExtensionParser should have its own file

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

[Kenichi Ishibashi]

The permessage compression extension requires "method" parameter. The value of the parameter is defined at http://tools.ietf.org/html/draft-ietf-hybi-permessage-compression-01#section-3. Here is the ABNFs.

       requested-method-list = 1#method-desc
       method-desc = method-name *(";" method-param)
       method-name = token
       method-param = token ["=" (token | quoted-string)]

This is almost the same as how the value of Sec-WebSocket-Extensions parsed. We can share the code of ExtensionParser to parse "method" parameter value of permessage compression. I'm going to factor out ExtensionParameter from WebSocketExtensionDispatcher and put it on its own file so that we can reuse the class.

[Kenichi Ishibashi]

Created attachment 167238 [details]
Patch

[Yuta Kitamura]

Comment on attachment 167238 [details]
Patch

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

Mostly looks good. Please fix the nits before landing.

> Source/WebCore/Modules/websockets/WebSocketExtensionParser.cpp:94
> +    m_currentToken = String::fromUTF8(buffer.data(), buffer.size());

(I know this code is moved from another file without modification, but I'm just curious.
 If you need to change the code, you can do it in a later patch.)

1. Is there any normative reference that says the content of a quoted string must be
   decoded as UTF-8?
2. What happens if the quoted string contains invalid UTF-8 sequence?

> Source/WebCore/Modules/websockets/WebSocketExtensionParser.h:37
> +#include <wtf/text/StringHash.h>

Is this necessary?

> Source/WebCore/Modules/websockets/WebSocketExtensionParser.h:56
> +    // The following member functions basically follow the grammer defined

nit: "grammar"

[Kenichi Ishibashi]

Created attachment 167260 [details]
Patch for landing

[Kenichi Ishibashi]

Comment on attachment 167238 [details]
Patch

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

Thank you for review!

>> Source/WebCore/Modules/websockets/WebSocketExtensionParser.cpp:94
>> +    m_currentToken = String::fromUTF8(buffer.data(), buffer.size());
> 
> (I know this code is moved from another file without modification, but I'm just curious.
>  If you need to change the code, you can do it in a later patch.)
> 
> 1. Is there any normative reference that says the content of a quoted string must be
>    decoded as UTF-8?
> 2. What happens if the quoted string contains invalid UTF-8 sequence?

I found my old comment on that (https://bugs.webkit.org/show_bug.cgi?id=78079#c20). I used it because WebSocketHandshake class uses it to parse header fields. If the quoted string contains invalid UtF-8 sequence, m_currentToken could be an empty string. It might be better to use CString. I'd like to do it as a separate patch because it affects other files.

>> Source/WebCore/Modules/websockets/WebSocketExtensionParser.h:37
>> +#include <wtf/text/StringHash.h>
> 
> Is this necessary?

I thought it's unnecessary, but when I removed it, I got following error.

../../third_party/WebKit/Source/WebCore/Modules/websockets/WebSocketExtensionParser.cpp:132:   instantiated from here
../../third_party/WebKit/Source/WTF/wtf/HashTable.h:844: error: incomplete type 
'WTF::StringHash' used in nested name specifier
...

>> Source/WebCore/Modules/websockets/WebSocketExtensionParser.h:56
>> +    // The following member functions basically follow the grammer defined
> 
> nit: "grammar"

Done. Thanks for pointing this out.

[WebKit Review Bot]

Comment on attachment 167260 [details]
Patch for landing

Clearing flags on attachment: 167260

Committed r130471: <http://trac.webkit.org/changeset/130471>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.