[webkit-reviews] review denied: [Bug 27206] Add --websocket flag and ENABLE_WEBSOCKET define : [Attachment 32698] Add "Web Sockets" feature configuration.

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Fumitoshi Ukai
<ukai at chromium.org>'s request for review:
Bug 27206: Add --websocket flag and ENABLE_WEBSOCKET define
https://bugs.webkit.org/show_bug.cgi?id=27206

Attachment 32698: Add "Web Sockets" feature configuration.
https://bugs.webkit.org/attachment.cgi?id=32698&action=review

------- Additional Comments from David Levin <levin at chromium.org>
Just a few last small details.

> diff --git a/ChangeLog b/ChangeLog
> +2009-07-12  Fumitoshi Ukai  <ukai at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   https://bugs.webkit.org/show_bug.cgi?id=27206
> +

The current changelog format has bug titles in addition to the bug link, so

  Add --websocket flag and ENABLE_WEBSOCKET define
  https://bugs.webkit.org/show_bug.cgi?id=27206

(for every ChangeLog in here).



> diff --git a/WebCore/GNUmakefile.am b/WebCore/GNUmakefile.am

> +# IDL_BINDINGS += WebCore/websockets/WebSocket.idl
...
> +#webcore_sources += \
> +#	WebCore/bindings/js/JSWebSocketConstructor.cpp \
> +#	WebCore/bindings/js/JSWebSocketConstructor.h \
> +#	WebCore/bindings/js/JSWebSocketCustom.cpp \
> +#	WebCore/websockets/WebSocket.cpp \
> +#	WebCore/websockets/WebSocket.h \
> +#	WebCore/websockets/WebSocketChannel.cpp \
> +#	WebCore/websockets/WebSocketChannel.h \
> +#	WebCore/websockets/WebSocketChannelClient.h \
> +#	WebCore/platform/network/WebSocketHandle.h \
> +#	WebCore/platform/network/WebSocketHandleClient.h \
> +#	WebCore/platform/network/WebSocketRequestBase.cpp \
> +#	WebCore/platform/network/WebSocketRequestBase.h \
> +#	WebCore/platform/network/WebSocketResponseBase.cpp \
> +#	WebCore/platform/network/WebSocketResponseBase.h
> +
> +# webcoregtk_sources += \
> +#	WebCore/platform/network/soup/WebSocketHandleSoup.cpp \
> +#	WebCore/platform/network/soup/WebSocketRequest.h \
> +#	WebCore/platform/network/soup/WebSocketResponse.h
> +
> +# if TARGET_WIN32
> +# webcore_sources += \
> +# endif

You're adding a lot of commented out "code".  I would just leave it out until
it is useful and add it as needed without commenting it out.


> diff --git a/WebKitTools/Scripts/build-webkit
b/WebKitTools/Scripts/build-webkit
> @@ -123,6 +123,8 @@ my @features = (
>  
>      { option => "xslt", desc => "Toggle XSLT support",
>	 define => "ENABLE_XSLT", default => 1, value => \$xsltSupport },
> +    { option => "web-sockets", desc => "Toggle Web Sockets support",
> +	 define => "ENABLE_WEB_SOCKETS", default => 1, value=>
\$webSocketsSupport },
>  );

This should be sorted as well.


> diff --git a/configure.ac b/configure.ac
>   WML support 					     : $enable_wml
>   Web Workers support 				     : $enable_workers
> + Web Sockets support 				     :
$enable_web_sockets

"Web Sockets support" should be just before "Web Workers support" instead of
after.