[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


Darin Fisher (:fishd, Google) <fishd at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #115458|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #18 from Darin Fisher (:fishd, Google) <fishd at chromium.org>  2011-11-17 23:21:08 PST ---
(From update of attachment 115458)
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.

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