Bug 78079 - [WebSocket] Add WebSocket extension support
Summary: [WebSocket] Add WebSocket extension support
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: 77522
  Show dependency treegraph
 
Reported: 2012-02-07 22:03 PST by Kenichi Ishibashi
Modified: 2012-02-09 22:44 PST (History)
7 users (show)

See Also:


Attachments
Patch (34.14 KB, patch)
2012-02-07 22:44 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch (34.77 KB, patch)
2012-02-07 23:07 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch (36.65 KB, patch)
2012-02-08 02:32 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
added test rebaseline (38.11 KB, patch)
2012-02-08 16:36 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch (44.12 KB, patch)
2012-02-09 01:06 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch (44.51 KB, patch)
2012-02-09 02:33 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch (44.40 KB, patch)
2012-02-09 02:45 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch for landing (44.32 KB, patch)
2012-02-09 15:40 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-07 22:03:10 PST
Add WebSocket extension support.
Comment 1 Kenichi Ishibashi 2012-02-07 22:44:00 PST
Created attachment 126007 [details]
Patch
Comment 2 Kenichi Ishibashi 2012-02-07 22:49:01 PST
(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.
Comment 3 Gyuyoung Kim 2012-02-07 22:59:55 PST
Comment on attachment 126007 [details]
Patch

Attachment 126007 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11460275
Comment 4 Kenichi Ishibashi 2012-02-07 23:07:50 PST
Created attachment 126012 [details]
Patch
Comment 5 Yuta Kitamura 2012-02-07 23:35:06 PST
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.
Comment 6 Kenichi Ishibashi 2012-02-08 02:32:15 PST
Created attachment 126036 [details]
Patch
Comment 7 Kenichi Ishibashi 2012-02-08 02:34:18 PST
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.
Comment 8 WebKit Review Bot 2012-02-08 03:42:53 PST
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
Comment 9 Kenichi Ishibashi 2012-02-08 16:36:59 PST
Created attachment 126186 [details]
added test rebaseline
Comment 10 WebKit Review Bot 2012-02-08 18:11:24 PST
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
Comment 11 Kenichi Ishibashi 2012-02-08 20:35:29 PST
Hi Alexey, Kent-san,

Could you please review the patch?
Comment 12 Kent Tamura 2012-02-08 22:08:02 PST
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.
Comment 13 Yuta Kitamura 2012-02-08 22:14:41 PST
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).
Comment 14 Kenichi Ishibashi 2012-02-09 01:06:19 PST
Created attachment 126255 [details]
Patch
Comment 15 Kenichi Ishibashi 2012-02-09 01:08:53 PST
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.
Comment 16 Kent Tamura 2012-02-09 01:30:12 PST
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?
Comment 17 Kent Tamura 2012-02-09 01:34:29 PST
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?
Comment 18 Yuta Kitamura 2012-02-09 01:43:08 PST
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.
Comment 19 Kenichi Ishibashi 2012-02-09 02:33:23 PST
Created attachment 126261 [details]
Patch
Comment 20 Kenichi Ishibashi 2012-02-09 02:35:06 PST
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.
Comment 21 Kent Tamura 2012-02-09 02:37:47 PST
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().
Comment 22 Kenichi Ishibashi 2012-02-09 02:39:39 PST
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.
Comment 23 Kenichi Ishibashi 2012-02-09 02:45:38 PST
Created attachment 126264 [details]
Patch
Comment 24 Kenichi Ishibashi 2012-02-09 02:46:37 PST
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.
Comment 25 Kent Tamura 2012-02-09 02:51:36 PST
Comment on attachment 126264 [details]
Patch

Ok
Comment 26 Kenichi Ishibashi 2012-02-09 02:52:42 PST
Comment on attachment 126264 [details]
Patch

Thank you!
Comment 27 Yuta Kitamura 2012-02-09 02:53:31 PST
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?
Comment 28 Kenichi Ishibashi 2012-02-09 03:09:10 PST
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.
Comment 29 WebKit Review Bot 2012-02-09 03:21:21 PST
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
Comment 30 Kenichi Ishibashi 2012-02-09 04:07:43 PST
Comment on attachment 126264 [details]
Patch

Rebaselined the test. Trying to cq+ again.
Comment 31 WebKit Review Bot 2012-02-09 05:45:17 PST
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
Comment 32 Kenichi Ishibashi 2012-02-09 15:40:55 PST
Created attachment 126391 [details]
Patch for landing
Comment 33 WebKit Review Bot 2012-02-09 22:44:46 PST
Comment on attachment 126391 [details]
Patch for landing

Clearing flags on attachment: 126391

Committed r107365: <http://trac.webkit.org/changeset/107365>
Comment 34 WebKit Review Bot 2012-02-09 22:44:53 PST
All reviewed patches have been landed.  Closing bug.