[Webkit-unassigned] [Bug 28038] WebSocket API implementation

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=28038


Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #38911|review?                     |review-
               Flag|                            |




--- Comment #21 from Alexey Proskuryakov <ap at webkit.org>  2009-09-02 12:06:42 PDT ---
(From update of attachment 38911)
> Other features in the same file has guard. Is it ok to remove the guard?

Looking at EventSource for example, I see that EventSource.idl is not present
in WebCore.gypi project file. So, Chromium build would break if not for this
ifdef in JSEventTarget.cpp.

However, it is generally better to generate idl files on all platforms, and
avoid the need for ifdefs around includes. Given that there is a lot of
precedent for ifdefs, this is not something I ask to fix right away, it was
just a suggestion for making he life a little easier.

> I removed code related to redirects and authentication for now.

I still see m_wwwAuthenticate and related code in the latest patch, as well as
Authenticate enum value. Do you think it's best to keep them for now? I'd just
make sure that the connection fails if authentication is requested, and remove
the code.

> not sure. by checking upgrade header and connection header first, we can find
> bad handshake as soon as possible.

Ok.

+    class CString;

No need to forward declare CString in WebSocketHandshake.h, there's already a
forward declaration in PlatformString.h that is included from it.

+    typedef void (WebSocket::*Method)(PassRefPtr<Event>);

The methods don't take ownership of the event, so a plain Event* should be used
for their arguments.

+    static PassRefPtr<ProcessWebSocketEventTask> create(PassRefPtr<WebSocket>
webSocket, Method method, PassRefPtr<Event> event)
+    {
+        return new ProcessWebSocketEventTask(webSocket, method, event);
+    }

This will leak - the object is created with initial refcount of 1, and
PassRefPtr constructor will referenced it again. Is this modeled after some
existing code?

The right way to return a new object is by using adoptRef.

+    virtual ~ProcessWebSocketEventTask() { }

This is equivalent to what a compiler would have generated, and thus should be
removed.

I see that you also changed port numbers to match updated spec, thanks! This is
pretty close to being landed, hopefully we only need one last iteration.

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