[webkit-reviews] review denied: [Bug 65247] WebSocket: Accept multiple subprotocols : [Attachment 103327] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Yuta Kitamura
<yutak at chromium.org>'s request for review:
Bug 65247: WebSocket: Accept multiple subprotocols
https://bugs.webkit.org/show_bug.cgi?id=65247

Attachment 103327: Patch
https://bugs.webkit.org/attachment.cgi?id=103327&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=103327&action=review


> Source/WebCore/websockets/WebSocket.cpp:59
> +static bool isSeparator(UChar character)

This should be 'inline'

> Source/WebCore/websockets/WebSocket.cpp:66
> +    if (character == '(' || character == ')' || character == '<' ||
character == '>' || character == '@'
> +	   || character == ',' || character == ';' || character == ':' ||
character == '\\' || character == '"'
> +	   || character == '/' || character == '[' || character == ']' ||
character == '?' || character == '='
> +	   || character == '{' || character == '}' || character == ' ' ||
character == '\t')
> +	   return true;
> +    return false;

You can write just
  return character == '(' ....

Test coverage for this code is very low.

Let's improve the performance of this code by checking "character is smaller
than the minimum separator character" and "character is larger than the minimum
separator character".

> Source/WebCore/websockets/WebSocket.cpp:75
> +    for (size_t i = 0; i < protocol.length(); ++i)
> +	   if (protocol[i] < 0x21 || protocol[i] > 0x7E ||
isSeparator(protocol[i]))
> +	       return false;

* You have to enclose the content of 'for' with {} because the content has two
physical lines.

* What are 0x21 and 0x7E?

> Source/WebCore/websockets/WebSocket.cpp:182
> +	       // Emulate JavaScript's Array.toString() behavior.
> +	       StringBuilder builder;

I have a question.  Should we support protocols-in-array in Hixie76?

> Source/WebCore/websockets/WebSocket.cpp:206
> +	   for (size_t i = 0; i < protocols.size(); ++i)
> +	       if (!isValidProtocolString(protocols[i])) {
> +		   scriptExecutionContext()->addMessage(JSMessageSource,
LogMessageType, ErrorMessageLevel, "Wrong protocol for WebSocket '" +
encodeProtocolString(protocols[i]) + "'", 0,
scriptExecutionContext()->securityOrigin()->toString(), 0);

You have to enclose the 'for' content with {}.

> Source/WebCore/websockets/WebSocket.cpp:218
> +	   for (size_t i = 0; i < protocols.size(); ++i)
> +	       for (size_t j = i + 1; j < protocols.size(); ++j)
> +		   if (protocols[i] == protocols[j]) {
> +		       scriptExecutionContext()->addMessage(JSMessageSource,
LogMessageType, ErrorMessageLevel, "WebSocket protocols contain duplicates: '"
+ encodeProtocolString(protocols[i]) + "'", 0,
scriptExecutionContext()->securityOrigin()->toString(), 0);
> +		       m_state = CLOSED;
> +		       ec = SYNTAX_ERR;
> +		       return;
> +		   }

O(N^2) algorithm is not acceptable.

> Source/WebCore/websockets/WebSocket.cpp:228
> +	   if (!protocols.isEmpty()) {
> +	       StringBuilder builder;
> +	       for (size_t i = 0; i < protocols.size(); ++i) {
> +		   if (i)
> +		       builder.append(", ");
> +		   builder.append(protocols[i]);
> +	       }
> +	       protocolString = builder.toString();
> +	   }

This phrase appears twice in this function.