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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Aug 3 11:01:18 PDT 2009


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 33967: add WebSocket.idl and js binding classes
https://bugs.webkit.org/attachment.cgi?id=33967&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
> +    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!


More information about the webkit-reviews mailing list