[webkit-reviews] review denied: [Bug 72016] [Chromium] [WebSocket] export WebSocketChannel interface for plugins : [Attachment 115458] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Nov 17 23:21:08 PST 2011
Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Takashi Toyoshima
<toyoshim at chromium.org>'s request for review:
Bug 72016: [Chromium] [WebSocket] export WebSocketChannel interface for plugins
https://bugs.webkit.org/show_bug.cgi?id=72016
Attachment 115458: Patch
https://bugs.webkit.org/attachment.cgi?id=115458&action=review
------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
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.
More information about the webkit-reviews
mailing list