[webkit-reviews] review denied: [Bug 27209] Add WebSocket.idl : [Attachment 33698] add WebSocket.idl and js binding classes.

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Fumitoshi Ukai
<ukai at chromium.org>'s request for review:
Bug 27209: Add WebSocket.idl
https://bugs.webkit.org/show_bug.cgi?id=27209

Attachment 33698: add WebSocket.idl and js binding classes.
https://bugs.webkit.org/attachment.cgi?id=33698&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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->obje
ctPrototype()), 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.