Bug 72016 – [Chromium] [WebSocket] export WebSocketChannel interface for plugins

• Component: WebKit API
• Status: RESOLVED FIXED

[Takashi Toyoshima]

WebSocket feature is implemented as WebSocket class and it uses WebSocketChannel class which implements WebSocket communication protocol.
Only SocketStreamHandle is exported as a WebKit API just to implement peer-to-peer bi-directional communication port via IPC.

Now, Chromium needs WebSocketChannel as a WebKit API to use WebSocket protocol via plugins interface.

Chromium side issue,
http://code.google.com/p/chromium/issues/detail?id=103719

[Takashi Toyoshima]

Created attachment 114489 [details]
interface plan

I'm planning to implement the interface like this.

Darin, could you look into this and check its implementation direction?
I'll start implementation tomorrow.

Thanks,

[Takashi Toyoshima]

Created attachment 114904 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Yuta Kitamura]

Comment on attachment 114904 [details]
Patch

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.

[Darin Fisher (:fishd, Google)]

Comment on attachment 114904 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114904&action=review

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

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

I have the same question.

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

[Takashi Toyoshima]

Created attachment 115112 [details]
Patch

[Takashi Toyoshima]

Comment on attachment 114904 [details]
Patch

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.

[Yuta Kitamura]

Comment on attachment 114904 [details]
Patch

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.

OK.

[Yuta Kitamura]

The patch sounds OK to me. [Note: I'm not a WebKit reviewer yet.]

[Takashi Toyoshima]

Thanks Yuta.

So, Darin,
I'd be happy if you could review this change again.

My best regards on your precious time.

[Sam Weinig]

It seems odd to me that you are adding new stuff to the WebCore namespace not in the WebCore directory.

[Takashi Toyoshima]

Oh, I see.
I read MediaPlayerPrivateChromium.cpp but I miss MediaPlayerPrivate itself is implemented in WebCore.
So, should I move them into WebCore/websocket/?

[Takashi Toyoshima]

I have another idea to merge WebSocketChannelClientPrivate to WebWebSocketChannelImpl.
It could be more reasonable?

[Takashi Toyoshima]

Created attachment 115216 [details]
Patch

[Darin Fisher (:fishd, Google)]

(In reply to comment #7)
> 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.

Wow, that's really unfortunate.  Normally, WebCore stuff does not use the Web* prefix.  I wish we could just call the WebCore types SocketChannel, dropping the Web prefix there.  But, assuming that is not possible, then how about just using a different name for the WebKit API?  Maybe you could just call this WebSocket?

I realize that WebCore also defines WebSocket, but it looks like that may not cause a conflict here since you are only exposing the lower-level WebSocketChannel functionality.  To the embedder, referring to the channel as a WebSocket won't seem strange.

[Takashi Toyoshima]

Created attachment 115458 [details]
Patch

[Takashi Toyoshima]

Thanks Darin,
Your suggestion works fine.
I revised my patch based on your suggestion, s/WebWebSocketChannel/WebSocket/g.

I'd happy if you could review it again.

[Darin Fisher (:fishd, Google)]

Comment on attachment 115458 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115458&action=review

> Source/WebKit/chromium/ChangeLog:11
> +        exported as WebWebSocketChannel because it is the class which implement

This ChangeLog entry needs to be revised given the renaming.

> Source/WebKit/chromium/public/WebSocket.h:37
> +namespace WebCore {

nit: In other header files, we just do this all on one line:

  namespace WebCore { class WebSocketChannel; }

I think that might be nicer here too--at least for consistency sake.

> Source/WebKit/chromium/public/WebSocket.h:66
> +    static WebSocket* create(WebDocument*, WebSocketClient*);

nit: you need to add WEBKIT_EXPORT to static methods so that they get exported from WebKit when it is built as a DLL (to support the component build).

nit: I recommend putting a new line above the 'create' function to separate it from the enum.

nit: Pass WebDocument as |const WebDocument&|

> Source/WebKit/chromium/public/WebSocketClient.h:46
> +    };

same nit: add new line after enum definition.

> Source/WebKit/chromium/src/WebSocketImpl.cpp:62
> +    ScriptExecutionContext* context = static_cast<PassRefPtr<Document> >(*document).get();

Hmm... too bad WebSocketChannel::create doesn't take a PassRefPtr type...

How about this instead:

  m_private = WebSocketChannel::create(PassRefPtr<Document>(document).get(), this);

> Source/WebKit/chromium/src/WebSocketImpl.cpp:93
> +    if (!m_private)

see WebNode.cpp where we don't bother with these null checks.  WebSocket::create() should probably return NULL if we were unable to create a m_private.  that way you shouldn't have to worry about m_private being null here and in other methods.

you can decide separately if you want to allow construction of a WebSocket with a null m_client.

[Takashi Toyoshima]

Created attachment 115761 [details]
Patch

[Takashi Toyoshima]

Comment on attachment 115458 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115458&action=review

>> Source/WebKit/chromium/ChangeLog:11
>> +        exported as WebWebSocketChannel because it is the class which implement
> 
> This ChangeLog entry needs to be revised given the renaming.

Sorry. I missed it.

>> Source/WebKit/chromium/public/WebSocket.h:37
>> +namespace WebCore {
> 
> nit: In other header files, we just do this all on one line:
> 
>   namespace WebCore { class WebSocketChannel; }
> 
> I think that might be nicer here too--at least for consistency sake.

Done.

I expanded it because check-webkit-style show a error on that before.
But, now I fixed and re-ran check-webkit-style, then it pass without any error.
Sorry for bothering.

>> Source/WebKit/chromium/public/WebSocket.h:66
>> +    static WebSocket* create(WebDocument*, WebSocketClient*);
> 
> nit: you need to add WEBKIT_EXPORT to static methods so that they get exported from WebKit when it is built as a DLL (to support the component build).
> 
> nit: I recommend putting a new line above the 'create' function to separate it from the enum.
> 
> nit: Pass WebDocument as |const WebDocument&|

Done.

>> Source/WebKit/chromium/public/WebSocketClient.h:46
>> +    };
> 
> same nit: add new line after enum definition.

Done.

>> Source/WebKit/chromium/src/WebSocketImpl.cpp:62
>> +    ScriptExecutionContext* context = static_cast<PassRefPtr<Document> >(*document).get();
> 
> Hmm... too bad WebSocketChannel::create doesn't take a PassRefPtr type...
> 
> How about this instead:
> 
>   m_private = WebSocketChannel::create(PassRefPtr<Document>(document).get(), this);

Thank you for good suggestion.
That work fine and looks better than before.

[Takashi Toyoshima]

Greetings,

Darin, thank you for your precious review.

I revised again.
I'd be happy if you have time to review it again.

Thank you so much.

[Darin Fisher (:fishd, Google)]

Comment on attachment 115761 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115761&action=review

> Source/WebKit/chromium/src/WebSocket.cpp:43
> +    WebSocketImpl* websocket = new WebSocketImpl(document, client);

nit: WebKit style is to use OwnPtr here.

  OwnPtr<WebSocketImpl> websocket = adoptPtr(new WebSocketImpl(document, client));
  if (websocket && websocket->isNull())
      return 0;
  return websocket.leakPtr();

[Takashi Toyoshima]

Created attachment 115887 [details]
Patch

[Takashi Toyoshima]

Oh, I miss your r+ because I upload new patch.
New patch is just changed by using OwnPtr as you mentioned.

I'm happy if you or another guy could make it r+ and cq+?
Because I'm not a WebKit committer yet.

[WebKit Review Bot]

Comment on attachment 115887 [details]
Patch

Clearing flags on attachment: 115887

Committed r100849: <http://trac.webkit.org/changeset/100849>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.