[webkit-reviews] review denied: [Bug 78079] [WebSocket] Add WebSocket extension support : [Attachment 126186] added test rebaseline

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Kenichi Ishibashi
<bashi at chromium.org>'s request for review:
Bug 78079: [WebSocket] Add WebSocket extension support
https://bugs.webkit.org/show_bug.cgi?id=78079

Attachment 126186: added test rebaseline
https://bugs.webkit.org/attachment.cgi?id=126186&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
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.