Bug 27209 – Add WebSocket.idl

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Fumitoshi Ukai]

Add WebSocket.idl in WebCore/websockets/
http://dev.w3.org/html5/websockets/#the-websocket-interface

[Fumitoshi Ukai]

Created attachment 32653 [details]
Add WebSocket.idl


---
 2 files changed, 63 insertions(+), 0 deletions(-)

[Jeremy Orlow]

> +module websockets {
> +
> +    interface [
> +        CustomMarkFunction,
> +        NoStaticTables

Why does this need a custom mark function or no static tables?  (Not saying it doesn't..just wondering.)

Also, add Conditional=WEB_SOCKETS

> +    ] WebSocket {
> +        readonly attribute DOMString URL;
> +
> +        // ready state
> +        const unsigned short CONNECTING = 0;
> +        const unsigned short OPEN = 1;
> +        const unsigned short CLOSED = 2;
> +        readonly attribute long readyState;

Why are the constants shorts and the attribute a long?

> +
> +        // networking
> +        attribute EventListener onopen;
> +        attribute EventListener onmessage;
> +        attribute EventListener onclose;
> +
> +        [Custom] void send(in DOMString data)
> +          raises(DOMException);

Why does this need to be custom?  (Not saying it doesn't, just wondering.)

> +        void close();
> +    };
> +}

[Sam Weinig]

It is a bit hard to review this without the implementation file it binds and the custom bindings needed by the extended attributes.  Can you add that as well, even if only in a stubbed out form?

[Fumitoshi Ukai]

Created attachment 32705 [details]
add WebSocket.idl and its implementation and bindings


---
 7 files changed, 603 insertions(+), 0 deletions(-)

[Fumitoshi Ukai]

Thanks for review.

Added some implementation and binding code.


> Why does this need a custom mark function or no static tables?  (Not saying it doesn't..just wondering.)

I think custom mark function is necessary to prevent GC when it's open and active.
I'm not sure no static tables.  I just follow XMLHttpRequest.
Added Conditional=WEB_SOCKETS.

> Why are the constants shorts and the attribute a long?
not sure, but spec says so: http://dev.w3.org/html5/websockets/


If you want to see more implementation, please take a look at http://codereview.chromium.org/155079

Thanks

[Fumitoshi Ukai]

Created attachment 32916 [details]
add WebSocket.idl


---
 7 files changed, 629 insertions(+), 0 deletions(-)

[Alexey Proskuryakov]

(In reply to comment #5)
> I'm not sure no static tables.  I just follow XMLHttpRequest.

NoStaticTables is needed for interfaces that will be used from worker threads.

> If you want to see more implementation, please take a look at
> http://codereview.chromium.org/155079

I do not think that implementing Web Sockets elsewhere and pushing it into Bugzilla just for landing will work smoothly.

The most interesting aspect of WebSockets implementation, and one that I didn't see discussed yet, is what kind of back-end we need. It is not clear whether existing HTTP back-ends for supported platforms allow for the level of control needed for Web Sockets - it is possible that we'll actually need a new cross-platform HTTP implementation for these.

[Jeremy Orlow]

(In reply to comment #7)
> (In reply to comment #5)
> > I'm not sure no static tables.  I just follow XMLHttpRequest.
> 
> NoStaticTables is needed for interfaces that will be used from worker threads.
> 
> > If you want to see more implementation, please take a look at
> > http://codereview.chromium.org/155079
> 
> I do not think that implementing Web Sockets elsewhere and pushing it into
> Bugzilla just for landing will work smoothly.

I'm not sure I agree.  WebKit developers often seem to write the entire implementation, break it into patches, discuss each patch on its own merit, and iterate.  I believe that's precisely what's happening here.

> The most interesting aspect of WebSockets implementation, and one that I didn't
> see discussed yet, is what kind of back-end we need. It is not clear whether
> existing HTTP back-ends for supported platforms allow for the level of control
> needed for Web Sockets - it is possible that we'll actually need a new
> cross-platform HTTP implementation for these.

My understanding is that this code has been tested end-to-end on at least one platform, so it's possible that won't be necessary.

If, however, there are http stack limitations, Chromium recently wrote a cross platform network stack that might be useful here.  It's my understanding that it was written to have minimal dependencies on Chromium as well.

But aren't we getting ahead of ourselves?  This bug is for adding the idl files.  This needs to happen whether or not radical changes to the http implementation are necessary, right?

[Alexey Proskuryakov]

> But aren't we getting ahead of ourselves?  This bug is for adding the idl
> files.  This needs to happen whether or not radical changes to the http
> implementation are necessary, right?

Do we have a better place for this discussion? I'll be happy to take it there.

[Jeremy Orlow]

(In reply to comment #9)
> > But aren't we getting ahead of ourselves?  This bug is for adding the idl
> > files.  This needs to happen whether or not radical changes to the http
> > implementation are necessary, right?
> 
> Do we have a better place for this discussion? I'll be happy to take it there.

Sorry, I guess I should have just created a new bug in the first place.  Let's discuss the protocol/http aspect of it here? https://bugs.webkit.org/show_bug.cgi?id=27459

[Fumitoshi Ukai]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > I'm not sure no static tables.  I just follow XMLHttpRequest.
> > 
> > NoStaticTables is needed for interfaces that will be used from worker threads.

Hmm, then WebSocket also needs NoStaticTables, because it will be used from worker threads as spec says (cf. http://dev.w3.org/html5/workers/)

> > 
> > > If you want to see more implementation, please take a look at
> > > http://codereview.chromium.org/155079
> > 
> > I do not think that implementing Web Sockets elsewhere and pushing it into
> > Bugzilla just for landing will work smoothly.
> 
> I'm not sure I agree.  WebKit developers often seem to write the entire
> implementation, break it into patches, discuss each patch on its own merit, and
> iterate.  I believe that's precisely what's happening here.

Yes, I've just uploaded my work-in-progress code to codereview.chromium.org  just to be shared with other people before ready to review in webkit.
I won't try to push it for landing.  I'll try to break it into patches (like this bug), discuss each patch and iterate. 

> 
> > The most interesting aspect of WebSockets implementation, and one that I didn't
> > see discussed yet, is what kind of back-end we need. It is not clear whether
> > existing HTTP back-ends for supported platforms allow for the level of control
> > needed for Web Sockets - it is possible that we'll actually need a new
> > cross-platform HTTP implementation for these.
> 
> My understanding is that this code has been tested end-to-end on at least one
> platform, so it's possible that won't be necessary.
> 
> If, however, there are http stack limitations, Chromium recently wrote a cross
> platform network stack that might be useful here.  It's my understanding that
> it was written to have minimal dependencies on Chromium as well.
> 
> But aren't we getting ahead of ourselves?  This bug is for adding the idl
> files.  This needs to happen whether or not radical changes to the http
> implementation are necessary, right?

[Fumitoshi Ukai]

Created attachment 33698 [details]
add WebSocket.idl and js binding classes.


---
 7 files changed, 619 insertions(+), 0 deletions(-)

[Alexey Proskuryakov]

Comment on attachment 33698 [details]
add WebSocket.idl and js binding classes.

I think that the patch should also update project files for all build systems (or at the very least, Mac). If the code is not functional as is, it should be guarded with ENABLE() ifdefs.

> +JSWebSocketConstructor::JSWebSocketConstructor(ExecState* exec, JSDOMGlobalObject* globalObject)
> +        : DOMConstructorObject(JSWebSocketConstructor::createStructure(globalObject->objectPrototype()), globalObject)

Please use 4 spaces to indent.

> +{
> +    putDirect(exec->propertyNames().prototype, JSWebSocketPrototype::self(exec, globalObject), None);
> +}

Looking at existing constructors, I believe you may also need to add a length property.

> +    RefPtr<WebSocket> websocket = WebSocket::create(context);

"Websocket" is not a word, please capitalize the variable name accordingly (webSocket).

> +        const String protocol = args.at(1).toString(exec);

I think you intended to assign to a reference here.

Please add a test for what happens when toString() raises an exception for either argument. Also, I'm not sure exactly what behavior is intended here, but you may need to consider null arguments, not just undefined ones.

> +        websocket->connect(url, protocol, ec);

Why is ec ignored?

> +    // TODO(ukai): garbage collection policy?

We use FIXME comments which say what exactly is wrong with the code. It is not clear how others could use this comment.

> +    if (m_impl->readyState() != WebSocket::CLOSED)
> +        markIfNotNull(m_impl->onmessage());
> +    // EventListeners?

Ditto.

> +    JSValue ret = jsBoolean(impl()->send(args.at(0).toString(exec), ec));

What happens if toString raises an exception? Please add a test (if the code is not functional as is, please try to add it locally for later submission, let's just not forget about it).

> +// TODO(ukai): addEventListener/removeEventListener?

Should be a FIXME, explaining what needs to be done (is it just unfinished code that can be pretty much copied from elsewhere, or something that needs to be carefully considered first?)
> +WebSocket::WebSocket(ScriptExecutionContext* context)
> +        : ActiveDOMObject(context, this)
> +        , m_state(CONNECTING)
> +        , m_protocol("")

Please use 4-space indentation.

> +    m_url = url.copy();

Why copy the URL? As a comment in KURL.h explains, this is only needed if you intend to use the KURL object on another thread.

> +    notImplemented();

No need to use notImplemented in cross-platform code that's just unfinished - it's helpful for port maintainers to learn what unimplemented code paths they hit, but not in this case.

Multiple TODOs in this file that need to be FIXMEs.

> +class WebSocket : public RefCounted<WebSocket>, public EventTarget, public ActiveDOMObject {

Inheriting from ActiveDOMObject is needed when you want to use its GC protection features. Is this the plan?

> +    // avoid error: request for member 'deref' is ambiguous.
> +    using RefCounted<WebSocket>::ref;
> +    using RefCounted<WebSocket>::deref;

This is something we routinely do in other code, probably no need to specifically comment here. If you prefer to keep this comment, please turn it into a full sentence starting with a capital letter.

This looks pretty good to me, but r- for now, since there are quite a few comments.

[Fumitoshi Ukai]

Created attachment 33966 [details]
Add WebSocket.idl and js binding classes.


---
 10 files changed, 678 insertions(+), 0 deletions(-)

[Fumitoshi Ukai]

Created attachment 33967 [details]
add WebSocket.idl and js binding classes


---
 10 files changed, 680 insertions(+), 0 deletions(-)

[Fumitoshi Ukai]

(In reply to comment #13)

Thanks for review!

> (From update of attachment 33698 [details])
> I think that the patch should also update project files for all build systems
> (or at the very least, Mac). If the code is not functional as is, it should be
> guarded with ENABLE() ifdefs.

I updated build system (for GNUmakefile.am and WebCore.xcodeproj).

> 
> > +JSWebSocketConstructor::JSWebSocketConstructor(ExecState* exec, JSDOMGlobalObject* globalObject)
> > +        : DOMConstructorObject(JSWebSocketConstructor::createStructure(globalObject->objectPrototype()), globalObject)
> 
> Please use 4 spaces to indent.

Fixed.

> 
> > +{
> > +    putDirect(exec->propertyNames().prototype, JSWebSocketPrototype::self(exec, globalObject), None);
> > +}
> 
> Looking at existing constructors, I believe you may also need to add a length
> property.

A length property is used for a number of expected arguments, so we should set 1 for WebSocket?

> 
> > +    RefPtr<WebSocket> websocket = WebSocket::create(context);
> 
> "Websocket" is not a word, please capitalize the variable name accordingly
> (webSocket).

Fixed.

> 
> > +        const String protocol = args.at(1).toString(exec);
> 
> I think you intended to assign to a reference here.

Yes, Fixed.

> 
> Please add a test for what happens when toString() raises an exception for
> either argument. Also, I'm not sure exactly what behavior is intended here, but
> you may need to consider null arguments, not just undefined ones.

Added check code for exceptions and null.

> 
> > +        websocket->connect(url, protocol, ec);
> 
> Why is ec ignored?

Added setDOMException.

> 
> > +    // TODO(ukai): garbage collection policy?
> 
> We use FIXME comments which say what exactly is wrong with the code. It is not
> clear how others could use this comment.

Ok.  Changed them to FIXME.

> 
> > +    if (m_impl->readyState() != WebSocket::CLOSED)
> > +        markIfNotNull(m_impl->onmessage());
> > +    // EventListeners?
> 
> Ditto.
> 
> > +    JSValue ret = jsBoolean(impl()->send(args.at(0).toString(exec), ec));
> 
> What happens if toString raises an exception? Please add a test (if the code is
> not functional as is, please try to add it locally for later submission, let's
> just not forget about it).

Sure.

> 
> > +// TODO(ukai): addEventListener/removeEventListener?
> 
> Should be a FIXME, explaining what needs to be done (is it just unfinished code
> that can be pretty much copied from elsewhere, or something that needs to be
> carefully considered first?)

I think it can be pretty much copied from elsewhere.  Should I include them now?

> > +WebSocket::WebSocket(ScriptExecutionContext* context)
> > +        : ActiveDOMObject(context, this)
> > +        , m_state(CONNECTING)
> > +        , m_protocol("")
> 
> Please use 4-space indentation.

Fixed.

> 
> > +    m_url = url.copy();
> 
> Why copy the URL? As a comment in KURL.h explains, this is only needed if you
> intend to use the KURL object on another thread.

Fixed.

> 
> > +    notImplemented();
> 
> No need to use notImplemented in cross-platform code that's just unfinished -
> it's helpful for port maintainers to learn what unimplemented code paths they
> hit, but not in this case.

I see.  Just marked with FIXME comments.

> 
> Multiple TODOs in this file that need to be FIXMEs.
> 
> > +class WebSocket : public RefCounted<WebSocket>, public EventTarget, public ActiveDOMObject {
> 
> Inheriting from ActiveDOMObject is needed when you want to use its GC
> protection features. Is this the plan?

I think we need to use GC protection as XMLHttpRequest does.  What do you think of?

> 
> > +    // avoid error: request for member 'deref' is ambiguous.
> > +    using RefCounted<WebSocket>::ref;
> > +    using RefCounted<WebSocket>::deref;
> 
> This is something we routinely do in other code, probably no need to
> specifically comment here. If you prefer to keep this comment, please turn it
> into a full sentence starting with a capital letter.

Ok, removed the comment.

> 
> This looks pretty good to me, but r- for now, since there are quite a few
> comments.

[Sam Weinig]

Comment on attachment 33967 [details]
add WebSocket.idl and js binding classes


> +static JSObject* constructWebSocket(ExecState* exec, JSObject* constructor, const ArgList& args)
> +{
> +    JSWebSocketConstructor* jsConstructor = static_cast<JSWebSocketConstructor*>(constructor);
> +    ScriptExecutionContext* context = jsConstructor->scriptExecutionContext();
> +    if (!context)
> +        return throwError(exec, ReferenceError, "WebSocket constructor associated document is unavailable");
> +
> +    if (args.size() == 0)
> +        return throwError(exec, SyntaxError, "Not enough arguments");
> +
> +    if (args.at(0).isUndefined())
> +        return throwError(exec, SyntaxError, "undefined URL");

I don't think this is necessary.  toStringing undefined will work fine.

> +
> +    const String& urlString = args.at(0).toString(exec);
> +    if (exec->hadException())
> +        return throwError(exec, SyntaxError, "wrong URL");
> +    const KURL& url = context->completeURL(urlString);
> +    RefPtr<WebSocket> webSocket = WebSocket::create(context);
> +    ExceptionCode ec = 0;
> +    if (args.size() < 2)
> +        webSocket->connect(url, ec);
> +    else {
> +        if (args.at(1).isUndefined())
> +            return throwError(exec, SyntaxError, "protocol is undefined");

This also seems unnecessary.


> +        const String& protocol = args.at(1).toString(exec);
> +        if (exec->hadException())
> +            return throwError(exec, SyntaxError, "wrong protocol");

I don't understand this exception.  We should just propagate the existing exception that was thrown by returning jsUndefined.

> +        if (!protocol.length())
> +            return throwError(exec, SyntaxError, "protocol is empty");

This also seems unnecessary.

> +        webSocket->connect(url, protocol, ec);
> +    }
> +    setDOMException(exec, ec);
> +    return CREATE_DOM_OBJECT_WRAPPER(exec, jsConstructor->globalObject(), WebSocket, webSocket.get());
> +}
> +

[Alexey Proskuryakov]

Comment on attachment 33967 [details]
add WebSocket.idl and js binding classes

> +    putDirect(exec->propertyNames().length, jsNumber(exec, 1), ReadOnly|DontDelete|DontEnum);

Per the coding style, there should be spaces around binary operators.

I do not have a good answer to your question - comparing with existing code, it seems ok, but it may be worth asking JS experts.

> +    // FIXME(ukai): mark if EventListeners is registered.

We also don't use names in FIXME comments - if it becomes really necessary who added a comment, one can use svn blame, but it's rarely interesting.

> +    , m_protocol("")

What's the difference between null and empty m_protocol? Is this initialization really necessary?

> +WebSocket::~WebSocket()
> +{
> +    if (m_state != CLOSED)
> +        close();
> +}

There's a check for m_state inside close() already.

> I updated build system (for GNUmakefile.am and WebCore.xcodeproj).

You could add a ChangeLog comment explaining that other build systems will be updated once the code is functional, to avoid confusing people into thinking that we forgot about other build systems.

> > > +// TODO(ukai): addEventListener/removeEventListener?
> > 
> I think it can be pretty much copied from elsewhere.  Should I include them
> now?

Either way is ok with me.

> I think we need to use GC protection as XMLHttpRequest does.  What do you think
> of?

Sure, sounds good. I was asking only because no ACtiveDOMObject methods were being called, but of course, most of implementation is not here yet.

Combined with Sam's comments, it seems that another quick review round would be useful, so r- for now. Thanks for updating the patch!

[Fumitoshi Ukai]

Created attachment 34035 [details]
Add WebSocket.idl and js binding classes


---
 10 files changed, 675 insertions(+), 0 deletions(-)

[Fumitoshi Ukai]

(In reply to comment #17)
> (From update of attachment 33967 [details])
> 
> > +static JSObject* constructWebSocket(ExecState* exec, JSObject* constructor, const ArgList& args)
> > +{
> > +    JSWebSocketConstructor* jsConstructor = static_cast<JSWebSocketConstructor*>(constructor);
> > +    ScriptExecutionContext* context = jsConstructor->scriptExecutionContext();
> > +    if (!context)
> > +        return throwError(exec, ReferenceError, "WebSocket constructor associated document is unavailable");
> > +
> > +    if (args.size() == 0)
> > +        return throwError(exec, SyntaxError, "Not enough arguments");
> > +
> > +    if (args.at(0).isUndefined())
> > +        return throwError(exec, SyntaxError, "undefined URL");
> 
> I don't think this is necessary.  toStringing undefined will work fine.

Fixed.
> 
> > +
> > +    const String& urlString = args.at(0).toString(exec);
> > +    if (exec->hadException())
> > +        return throwError(exec, SyntaxError, "wrong URL");
> > +    const KURL& url = context->completeURL(urlString);
> > +    RefPtr<WebSocket> webSocket = WebSocket::create(context);
> > +    ExceptionCode ec = 0;
> > +    if (args.size() < 2)
> > +        webSocket->connect(url, ec);
> > +    else {
> > +        if (args.at(1).isUndefined())
> > +            return throwError(exec, SyntaxError, "protocol is undefined");
> 
> This also seems unnecessary.

Fixed.
> 
> 
> > +        const String& protocol = args.at(1).toString(exec);
> > +        if (exec->hadException())
> > +            return throwError(exec, SyntaxError, "wrong protocol");
> 
> I don't understand this exception.  We should just propagate the existing
> exception that was thrown by returning jsUndefined.

Fixed.
> 
> > +        if (!protocol.length())
> > +            return throwError(exec, SyntaxError, "protocol is empty");
> 
> This also seems unnecessary.

WebSocket API spec says:
If protocol is specified but is either the empty string or contains characters that are not in the range U+0021 .. U+007E, then throw a SYNTAX_ERR  exception. 

So, I think we need to throw SyntaxErr exception when protocol is an empty string, isn't it?

> 
> > +        webSocket->connect(url, protocol, ec);
> > +    }
> > +    setDOMException(exec, ec);
> > +    return CREATE_DOM_OBJECT_WRAPPER(exec, jsConstructor->globalObject(), WebSocket, webSocket.get());
> > +}
> > +

[Fumitoshi Ukai]

(In reply to comment #18)
> (From update of attachment 33967 [details])
> > +    putDirect(exec->propertyNames().length, jsNumber(exec, 1), ReadOnly|DontDelete|DontEnum);
> 
> Per the coding style, there should be spaces around binary operators.
> 
> I do not have a good answer to your question - comparing with existing code, it
> seems ok, but it may be worth asking JS experts.
> 
> > +    // FIXME(ukai): mark if EventListeners is registered.
> 
> We also don't use names in FIXME comments - if it becomes really necessary who
> added a comment, one can use svn blame, but it's rarely interesting.

Fixed.

> 
> > +    , m_protocol("")
> 
> What's the difference between null and empty m_protocol? Is this initialization
> really necessary?

Hmm, may be not necessary.

> 
> > +WebSocket::~WebSocket()
> > +{
> > +    if (m_state != CLOSED)
> > +        close();
> > +}
> 
> There's a check for m_state inside close() already.

Thanks. Fixed.

> 
> > I updated build system (for GNUmakefile.am and WebCore.xcodeproj).
> 
> You could add a ChangeLog comment explaining that other build systems will be
> updated once the code is functional, to avoid confusing people into thinking
> that we forgot about other build systems.

I see. added a ChangeLog comment.

> 
> > > > +// TODO(ukai): addEventListener/removeEventListener?
> > > 
> > I think it can be pretty much copied from elsewhere.  Should I include them
> > now?
> 
> Either way is ok with me.
> 
> > I think we need to use GC protection as XMLHttpRequest does.  What do you think
> > of?
> 
> Sure, sounds good. I was asking only because no ACtiveDOMObject methods were
> being called, but of course, most of implementation is not here yet.
> 
> Combined with Sam's comments, it seems that another quick review round would be
> useful, so r- for now. Thanks for updating the patch!

Thanks for review!

[Sam Weinig]

> > > +        if (!protocol.length())
> > > +            return throwError(exec, SyntaxError, "protocol is empty");
> > 
> > This also seems unnecessary.
> 
> WebSocket API spec says:
> If protocol is specified but is either the empty string or contains characters
> that are not in the range U+0021 .. U+007E, then throw a SYNTAX_ERR  exception. 
> 
> So, I think we need to throw SyntaxErr exception when protocol is an empty
> string, isn't it?

This is actually saying you should throw a DOM exception of type SYNTAX_ERR, which is subtly different.  This should probably be handled in the DOM code and not in the bindings.

[Alexey Proskuryakov]

The rest of the patch looks good to me.

[Fumitoshi Ukai]

Created attachment 34112 [details]
Add WebSocket.idl and js binding classes


---
 10 files changed, 678 insertions(+), 0 deletions(-)

[Fumitoshi Ukai]

(In reply to comment #22)
> > > > +        if (!protocol.length())
> > > > +            return throwError(exec, SyntaxError, "protocol is empty");
> > > 
> > > This also seems unnecessary.
> > 
> > WebSocket API spec says:
> > If protocol is specified but is either the empty string or contains characters
> > that are not in the range U+0021 .. U+007E, then throw a SYNTAX_ERR  exception. 
> > 
> > So, I think we need to throw SyntaxErr exception when protocol is an empty
> > string, isn't it?
> 
> This is actually saying you should throw a DOM exception of type SYNTAX_ERR,
> which is subtly different.  This should probably be handled in the DOM code and
> not in the bindings.

Ah, I see.
I fixed it to DOM exception.
Thanks!

[Alexey Proskuryakov]

Comment on attachment 34112 [details]
Add WebSocket.idl and js binding classes

r=me

[Adam Barth]

Comment on attachment 34112 [details]
Add WebSocket.idl and js binding classes

The commit-queue says this patch does not build:

    /Developer/usr/bin/gcc-4.2 -x c++ -arch i386 -fmessage-length=0 -pipe -Wno-trigraphs -fno-exceptions -fno-rtti -fpascal-strings -fasm-blocks -O0 -Werror -Wnon-virtual-dtor -Wnewline-eof -DDISABLE_THREAD_CHECK -DENABLE_3D_RENDERING -DENABLE_CHANNEL_MESSAGING -DENABLE_DATABASE -DENABLE_DATAGRID -DENABLE_DOM_STORAGE -DENABLE_ICONDATABASE -DENABLE_JAVASCRIPT_DEBUGGER -DENABLE_OFFLINE_WEB_APPLICATIONS -DENABLE_RUBY -DENABLE_SVG -DENABLE_SVG_ANIMATION -DENABLE_SVG_AS_IMAGE -DENABLE_SVG_DOM_OBJC_BINDINGS -DENABLE_SVG_FONTS -DENABLE_SVG_FOREIGN_OBJECT -DENABLE_SVG_USE -DENABLE_VIDEO -DENABLE_WEB_SOCKETS -DENABLE_WORKERS -DENABLE_XPATH -DENABLE_XSLT -DWEBKIT_VERSION_MIN_REQUIRED=WEBKIT_VERSION_LATEST -fvisibility-inlines-hidden -fno-threadsafe-statics -mmacosx-version-min=10.5 -gdwarf-2 -I/Users/abarth/git/webkit/WebKitBuild/WebCore.build/Debug/WebCore.build/WebCore.hmap -Wall -Wextra -Wcast-align -Wcast-qual -Wchar-subscripts -Wextra-tokens -Wformat=2 -Winit-self -Wmissing-format-attribute -Wmissing-noreturn -Wpacked -Wpointer-arith -Wredundant-decls -Wundef -Wwrite-strings -Wshorten-64-to-32 -F/Users/abarth/git/webkit/WebKitBuild/Debug -F/System/Library/Frameworks/Carbon.framework/Frameworks -F/System/Library/Frameworks/ApplicationServices.framework/Frameworks -I/Users/abarth/git/webkit/WebKitBuild/Debug/include -IForwardingHeaders -Iicu -I/usr/include/libxslt -I/usr/include/libxml2 -I/Users/abarth/git/webkit/WebKitBuild/Debug/WebCoreSQLite3 -I/Users/abarth/git/webkit/WebKitBuild/Debug/DerivedSources/WebCore -I/Users/abarth/git/webkit/WebKitBuild/WebCore.build/Debug/WebCore.build/DerivedSources/i386 -I/Users/abarth/git/webkit/WebKitBuild/WebCore.build/Debug/WebCore.build/DerivedSources -include /var/folders/qt/qthS2Td+ExiMdusUTSRQrE+++TI/-Caches-/com.apple.Xcode.501/SharedPrecompiledHeaders/WebCorePrefix-diivbhagnqkgohdrnsdwdkawzyxk/WebCorePrefix.h -c /Users/abarth/git/webkit/WebCore/websockets/WebSocket.cpp -o /Users/abarth/git/webkit/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/i386/WebSocket.o
cc1plus: warnings being treated as errors
/Users/abarth/git/webkit/WebCore/websockets/WebSocket.cpp:79: warning: unused parameter ‘message’
/Users/abarth/git/webkit/WebCore/websockets/WebSocket.cpp:118: warning: unused parameter ‘eventType’
/Users/abarth/git/webkit/WebCore/websockets/WebSocket.cpp:118: warning: unused parameter ‘listener’
/Users/abarth/git/webkit/WebCore/websockets/WebSocket.cpp:118: warning: unused parameter ‘useCapture’
/Users/abarth/git/webkit/WebCore/websockets/WebSocket.cpp:123: warning: unused parameter ‘eventType’
/Users/abarth/git/webkit/WebCore/websockets/WebSocket.cpp:123: warning: unused parameter ‘listener’
/Users/abarth/git/webkit/WebCore/websockets/WebSocket.cpp:123: warning: unused parameter ‘useCapture’
/Users/abarth/git/webkit/WebCore/websockets/WebSocket.cpp:128: warning: unused parameter ‘event’
/Users/abarth/git/webkit/WebCore/websockets/WebSocket.cpp:128: warning: unused parameter ‘ec’
/Users/abarth/git/webkit/WebCore/websockets/WebSocket.cpp:162: warning: unused parameter ‘msg’

[Fumitoshi Ukai]

Created attachment 34202 [details]
Add WebSocket.idl and js binding classes


---
 10 files changed, 678 insertions(+), 0 deletions(-)

[Fumitoshi Ukai]

(In reply to comment #27)
> (From update of attachment 34112 [details])
> The commit-queue says this patch does not build:
> 
>     /Developer/usr/bin/gcc-4.2 -x c++ -arch i386 -fmessage-length=0 -pipe
> -Wno-trigraphs -fno-exceptions -fno-rtti -fpascal-strings -fasm-blocks -O0
> -Werror -Wnon-virtual-dtor -Wnewline-eof -DDISABLE_THREAD_CHECK
> -DENABLE_3D_RENDERING -DENABLE_CHANNEL_MESSAGING -DENABLE_DATABASE
> -DENABLE_DATAGRID -DENABLE_DOM_STORAGE -DENABLE_ICONDATABASE
> -DENABLE_JAVASCRIPT_DEBUGGER -DENABLE_OFFLINE_WEB_APPLICATIONS -DENABLE_RUBY
> -DENABLE_SVG -DENABLE_SVG_ANIMATION -DENABLE_SVG_AS_IMAGE
> -DENABLE_SVG_DOM_OBJC_BINDINGS -DENABLE_SVG_FONTS -DENABLE_SVG_FOREIGN_OBJECT
> -DENABLE_SVG_USE -DENABLE_VIDEO -DENABLE_WEB_SOCKETS -DENABLE_WORKERS
> -DENABLE_XPATH -DENABLE_XSLT
> -DWEBKIT_VERSION_MIN_REQUIRED=WEBKIT_VERSION_LATEST -fvisibility-inlines-hidden
> -fno-threadsafe-statics -mmacosx-version-min=10.5 -gdwarf-2
> -I/Users/abarth/git/webkit/WebKitBuild/WebCore.build/Debug/WebCore.build/WebCore.hmap
> -Wall -Wextra -Wcast-align -Wcast-qual -Wchar-subscripts -Wextra-tokens
> -Wformat=2 -Winit-self -Wmissing-format-attribute -Wmissing-noreturn -Wpacked
> -Wpointer-arith -Wredundant-decls -Wundef -Wwrite-strings -Wshorten-64-to-32
> -F/Users/abarth/git/webkit/WebKitBuild/Debug
> -F/System/Library/Frameworks/Carbon.framework/Frameworks
> -F/System/Library/Frameworks/ApplicationServices.framework/Frameworks
> -I/Users/abarth/git/webkit/WebKitBuild/Debug/include -IForwardingHeaders -Iicu
> -I/usr/include/libxslt -I/usr/include/libxml2
> -I/Users/abarth/git/webkit/WebKitBuild/Debug/WebCoreSQLite3
> -I/Users/abarth/git/webkit/WebKitBuild/Debug/DerivedSources/WebCore
> -I/Users/abarth/git/webkit/WebKitBuild/WebCore.build/Debug/WebCore.build/DerivedSources/i386
> -I/Users/abarth/git/webkit/WebKitBuild/WebCore.build/Debug/WebCore.build/DerivedSources
> -include
> /var/folders/qt/qthS2Td+ExiMdusUTSRQrE+++TI/-Caches-/com.apple.Xcode.501/SharedPrecompiledHeaders/WebCorePrefix-diivbhagnqkgohdrnsdwdkawzyxk/WebCorePrefix.h
> -c /Users/abarth/git/webkit/WebCore/websockets/WebSocket.cpp -o
> /Users/abarth/git/webkit/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/i386/WebSocket.o
> cc1plus: warnings being treated as errors
> /Users/abarth/git/webkit/WebCore/websockets/WebSocket.cpp:79: warning: unused
> parameter ‘message’
> /Users/abarth/git/webkit/WebCore/websockets/WebSocket.cpp:118: warning: unused
> parameter ‘eventType’
> /Users/abarth/git/webkit/WebCore/websockets/WebSocket.cpp:118: warning: unused
> parameter ‘listener’
> /Users/abarth/git/webkit/WebCore/websockets/WebSocket.cpp:118: warning: unused
> parameter ‘useCapture’
> /Users/abarth/git/webkit/WebCore/websockets/WebSocket.cpp:123: warning: unused
> parameter ‘eventType’
> /Users/abarth/git/webkit/WebCore/websockets/WebSocket.cpp:123: warning: unused
> parameter ‘listener’
> /Users/abarth/git/webkit/WebCore/websockets/WebSocket.cpp:123: warning: unused
> parameter ‘useCapture’
> /Users/abarth/git/webkit/WebCore/websockets/WebSocket.cpp:128: warning: unused
> parameter ‘event’
> /Users/abarth/git/webkit/WebCore/websockets/WebSocket.cpp:128: warning: unused
> parameter ‘ec’
> /Users/abarth/git/webkit/WebCore/websockets/WebSocket.cpp:162: warning: unused
> parameter ‘msg’

Omit the unused parameter names.

[Adam Barth]

Comment on attachment 34202 [details]
Add WebSocket.idl and js binding classes

rs=me, assuming this is the same patch as before with the extra parameter names removed.

[Adam Barth]

Comment on attachment 34202 [details]
Add WebSocket.idl and js binding classes

2 out of 7 hunks FAILED -- saving rejects to file WebCore/WebCore.xcodeproj/project.pbxproj.rej
patch -p0 "WebCore/WebCore.xcodeproj/project.pbxproj" returned 1.  Pass --force to ignore patch failures.

[Jeremy Orlow]

I'll just land this manually.

Assigning to myself for landing.

I assume landing is fine since the only reason it's marked review- is because the commit queue script choked on it.

[Adam Barth]

(In reply to comment #32)
> I'll just land this manually.
> 
> Assigning to myself for landing.
> 
> I assume landing is fine since the only reason it's marked review- is because
> the commit queue script choked on it.

Yeah.  Thanks Jeremy.

[Jeremy Orlow]

I just fixed the failed hunk issues and am building now.

Fumitoshi, I noticed that the diff style was pretty weird.  Could you make sure that whatever's generating it is using a more standard universal diff format?

I also noticed that the ChangeLog mentioned methods in newly added Files.  I triple checked with Dimitri (the first two checks were to myself :-) and he said that these are generally avoided, so I deleted them.

Lastly, I noticed that this does not define anything for visual studio.  Is this an oversight?  If it builds and runs the tests correctly and no one's responded to this with a "hold on!" by the time that's done, I'll commit as is an assume that'll be changed in another patch.  Sound good?

[Jeremy Orlow]

Landed  in 46864

[Fumitoshi Ukai]

(In reply to comment #34)
> I just fixed the failed hunk issues and am building now.

Thanks for landing!

> 
> Fumitoshi, I noticed that the diff style was pretty weird.  Could you make sure
> that whatever's generating it is using a more standard universal diff format?

I just used WebKitTools/Scripts/bugzilla-tool post-commits from git repository.
Do I need some configuration?
 
> I also noticed that the ChangeLog mentioned methods in newly added Files.  I
> triple checked with Dimitri (the first two checks were to myself :-) and he
> said that these are generally avoided, so I deleted them.

Thanks.
 
> Lastly, I noticed that this does not define anything for visual studio.  Is
> this an oversight?  If it builds and runs the tests correctly and no one's
> responded to this with a "hold on!" by the time that's done, I'll commit as is
> an assume that'll be changed in another patch.  Sound good?

Sounds good.
I'll add other platform build in another patch.

[Jeremy Orlow]

(In reply to comment #36)
> I just used WebKitTools/Scripts/bugzilla-tool post-commits from git repository.
> Do I need some configuration?

Well, if Bugzilla tool did it, I guess maybe that's the new style they want people to use or something?  Hm...donno.  I guess don't worry about it

> > Lastly, I noticed that this does not define anything for visual studio.  Is
> > this an oversight?  If it builds and runs the tests correctly and no one's
> > responded to this with a "hold on!" by the time that's done, I'll commit as is
> > an assume that'll be changed in another patch.  Sound good?
> 
> Sounds good.
> I'll add other platform build in another patch.

Be sure to cc [e-mail] on the patch.  He did some fix-up of the xcode project file after I committed things, so he might have some opinions on the visual studio one.

[David Levin]

I wouldn't do this "Be sure to cc [e-mail] on the patch. "

He doesn't like that much, but you can ping him in irc and ask him to take a look.