[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 #7 from Takashi Toyoshima <toyoshim at chromium.org>  2011-11-15 00:30:08 PST ---
(From update of attachment 114904)
View in context: https://bugs.webkit.org/attachment.cgi?id=114904&action=review

>> 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".

I see, but I'd like to split these refactoring into another change.
Because that change will affect other WebSocket related files.
It make the purpose of this change vague.

>> Source/WebKit/chromium/public/WebWebSocketChannel.h:52
>> +class WebWebSocketChannel {
> 
> ouch, the "WebWeb" prefix makes my head hurt! ;-)
> 
> since this is in the WebKit namespace, can we just call this one WebSocketChannel too?

Now, I try to rename it as WebKit::WebSocketChannel and WebKit::WebSocketChannelClient.
But I have a name confliction problem.

We can access the lower prioritized header by #include_next directive. But, the directive is not used in WebKit now.

Fortunately, WebKit public headers have higher priority than WebCore headers.
And WebCore files can be accessed with 'websockets/' prefix like #include "websockets/WebSocketChannel.h".
But, in both cases, we must have another policy on ifdef idiom which is used to avoid duplicated include on the head of header files.
The first WebSocketChannel_h definition disallow to include the second one. It means we must define another unique name for public headers.

Do you have any idea on that?
For now, I keep class names unchanged in the next change.

>> Source/WebKit/chromium/public/WebWebSocketChannel.h:71
>> +    virtual bool send(const WebData& binaryData);
> 
> The above two could be "sendText" and "sendBinary", as pointed out above.

Done.

>>> Source/WebKit/chromium/public/WebWebSocketChannel.h:75
>>> +    virtual void disconnect();
>> 
>> Why did you make these functions virtual?
> 
> I have the same question.

Sorry, I mistakenly think that all public WebKit API methods must be virtual function.

But, now I decide to move implementation into WebSocketChannelImpl.{h,cpp}.
So, I keep them virtual functions.

>> 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.

I think caller just call disconnect() instead of delete, then this object itself calls 'delete this' in disconnect().
But it could bring confusion of object ownership.
I fixed.

>> Source/WebKit/chromium/public/WebWebSocketChannel.h:82
>> +    WebCore::WebSocketChannelClientPrivate* m_client;
> 
> it is a bit atypical to store an additional pointer on classes like this.
> 
> perhaps it would be better to make WebKit::WebSocketChannel be a class with pure virtual functions (like WebFrame), and then provide a WebKit::WebSocketChannel::create() function that returns a WebKit::WebSocketChannel pointer.  You would then create WebSocketChannelImpl.{h,cpp}.

Done.

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

Done.

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

Done.

>> 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.

> Wrong indent
Fixed.

> WebPrivateOwnPtr
As Darin suggested, I move its implementation into WebWebSocketChannelImpl.
As a result, I use RefPtr and OwnPtr for each member.

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