Bug 28038 – WebSocket API implementation

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Fumitoshi Ukai]

WebSocket API implementation based on SocketStreamHandle (cf. http://bugs.webkit.org/show_bug.cgi?id=28037)

[Fumitoshi Ukai]

Created attachment 34548 [details]
WebSocket API implementation.  depends on bug 28037


---
 20 files changed, 1277 insertions(+), 71 deletions(-)

[Alexey Proskuryakov]

Comment on attachment 34548 [details]
WebSocket API implementation.  depends on bug 28037

+    $(WebCore)/websockets \
     $(WebCore)/svg \
     $(WebCore)/websockets \

It's already there.

+	WebCore/websockets/WebSocketRequest.cpp \
+	WebCore/websockets/WebSocketRequest.h \
+	WebCore/websockets/WebSocketResponse.cpp \
+	WebCore/websockets/WebSocketResponse.h

This looks quite wrong - WebSocket does not use a request/response model, so such class names are highly misguiding.

+#if ENABLE(WEB_SOCKETS)
+#include "JSWebSocket.h"
+#include "WebSocket.h"

In some places, generated files are included without guards, and in others, with guards. The guards are only needed if the headers are not generated in ports that do not have this feature enabled - otherwise, similar guards within headers themselves would do the job.

There's never a need to protect e.g. WebSocket.h with external include guards.

+bool IsValidProtocolString(const WebCore::String& protocol)

This should be static, and inside the WebCore namespace. We don't normally use anonymous namespaces.

+    // FIXME: need dispatchEvent?

Absolutely - it will dispatch to handlers installed via addEventListener(), as opposed to setting socket.onopen or similar.

+bool CheckWebSocketHandshake(const WebCore::WebSocketRequest &req, const WebCore::WebSocketResponse& resp)

Capitalization, argument naming, '&' positioning. This should also be static.

There are many style issues in protocol implementation, I'm not pointing out most, because they are the same as in other patches.

+            // FIXME: handle overflow.

Please do!

+KURL WebSocketRequest::URLoverHTTP() const

In WebKit style, this would be urlOverHTTP. I'm not sure what this is use for though, hopefully an even better name can be found.

+    if (url().hasFragmentIdentifier()) {
+        builder.append("#");
+        builder.append(url().fragmentIdentifier());
+    }

What are fragment identifiers good for when using WebSockets? Seems that they should be forbidden, or dropped immediately when passed in.

+const char* WebSocketResponse::parseHeader(const char* start, const char* end, Vector<std::pair<String, String> >* headers)

Are WebSockets headers case sensitive? Otherwise, it may be better to use HTTPHeaderMap.

I didn't closely review all the code; please fix style, possibly using check-webkit-style script. The direction looks good to me, except for request/response classes.

[Fumitoshi Ukai]

(In reply to comment #2)
> (From update of attachment 34548 [details])
> +    $(WebCore)/websockets \
>      $(WebCore)/svg \
>      $(WebCore)/websockets \
> 
> It's already there.
> 
> +    WebCore/websockets/WebSocketRequest.cpp \
> +    WebCore/websockets/WebSocketRequest.h \
> +    WebCore/websockets/WebSocketResponse.cpp \
> +    WebCore/websockets/WebSocketResponse.h
> 
> This looks quite wrong - WebSocket does not use a request/response model, so
> such class names are highly misguiding.

Hmm, how about WebSocketClientHandshake and WebSocketServerHandshake?
What name do you recommend?

> 
> +#if ENABLE(WEB_SOCKETS)
> +#include "JSWebSocket.h"
> +#include "WebSocket.h"
> 
> In some places, generated files are included without guards, and in others,
> with guards. The guards are only needed if the headers are not generated in
> ports that do not have this feature enabled - otherwise, similar guards within
> headers themselves would do the job.
> 
> There's never a need to protect e.g. WebSocket.h with external include guards.
> 
> +bool IsValidProtocolString(const WebCore::String& protocol)
> 
> This should be static, and inside the WebCore namespace. We don't normally use
> anonymous namespaces.
> 
> +    // FIXME: need dispatchEvent?
> 
> Absolutely - it will dispatch to handlers installed via addEventListener(), as
> opposed to setting socket.onopen or similar.

Should I add the implementation in this patch, or ok in another patch?

> 
> +bool CheckWebSocketHandshake(const WebCore::WebSocketRequest &req, const
> WebCore::WebSocketResponse& resp)
> 
> Capitalization, argument naming, '&' positioning. This should also be static.
> 
> There are many style issues in protocol implementation, I'm not pointing out
> most, because they are the same as in other patches.
> 
> +            // FIXME: handle overflow.
> 
> Please do!

What is the good way to handle overflow?
May I close the connection if too long length is detected?

Do we need to set maximum length of m_buffer? (512KB or so?)

> 
> +KURL WebSocketRequest::URLoverHTTP() const
> 
> In WebKit style, this would be urlOverHTTP. I'm not sure what this is use for
> though, hopefully an even better name can be found.
> 
> +    if (url().hasFragmentIdentifier()) {
> +        builder.append("#");
> +        builder.append(url().fragmentIdentifier());
> +    }
> 
> What are fragment identifiers good for when using WebSockets? Seems that they
> should be forbidden, or dropped immediately when passed in.

Sure.
 
> +const char* WebSocketResponse::parseHeader(const char* start, const char* end,
> Vector<std::pair<String, String> >* headers)
> 
> Are WebSockets headers case sensitive? Otherwise, it may be better to use
> HTTPHeaderMap.

Thanks for suggestion.

> 
> I didn't closely review all the code; please fix style, possibly using
> check-webkit-style script. The direction looks good to me, except for
> request/response classes.

I used check-webkit-style script, but it didn't catch some style issues.

[Alexey Proskuryakov]

> Hmm, how about WebSocketClientHandshake and WebSocketServerHandshake?
> What name do you recommend?

That's less misleading. I was thinking of having a single class to handle handshake, not a pair of classes, but I didn't fully think it through, so it may be a bad idea.

> > +    // FIXME: need dispatchEvent?
>
> Should I add the implementation in this patch, or ok in another patch?

I think it would be easiest to do it now, while this incomplete code is clearly visible in a patch - hunting it down in trunk will be more difficult. Please also make a test for addEventListener-installed listeners (as usual, to be landed once the code becomes functional).

> What is the good way to handle overflow?
> May I close the connection if too long length is detected?

Sounds good to me. We probably want to log something to console in this case (and in other error cases that can occur in WebSocket lifetime).

> Do we need to set maximum length of m_buffer? (512KB or so?)

It would probably be nice to use tryFastMalloc (to close the connection instead of crashing if the server sends too large frame). If we were to add an artificial limit on received frame size, I think that it should be impractically huge, dozens of megabytes on desktop platforms - all we want is to prevent crashing the browser with an incorrect frame size, right?

This doesn't seem like something that could happen during normal operation, it's just a protection from server bugs, and it makes DoS'ing the browser via WebSocket protocol more difficult.

[Alexey Proskuryakov]

Bugzilla has experienced amnesia today, please upload your updated patch again.

[Fumitoshi Ukai]

Created attachment 38465 [details]
WebSocket API implementation. depends on bug 28037


---
 17 files changed, 1276 insertions(+), 78 deletions(-)

[Fumitoshi Ukai]

Created attachment 38471 [details]
WebSocket API implementation. depends on bug 28037


---
 16 files changed, 1278 insertions(+), 78 deletions(-)

[Alexey Proskuryakov]

Comment on attachment 38471 [details]
WebSocket API implementation. depends on bug 28037

+static bool IsValidProtocolString(const WebCore::String& protocol)

Function names should start with lowercase letters.

+    if (!protocol.isEmpty() && !IsValidProtocolString(protocol)) {

Would it make sense to move isEmpty() check to the helper function?

+WebSocketChannel::WebSocketChannel(ScriptExecutionContext* context, WebSocketChannelClient* client, const KURL& url, const String& protocol)
+        : m_context(context)
+        , m_client(client)
+        , m_handshaker(WebSocketHandshaker(url, protocol, context))
+        , m_buffer(0)
+        , m_buffer_size(0)

Please use 4 spaces for indentation.

+    static const char frame_type[1] = {'\0'};
+    static const char frame_end[1] = {'\xff'};

No need to have these static arrays - there's a version of Vector::append that takes const U&.

+        virtual void didReceivedData(SocketStreamHandle*, const char*, int);

Typo: should be didReceiveData.

+    ASSERT(handle == m_handle.get() || !m_handle.get());

What are the situations when callbacks are called with null m_handle? Is it normal, or could it indicate problems?

+    KURL url(location);
+    m_handshaker.setURL(url);

I'd write this in one line.

+void WebSocketChannel::authenticate(const String& www_authenticate)

This will break the build, because www_authenticate is unused.

Also, this is wrong style, we use camelCase, not underlines for function and variable names. Please grep your patch with some expression like "[a-ln-z]_" to catch all cases, I won't point out each of them, as there are too many.

+#include "WebSocketHandshaker.h"

I had to google "handshaker", and although it's a word indeed, it seems to have too many unrelated meanings :). I think that "WebSocketHandshake" would be just fine.

+#include "StringBuilder.h"
+
+#include <wtf/Vector.h>

No need for an empty line here.

+        return WebCore::String();

This code is already in WebCore namespace, no need for the prefix. Same issue below.

+bool WebSocketHandshaker::headerProcess(const HTTPHeaderMap& headers)

Maybe this should be processHeader()?

[Fumitoshi Ukai]

Created attachment 38535 [details]
WebSocket API implementation. depends on bug 28037


---
 18 files changed, 1295 insertions(+), 78 deletions(-)

[Alexey Proskuryakov]

Coding style looks reasonably close to me now. One thing I did notice is that "Websocket" is still used in several places - since it's two words, it should be "WebSocket".

The biggest challenge left is to discuss naming of data and methods in WebSocketHandshake - currently, there are several of names that seem difficult to understand or maybe even misleading. It's also not yet clear to me how redirects and authentication are supported. I'll need to read the relevant parts of the spec more closely.

[Fumitoshi Ukai]

Created attachment 38594 [details]
WebSocket API implementation. depends on bug 28037


---
 18 files changed, 1357 insertions(+), 78 deletions(-)

[Fumitoshi Ukai]

Created attachment 38723 [details]
WebSocket API implementation


---
 18 files changed, 1358 insertions(+), 79 deletions(-)

[Fumitoshi Ukai]

Created attachment 38806 [details]
WebSocket API implementation


---
 18 files changed, 1382 insertions(+), 79 deletions(-)

[Fumitoshi Ukai]

Thanks for review.

(In reply to comment #10)
> Coding style looks reasonably close to me now. One thing I did notice is that
> "Websocket" is still used in several places - since it's two words, it should
> be "WebSocket".
> 
> The biggest challenge left is to discuss naming of data and methods in
> WebSocketHandshake - currently, there are several of names that seem difficult
> to understand or maybe even misleading. It's also not yet clear to me how
> redirects and authentication are supported. I'll need to read the relevant
> parts of the spec more closely.

I've updated a patch.
Could you tell me what data and methods are difficult to understand or misleading?  I'll fix them.

For authentication, I think we need some interface to obtain Credential for the relevant URL over http, to get AuthenticationChallenge from Authorization header or something like that.  What do you think?

[Alexey Proskuryakov]

Comment on attachment 38806 [details]
WebSocket API implementation

 #include "JSWebKitPointConstructor.h"
+#include "JSWebSocketConstructor.h"
 #include "JSWorkerConstructor.h"

JSWebSocketConstructor.h doesn't have an ENABLE(WEB_SOCKETS) include guard inside, so this will break compilation (please add the guard).

+#if ENABLE(WEB_SOCKETS)
+#include "JSWebSocket.h"
+#include "WebSocket.h"
+#endif

It would likely be easier to generate WebSocket files on all platforms than to maintain ENABLE(WEB_SOCKETS) around header includes.

+        void connect(const KURL& url, ExceptionCode&);
+        void connect(const KURL& url, const String& protocol, ExceptionCode&);

Please remove url argument names - they do not convey additional information over the type name.

 void WebSocket::dispatchCloseEvent()
 {
-    // FIXME: implement this.
+    RefPtr<Event> evt = Event::create(eventNames().closeEvent, false, false);

According to the spec, the events for open and close should be queued, as opposed to dispatched synchronously. I'm not sure if this spec provision makes sense, but we should fix either the code of the spec.

+    , m_handshake(WebSocketHandshake(url, protocol, context))

This constructs a WebSocketHandshake object, and then copies it in place. Please make WebSocketHandshake noncopyable by inheriting from WTF::Noncopyable, and either keep it in an OwnPtr, or construct it in place.

+        case WebSocketHandshake::Redirect:
+            LOG(Network, "Redirect to %s", m_handshake.redirectLocation().utf8().data());
+            redirect(m_handshake.redirectLocation());
+            return;
+        case WebSocketHandshake::Authenticate:
+            authenticate(m_handshake.wwwAuthenticate());
+            return;

I'd suggest removing code related to redirects and authentication for now. Support for redirect has been removed from the spec, and authentication doesn't seem to be in an implementable state either.

There is no much harm in reading the headers, but it doesn't seem likely to me that authentication support will be implemented in the form of a synchronous authenticate() function, so it is somewhat confusing.

+const char webSocketUpgradeHeader[] = "Upgrade: WebSocket\r\n"
+        "Connection: Upgrade\r\n";

These are two headers, so the variable shouldn't make it look as if it's just one.

+static bool hadServerHandshake(const char* start, const char* end)

There are two problems with this function's name:
- due to the use of past tense, it sounds as if it consults some internal state for events that happened earlier;
- it's not clear what it means to have a server handshake (which turns out to be that it has been received in full).

This is a simple string processing function, a name like "contains two CRLF pairs" would be better. However, with a name like this, it becomes obvious that you can just use strnstr instead of a hand-rolled helper function.

+    builder.append(url().path());

Some functions in WebSocketHandshake use m_url, and others use url(). Please change it to use m_url consistently.

Same for host(), secure() and clientProtocol().

+    if (len < sizeof(webSocketServerHandshakeHeader) - 1) {
+        LOG(Network, "short response header len=%d", len);
+        return false;

This sounds more grave than it is - nothing is wrong with the header, it just hasn't been received fully yet. Maybe there is even no need for logging this.

+        LOG(Network, "incomplete server handshake len=%d", len);

Same here. Perhaps it would be better to check this before parsing other headers?

+        LOG(Network, "header process failed");

It is fine to have errors logged to console - but it is only available for WebKit developers. Some day, we should log these to Inspector console for the benefit of Web developers.

+size_t WebSocketHandshake::serverHandshakeLength() const
+{
+    return m_serverHandshakeLength;
+}

The need to have public accessors for low-level data members like this seems to indicate that the responsibilities between WebSocketHandshake and WebSocketChannel are divided less than perfectly. I don't have any specific suggestions though.

+KURL WebSocketHandshake::urlOverHTTP() const

I'd call this httpURLForAuthenticationAndCookies() - a bit wordy, but this method isn't going to be used often.

+const char* WebSocketHandshake::parseHeader(const char* start, const char* end, HTTPHeaderMap* headers)

This function doesn't parse one header, it parses all of them, so a name like "readHTTPHeaders()" would be more appropriate. A comment should explain that it reads all headers except for the two predefined ones.

+                    LOG(Network, "CF doesn't follow LF p=%p end=%p", p, end);

CR

+            LOG(Network, "CR doesn't follow LF after value p=%p end=%p", p, end);

This sounds like too much detail - I think that it's sufficient for a WebKit developer to know that there's something wrong with handshake - from there, they could look at a tcpdump trace. Seems fine to leave this in for now, as both server and client are being developed at once, but the logging should be revisited in the future. 

+    LOG(Network, "Unexpected end of header");

Is this even possible? We have already checked that there's CRLFCRLF before calling this function.

+            if (it->first == "websocket-origin") {
+                if (!m_wsOrigin.isNull())
+                    return false;

Seems that this check is misplaced - there's always no more than one header with a given name in an HTTPHeaderMap, so uniqueness of headers should be verified before storing them in the map.

+        default:
+            LOG(Network, "unexpected mode: %d", m_mode);
+            return false;

We usually leave the default clause out to let the compiler complain about missing cases. Then, we put ASSERT_NOT_REACHED below the switch (all cases should end with "return" for this to work).

+void WebSocketHandshake::handshake()

I'd call this checkResponseHeaders().

+#include "CString.h"

CString is only used as a return value, I think that a forward declaration would be sufficient.

+        WebSocketHandshake(const KURL&, const String& protocol, ScriptExecutionContext* context);

Please leave out "context" (but not "protocol").

+        void setURL(const KURL& url);

Please leave out "url". There are many cases like this below, I won't enumerate them all.

Almost all of the comments above are pretty straightforward - the only major comment I had was about dividing responsibilities between handshake and channel classes. I don't have any ideas right now, but I'll try to think about this more, and if you have any, please don't hesitate to make suggestions, of course.

[Fumitoshi Ukai]

Thanks for review.

(In reply to comment #15)

> +#if ENABLE(WEB_SOCKETS)
> +#include "JSWebSocket.h"
> +#include "WebSocket.h"
> +#endif
> 
> It would likely be easier to generate WebSocket files on all platforms than to
> maintain ENABLE(WEB_SOCKETS) around header includes.

Other features in the same file has guard. Is it ok to remove the guard?

>  void WebSocket::dispatchCloseEvent()
>  {
> -    // FIXME: implement this.
> +    RefPtr<Event> evt = Event::create(eventNames().closeEvent, false, false);
> 
> According to the spec, the events for open and close should be queued, as
> opposed to dispatched synchronously. I'm not sure if this spec provision makes
> sense, but we should fix either the code of the spec.

Ah, that's right. I think event for message also should be queued.

> +        case WebSocketHandshake::Redirect:
> +            LOG(Network, "Redirect to %s",
> m_handshake.redirectLocation().utf8().data());
> +            redirect(m_handshake.redirectLocation());
> +            return;
> +        case WebSocketHandshake::Authenticate:
> +            authenticate(m_handshake.wwwAuthenticate());
> +            return;
> 
> I'd suggest removing code related to redirects and authentication for now.
> Support for redirect has been removed from the spec, and authentication doesn't
> seem to be in an implementable state either.

Thanks for pointing out redirect has been removed from the spec.
I removed code related to redirects and authentication for now.

> +static bool hadServerHandshake(const char* start, const char* end)
> 
> There are two problems with this function's name:
> - due to the use of past tense, it sounds as if it consults some internal state
> for events that happened earlier;
> - it's not clear what it means to have a server handshake (which turns out to
> be that it has been received in full).
> 
> This is a simple string processing function, a name like "contains two CRLF
> pairs" would be better. However, with a name like this, it becomes obvious that
> you can just use strnstr instead of a hand-rolled helper function.

I don't think strnstr exists in common.  I found memmem, but it is GNU extension.
Renamed containsTwoCRLFPairs().


> +        LOG(Network, "incomplete server handshake len=%d", len);
> 
> Same here. Perhaps it would be better to check this before parsing other
> headers?

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

> +        LOG(Network, "header process failed");
> 
> It is fine to have errors logged to console - but it is only available for
> WebKit developers. Some day, we should log these to Inspector console for the
> benefit of Web developers.

Yes, that would be great. Will do in later.

> +size_t WebSocketHandshake::serverHandshakeLength() const
> +{
> +    return m_serverHandshakeLength;
> +}
> 
> The need to have public accessors for low-level data members like this seems to
> indicate that the responsibilities between WebSocketHandshake and
> WebSocketChannel are divided less than perfectly. I don't have any specific
> suggestions though.

Changed to return header length in readServerHandshake() (or return -1 if it has not receive full response header).

[Fumitoshi Ukai]

Created attachment 38849 [details]
WebSocket API implementation


---
 18 files changed, 1352 insertions(+), 84 deletions(-)

[Alexey Proskuryakov]

> I don't think strnstr exists in common. 

When some C library extension seems useful enough to use in WebKit, we usually add it to StringExtras in wtf (this is how we can use snprintf, for example).

[Fumitoshi Ukai]

(In reply to comment #18)
> > I don't think strnstr exists in common. 
> 
> When some C library extension seems useful enough to use in WebKit, we usually
> add it to StringExtras in wtf (this is how we can use snprintf, for example).

I see.
File a bug as http://bugs.webkit.org/show_bug.cgi?id=28901

[Fumitoshi Ukai]

Created attachment 38911 [details]
WebSocket API implementation


---
 18 files changed, 1336 insertions(+), 84 deletions(-)

[Alexey Proskuryakov]

Comment on attachment 38911 [details]
WebSocket API implementation

> 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.

[Fumitoshi Ukai]

Created attachment 38961 [details]
WebSocket API implementation


---
 18 files changed, 1310 insertions(+), 84 deletions(-)

[Fumitoshi Ukai]

Created attachment 38963 [details]
WebSocket API implementation


---
 18 files changed, 1309 insertions(+), 84 deletions(-)

[Alexey Proskuryakov]

Comment on attachment 38963 [details]
WebSocket API implementation

> +        OwnPtr<char> m_buffer;
> +        int m_bufferSize;

The buffer is allocated with tryFastMalloc, but freed with delete, which will cause problems on some configurations - in debug mode, it will be allocated with malloc, but freed with delete. OwnPtr is not suitable for holding arrays of data, I think that you may need to work with plain pointers here.

I should have noticed this earlier, sorry!

[Darin Adler]

Comment on attachment 38963 [details]
WebSocket API implementation

OwnArrayPtr can be used if you use "new (nothrow) char[]" to allocate the memory, but not if you use tryFastMalloc to allocate the memory.

[Fumitoshi Ukai]

Created attachment 39039 [details]
WebSocket API implementation


---
 18 files changed, 1312 insertions(+), 84 deletions(-)

[Fumitoshi Ukai]

(In reply to comment #24)
> (From update of attachment 38963 [details])
> > +        OwnPtr<char> m_buffer;
> > +        int m_bufferSize;
> 
> The buffer is allocated with tryFastMalloc, but freed with delete, which will
> cause problems on some configurations - in debug mode, it will be allocated
> with malloc, but freed with delete. OwnPtr is not suitable for holding arrays
> of data, I think that you may need to work with plain pointers here.
> 
> I should have noticed this earlier, sorry!

Oh, thanks for pointing it out!

[Fumitoshi Ukai]

(In reply to comment #25)
> (From update of attachment 38963 [details])
> OwnArrayPtr can be used if you use "new (nothrow) char[]" to allocate the
> memory, but not if you use tryFastMalloc to allocate the memory.

Thanks for info.
I found OwnFastMallocPtr and it can be used with tryFastMalloc, right?
But it doesn't have set(), so I use plain char* and fastFree().

it might be better to add set() and/or clear() in OwnFastMallocPtr?

[Alexey Proskuryakov]

Comment on attachment 39039 [details]
WebSocket API implementation

> +    if (m_buffer)
> +        fastFree(m_buffer);

It's safe to call fastFree on a null pointer (just like it's safe to call free() or delete).

r=me

[Fumitoshi Ukai]

Created attachment 39045 [details]
WebSocket API implementation


---
 18 files changed, 1311 insertions(+), 84 deletions(-)

[Eric Seidel (no email)]

Comment on attachment 39045 [details]
WebSocket API implementation

Rejecting patch 39045 from commit-queue.  This patch will require manual commit.

['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1

[Eric Seidel (no email)]

fast/dom/prototype-inheritance-2.html -> failed

That looks like a real failure.  The patch is likely missing some updated results.  Unless you can find a committer to update them when landing, you'll need to post a new patch with all updated results.

[Fumitoshi Ukai]

Created attachment 39134 [details]
WebSocket API implementation


---
 23 files changed, 1336 insertions(+), 84 deletions(-)

[Fumitoshi Ukai]

(In reply to comment #32)
> fast/dom/prototype-inheritance-2.html -> failed
> 
> That looks like a real failure.  The patch is likely missing some updated
> results.  Unless you can find a committer to update them when landing, you'll
> need to post a new patch with all updated results.

Sorry!
I found 4 layout tests failed on mac, but it's ok to rebaseline them (just because WebSocket constructor becomes available in window object).
Posted a new patch with all updated results.
I could pass all 11199 test cases succeeded on mac with this patch.

[Eric Seidel (no email)]

Comment on attachment 39134 [details]
WebSocket API implementation

Clearing flags on attachment: 39134

Committed r48121: <http://trac.webkit.org/changeset/48121>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.

[Andrew Wilson]

Reverted this patch because it broke the windows build.

We should not check this in until we have build implementations for all platforms, since WEB_SOCKETS is enabled on all platforms.

And, of course, you should stick around to make sure the build goes green after landing patches :)

[Fumitoshi Ukai]

Created attachment 39172 [details]
WebSocket API implementation. (add vcproj build)


---
 27 files changed, 1580 insertions(+), 97 deletions(-)

[Fumitoshi Ukai]

(In reply to comment #37)
> Reverted this patch because it broke the windows build.
> 
> We should not check this in until we have build implementations for all
> platforms, since WEB_SOCKETS is enabled on all platforms.
> 
> And, of course, you should stick around to make sure the build goes green after
> landing patches :)

I think I fixed the windows build.
Is there any other build I need to check?
Thanks!

[Andrew Wilson]

The windows build was the only one that was failing, I believe.

[Alexey Proskuryakov]

> We should not check this in until we have build implementations for all
> platforms, since WEB_SOCKETS is enabled on all platforms.

I'm confused - why is it enabled on all platforms? If that's the case, then all build systems need to be updated. The intention was to postpone enabling it on other platforms.

My guess is that Windows build was broken because files were added in DerivedSources.cpp. It's fine to update the Windows project file, but I'm worried whether it was done for the right reasons now, and I'm worried if other platforms are still affected.

Maybe some platforms also cannot find non-generated include files from the new websockets directory? In this case, at least search paths will need to be updated for all of them.

+        Fix windows build to r48121, that was reverted by r48137.

Since the patch was already reverted, this is not a build fix any more. This line can be just removed from all ChangeLogs.

[Andrew Wilson]

FYI, ENABLE_WEB_SOCKETS is enabled on every platform (from build-webkit):
    { option => "web-sockets", desc => "Toggle Web Sockets support",
      define => "ENABLE_WEB_SOCKETS", default => 1, value=> \$webSocketsSupport },

Also, part of this change updates test expections:

* fast/dom/Window/window-properties-expected.txt:
> +        * fast/dom/prototype-inheritance-2-expected.txt:
> +        * fast/dom/prototype-inheritance-expected.txt:
> +        * fast/js/global-constructors-expected.txt:

You can't do that if the feature is only enabled on some platforms.

[Fumitoshi Ukai]

Created attachment 39226 [details]
WebSocket API implementation. (add vcproj build)


---
 27 files changed, 1518 insertions(+), 90 deletions(-)

[Alexey Proskuryakov]

> You can't do that if the feature is only enabled on some platforms.

Actually, these tests are skipped on Qt. Not on Windows, though.

[Fumitoshi Ukai]

(In reply to comment #41)
> > We should not check this in until we have build implementations for all
> > platforms, since WEB_SOCKETS is enabled on all platforms.
> 
> I'm confused - why is it enabled on all platforms? If that's the case, then all
> build systems need to be updated. The intention was to postpone enabling it on
> other platforms.
> 
> My guess is that Windows build was broken because files were added in
> DerivedSources.cpp. It's fine to update the Windows project file, but I'm
> worried whether it was done for the right reasons now, and I'm worried if other
> platforms are still affected.
> 
> Maybe some platforms also cannot find non-generated include files from the new
> websockets directory? In this case, at least search paths will need to be
> updated for all of them.
> 
> +        Fix windows build to r48121, that was reverted by r48137.
> 
> Since the patch was already reverted, this is not a build fix any more. This
> line can be just removed from all ChangeLogs.

Fixed ChangeLogs.
Is there anything I need to fix more?(In reply to comment #41)
> > We should not check this in until we have build implementations for all
> > platforms, since WEB_SOCKETS is enabled on all platforms.
> 
> I'm confused - why is it enabled on all platforms? If that's the case, then all
> build systems need to be updated. The intention was to postpone enabling it on
> other platforms.
> 
> My guess is that Windows build was broken because files were added in
> DerivedSources.cpp. It's fine to update the Windows project file, but I'm
> worried whether it was done for the right reasons now, and I'm worried if other
> platforms are still affected.
> 
> Maybe some platforms also cannot find non-generated include files from the new
> websockets directory? In this case, at least search paths will need to be
> updated for all of them.
> 
> +        Fix windows build to r48121, that was reverted by r48137.
> 
> Since the patch was already reverted, this is not a build fix any more. This
> line can be just removed from all ChangeLogs.

Fixed ChangeLogs.
Is there anything I need to fix?

[Alexey Proskuryakov]

Which of the two patches that are marked for review is the right one? Please don't forget to obsolete old patches (I previously said that review- flags should not be removed when obsoleting, but obsoleting is important).

Since one of the build problems was that websockets/WebSocket.h could not be found, you likely need to add this directory to Qt and wx search paths. Also, per comment 42, this feature is enabled on all platforms, so you need to either disable it for Qt and wx, or to fully update their project files.

[Fumitoshi Ukai]

Created attachment 39338 [details]
WebSocket API implementation. (add vcproj build, fix qt and wx build)


---
 30 files changed, 1538 insertions(+), 94 deletions(-)

[Fumitoshi Ukai]

(In reply to comment #46)
> Which of the two patches that are marked for review is the right one? Please
> don't forget to obsolete old patches (I previously said that review- flags
> should not be removed when obsoleting, but obsoleting is important).

Sorry. Obsoleted old patches.  Do I need to obsolete a patch that was reverted, too?

> Since one of the build problems was that websockets/WebSocket.h could not be
> found, you likely need to add this directory to Qt and wx search paths. Also,
> per comment 42, this feature is enabled on all platforms, so you need to either
> disable it for Qt and wx, or to fully update their project files.

Added websockets directory in Qt and wx build systems.
Does it fix the build issues?

[Alexey Proskuryakov]

Comment on attachment 39338 [details]
WebSocket API implementation. (add vcproj build, fix qt and wx build)

I can't tell if it does, let's try landing this again.

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/projects/webkit/changeset/48270>.

[Alexey Proskuryakov]

Windows build fix in<http://trac.webkit.org/projects/webkit/changeset/48282>, crash on launch fix in <http://trac.webkit.org/projects/webkit/changeset/48285>.