[Webkit-unassigned] [Bug 72016] [Chromium] [WebSocket] export WebSocketChannel interface for plugins

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=72016





--- Comment #4 from Yuta Kitamura <yutak at chromium.org>  2011-11-14 05:06:49 PST ---
(From update of attachment 114904)
View in context: https://bugs.webkit.org/attachment.cgi?id=114904&action=review

[Note: I'm not a WebKit reviewer.]

> Source/WebCore/websockets/WebSocketChannel.h:72
> +    virtual bool send(const char* data, int length);

This overload might be confusing because it isn't clear whether this version sends a text message or a binary message; maybe we should rename the first overload to "sendText" and the other three to "sendBinary".

> Source/WebKit/chromium/public/WebWebSocketChannel.h:71
> +    virtual bool send(const WebString& message);
> +    virtual bool send(const WebData& binaryData);

The above two could be "sendText" and "sendBinary", as pointed out above.

> Source/WebKit/chromium/public/WebWebSocketChannel.h:75
> +    virtual void connect(const WebURL&, const WebString& protocol);
> +    virtual WebString subprotocol();
> +    virtual bool send(const WebString& message);
> +    virtual bool send(const WebData& binaryData);
> +    virtual unsigned long bufferedAmount() const;
> +    virtual void close(int code, const WebString& reason);
> +    virtual void fail(const WebString& reason);
> +    virtual void disconnect();

Why did you make these functions virtual?

> Source/WebKit/chromium/public/WebWebSocketChannel.h:79
> +    virtual ~WebWebSocketChannel();

I guess private destructor won't compile because nobody can call this destructor (hence nobody can free any instance). IMO this destructor has to be public.

> Source/WebKit/chromium/public/WebWebSocketChannelClient.h:47
> +    virtual ~WebWebSocketChannelClient() { };

Semicolon is not necessary.

> Source/WebKit/chromium/src/WebSocketChannelClientPrivate.h:48
> +    WebSocketChannelClientPrivate(WebKit::WebWebSocketChannelClient* client) : m_private(client) { };

Semicolon is not necessary.

> Source/WebKit/chromium/src/WebWebSocketChannel.cpp:74
> +      delete m_client;

- Wrong indent. (hey, why did our style checker overlook this?)

- I think it's recommended to use WebPrivateOwnPtr to avoid writing "delete" manually.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.