Bug 78079 – [WebSocket] Add WebSocket extension support

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

[Kenichi Ishibashi]

Add WebSocket extension support.

[Kenichi Ishibashi]

Created attachment 126007 [details]
Patch

[Kenichi Ishibashi]

(In reply to comment #1)
> Created an attachment (id=126007) [details]
> Patch

It's difficult to write layout tests because there is no behavior change so far. I added a chromium's unit test for server response parsing. It's port specific, but it would be better than not writing tests.

[Gyuyoung Kim]

Comment on attachment 126007 [details]
Patch

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

[Kenichi Ishibashi]

Created attachment 126012 [details]
Patch

[Yuta Kitamura]

Comment on attachment 126007 [details]
Patch

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

[My comments are only informational as I'm not a WebKit reviewer yet.]

> Source/WebCore/websockets/WebSocketHandshake.cpp:707
> +        if (!m_extensions.processHeaderValue(serverWebSocketExtensions)) {

optional: If you could provide detailed reason of failure, the script authors will be able to diagnose their issues better.

> Source/WebCore/websockets/WebSocketHandshakeResponse.cpp:91
> +    // they are connected with commna.

typo: commna

> Source/WebCore/websockets/WebSocketHandshakeResponse.cpp:96
> +    HTTPHeaderMap::iterator iterator = m_headerFields.find(name);
> +    if (iterator != m_headerFields.end() && equalIgnoringCase(iterator->first, "sec-websocket-extensions"))
> +        m_headerFields.set(name, iterator->second + "," + value);
> +    else
> +        m_headerFields.add(name, value);

It may be a bad idea to simply concatenate all header values with commas. Consider the following (broken) case:

Sec-WebSocket-Extensions: foo; param="bar
Sec-WebSocket-Extensions: "

We want to signal an error when we receive headers like this, instead of treating these two lines as a valid extension line 'foo; param="bar,"'.

> Source/WebKit/chromium/tests/WebSocketExtensionsTest.cpp:1
> +/*

Maybe you could make use of TestWebKitAPI to write unit tests like this. I'm not sure which (TestWebKitAPI or Chromium tests) is better for this purpose, though.

> Source/WebKit/chromium/tests/WebSocketExtensionsTest.cpp:133
> +        HashMap<String, String>& expectedParameters = expected[i].parameters;
> +        HashMap<String, String>& parsedParameters = m_parsedParameters[i];

const reference should be better.

[Kenichi Ishibashi]

Created attachment 126036 [details]
Patch

[Kenichi Ishibashi]

Comment on attachment 126007 [details]
Patch

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

Thank you for review!

>> Source/WebCore/websockets/WebSocketHandshake.cpp:707
>> +        if (!m_extensions.processHeaderValue(serverWebSocketExtensions)) {
> 
> optional: If you could provide detailed reason of failure, the script authors will be able to diagnose their issues better.

Added WebSocketExtensions::failureReason(). It will be called if WebSocketExtensions::processHeaderValue() fails.

>> Source/WebCore/websockets/WebSocketHandshakeResponse.cpp:91
>> +    // they are connected with commna.
> 
> typo: commna

Removed. See below.

>> Source/WebCore/websockets/WebSocketHandshakeResponse.cpp:96
>> +        m_headerFields.add(name, value);
> 
> It may be a bad idea to simply concatenate all header values with commas. Consider the following (broken) case:
> 
> Sec-WebSocket-Extensions: foo; param="bar
> Sec-WebSocket-Extensions: "
> 
> We want to signal an error when we receive headers like this, instead of treating these two lines as a valid extension line 'foo; param="bar,"'.

Thank you for pointing this out. Removed this change and revised the patch to parse and check the header value every time the header appears. The revised patch also removes WebSocketHandshake::serverWebSocketExtensions(), which is no longer needed.

>> Source/WebKit/chromium/tests/WebSocketExtensionsTest.cpp:1
>> +/*
> 
> Maybe you could make use of TestWebKitAPI to write unit tests like this. I'm not sure which (TestWebKitAPI or Chromium tests) is better for this purpose, though.

Didn't move the test into TestWebKitAPI for now. If TestWebKitAPI is better for this test, I'll move it.

>> Source/WebKit/chromium/tests/WebSocketExtensionsTest.cpp:133
>> +        HashMap<String, String>& parsedParameters = m_parsedParameters[i];
> 
> const reference should be better.

Done.

[WebKit Review Bot]

Comment on attachment 126036 [details]
Patch

Attachment 126036 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11460366

New failing tests:
http/tests/websocket/tests/hybi/handshake-fail-by-extensions-header.html

[Kenichi Ishibashi]

Created attachment 126186 [details]
added test rebaseline

[WebKit Review Bot]

Comment on attachment 126186 [details]
added test rebaseline

Attachment 126186 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11463664

New failing tests:
platform/chromium/compositing/layout-width-change.html

[Kenichi Ishibashi]

Hi Alexey, Kent-san,

Could you please review the patch?

[Kent Tamura]

Comment on attachment 126186 [details]
added test rebaseline

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

> Source/WebCore/websockets/WebSocketExtensions.cpp:58
> +    void consumeSpaces();

The name should not start with "consume", and should be skipSpaces() or something because other consumeFoo() functions return a bool value.

> Source/WebCore/websockets/WebSocketExtensions.cpp:60
> +    bool isControl(char);
> +    bool isSeparator(char);

They should be static function members, or static functions which are not members of ExtensionParser.

> Source/WebCore/websockets/WebSocketExtensions.cpp:77
> +// These functions basically follow the grammer defined in Section 2.2 of RFC 2616.

"These" is unclear.

> Source/WebCore/websockets/WebSocketExtensions.cpp:97
> +    for (const char* current = separatorCharacters; *current; ++current) {
> +        if (*current == character)
> +            return true;
> +    }
> +    return false;

char* p = strchr(separatorCharacters, character);
return p && *p;

is enough.

> Source/WebCore/websockets/WebSocketExtensions.cpp:107
> +        m_currentToken = String(start, m_current - start);

What's the encoding of the token?

> Source/WebCore/websockets/WebSocketExtensions.cpp:151
> +    m_extensions.append(adoptPtr(new Extension(processor)));
> +}

We should have an assertion of validity of processor->handshakeString().

> Source/WebCore/websockets/WebSocketExtensions.cpp:173
> +    // If we don't send Sec-WebSocket-Extensions header, the server should not return the header

A comment should end with '.'

> Source/WebCore/websockets/WebSocketExtensions.cpp:183
> +        // Parse extension-token

ditto.

> Source/WebCore/websockets/WebSocketExtensions.cpp:185
> +            m_failureReason = "Sec-WebSocket-Extensions header is invalid";

Should it end with '.'?

> Source/WebCore/websockets/WebSocketExtensions.cpp:190
> +        // Parse extension-parameters if exists

A comment should end with '.'

> Source/WebCore/websockets/WebSocketExtensions.cpp:200
> +                if (parser.consumeQuotedString() || parser.consumeToken())

I know this code works correctly now, but I needed to read the content of consumeQuotedString() carefully in order to confirm consumeQuotedString() never fail with unexpected ExtensionParser::m_current.  I recommend introduce ExtensionParser::consumeQuotedStringOrToken().

> Source/WebCore/websockets/WebSocketExtensions.cpp:225
> +        // There is no extension which can process the response

A comment should end with '.'

> Source/WebCore/websockets/WebSocketExtensions.h:44
> +class WebSocketExtensionProcessor {

The class should be moved to its own file. An implementation of WebSocketExtensionProcessor doesn't need to know about WebSocketExtensions class.

> Source/WebCore/websockets/WebSocketExtensions.h:48
> +    const String extensionToken() const { return m_extensionToken; }

Whey it returns "const String"?

> Source/WebCore/websockets/WebSocketExtensions.h:71
> +class WebSocketExtensions {

I don't like the name "WebSocketExtensions".  It's not a simple container of extensions.
Maybe "WebSocketExtensionController", "WebSocketExtensionDispatcher", or something.

> Source/WebKit/chromium/tests/WebSocketExtensionsTest.cpp:47
> +    virtual const String handshakeString() { return extensionToken(); }
> +    virtual bool processResponse(const HashMap<String, String>&);

should append OVERRIDE.

> Source/WebKit/chromium/tests/WebSocketExtensionsTest.cpp:50
> +

The blank line is not needed.

> Source/WebKit/chromium/tests/WebSocketExtensionsTest.cpp:66
> +    WebSocketExtensionsTest()
> +    {
> +    }
> +
> +    void SetUp()
> +    {
> +    }
> +
> +    void TearDown()
> +    {
> +    }

You may write like
  WebSocketExtensionTest() { }

> Source/WebKit/chromium/tests/WebSocketExtensionsTest.cpp:71
> +

THe blank line is not needed.

[Yuta Kitamura]

Comment on attachment 126186 [details]
added test rebaseline

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

Looks almost fine to me.

> Source/WebCore/websockets/WebSocketExtensions.cpp:224
> +        for (index = 0; index < m_extensions.size(); ++index) {
> +            Extension* extension = m_extensions[index].get();
> +            if (equalIgnoringCase(extensionToken, extension->m_processor->extensionToken()) && !extension->m_responseHandled) {
> +                extension->m_responseHandled = true;
> +                if (extension->m_processor->processResponse(extensionParameters))
> +                    break;
> +                m_failureReason = extension->m_processor->failureReason();
> +                return false;
> +            }
> +        }

I have two questions:

(1) What will happen if the server provides two extensions having the same extention identifier, such as "foo;a=1, foo;b=2"?
(2) Why do you compare the extention name case-insensitively? I could not find such a requirement in the spec (but I may have missed it).

[Kenichi Ishibashi]

Created attachment 126255 [details]
Patch

[Kenichi Ishibashi]

Comment on attachment 126186 [details]
added test rebaseline

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

Hi Kent-san, Yuta-san,

Thank you for review! Addressed your comments.

>> Source/WebCore/websockets/WebSocketExtensions.cpp:58
>> +    void consumeSpaces();
> 
> The name should not start with "consume", and should be skipSpaces() or something because other consumeFoo() functions return a bool value.

Renamed as skipSpaces().

>> Source/WebCore/websockets/WebSocketExtensions.cpp:60
>> +    bool isSeparator(char);
> 
> They should be static function members, or static functions which are not members of ExtensionParser.

These functions integrated into a static function isTokenCharacter().

>> Source/WebCore/websockets/WebSocketExtensions.cpp:77
>> +// These functions basically follow the grammer defined in Section 2.2 of RFC 2616.
> 
> "These" is unclear.

Moved the comment in the declaration of ExtensionParser class.

>> Source/WebCore/websockets/WebSocketExtensions.cpp:97
>> +    return false;
> 
> char* p = strchr(separatorCharacters, character);
> return p && *p;
> 
> is enough.

Done.

>> Source/WebCore/websockets/WebSocketExtensions.cpp:107
>> +        m_currentToken = String(start, m_current - start);
> 
> What's the encoding of the token?

The spec refers http://tools.ietf.org/html/rfc2616#section-4.2, and it says token consists of any US-ASCII characters (0 - 127) except for control characters (0 - 31 and 127). We can assume the encoding is ASCII.

>> Source/WebCore/websockets/WebSocketExtensions.cpp:151
>> +}
> 
> We should have an assertion of validity of processor->handshakeString().

Done.

>> Source/WebCore/websockets/WebSocketExtensions.cpp:173
>> +    // If we don't send Sec-WebSocket-Extensions header, the server should not return the header
> 
> A comment should end with '.'

Done.

>> Source/WebCore/websockets/WebSocketExtensions.cpp:183
>> +        // Parse extension-token
> 
> ditto.

Done.

>> Source/WebCore/websockets/WebSocketExtensions.cpp:185
>> +            m_failureReason = "Sec-WebSocket-Extensions header is invalid";
> 
> Should it end with '.'?

Other failure messages in WebSocketHandshake class don't end with '.' so I don't append '.' here. I think we should keep consistency.

>> Source/WebCore/websockets/WebSocketExtensions.cpp:190
>> +        // Parse extension-parameters if exists
> 
> A comment should end with '.'

Done.

>> Source/WebCore/websockets/WebSocketExtensions.cpp:200
>> +                if (parser.consumeQuotedString() || parser.consumeToken())
> 
> I know this code works correctly now, but I needed to read the content of consumeQuotedString() carefully in order to confirm consumeQuotedString() never fail with unexpected ExtensionParser::m_current.  I recommend introduce ExtensionParser::consumeQuotedStringOrToken().

Done.

>> Source/WebCore/websockets/WebSocketExtensions.cpp:224
>> +        }
> 
> I have two questions:
> 
> (1) What will happen if the server provides two extensions having the same extention identifier, such as "foo;a=1, foo;b=2"?
> (2) Why do you compare the extention name case-insensitively? I could not find such a requirement in the spec (but I may have missed it).

(1) If my reading is correct, RFC6455 allows such case. I think we should accept such response if the extension allows such duplication. The current implementation doesn't allow the case so I changed the behavior.
(2) You are right. Changed to just compare the tokens.

>> Source/WebCore/websockets/WebSocketExtensions.cpp:225
>> +        // There is no extension which can process the response
> 
> A comment should end with '.'

Done.

>> Source/WebCore/websockets/WebSocketExtensions.h:44
>> +class WebSocketExtensionProcessor {
> 
> The class should be moved to its own file. An implementation of WebSocketExtensionProcessor doesn't need to know about WebSocketExtensions class.

Done.

>> Source/WebCore/websockets/WebSocketExtensions.h:48
>> +    const String extensionToken() const { return m_extensionToken; }
> 
> Whey it returns "const String"?

Removed const.

>> Source/WebCore/websockets/WebSocketExtensions.h:71
>> +class WebSocketExtensions {
> 
> I don't like the name "WebSocketExtensions".  It's not a simple container of extensions.
> Maybe "WebSocketExtensionController", "WebSocketExtensionDispatcher", or something.

Changed the name to WebKitExtensionDispatcher.

>> Source/WebKit/chromium/tests/WebSocketExtensionsTest.cpp:47
>> +    virtual bool processResponse(const HashMap<String, String>&);
> 
> should append OVERRIDE.

Done.

>> Source/WebKit/chromium/tests/WebSocketExtensionsTest.cpp:50
>> +
> 
> The blank line is not needed.

Done.

>> Source/WebKit/chromium/tests/WebSocketExtensionsTest.cpp:66
>> +    }
> 
> You may write like
>   WebSocketExtensionTest() { }

Done.

>> Source/WebKit/chromium/tests/WebSocketExtensionsTest.cpp:71
>> +
> 
> THe blank line is not needed.

Done.

[Kent Tamura]

Comment on attachment 126255 [details]
Patch

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

> Source/WebCore/websockets/WebSocketExtensionDispatcher.cpp:60
> +    bool consumeCharacter(char);
> +private:

need a blank line before private:.

> Source/WebCore/websockets/WebSocketExtensionDispatcher.cpp:81
> +    // A token character is a CHAR (octets 0 - 127) but not CTL (octets 0 - 31 and 127)
> +    return character > 31 && character < 127;

This looks same as isASCIIPrintable() in wtf/ASCIIType.h.

> Source/WebCore/websockets/WebSocketExtensionDispatcher.cpp:132
> +bool ExtensionParser::consumeQuotedStringOrToken()
> +{
> +    return consumeQuotedString() || consumeToken();

I recommend adding a comment that consumeToken() works well because consumeQuotedString() doesn't update m_current or makes it same as m_end on failure.

> Source/WebCore/websockets/WebSocketExtensionDispatcher.cpp:166
> +    for (size_t i = 1; i < numProcessors; ++i) {
> +        builder.append(", ");
> +        builder.append(m_processors[i]->handshakeString());

What happens if handshakString() contains \n or \0?

[Kent Tamura]

Comment on attachment 126255 [details]
Patch

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

> Source/WebCore/websockets/WebSocketExtensionDispatcher.cpp:121
> +        builder.append(*m_current);
> +        ++m_current;

What's the encoding of a quoted string?

[Yuta Kitamura]

Comment on attachment 126186 [details]
added test rebaseline

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

>>> Source/WebCore/websockets/WebSocketExtensions.cpp:224
>>> +        }
>> 
>> I have two questions:
>> 
>> (1) What will happen if the server provides two extensions having the same extention identifier, such as "foo;a=1, foo;b=2"?
>> (2) Why do you compare the extention name case-insensitively? I could not find such a requirement in the spec (but I may have missed it).
> 
> (1) If my reading is correct, RFC6455 allows such case. I think we should accept such response if the extension allows such duplication. The current implementation doesn't allow the case so I changed the behavior.
> (2) You are right. Changed to just compare the tokens.

For (1), how do we treat the case above, "foo;a=1;b=2", "foo;a=1" or "foo;b=2"? How about cases like "bar;x=3, bar;x=4"?

This seems like a blackhole that the spec has missed describing, but anyway we need to determine what is the expected behavior for such cases.

[Kenichi Ishibashi]

Created attachment 126261 [details]
Patch

[Kenichi Ishibashi]

Comment on attachment 126255 [details]
Patch

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

Hi kent-san,

Thanks for review.

>> Source/WebCore/websockets/WebSocketExtensionDispatcher.cpp:60
>> +private:
> 
> need a blank line before private:.

Done.

>> Source/WebCore/websockets/WebSocketExtensionDispatcher.cpp:81
>> +    return character > 31 && character < 127;
> 
> This looks same as isASCIIPrintable() in wtf/ASCIIType.h.

Removed the function and used isASCIIPrintable(). Thanks!

>> Source/WebCore/websockets/WebSocketExtensionDispatcher.cpp:121
>> +        ++m_current;
> 
> What's the encoding of a quoted string?

According to http://tools.ietf.org/html/rfc2616#section-2.2, quoted strings MAY contain ISO-8859-1 characters. In general it can contain any octets except for non-printable characters. I changed to use String::fromUTF8() because WebSocketHandshake class uses it to parse header fields.

>> Source/WebCore/websockets/WebSocketExtensionDispatcher.cpp:132
>> +    return consumeQuotedString() || consumeToken();
> 
> I recommend adding a comment that consumeToken() works well because consumeQuotedString() doesn't update m_current or makes it same as m_end on failure.

Done.

>> Source/WebCore/websockets/WebSocketExtensionDispatcher.cpp:166
>> +        builder.append(m_processors[i]->handshakeString());
> 
> What happens if handshakString() contains \n or \0?

Added assertions to check handshakeString() doesn't contain these characters.

[Kent Tamura]

Comment on attachment 126261 [details]
Patch

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

> Source/WebCore/websockets/WebSocketExtensionDispatcher.cpp:162
> +    ASSERT(!headerValue.contains('\n'));
> +    ASSERT(!headerValue.contains(static_cast<UChar>('\0')));

These assertions should be moved to addProcessor().

[Kenichi Ishibashi]

Comment on attachment 126186 [details]
added test rebaseline

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

Hi Yuta-san,

>>>> Source/WebCore/websockets/WebSocketExtensions.cpp:224
>>>> +        }
>>> 
>>> I have two questions:
>>> 
>>> (1) What will happen if the server provides two extensions having the same extention identifier, such as "foo;a=1, foo;b=2"?
>>> (2) Why do you compare the extention name case-insensitively? I could not find such a requirement in the spec (but I may have missed it).
>> 
>> (1) If my reading is correct, RFC6455 allows such case. I think we should accept such response if the extension allows such duplication. The current implementation doesn't allow the case so I changed the behavior.
>> (2) You are right. Changed to just compare the tokens.
> 
> For (1), how do we treat the case above, "foo;a=1;b=2", "foo;a=1" or "foo;b=2"? How about cases like "bar;x=3, bar;x=4"?
> 
> This seems like a blackhole that the spec has missed describing, but anyway we need to determine what is the expected behavior for such cases.

I agree it's somewhat unclear.

For now, I think extensions should determine how to treat these cases. Extensions may reject duplicate response or may override parameters, depending on what the corresponding spec requires. For example, "deflate-frame" extension will reject duplicate response because it uses a reserved frame flag and duplicate response doesn't make sense. The current implementation just delegates validation to extensions.

[Kenichi Ishibashi]

Created attachment 126264 [details]
Patch

[Kenichi Ishibashi]

Comment on attachment 126261 [details]
Patch

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

>> Source/WebCore/websockets/WebSocketExtensionDispatcher.cpp:162
>> +    ASSERT(!headerValue.contains(static_cast<UChar>('\0')));
> 
> These assertions should be moved to addProcessor().

Done.

[Kent Tamura]

Comment on attachment 126264 [details]
Patch

Ok

[Kenichi Ishibashi]

Comment on attachment 126264 [details]
Patch

Thank you!

[Yuta Kitamura]

Comment on attachment 126186 [details]
added test rebaseline

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

>>>>> Source/WebCore/websockets/WebSocketExtensions.cpp:224
>>>>> +        }
>>>> 
>>>> I have two questions:
>>>> 
>>>> (1) What will happen if the server provides two extensions having the same extention identifier, such as "foo;a=1, foo;b=2"?
>>>> (2) Why do you compare the extention name case-insensitively? I could not find such a requirement in the spec (but I may have missed it).
>>> 
>>> (1) If my reading is correct, RFC6455 allows such case. I think we should accept such response if the extension allows such duplication. The current implementation doesn't allow the case so I changed the behavior.
>>> (2) You are right. Changed to just compare the tokens.
>> 
>> For (1), how do we treat the case above, "foo;a=1;b=2", "foo;a=1" or "foo;b=2"? How about cases like "bar;x=3, bar;x=4"?
>> 
>> This seems like a blackhole that the spec has missed describing, but anyway we need to determine what is the expected behavior for such cases.
> 
> I agree it's somewhat unclear.
> 
> For now, I think extensions should determine how to treat these cases. Extensions may reject duplicate response or may override parameters, depending on what the corresponding spec requires. For example, "deflate-frame" extension will reject duplicate response because it uses a reserved frame flag and duplicate response doesn't make sense. The current implementation just delegates validation to extensions.

Hm I'm okay with leaving the validation to each extension, but could you add comments describing this issue?

[Kenichi Ishibashi]

Comment on attachment 126186 [details]
added test rebaseline

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

>>>>>> Source/WebCore/websockets/WebSocketExtensions.cpp:224
>>>>>> +        }
>>>>> 
>>>>> I have two questions:
>>>>> 
>>>>> (1) What will happen if the server provides two extensions having the same extention identifier, such as "foo;a=1, foo;b=2"?
>>>>> (2) Why do you compare the extention name case-insensitively? I could not find such a requirement in the spec (but I may have missed it).
>>>> 
>>>> (1) If my reading is correct, RFC6455 allows such case. I think we should accept such response if the extension allows such duplication. The current implementation doesn't allow the case so I changed the behavior.
>>>> (2) You are right. Changed to just compare the tokens.
>>> 
>>> For (1), how do we treat the case above, "foo;a=1;b=2", "foo;a=1" or "foo;b=2"? How about cases like "bar;x=3, bar;x=4"?
>>> 
>>> This seems like a blackhole that the spec has missed describing, but anyway we need to determine what is the expected behavior for such cases.
>> 
>> I agree it's somewhat unclear.
>> 
>> For now, I think extensions should determine how to treat these cases. Extensions may reject duplicate response or may override parameters, depending on what the corresponding spec requires. For example, "deflate-frame" extension will reject duplicate response because it uses a reserved frame flag and duplicate response doesn't make sense. The current implementation just delegates validation to extensions.
> 
> Hm I'm okay with leaving the validation to each extension, but could you add comments describing this issue?

I've added a comment on WebSocketExtensionProcessor::processResponse(). If the comment isn't enough, I'll add comments here.

[WebKit Review Bot]

Comment on attachment 126264 [details]
Patch

Attachment 126264 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11487198

New failing tests:
platform/chromium/compositing/layout-width-change.html

[Kenichi Ishibashi]

Comment on attachment 126264 [details]
Patch

Rebaselined the test. Trying to cq+ again.

[WebKit Review Bot]

Comment on attachment 126264 [details]
Patch

Rejecting attachment 126264 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
git/webkit-commit-queue/Source/WebKit/chromium/ui --revision 121041 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
45>At revision 121041.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/11473219

[Kenichi Ishibashi]

Created attachment 126391 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 126391 [details]
Patch for landing

Clearing flags on attachment: 126391

Committed r107365: <http://trac.webkit.org/changeset/107365>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.