Bug 27206 – Add --web-sockets flag and ENABLE_WEB_SOCKETS define

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Fumitoshi Ukai]

Add --websocket flag to build-webkit to make it enable websocket feature in build time.
Add ENABLE_WEBSOCKET define to put bits for websocket implementation.

[Fumitoshi Ukai]

Created attachment 32649 [details]
Add --websocket flag in build-webkit and ENABLE_WEBSOCKET define.


---
 7 files changed, 85 insertions(+), 2 deletions(-)

[Mark Rowe (bdash)]

Comment on attachment 32649 [details]
Add --websocket flag in build-webkit and ENABLE_WEBSOCKET define.

> diff --git a/WebCore/Configurations/FeatureDefines.xcconfig b/WebCore/Configurations/FeatureDefines.xcconfig
> index 1f196ea..3dade71 100644
> --- a/WebCore/Configurations/FeatureDefines.xcconfig
> +++ b/WebCore/Configurations/FeatureDefines.xcconfig
> @@ -52,5 +52,6 @@ ENABLE_WML = ;
>  ENABLE_WORKERS = ENABLE_WORKERS;
>  ENABLE_XPATH = ENABLE_XPATH;
>  ENABLE_XSLT = ENABLE_XSLT;
> +ENABLE_WEBSOCKET = ;
>  
> -FEATURE_DEFINES = $(ENABLE_3D_RENDERING) $(ENABLE_CHANNEL_MESSAGING) $(ENABLE_DATABASE) $(ENABLE_DATAGRID) $(ENABLE_DOM_STORAGE) $(ENABLE_FILTERS) $(ENABLE_GEOLOCATION) $(ENABLE_ICONDATABASE) $(ENABLE_JAVASCRIPT_DEBUGGER) $(ENABLE_OFFLINE_WEB_APPLICATIONS) $(ENABLE_SVG) $(ENABLE_SVG_ANIMATION) $(ENABLE_SVG_AS_IMAGE) $(ENABLE_SVG_DOM_OBJC_BINDINGS) $(ENABLE_SVG_FONTS) $(ENABLE_SVG_FOREIGN_OBJECT) $(ENABLE_SVG_USE) $(ENABLE_VIDEO) $(ENABLE_WML) $(ENABLE_WORKERS) $(ENABLE_XPATH) $(ENABLE_XSLT);
> +FEATURE_DEFINES = $(ENABLE_3D_RENDERING) $(ENABLE_CHANNEL_MESSAGING) $(ENABLE_DATABASE) $(ENABLE_DATAGRID) $(ENABLE_DOM_STORAGE) $(ENABLE_FILTERS) $(ENABLE_GEOLOCATION) $(ENABLE_ICONDATABASE) $(ENABLE_JAVASCRIPT_DEBUGGER) $(ENABLE_OFFLINE_WEB_APPLICATIONS) $(ENABLE_SVG) $(ENABLE_SVG_ANIMATION) $(ENABLE_SVG_AS_IMAGE) $(ENABLE_SVG_DOM_OBJC_BINDINGS) $(ENABLE_SVG_FONTS) $(ENABLE_SVG_FOREIGN_OBJECT) $(ENABLE_SVG_USE) $(ENABLE_VIDEO) $(ENABLE_WML) $(ENABLE_WORKERS) $(ENABLE_XPATH) $(ENABLE_XSLT) $(ENABLE_WEBSOCKET);

You need to make the same change to FeatureDefines.xcconfig in the other locations that it exists (JavaScriptCore and WebKit/mac).  You should also continue the tradition of keeping this list of variables sorted in alphabetical order.

> diff --git a/WebKitTools/Scripts/build-webkit b/WebKitTools/Scripts/build-webkit
> index 9e2f41c..83013dd 100755
> --- a/WebKitTools/Scripts/build-webkit
> +++ b/WebKitTools/Scripts/build-webkit
> @@ -48,7 +48,7 @@ my ($threeDRenderingSupport, $channelMessagingSupport, $databaseSupport, $domSto
>      $filtersSupport, $geolocationSupport, $gnomeKeyringSupport, $iconDatabaseSupport,
>      $javaScriptDebuggerSupport, $offlineWebApplicationSupport, $svgSupport, $svgAnimationSupport,
>      $svgAsImageSupport, $svgDOMObjCBindingsSupport, $svgFontsSupport, $svgForeignObjectSupport,
> -    $svgUseSupport, $videoSupport, $wmlSupport, $workersSupport, $xpathSupport, $xsltSupport,
> +    $svgUseSupport, $videoSupport, $wmlSupport, $workersSupport, $xpathSupport, $xsltSupport, $websocketSupport,
>      $coverageSupport);
>  
>  my @features = (
> @@ -120,6 +120,8 @@ my @features = (
>  
>      { option => "xslt", desc => "Toggle XSLT support",
>        define => "ENABLE_XSLT", default => 1, value => \$xsltSupport },
> +    { option => "websocket", desc => "Toggle WebSocket support",
> +      define => "ENABLE_WEBSOCKET", default => 0, value=> \$websocketSupport },
>  );

Is it "Web Socket", "WebSocket" or "web socket"?  You use different names here than in configure.ac.  If it's "Web Socket" I would expect the related flag names to use "web-socket".  If it's the latter, I'd expect "websocket".  I couldn't find the spec to check this myself, but either way we should be consistent.

You appear to have missed the related .vcproj changes to apply this to Windows as well.

r- to address these issues.

[Jeremy Orlow]

Hm...do we want this on by default yet, or should this wait until the implementation is in and somewhat stable?

[Sam Weinig]

I think it should be turned on so that it will be tested as it is brought up.

[Fumitoshi Ukai]

Created attachment 32695 [details]
Add "Web Sockets" feature configuration.


---
 15 files changed, 121 insertions(+), 7 deletions(-)

[Fumitoshi Ukai]

Thanks for review.

Added ENABLE_WEB_SOCKETS to FeatureDefines.xcconfig in other places.
Changed ENABLE_* in alphabetical order.

Asking on #webkit, "Web Sockets" is used for the feature/technology name, so changed to use "Web Sockets", ENABLE_WEB_SOCKETS, --enable-web-sockets, ...

Added ENABLE_WEB_SOCKETS in *.vcproj.

Turned --enable-web-sockets on by default.

[Fumitoshi Ukai]

Created attachment 32698 [details]
Add "Web Sockets" feature configuration.


---
 15 files changed, 121 insertions(+), 7 deletions(-)

[David Levin]

Comment on attachment 32698 [details]
Add "Web Sockets" feature configuration.

Just a few last small details.

> diff --git a/ChangeLog b/ChangeLog
> [phone]  Fumitoshi Ukai  <[e-mail]>
> +
> +        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.

[Fumitoshi Ukai]

Created attachment 32915 [details]
Add --web-sockets flag and ENABLE_WEB_SOCKETS define


---
 15 files changed, 106 insertions(+), 7 deletions(-)

[Fumitoshi Ukai]

Thanks for review.

Fixed all David commented.

[David Levin]

Assign to levin for landing.

[David Levin]

Committed as http://trac.webkit.org/changeset/46021