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
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,
Created attachment 114904 [details] Patch
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
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.
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}.
Created attachment 115112 [details] Patch
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.
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.
The patch sounds OK to me. [Note: I'm not a WebKit reviewer yet.]
Thanks Yuta. So, Darin, I'd be happy if you could review this change again. My best regards on your precious time.
It seems odd to me that you are adding new stuff to the WebCore namespace not in the WebCore directory.
Oh, I see. I read MediaPlayerPrivateChromium.cpp but I miss MediaPlayerPrivate itself is implemented in WebCore. So, should I move them into WebCore/websocket/?
I have another idea to merge WebSocketChannelClientPrivate to WebWebSocketChannelImpl. It could be more reasonable?
Created attachment 115216 [details] Patch
(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.
Created attachment 115458 [details] Patch
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.
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.
Created attachment 115761 [details] Patch
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.
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.
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();
Created attachment 115887 [details] Patch
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.
Comment on attachment 115887 [details] Patch Clearing flags on attachment: 115887 Committed r100849: <http://trac.webkit.org/changeset/100849>
All reviewed patches have been landed. Closing bug.