[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